Fix: loggion console add rule bug#6291
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts rule paging search logic in shenyu-admin to address an issue when adding rules from the logging console context (per PR title), by changing how rule search conditions are prepared before querying.
Changes:
- Updates
RuleServiceImpl#searchByPageto populateRuleQueryCondition.selectorsfrom all selectors in the given namespace before executing the paged query. - Removes an extraneous blank line in the MyBatis
rule-sqlmap.xmlmapper.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| shenyu-admin/src/main/resources/mappers/rule-sqlmap.xml | Minor formatting cleanup in the rule mapper XML. |
| shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/RuleServiceImpl.java | Changes paging search condition preparation by deriving selector IDs from namespace prior to the paged query. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| condition.setSelectors( | ||
| Optional.ofNullable(selectorMapper.selectAllByNamespaceId(condition.getNamespaceId())) | ||
| .map(list -> list.stream().map(SelectorDO::getId).collect(Collectors.toList())) | ||
| .orElse(null) | ||
| ); |
There was a problem hiding this 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).
| condition.setSelectors( | |
| Optional.ofNullable(selectorMapper.selectAllByNamespaceId(condition.getNamespaceId())) | |
| .map(list -> list.stream().map(SelectorDO::getId).collect(Collectors.toList())) | |
| .orElse(null) | |
| ); | |
| 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)); | |
| } |
| condition.setSelectors( | ||
| Optional.ofNullable(selectorMapper.selectAllByNamespaceId(condition.getNamespaceId())) | ||
| .map(list -> list.stream().map(SelectorDO::getId).collect(Collectors.toList())) | ||
| .orElse(null) | ||
| ); |
There was a problem hiding this 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.
| condition.setSelectors( | |
| Optional.ofNullable(selectorMapper.selectAllByNamespaceId(condition.getNamespaceId())) | |
| .map(list -> list.stream().map(SelectorDO::getId).collect(Collectors.toList())) | |
| .orElse(null) | |
| ); |
| 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)); |
There was a problem hiding this 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.
Fix: loggion console add rule bug
Make sure that:
./mvnw clean install -Dmaven.javadoc.skip=true.