Copilot commented on code in PR #6291:
URL: https://github.com/apache/shenyu/pull/6291#discussion_r2791965386
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/RuleServiceImpl.java:
##########
@@ -116,8 +116,13 @@ public void doConditionPreProcessing(final
RuleQueryCondition condition) {
public PageInfo<RuleVO> searchByPage(final
PageCondition<RuleQueryCondition> pageCondition) {
RuleQueryCondition condition = pageCondition.getCondition();
doConditionPreProcessing(condition);
- PageHelper.startPage(pageCondition.getPageNum(),
pageCondition.getPageSize());
condition.init();
+ condition.setSelectors(
+
Optional.ofNullable(selectorMapper.selectAllByNamespaceId(condition.getNamespaceId()))
+ .map(list ->
list.stream().map(SelectorDO::getId).collect(Collectors.toList()))
+ .orElse(null)
+ );
Review Comment:
Populating `condition.selectors` with *all* selector IDs in the namespace
can create a very large `selector_id IN (...)` clause (see `rule-sqlmap.xml`),
which is also redundant with the existing `namespace_id = ...` filter in the
same query. This can hurt query planning/performance and may hit DB parameter
limits for large namespaces. Prefer relying on the `namespace_id` predicate
alone, or change the mapper query to join to `selector` by `namespace_id`
instead of expanding an IN-list.
```suggestion
```
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/RuleServiceImpl.java:
##########
@@ -116,8 +116,13 @@ public void doConditionPreProcessing(final
RuleQueryCondition condition) {
public PageInfo<RuleVO> searchByPage(final
PageCondition<RuleQueryCondition> pageCondition) {
RuleQueryCondition condition = pageCondition.getCondition();
doConditionPreProcessing(condition);
- PageHelper.startPage(pageCondition.getPageNum(),
pageCondition.getPageSize());
condition.init();
+ condition.setSelectors(
+
Optional.ofNullable(selectorMapper.selectAllByNamespaceId(condition.getNamespaceId()))
+ .map(list ->
list.stream().map(SelectorDO::getId).collect(Collectors.toList()))
+ .orElse(null)
+ );
+ PageHelper.startPage(pageCondition.getPageNum(),
pageCondition.getPageSize());
final Page<RuleDO> doList =
CastUtils.cast(ruleMapper.selectByCondition(condition));
Review Comment:
This change modifies the paging search behavior and adds an extra selector
lookup, but there are no accompanying unit tests to cover the new logic/edge
cases (e.g., when selectors are already set, when the namespace has 0
selectors, etc.). Please add/update tests in the existing `RuleServiceTest`
suite to lock in the intended behavior and prevent regressions.
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/RuleServiceImpl.java:
##########
@@ -116,8 +116,13 @@ public void doConditionPreProcessing(final
RuleQueryCondition condition) {
public PageInfo<RuleVO> searchByPage(final
PageCondition<RuleQueryCondition> pageCondition) {
RuleQueryCondition condition = pageCondition.getCondition();
doConditionPreProcessing(condition);
- PageHelper.startPage(pageCondition.getPageNum(),
pageCondition.getPageSize());
condition.init();
+ condition.setSelectors(
+
Optional.ofNullable(selectorMapper.selectAllByNamespaceId(condition.getNamespaceId()))
+ .map(list ->
list.stream().map(SelectorDO::getId).collect(Collectors.toList()))
+ .orElse(null)
+ );
Review Comment:
`searchByPage` now unconditionally overwrites `condition.selectors` with all
selector IDs from the namespace. This breaks callers that intentionally pass a
specific selector filter (e.g. `RuleController#queryRules` sets selectors from
the `selectorId` query param), because the selectorId constraint is lost and
results broaden unexpectedly. Consider only populating selectors from namespace
when `condition.getSelectors()` is null/empty (or otherwise merge rather than
replace).
```suggestion
List<String> namespaceSelectorIds = Optional
.ofNullable(selectorMapper.selectAllByNamespaceId(condition.getNamespaceId()))
.map(list ->
list.stream().map(SelectorDO::getId).collect(Collectors.toList()))
.orElse(null);
if (CollectionUtils.isEmpty(condition.getSelectors())) {
// If no selectors were specified by the caller, use all
selectors from the namespace.
condition.setSelectors(namespaceSelectorIds);
} else if (!CollectionUtils.isEmpty(namespaceSelectorIds)) {
// Merge caller-specified selectors with namespace selectors,
avoiding duplicates.
Set<String> mergedSelectors = new
HashSet<>(condition.getSelectors());
mergedSelectors.addAll(namespaceSelectorIds);
condition.setSelectors(new ArrayList<>(mergedSelectors));
}
```
--
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]
