Copilot commented on code in PR #6305:
URL: https://github.com/apache/shenyu/pull/6305#discussion_r2999016228
##########
shenyu-admin/src/test/java/org/apache/shenyu/admin/service/RuleServiceTest.java:
##########
@@ -295,6 +295,46 @@ public void testSearchByPage() {
2,
Arrays.asList("s1", "s2")
);
+
+ testSearchByPageWithSpecifiedSelectors();
+ testSearchByPageExpandsNamespaceSelectorsWhenSelectorsEmpty();
+ }
+
+ private void testSearchByPageWithSpecifiedSelectors() {
+ Page<RuleDO> emptyPage = new Page<>();
+ PageCondition<RuleQueryCondition> pageCondition = buildPageCondition();
+ RuleQueryCondition condition = pageCondition.getCondition();
+ List<String> userSelectors = new ArrayList<>(Arrays.asList("s1",
"s3"));
+ SelectorDO selectorDO1 = SelectorDO.builder().id("s1").build();
+ SelectorDO selectorDO2 = SelectorDO.builder().id("s2").build();
+ condition.setSelectors(userSelectors);
+
+
given(this.selectorMapper.selectAllByNamespaceId(anyString())).willReturn(Arrays.asList(selectorDO1,
selectorDO2));
+
given(this.ruleMapper.selectByCondition(any(RuleQueryCondition.class))).willReturn(emptyPage);
Review Comment:
In `testSearchByPageWithSpecifiedSelectors`, the test stubs
`selectorMapper.selectAllByNamespaceId(...)` even though the new
behavior/optimization is that this call should not happen when selectors are
already provided. Consider removing the unused stubbing and adding an explicit
`verify(..., never())` to ensure the namespace selector query is not triggered
in this scenario (otherwise a performance regression could slip through while
still passing assertions).
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/RuleServiceImpl.java:
##########
@@ -119,19 +118,15 @@ public PageInfo<RuleVO> searchByPage(final
PageCondition<RuleQueryCondition> pag
RuleQueryCondition condition = pageCondition.getCondition();
doConditionPreProcessing(condition);
condition.init();
- List<String> namespaceSelectors =
Optional.ofNullable(selectorMapper.selectAllByNamespaceId(condition.getNamespaceId()))
- .map(list ->
list.stream().map(SelectorDO::getId).collect(Collectors.toList()))
- .orElse(Collections.emptyList());
- List<String> finalSelectors =
Optional.ofNullable(condition.getSelectors())
- .orElseGet(Collections::emptyList);
-
- if (!namespaceSelectors.isEmpty()) {
- Set<String> selectorSet = new LinkedHashSet<>(finalSelectors);
- selectorSet.addAll(namespaceSelectors);
- finalSelectors = new ArrayList<>(selectorSet);
+ if (CollectionUtils.isEmpty(condition.getSelectors())) {
+ // Only populating selectors from namespace when
condition.getSelectors() is null/empty.
Review Comment:
The inline comment reads a bit awkwardly (“Only populating selectors…”).
Consider rephrasing it (e.g., “Populate selectors from the namespace only when
condition selectors are null/empty”) or removing it since the `if
(CollectionUtils.isEmpty(...))` already documents the intent.
```suggestion
// Populate selectors from the namespace only when condition
selectors are null or empty.
```
--
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]
