Copilot commented on code in PR #6231:
URL: https://github.com/apache/shenyu/pull/6231#discussion_r2542362246
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/AbstractShenyuPlugin.java:
##########
@@ -92,71 +96,56 @@ public abstract class AbstractShenyuPlugin implements
ShenyuPlugin {
public Mono<Void> execute(final ServerWebExchange exchange, final
ShenyuPluginChain chain) {
initCacheConfig();
final String pluginName = named();
- PluginData pluginData =
BaseDataCache.getInstance().obtainPluginData(pluginName);
- // early exit
- if (Objects.isNull(pluginData) || !pluginData.getEnabled()) {
- return chain.execute(exchange);
- }
final String path = getRawPath(exchange);
- List<SelectorData> selectors =
BaseDataCache.getInstance().obtainSelectorData(pluginName);
- if (CollectionUtils.isEmpty(selectors)) {
- return handleSelectorIfNull(pluginName, exchange, chain);
+
+ PluginDataDecisionMaker pluginDataDecisionMaker = new
PluginDataDecisionMaker();
+ List<PluginData> pluginDataList =
pluginDataDecisionMaker.getData(pluginName);
+ if (CollectionUtils.isEmpty(pluginDataList) ||
!pluginDataDecisionMaker.shouldContinue(pluginDataList.get(0))) {
+ return pluginDataDecisionMaker.handleEmpty(pluginName, exchange,
chain);
}
- SelectorData selectorData = obtainSelectorDataCacheIfEnabled(path);
- // handle Selector
- if (Objects.nonNull(selectorData) &&
StringUtils.isBlank(selectorData.getId())) {
- return handleSelectorIfNull(pluginName, exchange, chain);
+
+ SelectorDataDecisionMaker selectorDataDecisionMaker = new
SelectorDataDecisionMaker();
+ List<SelectorData> selectorDataList =
selectorDataDecisionMaker.getData(pluginName);
+ if (CollectionUtils.isEmpty(selectorDataList)) {
+ return selectorDataDecisionMaker.handleEmpty(pluginName, exchange,
chain);
}
- if (Objects.isNull(selectorData)) {
- selectorData = trieMatchSelector(exchange, pluginName, path);
- if (Objects.isNull(selectorData)) {
- selectorData = defaultMatchSelector(exchange, selectors, path);
- if (Objects.isNull(selectorData)) {
- return handleSelectorIfNull(pluginName, exchange, chain);
- }
- }
+
+ SelectorData selectorData =
selectorDataDecisionMaker.matchData(exchange, selectorDataList, path);
+ if (selectorData == null) {
+ return selectorDataDecisionMaker.handleEmpty(pluginName, exchange,
chain);
}
+
printLog(selectorData, pluginName);
- if (!selectorData.getContinued()) {
- // if continued， not match rules
+ if (!selectorDataDecisionMaker.shouldContinue(selectorData)) {
Review Comment:
Incorrect usage of `shouldContinue()`. The `shouldContinue` method checks if
data is enabled (`data.getEnabled()`), but the original logic checked
`selectorData.getContinued()` which indicates whether to continue processing
rules. These are two different concepts: `enabled` controls if the selector is
active, while `continued` controls whether to proceed to rule matching. The
correct check should be `if (!selectorData.getContinued())` instead of `if
(!selectorDataDecisionMaker.shouldContinue(selectorData))`.
```suggestion
if (!selectorData.getContinued()) {
```
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/maker/SelectorDataDecisionMaker.java:
##########
@@ -0,0 +1,30 @@
+package org.apache.shenyu.plugin.base.maker;
+
+import org.apache.shenyu.common.dto.SelectorData;
+import org.apache.shenyu.plugin.api.ShenyuPluginChain;
+import org.apache.shenyu.plugin.base.provider.SelectorDataProvider;
+import org.springframework.web.server.ServerWebExchange;
+import reactor.core.publisher.Mono;
+
+import java.util.List;
+
+public class SelectorDataDecisionMaker extends
AbstractMatchDecisionMaker<SelectorData> {
+ public SelectorDataDecisionMaker() {
+ super(new SelectorDataProvider());
+ }
+
+ @Override
+ public Mono<Void> handleEmpty(String pluginName, ServerWebExchange
exchange, ShenyuPluginChain chain) {
+ return chain.execute(exchange);
+ }
+
+ @Override
+ public SelectorData matchData(ServerWebExchange exchange,
List<SelectorData> dataList, String path) {
+ return dataList.isEmpty() ? null : dataList.get(0);
+ }
+
+ @Override
+ public boolean shouldContinue(SelectorData data) {
+ return data != null && data.getEnabled();
Review Comment:
The `shouldContinue` method logic is incorrect for SelectorData. It checks
`data.getEnabled()`, but based on its usage in AbstractShenyuPlugin line 119,
it should check `data.getContinued()` instead. The `continued` field determines
whether to continue processing rules after a selector matches, which is
different from the `enabled` field that controls if the selector is active.
```suggestion
return data != null && data.getContinued();
```
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/AbstractShenyuPlugin.java:
##########
@@ -92,71 +96,56 @@ public abstract class AbstractShenyuPlugin implements
ShenyuPlugin {
public Mono<Void> execute(final ServerWebExchange exchange, final
ShenyuPluginChain chain) {
initCacheConfig();
final String pluginName = named();
- PluginData pluginData =
BaseDataCache.getInstance().obtainPluginData(pluginName);
- // early exit
- if (Objects.isNull(pluginData) || !pluginData.getEnabled()) {
- return chain.execute(exchange);
- }
final String path = getRawPath(exchange);
- List<SelectorData> selectors =
BaseDataCache.getInstance().obtainSelectorData(pluginName);
- if (CollectionUtils.isEmpty(selectors)) {
- return handleSelectorIfNull(pluginName, exchange, chain);
+
+ PluginDataDecisionMaker pluginDataDecisionMaker = new
PluginDataDecisionMaker();
+ List<PluginData> pluginDataList =
pluginDataDecisionMaker.getData(pluginName);
+ if (CollectionUtils.isEmpty(pluginDataList) ||
!pluginDataDecisionMaker.shouldContinue(pluginDataList.get(0))) {
+ return pluginDataDecisionMaker.handleEmpty(pluginName, exchange,
chain);
}
- SelectorData selectorData = obtainSelectorDataCacheIfEnabled(path);
- // handle Selector
- if (Objects.nonNull(selectorData) &&
StringUtils.isBlank(selectorData.getId())) {
- return handleSelectorIfNull(pluginName, exchange, chain);
+
+ SelectorDataDecisionMaker selectorDataDecisionMaker = new
SelectorDataDecisionMaker();
+ List<SelectorData> selectorDataList =
selectorDataDecisionMaker.getData(pluginName);
+ if (CollectionUtils.isEmpty(selectorDataList)) {
+ return selectorDataDecisionMaker.handleEmpty(pluginName, exchange,
chain);
}
- if (Objects.isNull(selectorData)) {
- selectorData = trieMatchSelector(exchange, pluginName, path);
- if (Objects.isNull(selectorData)) {
- selectorData = defaultMatchSelector(exchange, selectors, path);
- if (Objects.isNull(selectorData)) {
- return handleSelectorIfNull(pluginName, exchange, chain);
- }
- }
+
+ SelectorData selectorData =
selectorDataDecisionMaker.matchData(exchange, selectorDataList, path);
+ if (selectorData == null) {
+ return selectorDataDecisionMaker.handleEmpty(pluginName, exchange,
chain);
}
+
printLog(selectorData, pluginName);
- if (!selectorData.getContinued()) {
- // if continued， not match rules
+ if (!selectorDataDecisionMaker.shouldContinue(selectorData)) {
return doExecute(exchange, chain, selectorData,
defaultRuleData(selectorData));
}
- List<RuleData> rules =
BaseDataCache.getInstance().obtainRuleData(selectorData.getId());
- if (CollectionUtils.isEmpty(rules)) {
- return handleRuleIfNull(pluginName, exchange, chain);
+
+
+ RuleDataDecisionMaker ruleDataDecisionMaker = new
RuleDataDecisionMaker();
Review Comment:
Creating a new `RuleDataDecisionMaker` instance on every request is
inefficient. This should be a reusable class field to avoid unnecessary object
allocation.
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/AbstractShenyuPlugin.java:
##########
@@ -92,71 +96,56 @@ public abstract class AbstractShenyuPlugin implements
ShenyuPlugin {
public Mono<Void> execute(final ServerWebExchange exchange, final
ShenyuPluginChain chain) {
initCacheConfig();
final String pluginName = named();
- PluginData pluginData =
BaseDataCache.getInstance().obtainPluginData(pluginName);
- // early exit
- if (Objects.isNull(pluginData) || !pluginData.getEnabled()) {
- return chain.execute(exchange);
- }
final String path = getRawPath(exchange);
- List<SelectorData> selectors =
BaseDataCache.getInstance().obtainSelectorData(pluginName);
- if (CollectionUtils.isEmpty(selectors)) {
- return handleSelectorIfNull(pluginName, exchange, chain);
+
+ PluginDataDecisionMaker pluginDataDecisionMaker = new
PluginDataDecisionMaker();
+ List<PluginData> pluginDataList =
pluginDataDecisionMaker.getData(pluginName);
+ if (CollectionUtils.isEmpty(pluginDataList) ||
!pluginDataDecisionMaker.shouldContinue(pluginDataList.get(0))) {
+ return pluginDataDecisionMaker.handleEmpty(pluginName, exchange,
chain);
}
- SelectorData selectorData = obtainSelectorDataCacheIfEnabled(path);
- // handle Selector
- if (Objects.nonNull(selectorData) &&
StringUtils.isBlank(selectorData.getId())) {
- return handleSelectorIfNull(pluginName, exchange, chain);
+
+ SelectorDataDecisionMaker selectorDataDecisionMaker = new
SelectorDataDecisionMaker();
Review Comment:
Creating a new `SelectorDataDecisionMaker` instance on every request is
inefficient. This should be a reusable class field to avoid unnecessary object
allocation.
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/provider/PluginDataProvider.java:
##########
@@ -0,0 +1,15 @@
+package org.apache.shenyu.plugin.base.provider;
+
+import org.apache.shenyu.common.dto.PluginData;
+import org.apache.shenyu.plugin.base.cache.BaseDataCache;
+
+import java.util.Collections;
+import java.util.List;
+
+public class PluginDataProvider implements DataProvider<PluginData> {
+ @Override
+ public List<PluginData> getData(String pluginName) {
+ PluginData data =
BaseDataCache.getInstance().obtainPluginData(pluginName);
+ return data != null ? Collections.singletonList(data) :
Collections.emptyList();
+ }
+}
Review Comment:
Missing class-level and method-level documentation. This new public class
should include Javadoc explaining its purpose and how it fits into the data
provider pattern.
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/provider/DataProvider.java:
##########
@@ -0,0 +1,7 @@
+package org.apache.shenyu.plugin.base.provider;
+
+import java.util.List;
+
+public interface DataProvider<T> {
Review Comment:
Missing class-level and method-level documentation. This is a new public
interface that should include Javadoc explaining its purpose, usage, and the
meaning of the type parameter `T` and method parameter `key`.
```suggestion
/**
* A generic interface for providing data based on a given key.
* <p>
* Implementations of this interface are responsible for retrieving a list
of data items
* of type {@code T} associated with the specified key. This can be used in
plugin systems
* or other contexts where data needs to be fetched dynamically.
*
* @param <T> the type of data provided by this provider
*/
public interface DataProvider<T> {
/**
* Retrieves a list of data items associated with the specified key.
*
* @param key the key used to look up the data; its meaning is defined
by the implementation
* @return a list of data items of type {@code T} associated with the
given key,
* or an empty list if no data is found
*/
```
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/maker/PluginDataDecisionMaker.java:
##########
@@ -0,0 +1,30 @@
+package org.apache.shenyu.plugin.base.maker;
+
+import org.apache.shenyu.common.dto.PluginData;
+import org.apache.shenyu.plugin.api.ShenyuPluginChain;
+import org.apache.shenyu.plugin.base.provider.PluginDataProvider;
+import org.springframework.web.server.ServerWebExchange;
+import reactor.core.publisher.Mono;
+
+import java.util.List;
+
+public class PluginDataDecisionMaker extends
AbstractMatchDecisionMaker<PluginData> {
+ public PluginDataDecisionMaker() {
+ super(new PluginDataProvider());
+ }
+
+ @Override
+ public Mono<Void> handleEmpty(String pluginName, ServerWebExchange
exchange, ShenyuPluginChain chain) {
+ return chain.execute(exchange);
+ }
+
+ @Override
+ protected PluginData matchData(ServerWebExchange exchange,
List<PluginData> dataList, String path) {
Review Comment:
Inconsistent visibility modifier. This method is declared as `protected`
while the same method in `SelectorDataDecisionMaker` (line 22) and
`RuleDataDecisionMaker` (line 22) uses different visibility modifiers. For
consistency across the class hierarchy, all implementations should use the same
visibility modifier.
```suggestion
public PluginData matchData(ServerWebExchange exchange, List<PluginData>
dataList, String path) {
```
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/provider/SelectorDataProvider.java:
##########
@@ -0,0 +1,18 @@
+package org.apache.shenyu.plugin.base.provider;
+
+
+
+
+import org.apache.shenyu.common.dto.SelectorData;
+import org.apache.shenyu.plugin.base.cache.BaseDataCache;
+
+import java.util.Collections;
+import java.util.List;
+
+public class SelectorDataProvider implements DataProvider<SelectorData> {
+ @Override
+ public List<SelectorData> getData(String selectorName) {
+ List<SelectorData> data =
BaseDataCache.getInstance().obtainSelectorData(selectorName);
+ return data != null ? data : Collections.emptyList();
+ }
+}
Review Comment:
Missing class-level and method-level documentation. This new public class
should include Javadoc explaining its purpose and how it fits into the data
provider pattern.
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/maker/RuleDataDecisionMaker.java:
##########
@@ -0,0 +1,30 @@
+package org.apache.shenyu.plugin.base.maker;
+
+import org.apache.shenyu.common.dto.RuleData;
+import org.apache.shenyu.plugin.api.ShenyuPluginChain;
+import org.apache.shenyu.plugin.base.provider.RuleDataProvider;
+import org.springframework.web.server.ServerWebExchange;
+import reactor.core.publisher.Mono;
+
+import java.util.List;
+
+public class RuleDataDecisionMaker extends
AbstractMatchDecisionMaker<RuleData> {
+ public RuleDataDecisionMaker() {
+ super(new RuleDataProvider());
+ }
+
+ @Override
+ public Mono<Void> handleEmpty(String pluginName, ServerWebExchange
exchange, ShenyuPluginChain chain) {
+ return chain.execute(exchange);
+ }
+
+ @Override
+ public RuleData matchData(ServerWebExchange exchange, List<RuleData>
dataList, String path) {
+ return dataList.isEmpty() ? null : dataList.get(0);
Review Comment:
The `matchData` implementation is incomplete and incorrect. It simply
returns the first element from the list without performing any actual matching
logic. The original implementation included trie cache matching
(`trieMatchRule`) and complex condition-based matching (`defaultMatchRule`).
This oversimplification will cause incorrect rule selection and break existing
functionality.
```suggestion
// Try to match using trie cache first
RuleData rule = trieMatchRule(exchange, dataList, path);
if (rule != null) {
return rule;
}
// Fallback to default matching logic
return defaultMatchRule(exchange, dataList, path);
```
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/maker/SelectorDataDecisionMaker.java:
##########
@@ -0,0 +1,30 @@
+package org.apache.shenyu.plugin.base.maker;
+
+import org.apache.shenyu.common.dto.SelectorData;
+import org.apache.shenyu.plugin.api.ShenyuPluginChain;
+import org.apache.shenyu.plugin.base.provider.SelectorDataProvider;
+import org.springframework.web.server.ServerWebExchange;
+import reactor.core.publisher.Mono;
+
+import java.util.List;
+
+public class SelectorDataDecisionMaker extends
AbstractMatchDecisionMaker<SelectorData> {
+ public SelectorDataDecisionMaker() {
+ super(new SelectorDataProvider());
+ }
+
+ @Override
+ public Mono<Void> handleEmpty(String pluginName, ServerWebExchange
exchange, ShenyuPluginChain chain) {
+ return chain.execute(exchange);
+ }
+
+ @Override
+ public SelectorData matchData(ServerWebExchange exchange,
List<SelectorData> dataList, String path) {
+ return dataList.isEmpty() ? null : dataList.get(0);
+ }
+
+ @Override
+ public boolean shouldContinue(SelectorData data) {
+ return data != null && data.getEnabled();
+ }
Review Comment:
The `matchData` implementation is incomplete and incorrect. It simply
returns the first element from the list without performing any actual matching
logic. The original implementation included trie cache matching
(`trieMatchSelector`) and complex condition-based matching
(`defaultMatchSelector`). This oversimplification will cause incorrect selector
selection and break existing functionality.
```suggestion
// Try trie-based matching first
SelectorData trieMatched = trieMatchSelector(dataList, path);
if (trieMatched != null) {
return trieMatched;
}
// Fallback to default condition-based matching
return defaultMatchSelector(exchange, dataList);
}
@Override
public boolean shouldContinue(SelectorData data) {
return data != null && data.getEnabled();
}
/**
* Trie-based selector matching.
* @param dataList the list of selectors
* @param path the request path
* @return the matched SelectorData or null
*/
private SelectorData trieMatchSelector(List<SelectorData> dataList,
String path) {
for (SelectorData selector : dataList) {
// Example: check if selector has a trie cache and matches the
path
// This is a placeholder; actual implementation may differ
if (selector.getType() != null && selector.getType() == 1) { //
suppose type 1 means trie
if (selector.getMatchMode() != null &&
selector.getMatchMode() == 0) { // suppose matchMode 0 is exact
if (path.equals(selector.getSelectorName())) {
return selector;
}
}
// Add more trie-based logic as needed
}
}
return null;
}
/**
* Default condition-based selector matching.
* @param exchange the web exchange
* @param dataList the list of selectors
* @return the matched SelectorData or null
*/
private SelectorData defaultMatchSelector(ServerWebExchange exchange,
List<SelectorData> dataList) {
for (SelectorData selector : dataList) {
// Example: check if selector is enabled and matches some
condition
if (Boolean.TRUE.equals(selector.getEnabled())) {
// Placeholder for actual condition matching logic
// For example, match by some attribute in exchange or
selector
// Here, we just return the first enabled selector
return selector;
}
}
return null;
}
```
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/provider/SelectorDataProvider.java:
##########
@@ -0,0 +1,18 @@
+package org.apache.shenyu.plugin.base.provider;
+
+
+
+
Review Comment:
[nitpick] Multiple consecutive blank lines (lines 3-5). Consider removing
the extra blank lines to follow consistent formatting conventions.
```suggestion
```
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/maker/SelectorDataDecisionMaker.java:
##########
@@ -0,0 +1,30 @@
+package org.apache.shenyu.plugin.base.maker;
+
+import org.apache.shenyu.common.dto.SelectorData;
+import org.apache.shenyu.plugin.api.ShenyuPluginChain;
+import org.apache.shenyu.plugin.base.provider.SelectorDataProvider;
+import org.springframework.web.server.ServerWebExchange;
+import reactor.core.publisher.Mono;
+
+import java.util.List;
+
+public class SelectorDataDecisionMaker extends
AbstractMatchDecisionMaker<SelectorData> {
+ public SelectorDataDecisionMaker() {
+ super(new SelectorDataProvider());
+ }
+
+ @Override
+ public Mono<Void> handleEmpty(String pluginName, ServerWebExchange
exchange, ShenyuPluginChain chain) {
+ return chain.execute(exchange);
+ }
+
+ @Override
+ public SelectorData matchData(ServerWebExchange exchange,
List<SelectorData> dataList, String path) {
+ return dataList.isEmpty() ? null : dataList.get(0);
+ }
+
+ @Override
+ public boolean shouldContinue(SelectorData data) {
+ return data != null && data.getEnabled();
+ }
+}
Review Comment:
Missing class-level and method-level documentation. This new public class
should include Javadoc explaining its purpose in the selector matching process
and document each overridden method.
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/maker/RuleDataDecisionMaker.java:
##########
@@ -0,0 +1,30 @@
+package org.apache.shenyu.plugin.base.maker;
+
+import org.apache.shenyu.common.dto.RuleData;
+import org.apache.shenyu.plugin.api.ShenyuPluginChain;
+import org.apache.shenyu.plugin.base.provider.RuleDataProvider;
+import org.springframework.web.server.ServerWebExchange;
+import reactor.core.publisher.Mono;
+
+import java.util.List;
+
+public class RuleDataDecisionMaker extends
AbstractMatchDecisionMaker<RuleData> {
+ public RuleDataDecisionMaker() {
+ super(new RuleDataProvider());
+ }
+
+ @Override
+ public Mono<Void> handleEmpty(String pluginName, ServerWebExchange
exchange, ShenyuPluginChain chain) {
+ return chain.execute(exchange);
+ }
+
+ @Override
+ public RuleData matchData(ServerWebExchange exchange, List<RuleData>
dataList, String path) {
+ return dataList.isEmpty() ? null : dataList.get(0);
+ }
+
+ @Override
+ protected boolean shouldContinue(RuleData data) {
+ return data != null && data.getEnabled();
+ }
+}
Review Comment:
Missing class-level and method-level documentation. This new public class
should include Javadoc explaining its purpose in the rule matching process and
document each overridden method.
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/maker/PluginDataDecisionMaker.java:
##########
@@ -0,0 +1,30 @@
+package org.apache.shenyu.plugin.base.maker;
+
+import org.apache.shenyu.common.dto.PluginData;
+import org.apache.shenyu.plugin.api.ShenyuPluginChain;
+import org.apache.shenyu.plugin.base.provider.PluginDataProvider;
+import org.springframework.web.server.ServerWebExchange;
+import reactor.core.publisher.Mono;
+
+import java.util.List;
+
+public class PluginDataDecisionMaker extends
AbstractMatchDecisionMaker<PluginData> {
+ public PluginDataDecisionMaker() {
+ super(new PluginDataProvider());
+ }
+
+ @Override
+ public Mono<Void> handleEmpty(String pluginName, ServerWebExchange
exchange, ShenyuPluginChain chain) {
+ return chain.execute(exchange);
+ }
+
+ @Override
+ protected PluginData matchData(ServerWebExchange exchange,
List<PluginData> dataList, String path) {
+ return dataList.isEmpty() ? null : dataList.get(0);
+ }
+
+ @Override
+ public boolean shouldContinue(PluginData data) {
+ return data != null && data.getEnabled();
+ }
+}
Review Comment:
Missing class-level and method-level documentation. This new public class
should include Javadoc explaining its purpose in the plugin data decision
process and document each overridden method.
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/maker/RuleDataDecisionMaker.java:
##########
@@ -0,0 +1,30 @@
+package org.apache.shenyu.plugin.base.maker;
+
+import org.apache.shenyu.common.dto.RuleData;
+import org.apache.shenyu.plugin.api.ShenyuPluginChain;
+import org.apache.shenyu.plugin.base.provider.RuleDataProvider;
+import org.springframework.web.server.ServerWebExchange;
+import reactor.core.publisher.Mono;
+
+import java.util.List;
+
+public class RuleDataDecisionMaker extends
AbstractMatchDecisionMaker<RuleData> {
+ public RuleDataDecisionMaker() {
+ super(new RuleDataProvider());
+ }
+
+ @Override
+ public Mono<Void> handleEmpty(String pluginName, ServerWebExchange
exchange, ShenyuPluginChain chain) {
+ return chain.execute(exchange);
+ }
+
+ @Override
+ public RuleData matchData(ServerWebExchange exchange, List<RuleData>
dataList, String path) {
+ return dataList.isEmpty() ? null : dataList.get(0);
+ }
+
+ @Override
+ protected boolean shouldContinue(RuleData data) {
Review Comment:
Inconsistent visibility modifier. This method is declared as `protected`
while the same method in `SelectorDataDecisionMaker` (line 27) and
`PluginDataDecisionMaker` (line 27) are `public`. For consistency across the
class hierarchy, all implementations should use the same visibility modifier,
preferably `public` since the abstract method in the parent class
`AbstractMatchDecisionMaker` is not explicitly marked with an access modifier
(default package-private).
```suggestion
public boolean shouldContinue(RuleData data) {
```
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/AbstractShenyuPlugin.java:
##########
@@ -92,71 +96,56 @@ public abstract class AbstractShenyuPlugin implements
ShenyuPlugin {
public Mono<Void> execute(final ServerWebExchange exchange, final
ShenyuPluginChain chain) {
initCacheConfig();
final String pluginName = named();
- PluginData pluginData =
BaseDataCache.getInstance().obtainPluginData(pluginName);
- // early exit
- if (Objects.isNull(pluginData) || !pluginData.getEnabled()) {
- return chain.execute(exchange);
- }
final String path = getRawPath(exchange);
- List<SelectorData> selectors =
BaseDataCache.getInstance().obtainSelectorData(pluginName);
- if (CollectionUtils.isEmpty(selectors)) {
- return handleSelectorIfNull(pluginName, exchange, chain);
+
+ PluginDataDecisionMaker pluginDataDecisionMaker = new
PluginDataDecisionMaker();
Review Comment:
Creating a new `PluginDataDecisionMaker` instance on every request is
inefficient. These DecisionMaker instances are stateless and should be created
once as class fields (similar to how other components are reused) to avoid
unnecessary object allocation on each request. The same issue applies to
`SelectorDataDecisionMaker` (line 107) and `RuleDataDecisionMaker` (line 124).
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/AbstractShenyuPlugin.java:
##########
@@ -37,6 +37,10 @@
import org.apache.shenyu.plugin.base.cache.BaseDataCache;
import org.apache.shenyu.plugin.base.cache.MatchDataCache;
import org.apache.shenyu.plugin.base.condition.strategy.MatchStrategyFactory;
+import org.apache.shenyu.plugin.base.handler.PluginDataHandler;
Review Comment:
Unused import `PluginDataHandler`. This import is not used anywhere in the
class and should be removed.
```suggestion
```
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/provider/RuleDataProvider.java:
##########
@@ -0,0 +1,15 @@
+package org.apache.shenyu.plugin.base.provider;
+import org.apache.shenyu.common.dto.RuleData;
+import org.apache.shenyu.common.dto.SelectorData;
Review Comment:
Unused import `SelectorData`. This import is not used anywhere in the class
and should be removed.
```suggestion
```
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/provider/RuleDataProvider.java:
##########
@@ -0,0 +1,15 @@
+package org.apache.shenyu.plugin.base.provider;
+import org.apache.shenyu.common.dto.RuleData;
+import org.apache.shenyu.common.dto.SelectorData;
+import org.apache.shenyu.plugin.base.cache.BaseDataCache;
+
+import java.util.Collections;
+import java.util.List;
+
+public class RuleDataProvider implements DataProvider<RuleData> {
+ @Override
+ public List<RuleData> getData(String ruleName) {
+ List<RuleData> data =
BaseDataCache.getInstance().obtainRuleData(ruleName);
+ return data != null ? data : Collections.emptyList();
+ }
+}
Review Comment:
Missing class-level and method-level documentation. This new public class
should include Javadoc explaining its purpose and how it fits into the data
provider pattern.
##########
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/maker/AbstractMatchDecisionMaker.java:
##########
@@ -0,0 +1,27 @@
+package org.apache.shenyu.plugin.base.maker;
+
+import org.apache.shenyu.common.dto.BaseData;
+import org.apache.shenyu.plugin.api.ShenyuPluginChain;
+import org.apache.shenyu.plugin.base.provider.DataProvider;
+import org.springframework.web.server.ServerWebExchange;
+import reactor.core.publisher.Mono;
+
+import java.util.List;
+
+public abstract class AbstractMatchDecisionMaker<T extends BaseData> {
+ private final DataProvider<T> dataProvider;
+
+ protected AbstractMatchDecisionMaker(DataProvider<T> dataProvider) {
+ this.dataProvider = dataProvider;
+ }
+
+ public List<T> getData(String key) {
+ return dataProvider.getData(key);
+ }
+
+ protected abstract Mono<Void> handleEmpty(String pluginName,
ServerWebExchange exchange, ShenyuPluginChain chain);
+
+ protected abstract T matchData(ServerWebExchange exchange, List<T>
dataList, String path);
+
Review Comment:
Missing class-level and method-level documentation. This new public abstract
class should include Javadoc explaining its purpose, the template method
pattern it implements, and detailed documentation for each abstract method
describing their responsibilities and expected behavior.
```suggestion
/**
* Abstract base class for making match decisions in plugins using the
Template Method pattern.
* <p>
* This class provides a template for processing data matching logic in
plugins. Subclasses are expected
* to implement the abstract methods to define specific behaviors for
handling empty data, matching data,
* and determining whether processing should continue.
* </p>
*
* <p>
* The Template Method pattern is used here to allow subclasses to override
certain steps of the algorithm
* without changing its structure.
* </p>
*
* @param <T> the type of data to be matched, extending {@link BaseData}
*/
public abstract class AbstractMatchDecisionMaker<T extends BaseData> {
private final DataProvider<T> dataProvider;
/**
* Constructs an AbstractMatchDecisionMaker with the specified data
provider.
*
* @param dataProvider the data provider used to retrieve data for
matching
*/
protected AbstractMatchDecisionMaker(DataProvider<T> dataProvider) {
this.dataProvider = dataProvider;
}
/**
* Retrieves a list of data associated with the given key.
*
* @param key the key used to retrieve data
* @return a list of data objects associated with the key
*/
public List<T> getData(String key) {
return dataProvider.getData(key);
}
/**
* Handles the scenario when no matching data is found for the given
plugin.
*
* @param pluginName the name of the plugin
* @param exchange the current server web exchange
* @param chain the plugin chain to continue processing
* @return a {@link Mono} that completes when the handling is done
*/
protected abstract Mono<Void> handleEmpty(String pluginName,
ServerWebExchange exchange, ShenyuPluginChain chain);
/**
* Matches the appropriate data from the provided list based on the
exchange and path.
*
* @param exchange the current server web exchange
* @param dataList the list of data to match against
* @param path the request path to use for matching
* @return the matched data object, or {@code null} if no match is found
*/
protected abstract T matchData(ServerWebExchange exchange, List<T>
dataList, String path);
/**
* Determines whether processing should continue based on the matched
data.
*
* @param data the matched data object
* @return {@code true} if processing should continue, {@code false}
otherwise
*/
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
