btlqql opened a new pull request, #4713:
URL: https://github.com/apache/rocketmq-dashboard/pull/4713
<!-- Base branch: `rocketmq-studio`, the RocketMQ Studio trunk. -->
### Related issue
No open issue covers this defect: the tracker has no report naming
`InstanceService`'s endpoint,
remark or admin credential reference, so there is nothing to close here.
### Brief Description
`InstanceService` stored three request fields exactly as the request carried
them, while
`rmq_instance` declares:
```sql
endpoint VARCHAR(512) NOT NULL, --
db/schema.sql:83
remark VARCHAR(255), --
db/schema.sql:81
admin_credential_ref VARCHAR(128) COMMENT '...no secret material', --
db/schema.sql:87
```
The `name` field is already validated (`requireInstanceName`,
`InstanceService.java:554-563`, 64
characters - deliberately below its own `VARCHAR(128)`), so a longer value
for the other three
reached MySQL instead of being refused:
- under the default strict mode the write fails and the caller gets the
generic
`GlobalExceptionHandler` 500 rather than the 400 a too-long name gets for
the same request; in
non-strict mode the column is truncated silently, so the stored instance
is not the one that was
sent;
- `updateInstance` rewrites the whole row (`updated = copyOf(existing)`,
then `save(updated)`), so
an over-long value also fails an update that only meant to change the
remark;
- the endpoint was the worst of the three: `requireValidEndpoint` normalises
the value and returns
it, so `"n".repeat(512) + ";namesrv-2:9876"` looked perfectly valid.
One helper now applies the column widths, using the same 400/`InstanceVO
<field> must not exceed N
characters` wording the name check already produces:
```java
private static String requireTextWithin(String value, int maxLength, String
field) {
if (value != null && value.length() > maxLength) {
throw new BusinessException(400, "InstanceVO " + field + " must not
exceed "
+ maxLength + " characters");
}
return value;
}
```
Call sites: `requireValidEndpoint` (checked on the trimmed value, before the
empty-address-segment
check), `createInstance` for the remark (after the vendor switch, so a
remark resolved from the
cloud catalog is covered too), `createApacheInstance` and `updateInstance`
for the admin credential
reference, and `updateInstance` for the remark. Everything that fits today
is stored unchanged, so
no accepted request changes behaviour.
### How Did You Test This Change?
Six tests in `InstanceServiceTest` pin the boundary for create and update of
each field: a value of
exactly 512 (endpoint) / 255 (remark) / 128 (credential reference)
characters is accepted and
stored, one character more is refused with a 400.
Before the fix (red) - the test file on the unmodified `InstanceService`:
```
$ cd server && mvn -B -ntp test
-Dtest='InstanceServiceTest#createInstanceShouldBoundTheEndpointToTheColumnWidthTest+updateInstanceShouldBoundTheEndpointToTheColumnWidthTest+createInstanceShouldBoundTheRemarkToTheColumnWidthTest+updateInstanceShouldBoundTheRemarkToTheColumnWidthTest+createInstanceShouldBoundTheAdminCredentialReferenceToTheColumnWidthTest+updateInstanceShouldBoundTheAdminCredentialReferenceToTheColumnWidthTest'
[ERROR] Tests run: 6, Failures: 6, Errors: 0, Skipped: 0, Time elapsed:
2.665 s <<< FAILURE! -- in
org.apache.rocketmq.studio.instance.InstanceServiceTest
[ERROR]
org.apache.rocketmq.studio.instance.InstanceServiceTest.createInstanceShouldBoundTheEndpointToTheColumnWidthTest
-- Time elapsed: 0.014 s <<< FAILURE!
Expecting code to raise a throwable.
at
org.apache.rocketmq.studio.instance.InstanceServiceTest.createInstanceShouldBoundTheEndpointToTheColumnWidthTest(InstanceServiceTest.java:525)
[ERROR]
org.apache.rocketmq.studio.instance.InstanceServiceTest.updateInstanceShouldBoundTheEndpointToTheColumnWidthTest
-- Time elapsed: 0.015 s <<< FAILURE!
Expecting code to raise a throwable.
at
org.apache.rocketmq.studio.instance.InstanceServiceTest.updateInstanceShouldBoundTheEndpointToTheColumnWidthTest(InstanceServiceTest.java:545)
[ERROR]
org.apache.rocketmq.studio.instance.InstanceServiceTest.createInstanceShouldBoundTheRemarkToTheColumnWidthTest
-- Time elapsed: 0.015 s <<< FAILURE!
Expecting code to raise a throwable.
at
org.apache.rocketmq.studio.instance.InstanceServiceTest.createInstanceShouldBoundTheRemarkToTheColumnWidthTest(InstanceServiceTest.java:561)
[ERROR]
org.apache.rocketmq.studio.instance.InstanceServiceTest.updateInstanceShouldBoundTheRemarkToTheColumnWidthTest
-- Time elapsed: 0.016 s <<< FAILURE!
Expecting code to raise a throwable.
at
org.apache.rocketmq.studio.instance.InstanceServiceTest.updateInstanceShouldBoundTheRemarkToTheColumnWidthTest(InstanceServiceTest.java:582)
[ERROR]
org.apache.rocketmq.studio.instance.InstanceServiceTest.createInstanceShouldBoundTheAdminCredentialReferenceToTheColumnWidthTest
-- Time elapsed: 2.478 s <<< FAILURE!
Expecting code to raise a throwable.
at
org.apache.rocketmq.studio.instance.InstanceServiceTest.createInstanceShouldBoundTheAdminCredentialReferenceToTheColumnWidthTest(InstanceServiceTest.java:597)
[ERROR]
org.apache.rocketmq.studio.instance.InstanceServiceTest.updateInstanceShouldBoundTheAdminCredentialReferenceToTheColumnWidthTest
-- Time elapsed: 0.062 s <<< FAILURE!
Expecting code to raise a throwable.
at
org.apache.rocketmq.studio.instance.InstanceServiceTest.updateInstanceShouldBoundTheAdminCredentialReferenceToTheColumnWidthTest(InstanceServiceTest.java:616)
[ERROR] Tests run: 6, Failures: 6, Errors: 0, Skipped: 0
[INFO] BUILD FAILURE
```
`Expecting code to raise a throwable` is the base behaviour: the call
returned normally and handed
the over-long value to the repository.
After the fix (green) - the whole instance package test set:
```
$ cd server && mvn -B -ntp test
-Dtest='InstanceServiceTest,InstanceResolverTest,InstanceCapabilityServiceTest,MybatisPlusInstanceRepositoryTest,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: 89, Failures: 0, Errors: 0, Skipped: 0 -- in
org.apache.rocketmq.studio.instance.InstanceServiceTest
[INFO] Tests run: 18, Failures: 0, Errors: 0, Skipped: 0 -- in
org.apache.rocketmq.studio.instance.MybatisPlusInstanceRepositoryTest
[INFO] Tests run: 134, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
[INFO] You have 0 Checkstyle violations.
```
The 83 pre-existing `InstanceServiceTest` cases (trim on create, endpoint
segment check, cloud
catalog paths, credential-ref release logic, …) all still pass, so nothing
that fits the columns
behaves differently.
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 these files, in different hunks, so a rebase will be
trivial:
#4660 (`unbridled-41`) changes `finishCloudImport` and adds tests at the end
of
`InstanceServiceTest`; #4477 changes the list/vendor-filter path in
`InstanceService` and the web
page. Neither touches `requireValidEndpoint`, `createInstance`'s remark
handling, or
`updateInstance`.
### 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 new constants are
documented as column widths; no user-visible contract change, since a value
that fits the column is stored exactly as before)
--
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]
