Frun1na opened a new pull request, #6536:
URL: https://github.com/apache/rocketmq-dashboard/pull/6536
### Which Issue(s) This PR Fixes
- Fixes #6535
### Brief Description
#4574 made `rmq.acl.list`/`rmq.acl.get` tolerate an ACL rule that has no
numeric id (Tencent Cloud
roles) and left the update path for a follow-up:
`AclUpdateToolHandler.parseId` still demanded a
number in both `preview` and `execute`
(`handler/acl/AclUpdateToolHandler.java:69-78`), so on such an instance
every `rmq.acl.update` call
failed with `ACL id must be numeric: <role>` before the service was
consulted - while
`AclService.getRule` already resolves the role name
(`id.equals(rule.getPrincipal())`),
`AclService.updateRule`'s Tencent branch never asks for an id
(`TencentAclService.updateRule` maps the principal onto `ModifyRole`), and
the sibling tools
`rmq.acl.get`/`rmq.acl.delete` pass the identifier through and work. The
handler now resolves the
addressed rule through `AclService.getRule` - the service that owns the
"numeric id or principal"
decision - and updates by that rule's id in both `preview` and `execute`.
User-visible change: a
Tencent ACL rule can be updated through the tool again, and a bogus
identifier is now graded by the
service (400 "id must be a numeric value" from `EntityIds.parseId`, 404 for
a missing rule). The
`id` field of `rmq.acl.get`/`rmq.acl.update`/`rmq.acl.delete` in `acl.yaml`
documents both accepted
forms (catalog regenerated), and `ToolError.ACL_ID_INVALID` is removed since
this change leaves it
without a reference (verified: no reference in
`server/src/main/resources/tool-catalog/`, `docs/` or
`rmqctl/`, and the only remaining one was the handler changed here).
Two earlier attempts at this fix, #5115 by @X-LightYear and #5345 by
@yyqdbngt, were closed by the
stale bot before they could be reviewed; this change keeps the same approach
they took - let the
service decide whether an id is required - and carries its own regression
tests.
### How Did You Test This Change?
```
$ mvn -B -ntp test -DforkCount=1
-Dtest='org.apache.rocketmq.studio.ops.ai.**.*Test'
[INFO] --- checkstyle:3.6.0:check (validate) @ rocketmq-studio ---
[INFO] You have 0 Checkstyle violations.
[INFO] Tests run: 954, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
$ make -C rmqctl catalog-verify # after `make -C rmqctl catalog-generate`
go run ./internal/catalog/generate -input-dir
../server/src/main/resources/tool-catalog/tools -output
internal/catalog/catalog_gen.go -check
(no output: the committed catalog matches the shard)
```
The new tests fail without the fix (the product change was reverted, the
tests were kept):
```
$ mvn -B -ntp test -DforkCount=1 -Dtest=AclUpdateToolHandlerTest
[ERROR] Tests run: 4, Failures: 1, Errors: 3, Skipped: 0, Time elapsed:
0.846 s <<< FAILURE! -- in ...AclUpdateToolHandlerTest
...applyShouldUpdateARuleAddressedByPrincipalTest -- ERROR!
org.apache.rocketmq.studio.ops.ai.tool.core.ToolExecutionException: ACL id
must be numeric: GID_role
at ...AclUpdateToolHandler.parseId(AclUpdateToolHandler.java:76)
at ...AclUpdateToolHandler.execute(AclUpdateToolHandler.java:66)
...planShouldPreviewARuleAddressedByPrincipalTest -- ERROR!
org.apache.rocketmq.studio.ops.ai.tool.core.ToolExecutionException: ACL id
must be numeric: GID_role
at ...AclUpdateToolHandler.parseId(AclUpdateToolHandler.java:76)
at ...AclUpdateToolHandler.preview(AclUpdateToolHandler.java:55)
...applyShouldReportAnUnknownRuleFromTheServiceTest -- FAILURE! (Expecting
code to raise a throwable)
```
The two principal cases turn red because `parseId` rejects the role name
before the service is
reached - the assertion that fails is the handler call itself, whose message
names the identifier it
refused; the third turns red because a handler that never consults
`AclService` cannot report a rule
the service does not have. `applyShouldUpdateARuleAddressedByPrincipalTest`
and
`planShouldPreviewARuleAddressedByPrincipalTest` stub
`aclService.getRule("GID_role", "cluster-1")`
the way `AclService` resolves a Tencent rule;
`applyShouldReportAnUnknownRuleFromTheServiceTest` pins
that an unknown identifier is reported by the service instead of a
handler-side numeric check. The
existing `applyShouldUpdateRule` gained the `getRule("1", "cluster-1")` stub
the new resolution needs
(its assertions are unchanged); `AclMutationPlanTest` is unchanged and green.
### Checklist
- [x] One coherent change; unrelated modifications are not bundled in
- [x] Commit subject follows Conventional Commits (`feat:` / `fix:` /
`refactor:` / `chore:` / `docs:` / `perf:`)
- [x] Tests added or updated for non-trivial changes, test methods named
`...Test`
- [ ] New UI text has both Chinese and English entries under `web/src/i18n/`
- [ ] Architecture constraints stay green (`mvn test` runs the ArchUnit
checks)
- [ ] New source files carry the ASF license header
- [ ] Documentation touched where behaviour changed (README / `docs/` /
in-app help)
--
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]
