unbridled-41 opened a new pull request, #3342:
URL: https://github.com/apache/rocketmq-dashboard/pull/3342
### Problem / Evidence
Editing an ACL user and clearing the **Associated clusters** tag select (or
clearing a rule's actions through `POST /api/acl/rules/update` with `actions:
[]`) reports success — the UI even shows the cleared list — but the next reload
brings the old values back: the update was never written to the database.
Trigger path (user update): ACL page user modal (clusters `Select
mode="tags" allowClear`, sends `clusters: []`) → `POST /api/acl/users/update` →
`AclService.updateUser` merges `[]` as the explicit new value →
`MybatisPlusAclRepository.replaceUser` → `toUserEntity` maps `[]` to `clusters
= null` via `joinNormalizedCsv` → MyBatis-Plus default `NOT_NULL` update
strategy omits the column from `updateById`, so the old CSV survives.
The regression test fails before the fix with:
```
Wanted but not invoked:
userMapper.update(isNull(), <Capturing argument: UpdateWrapper>);
userMapper.updateById(RmqAclUser(id=1, ..., clusters=null, ...))
```
### Root cause / Fix
`replaceUser`/`replaceRule` rely on `updateById` with a null field for a
"clear" intent, which MyBatis-Plus silently skips. This exact trap is already
known in the same class: `createAndUpdatePlainAccessConfig` assigns
`white_remote_address` explicitly for the cleared case ("MyBatis-Plus omits
null entity fields from updateById"). The user/rule list columns got no such
handling.
Fix: in `replaceUser`, after a successful `updateById`, issue an explicit
`UpdateWrapper.set("clusters", null)` when the incoming VO carries a non-null
list that normalizes to empty. Same for `replaceRule` and `actions`. Null still
means "keep existing", so partial updates are unaffected, and the plain-access
path that intentionally passes `clusters = null` (relying on the skip) is
untouched.
### Priority & scoring
- Impact 34/40 — silent data loss masked as success on a core admin flow (a
user believed to be cluster-unbound keeps its bindings).
- Scope 14/20 — ACL user update (UI-reachable) and rule update
(API-reachable), one repository.
- Reproducibility 20/20 — deterministic; UI-only trigger for the user case.
- Maintenance value 16/20 — follows an existing in-repo workaround pattern;
low-risk addition.
- **PRIORITY = 84, FIX_CONFIDENCE = 85** (≥70/≥80 per the contribution bar).
### Tests
- New `MybatisPlusAclRepositoryTest` cases:
`replaceUserShouldExplicitlyClearClusterBindingsWhenListIsEmpty`,
`replaceUserShouldKeepClusterBindingsWhenNoneProvided`,
`replaceRuleShouldExplicitlyClearActionsWhenListIsEmpty`. The two clear-cases
failed before the fix and pass after; the keep-case pins the
null-keeps-existing semantics.
- `mvn -B -ntp test
-Dtest='MybatisPlusAclRepositoryTest,AclServiceTest,AclControllerTest'` →
22/22, 64/64, 27/27 passing.
- Full `mvn -B -ntp test` on this branch: 2038 tests (pristine baseline 2035
+ 3 new). Failures: `AuthCorsIntegrationTest` ×2 and
`AliyunInstanceProviderTest.getGroupProgressShouldMapLagRowsTest` — identical
to the recorded pristine baseline — plus one load-flaky
`OpenAiCompatibleLlmGatewayTest` case ("AI chat capacity is temporarily
exhausted") that passes 9/9 in isolation and touches no ACL code. Zero new
failures.
### Risk
Low. The extra statement runs only when a non-null list normalizes to empty
(the clear case); every other update path keeps the previous behavior. The
explicit set uses the same `UpdateWrapper` shape as the existing
`white_remote_address` workaround.
--
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]
