btlqql opened a new pull request, #4716:
URL: https://github.com/apache/rocketmq-dashboard/pull/4716
<!-- Base branch: `rocketmq-studio`, the RocketMQ Studio trunk. -->
### Related issue
No open issue covers this defect: the tracker has no report about a
persisted instance row without a
vendor, so there is nothing to close here.
### Brief Description
`MybatisPlusInstanceRepository.parseVendor`
(`server/src/main/java/org/apache/rocketmq/studio/instance/MybatisPlusInstanceRepository.java:194`)
passed the persisted value straight to `InstanceVendor.valueOf`:
```java
private InstanceVendor parseVendor(Long instanceId, String vendor) {
try {
return InstanceVendor.valueOf(vendor);
} catch (IllegalArgumentException | NullPointerException ex) {
throw invalidPersistedValue(instanceId, "vendor", vendor);
}
}
```
`rmq_instance` declares the column as optional - `vendor VARCHAR(32)` with
no `NOT NULL` and no
default (`server/src/main/resources/db/schema.sql:84`) - so a row written
before the column existed
carries no vendor, and `valueOf(null)` (or `valueOf("")`) turned that into
`BusinessException(500, "Invalid persisted instance vendor for instance 5:
null")`. Because the
mapping runs on every read (`findAll`, `findByType`, `search`,
`findByTypeAndSearch`,
`findByIdentifier`), a single such row failed the entire instance page with
HTTP 500 instead of
listing the instances - and the row could not be repaired through the page
either, since editing it
loads it with the same mapper.
A missing vendor is not a corrupt row everywhere else in the codebase:
- `InstanceService` defaults it to APACHE for the resource counts, the
delete-time check, the list
ordering and the copy helper (`InstanceService.java:185`, `:650`, `:125`,
`:789`);
- `InstanceCapabilityService.java:43` and `InstanceProviderRegistry.java:75`
do the same, and
`InstanceCapabilityServiceTest#getCapabilitiesShouldDefaultLegacyNullVendorToApacheTest`
pins that
rule;
- `toEntity` normalises `null` to `APACHE` on every write
(`MybatisPlusInstanceRepository.java:214`), so the repository's own write
path already guarantees
the column is only ever missing for rows it did not write.
The read path was the only place that treated the same value as an error.
`parseVendor` now returns
`InstanceVendor.APACHE` for a null or blank value, and still fails with the
same 500 for a value
that names no vendor at all (`"UNKNOWN_VENDOR"`), which
`findByIdShouldRejectInvalidPersistedInstanceVendor` keeps covering.
`parseType` is unchanged
(`type` is `NOT NULL` in the schema, so it has no legacy-null case).
### How Did You Test This Change?
Two cases in `MybatisPlusInstanceRepositoryTest`: a NULL vendor read through
`findAll` (the path that
breaks the whole page) and a blank vendor read through `findById`.
Before the fix (red) - the test file on the unmodified repository:
```
$ cd server && mvn -B -ntp test
-Dtest='MybatisPlusInstanceRepositoryTest#findAllShouldDefaultALegacyNullVendorToApacheTest+findByIdShouldDefaultABlankVendorToApacheTest'
[ERROR] Tests run: 2, Failures: 0, Errors: 2, Skipped: 0, Time elapsed:
2.708 s <<< FAILURE! -- in
org.apache.rocketmq.studio.instance.MybatisPlusInstanceRepositoryTest
[ERROR]
org.apache.rocketmq.studio.instance.MybatisPlusInstanceRepositoryTest.findAllShouldDefaultALegacyNullVendorToApacheTest
-- Time elapsed: 2.607 s <<< ERROR!
org.apache.rocketmq.studio.common.exception.BusinessException: Invalid
persisted instance vendor for instance 5: null
at
org.apache.rocketmq.studio.instance.MybatisPlusInstanceRepositoryTest.findAllShouldDefaultALegacyNullVendorToApacheTest(MybatisPlusInstanceRepositoryTest.java:181)
[ERROR]
org.apache.rocketmq.studio.instance.MybatisPlusInstanceRepositoryTest.findByIdShouldDefaultABlankVendorToApacheTest
-- Time elapsed: 0.030 s <<< ERROR!
org.apache.rocketmq.studio.common.exception.BusinessException: Invalid
persisted instance vendor for instance 6:
at
org.apache.rocketmq.studio.instance.MybatisPlusInstanceRepositoryTest.findByIdShouldDefaultABlankVendorToApacheTest(MybatisPlusInstanceRepositoryTest.java:193)
[ERROR] Tests run: 2, Failures: 0, Errors: 2, Skipped: 0
[INFO] BUILD FAILURE
```
Those two lines are the production behaviour: the read did not return the
instance, it threw the
500 that the page shows to the user.
After the fix (green) - the whole instance package test set, including the
untouched
`findByIdShouldRejectInvalidPersistedInstanceVendor` and
`findByIdShouldRejectInvalidPersistedInstanceType` cases:
```
$ cd server && mvn -B -ntp test
-Dtest='MybatisPlusInstanceRepositoryTest,InstanceServiceTest,InstanceResolverTest,InstanceCapabilityServiceTest,InstanceControllerTest,InstanceResourceCountRunnerTest'
[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0 -- in
org.apache.rocketmq.studio.instance.InstanceCapabilityServiceTest
[INFO] Tests run: 12, Failures: 0, Errors: 0, Skipped: 0 -- in
org.apache.rocketmq.studio.instance.InstanceControllerTest
[INFO] Tests run: 6, Failures: 0, Errors: 0, Skipped: 0 -- in
org.apache.rocketmq.studio.instance.InstanceResolverTest
[INFO] Tests run: 6, Failures: 0, Errors: 0, Skipped: 0 -- in
org.apache.rocketmq.studio.instance.InstanceResourceCountRunnerTest
[INFO] Tests run: 83, Failures: 0, Errors: 0, Skipped: 0 -- in
org.apache.rocketmq.studio.instance.InstanceServiceTest
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0 -- in
org.apache.rocketmq.studio.instance.MybatisPlusInstanceRepositoryTest
[INFO] Tests run: 130, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
[INFO] You have 0 Checkstyle violations.
```
Note on the full suite: on a clean `rocketmq-studio` checkout `mvn -B -ntp
test` already reports
`Tests run: 3051, Failures: 6, Errors: 25, Skipped: 4`. The 11 red classes
are the MySQL 8 backed
Spring integration tests (`AuthServiceBootstrapIntegrationTest`,
`AuthServiceConcurrencyIntegrationTest`,
`AuthServiceSessionOverviewIntegrationTest`,
`HealthProbeIntegrationTest`, `QueryHistoryServiceIntegrationTest`,
`NativeAlertEvaluationTransactionTest`,
`NotificationOutboxMapperIntegrationTest`,
`RmqAlertStateMapperIntegrationTest`, `StudioApplicationTest`) plus the
external-CLI ones
(`CliAgentProviderTest`, `ClaudeCodeAgentProviderTest`). None of them are
touched by this change; the
six classes above are the ones this change can affect.
### Overlap with other open pull requests
Two open PRs also touch this file, in unrelated hunks: #4458 (LIKE wildcard
escaping) edits the
`search` / `findByTypeAndSearch` wrappers around lines 66-84, and #4455
edits the file for its cloud
direct-consume feature. This change is confined to `parseVendor` (line 194)
and the two new tests, so
a rebase stays mechanical.
### Checklist
- [x] One coherent change; unrelated modifications are not bundled in
- [x] Commit subject follows Conventional Commits (`fix(instance): …`)
- [x] Tests added or updated for non-trivial changes, test methods named
`...Test`
- [x] New UI text has both Chinese and English entries under `web/src/i18n/`
(no UI text in this change)
- [x] Architecture constraints stay green (`mvn test` runs the ArchUnit
checks)
- [x] New source files carry the ASF license header (no new files)
- [x] Documentation touched where behaviour changed (the default is
documented at the mapping; no API contract change, since the alternative was a
500)
--
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]
