Aias00 commented on PR #7001:
URL: https://github.com/apache/shenyu/pull/7001#issuecomment-5808352212
Follow-up to my review: I have now scanned **all** Java sources on this
branch head, and the set of call sites not updated for the new two-argument
signature is **larger than the three I listed** — it also includes two
production classes, not just tests.
`SelectorMapper#selectByIdSet` / `#deleteByIds` now require `namespaceId`,
but these still pass a single argument:
**Production code**
```
shenyu-admin/.../service/impl/RuleServiceImpl.java:478
Map<String, String> pluginIdMap =
Optional.ofNullable(selectorMapper.selectByIdSet(new
HashSet<>(ruleDOMap.values()...)))...
shenyu-admin/.../service/impl/RuleServiceImpl.java:525
(same pattern)
shenyu-admin/.../AiProxyRealKeyResolver.java:226
java.util.List<SelectorDO> selectors =
selectorMapper.selectByIdSet(missing);
```
**Tests**
```
shenyu-admin/src/test/.../mapper/SelectorMapperTest.java:75
selectorMapper.selectByIdSet(idSet)
shenyu-admin/src/test/.../service/SelectorServiceTest.java:182
given(selectorMapper.selectByIdSet(Stream.of(correctId).collect(Collectors.toSet()))).willReturn(...)
shenyu-admin/src/test/.../service/SelectorServiceTest.java:198
given(selectorMapper.deleteByIds(ids)).willReturn(ids.size());
shenyu-admin/src/test/.../service/RuleServiceTest.java:392
given(this.selectorMapper.selectByIdSet(Sets.newHashSet("456"))).willReturn(...)
shenyu-admin/src/test/.../AiProxyRealKeyResolverTest.java (lines 130, 137,
153, 161, 170, 177, 183, 193, 209, 225, 242)
when(selectorMapper.selectByIdSet(selectorIds)).thenReturn(...)
verify(selectorMapper, times(1)).selectByIdSet(selectorIds);
```
(For clarity: the many other `deleteByIds` hits in that scan —
`PluginMapper`, `RuleMapper`, `TagMapper`, etc. — are **different** mappers
with their own unrelated `deleteByIds(List)` methods and are unaffected. Only
`selectorMapper.*` calls need updating.)
Why this matters beyond compilation: `RuleServiceImpl` and
`AiProxyRealKeyResolver` are real runtime paths. `AiProxyRealKeyResolver`
resolves API keys to selectors and currently has **no** namespace context at
that call site, so it needs a deliberate decision — is it operating in the
default namespace (use `Constants.SYS_DEFAULT_NAMESPACE_ID`), or does it need
the namespace threaded in from its caller? Same question for both
`RuleServiceImpl` sites. Please don't just pass `null` there: with the new SQL
the predicate becomes `namespace_id = NULL`, which matches **no rows** in SQL
(three-valued logic), so those code paths would silently start returning empty
results instead of failing loudly.
Once these are updated consistently — and ideally with a test asserting a
selector in another namespace is neither returned nor deleted — this is ready
to approve. The change itself is the right fix.
--
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]
