You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "btlqql (via GitHub)" <gi...@apache.org> on 2026/09/13 08:57:25 UTC
[PR] fix(instance): persist a cleared admin credential reference [rocketmq-dashboard]
btlqql opened a new pull request, #4276:
URL: https://github.com/apache/rocketmq-dashboard/pull/4276
## What is the purpose of the change
An instance's admin credential reference could not be removed. `InstanceService.updateInstance` maps a blank submitted reference to `null` through `normalizeCredentialRef` (`InstanceService.java:561-563`), so a blank is an explicit clear:
```java
if (!cloudInstance && instance.getAdminCredentialRef() != null) {
updated.setAdminCredentialRef(normalizeCredentialRef(instance.getAdminCredentialRef()));
}
```
The updated instance is then written by `instanceRepository.save(updated)` (`InstanceService.java:607`), and `MybatisPlusInstanceRepository.save` issues `instanceMapper.updateById(entity)` (`MybatisPlusInstanceRepository.java:122`). MyBatis-Plus `updateById` omits null entity fields (`FieldStrategy.NOT_NULL`; there is no `update-strategy` override in `application.yml`), so the null reference never reaches the SET clause: the stored `admin_credential_ref` survived the update while the service returned a VO whose reference was null.
The user-visible effect is the same one described in #3342 for the ACL columns: the edit dialog accepts the clear, the API answers 200 with `adminCredentialRef: null`, the page replaces its row with that response without refetching (`web/src/pages/instance/index.tsx:447-455`), and the reference reappears on the next list load. An operator can therefore set or change an external admin credential reference but never remove it.
The repository is the only layer that can express the clear: `InstanceRepository.save` is called just twice, from `InstanceService.createInstance` (insert) and `InstanceService.updateInstance` (update), so the update branch now assigns `admin_credential_ref` explicitly when the incoming VO carries no reference.
## Brief changelog
- `MybatisPlusInstanceRepository.save`: after the entity update, assign `admin_credential_ref` explicitly when the saved instance has no reference, because `updateById` skips null entity fields.
- `MybatisPlusInstanceRepositoryTest`: two new cases — a null reference clears the column through an explicit assignment; a submitted reference issues no extra assignment.
## Verification
Red first, on the unmodified implementation with only the new tests added:
```
mvn -f server/pom.xml -B -DskipITs -Dtest=MybatisPlusInstanceRepositoryTest -DfailIfNoTests=false test
[ERROR] Tests run: 18, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.140 s <<< FAILURE! -- in org.apache.rocketmq.studio.instance.MybatisPlusInstanceRepositoryTest
[ERROR] MybatisPlusInstanceRepositoryTest.saveShouldClearTheAdminCredentialRefWhenTheUpdateRemovesIt:254
[ERROR] Tests run: 18, Failures: 1, Errors: 0, Skipped: 0
[INFO] BUILD FAILURE
```
(`...:254` is the `verify(instanceMapper).update(isNull(), captor.capture())` assertion — no assignment was issued for the cleared column.)
Green after the change:
```
mvn -f server/pom.xml -B -DskipITs -Dtest=MybatisPlusInstanceRepositoryTest -DfailIfNoTests=false test
[INFO] Tests run: 18, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.199 s -- in org.apache.rocketmq.studio.instance.MybatisPlusInstanceRepositoryTest
[INFO] BUILD SUCCESS
```
Whole backend suite on this branch (`mvn -f server/pom.xml -B -DskipITs test`): `Tests run: 2153, Failures: 5, Errors: 5, Skipped: 0`. I also ran the same command on the unmodified base commit `6c24d2ed`: `Tests run: 2151, Failures: 5, Errors: 5` with the identical 10 failing test ids — 8 need a POSIX `sh` CLI that this Windows machine does not have (`CliAgentProviderTest`, `ClaudeCodeAgentProviderTest`) and 2 are `AuthCorsIntegrationTest.shouldRejectNonAdminMutationBeforeControllerExecution` / `shouldStillRejectAnonymousProtectedRequests`, which also fail on the base and are aligned separately in #4217. No other test moved.
The DB path itself was not exercised end to end here (no MySQL in this environment); the regression test asserts the assignment that the fix issues, in the same style as the accepted ACL fix in #3342.
Fixes #4273
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(instance): persist a cleared admin credential reference [rocketmq-dashboard]
Posted by "lizhimins (via GitHub)" <gi...@apache.org>.
lizhimins closed pull request #4276: fix(instance): persist a cleared admin credential reference
URL: https://github.com/apache/rocketmq-dashboard/pull/4276
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(instance): persist a cleared admin credential reference [rocketmq-dashboard]
Posted by "lizhimins (via GitHub)" <gi...@apache.org>.
lizhimins commented on PR #4276:
URL: https://github.com/apache/rocketmq-dashboard/pull/4276#issuecomment-5694506834
Clearing the admin credential reference in the instance dialog really did leave the old value persisted, and the fix targets the right layer (`normalizeCredentialRef`).
The fix itself is sound and comes with a test, so this is not a quality call. We have stopped
merging one-defect-per-PR patches out of this queue: the review and merge overhead per change is
high, and several of them touch files that other open PRs also touch.
Please regroup your open Studio fixes into one PR per functional domain (for example
instance/cluster persistence, alert rendering, AI tool input validation) and resubmit against the
current `rocketmq-studio` baseline — we will review the consolidated PRs. When you regroup, note
that other authors' patches on the same files are also being closed, so please pick one approach
per file rather than resubmitting the overlap.
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org