unbridled-41 opened a new pull request, #4790:
URL: https://github.com/apache/rocketmq-dashboard/pull/4790
Fixes #4789.
## Problem / Evidence
`InstanceService.loadCounts` (instance list) and the `deleteInstance`
managed-resource guard held an already-resolved instance but passed
`String.valueOf(instance.getId())` to the vendor provider. Every provider
resolves that string through `InstanceRepository.findByIdentifier`, which
matches the unique **name** before the numeric-id fallback. Because instance
names are free-form (digits allowed, `requireInstanceName` only
trims/length-checks), an instance named `42` shadows the instance whose primary
key is `42`:
- the list shows the named instance's topic/group counts on the numeric
instance's row (or a misleading "missing cloud binding" 400 across vendors),
- the delete guard reads the wrong instance's counts, so deleting a
non-empty instance can succeed and orphan its managed resources, while a
genuinely empty one can be blocked with another instance's counts.
Line references in the linked issue are as of trunk `4c697f07`; references
below are as of the patched file at this PR's head.
## Root cause / Fix
Callers that already hold the resolved `InstanceVO` must not round-trip
through the ambiguous identifier resolution. Both call sites now pass the
canonical `instance.getName()`, which `findByIdentifier` always resolves to the
instance itself. No provider or repository behaviour changes; the numeric-id
fallback stays for external API callers that address instances by id.
## Priority
PRIORITY 70 / 100 — impact 28 (wrong data on the fleet-wide instance list
plus a destructive-path guard reading the wrong instance's state: delete
succeeds for a non-empty instance and orphans its resources), blast radius 12
(Apache/cloud list counts and the single delete guard; batch delete routes
through the same guard), reproducibility 16 (deterministic once a numeric name
exists — storable through the public create API today), maintenance value 14
(two call sites, one contract, matches the project's own "name is the immutable
external identifier" stance from #2050). FIX_CONFIDENCE 95: minimal and
semantically forced — the resolved object's canonical name cannot shadow
anything.
## Tests
-
`InstanceServiceTest.deleteInstanceShouldResolveResourceCountsByInstanceNameTest`
(new): the guard receives `countTopics("with-topics")` and never
`countTopics("1")`.
-
`InstanceServiceTest.listInstancesShouldResolveResourceCountsByInstanceNameTest`
(new): an instance named `42` with id 3 gets counts via `countTopics("42")`,
never `countTopics("3")`.
- Teeth, measured on this machine: with only the test change on base
`4c697f07` source, both tests fail (stubbed name never called → `Wanted but not
invoked`/zero counts); with the fix restored, `InstanceServiceTest` 91/91.
- Pre-existing tests updated to the canonical-name stubbing contract (11
tests): they stubbed `countTopics("1")` etc., encoding exactly the behaviour
being fixed; assertions and semantics unchanged otherwise.
- Related suites: `ApacheInstanceProviderTest` 11/11,
`AliyunInstanceProviderTest` 35/35, `TencentInstanceProviderTest` 50/50,
`MybatisPlusInstanceRepositoryTest` 20/20. `mvn checkstyle:check` passes.
## Risk
Low. For every well-formed deployment the canonical name and the id string
resolve to the same instance, so behaviour is identical; the change only
removes the ambiguity window. Cloud providers re-resolve by name and read their
own binding — unchanged for non-shadowed names.
--
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]
