apache / apache/rocketmq-dashboard

[Studio][Bug] Clearing an ACL user's cluster bindings (or a rule's actions) is silently dropped on update

Open
#3,357 1 comment 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.