Aias00 commented on code in PR #7289:
URL: https://github.com/apache/shenyu/pull/7289#discussion_r4109933484
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/SelectorServiceImpl.java:
##########
@@ -308,22 +309,54 @@ public int deleteByNamespaceId(final List<String> ids,
final String namespaceId)
* @param selectors selectors
*/
private void unbindDiscovery(final List<SelectorDO> selectors, final
List<PluginDO> pluginDOS) {
- Map<String, String> pluginMap = ListUtil.toMap(pluginDOS,
PluginDO::getId, PluginDO::getName);
+ Map<String, String> pluginMap = new
HashMap<>(ListUtil.toMap(pluginDOS, PluginDO::getId, PluginDO::getName));
+ List<ResolvedDiscovery> resolvedDiscoveries = new ArrayList<>();
+ // Validate the whole batch before deleting any discovery rows or
publishing removal events.
for (SelectorDO selector : selectors) {
DiscoveryHandlerDO discoveryHandlerDO =
discoveryHandlerMapper.selectBySelectorId(selector.getId());
if (Objects.isNull(discoveryHandlerDO)) {
continue;
}
+ DiscoveryDO discoveryDO =
discoveryMapper.selectById(discoveryHandlerDO.getDiscoveryId());
+ String pluginName = null;
+ if (Objects.nonNull(discoveryDO)) {
+ pluginName = pluginMap.get(selector.getPluginId());
+ if (StringUtils.isBlank(pluginName)) {
+ PluginDO pluginDO =
pluginMapper.selectById(selector.getPluginId());
+ pluginName = Objects.isNull(pluginDO) ? null :
pluginDO.getName();
+ }
+ if (StringUtils.isBlank(pluginName)) {
+ pluginName = discoveryDO.getPluginName();
+ }
+ if (StringUtils.isBlank(pluginName)) {
+ throw new IllegalStateException("Cannot delete selector
batch: selector " + selector.getId()
Review Comment:
Request changes: failing fast here is defensible (publishing a removal whose
path cannot be built is exactly what caused #6479), and validating the whole
batch before touching any row is a good structure. The consequence I am worried
about is different: one selector with unresolvable plugin metadata - for
example an orphaned discovery row left behind by an earlier version - now
aborts and rolls back the whole delete, so operators can no longer delete the
healthy selectors in that batch either, and the admin REST layer receives a raw
IllegalStateException rather than a mapped error. Please either degrade
gracefully (skip the unbind for that selector, log ERROR, keep deleting the
rest) or throw the admin-side typed exception that maps to a clean error
response. Also worth mentioning in the PR description so reviewers know the
delete API can now fail.
##########
shenyu-admin-listener/shenyu-admin-listener-api/src/main/java/org/apache/shenyu/admin/listener/AbstractPathDataChangedListener.java:
##########
@@ -97,6 +98,10 @@ public void onProxySelectorChanged(final
List<ProxySelectorData> changed, final
@Override
public void onDiscoveryUpstreamChanged(final List<DiscoverySyncData>
changed, final DataEventTypeEnum eventType) {
for (DiscoverySyncData data : changed) {
+ if (StringUtils.isBlank(data.getPluginName())) {
Review Comment:
Non-blocking: nice safety net. selectorId has the same failure shape -
buildDiscoveryUpstreamPath would render ".../divide/null" for a null id - so
extending this guard to it would keep the two consistent. Perhaps also worth a
counters-friendly WARN that includes the namespaceId, not only the selectorId.
--
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]
