apache / apache/rocketmq-dashboard
[Studio][Bug] Clearing an ACL user's cluster bindings (or a rule's actions) is silently dropped on update
- Dominant language
- Java
- Stars
- 1.4k
- Forks
- 683
- Avg merge
- 2d 14h
- Merged PRs (30d)
- 58
Description
## Problem
In the ACL user edit modal, clearing the **Associated clusters** tag select and saving reports success — the row even shows no clusters — but on the next reload the old cluster bindings reappear: the database column was never updated. The API-level twin: `POST /api/acl/rules/update` with `actions: []` retains the old actions while the response reports them removed.
## Evidence / source
- Code inspection of the current `rocketmq-studio` branch (base commit 36126024): `AclService.updateUser` merges an explicit empty list as the new value; `MybatisPlusAclRepository.toUserEntity` maps it to `clusters = null` via `joinNormalizedCsv`; MyBatis-Plus' default `NOT_NULL` update strategy omits null fields from `updateById`, so the old CSV survives while the merged VO returned to the caller reports the cleared list.
- The same class already documents this exact trap for `white_remote_address` ("MyBatis-Plus omits null entity fields from updateById") and assigns that column explicitly — the user/rule list columns got no such handling.
- Red-first Mockito regressions in `MybatisPlusAclRepositoryTest` fail on the unfixed source with `Wanted but not invoked: userMapper.update(isNull(), ...)` while `updateById` carries `clusters=null`: `replaceUserShouldExplicitlyClearClusterBindingsWhenListIsEmpty` and `replaceRuleShouldExplicitlyClearActionsWhenListIsEmpty`.
## Impact
Silent data loss masked as success in a core admin flow: an operator who believes a user is cluster-unbound leaves the old bindings in effect, with no error anywhere. Applies to every Apache-backed instance using the dashboard ACL store.
## Expected behavior / acceptance criteria
- An explicit empty list clears the column (an UPDATE that assigns null), instead of being skipped.
- `null` continues to mean "keep existing" so partial updates and the plain-access path (which intentionally passes `clusters = null` relying on the skip) are unaffected.
- Regression coverage for the user-clusters and rule-actions clear cases, failing on the unfixed code.
## Related work
- Closed #2007 / #2006 addressed concurrent-delete semantics for ACL users, not cleared values.
- Open PR #2928 fixes a different gap in the same file (`whiteRemoteAddress` is never mapped into `toUserEntity` at all); mechanism and column differ.
- Closed #2061 is an earlier fix for a "cleared value resurrects" symptom in a different subsystem (system alerts), caused there by a concurrency race rather than the null-skip.
## PR
PR #3342 (fix plus three regression tests, red-first).
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with AclService.updateUser, MybatisPlusAclRepository.toUserEntity, and MybatisPlusAclRepositoryTest; review the named red-first Mockito regressions for empty user clusters and rule actions. Done means explicit empty lists clear the stored columns, null still preserves existing values, and the regression tests pass; PR #3342 indicates this work is already underway.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend-api-design, databases
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 25/100