You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shenyu.apache.org by "juicewcode (via GitHub)" <gi...@apache.org> on 2026/09/02 13:36:20 UTC
[PR] fix: validate Shenyu client HTTP registry request bodies (#6713) [shenyu]
juicewcode opened a new pull request, #7026:
URL: https://github.com/apache/shenyu/pull/7026
Fixes #6713
## Summary
- Added `@Valid` to the request bodies of:
- `registerMetadata`
- `registerURI`
- `registerApiDoc`
- `registerDiscoveryConfig`
- `registerMcpTools`
- `offline`
- Added `@NotBlank` and `@NotNull` constraints to required registration DTO fields.
- Added nested validation for `McpToolsRegisterDTO.metaDataRegisterDTO`.
- Updated existing valid request fixtures and added validation-focused tests.
## Test
- Added `ShenyuClientHttpRegistryControllerValidationTest`.
- Covered validation annotations on all six endpoints.
- Covered required-field constraints across metadata, URI, API Doc, Discovery, and MCP DTOs.
- Existing controller test fixtures were updated to satisfy the new constraints.
<!-- Describe your PR here; e.g. Fixes #issueNo -->
<!--
Thank you for proposing a pull request. This template will guide you through the essential steps necessary for a pull request.
-->
Make sure that:
- [x] You have read the [contribution guidelines](https://shenyu.apache.org/community/contributor-guide).
- [x] You submit test cases (unit or integration tests) that back your changes.
- [x] Your local test passed `./mvnw clean install -Dmaven.javadoc.skip=true`.
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: validate Shenyu client HTTP registry request bodies (#6713) [shenyu]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on PR #7026:
URL: https://github.com/apache/shenyu/pull/7026#issuecomment-5808319187
Re-checking the failing `it / build (shenyu-integrated-test-sdk-http)` job attributed to this PR.
On the current head, **every** integration-test job in the run is failing, not just `sdk-http`:
```
build (shenyu-integrated-test-sdk-http) fail
build (shenyu-integrated-test-apache-dubbo) fail
build (shenyu-integrated-test-combination) fail
build (shenyu-integrated-test-grpc) fail
build (shenyu-integrated-test-http) fail
build (shenyu-integrated-test-https) fail
build (shenyu-integrated-test-rewrite) fail
build (shenyu-integrated-test-sdk-apache-dubbo) fail
build (shenyu-integrated-test-spring-cloud) fail
build (shenyu-integrated-test-websocket) fail
```
while the unit-test matrix passes:
```
build (17, ubuntu-latest) pass
build (21, ubuntu-latest) pass
```
A single PR's change breaking all ten integration suites at once, while every unit test passes, is not a plausible failure mode — this pattern points at the shared integration-test environment (image build / compose startup / network) rather than at this diff. For reference, `master` has been showing the same class of intermittent red on `ci` (`Could not find artifact org.apache.shenyu:*:2.7.2-SNAPSHOT in apache.snapshots`), which is infrastructure rather than code.
That said, I don't want to hand-wave: if the sdk-http failure has a distinct root cause, the decisive evidence would be a log line from that job pointing into this PR's code path (a validation error raised by the new `@Valid` constraints during registration). If someone has that log excerpt, please share it and I'll re-evaluate immediately. Absent that, I'm keeping my approval — the validation change itself is sound and covered by unit tests.
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: validate Shenyu client HTTP registry request bodies (#6713) [shenyu]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on PR #7026:
URL: https://github.com/apache/shenyu/pull/7026#issuecomment-5737446579
Heads-up (PMC Aias00): your PR's CI failures are in shared infrastructure checks (`k8s-examples-http`, `e2e-case-logging-rocketmq`, `build-docker-images`) that are currently failing across many unrelated PRs. This looks like a CI-environment issue, not a problem with your change. Please re-run the failed checks; the code change itself appears fine. No code changes needed for these specific failures.
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: validate Shenyu client HTTP registry request bodies (#6713) [shenyu]
Posted by "juicewcode (via GitHub)" <gi...@apache.org>.
juicewcode closed pull request #7026: fix: validate Shenyu client HTTP registry request bodies (#6713)
URL: https://github.com/apache/shenyu/pull/7026
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: validate Shenyu client HTTP registry request bodies (#6713) [shenyu]
Posted by "juicewcode (via GitHub)" <gi...@apache.org>.
juicewcode commented on PR #7026:
URL: https://github.com/apache/shenyu/pull/7026#issuecomment-5529004243
@Aias00
The CI failure is related to the new validation on MetaDataRegisterDTO.host.
The Spring Cloud client currently does not configure host, so the register-metadata request is sent with host=null and rejected by Admin. Consequently, the /springcloud selector is not created, which causes the subsequent discovery registration to fail.
Should I add a host configuration for Spring Cloud? If so, which address should be used in this environment?
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: validate Shenyu client HTTP registry request bodies (#6713) [shenyu]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on PR #7026:
URL: https://github.com/apache/shenyu/pull/7026#issuecomment-6333867484
Cross-PR note, since this changes a documented contract: #7947 (`docs: define client boundaries, compatibility and server evolution`) is in review in the same session, and its `docs/registration-protocol-compatibility.md` currently states the opposite of what this PR introduces.
The three places that will be wrong once this merges:
1. **The "Required" preamble** says "'Required' below means needed to carry out the operation, not necessarily a Jakarta @NotNull annotation **or immediate HTTP rejection**. Several controllers accept a DTO without `@Valid` and downstream failures can occur after acknowledgement." After this PR all six endpoints reject with 400 before publishing.
2. **The metadata table** marks `host` as "supply a valid host/port for consumers that also register upstreams" and says "a missing context path can fall back to appName" — this PR requires `contextPath`, `host` and `ruleName`.
3. **The URI table** does not mark `appName`/`host` as required, and describes the old "`contextPath` **or** `appName`" rule — this PR requires `appName`, `contextPath`, `rpcType` **and** `host` for both `/register-uri` and `/offline`.
I approved #7947 on the basis that it is accurate for the tree today, with the request that these be corrected in the same release as this PR. Flagging it here so the two do not ship in different versions with contradictory guidance — that file exists specifically so independent client implementations can trust it.
Separately, still relevant to this PR: the `RELEASE-NOTES.md` entry for the newly-required fields, and rewriting the description so it no longer describes the `@NotBlank`/`@NotNull`-on-DTOs approach that was replaced.
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: validate Shenyu client HTTP registry request bodies (#6713) [shenyu]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on PR #7026:
URL: https://github.com/apache/shenyu/pull/7026#issuecomment-6117397165
Hi @juicewcode, this PR currently has merge conflicts with the `master` base branch, so CI cannot validate it against the latest code. Please rebase/merge the latest `master` and resolve the conflicts, then push the updated branch. Once the conflicts are cleared, CI will re-run automatically and we can continue the review. Thanks!
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: validate Shenyu client HTTP registry request bodies (#6713) [shenyu]
Posted by "juicewcode (via GitHub)" <gi...@apache.org>.
juicewcode commented on PR #7026:
URL: https://github.com/apache/shenyu/pull/7026#issuecomment-6250784155
Thanks for your review. Added host(super.getHost()) to SpringWebSocketClientEventListener.buildMetaDataDTO, and also covered full mode registration. Extended the WebSocket listener tests to assert that metadata contains the configured host. The targeted module tests pass successfully.
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: validate Shenyu client HTTP registry request bodies (#6713) [shenyu]
Posted by "juicewcode (via GitHub)" <gi...@apache.org>.
juicewcode commented on PR #7026:
URL: https://github.com/apache/shenyu/pull/7026#issuecomment-5528989510
> ## Summary
> Re-review after my previous APPROVE was dismissed (the branch was force-pushed). Re-verified the current head — the change is the same request-body validation feature, now with strengthened test coverage.
>
> ## What is correct
> * Adds `@Valid` to all six mutating Shenyu client HTTP registry endpoints (`registerMetadata`, `registerURI`, `registerApiDoc`, `registerDiscoveryConfig`, `registerMcpTools`, `offline`) and `@NotBlank`/`@NotNull` constraints to the register DTOs in `shenyu-register-common`. `jakarta.validation-api` is added to that module's `pom.xml` so the DTOs (which live there) compile with the constraint annotations.
> * Nested validation is correct: `McpToolsRegisterDTO.metaDataRegisterDTO` is annotated `@NotNull @Valid`, so the nested `MetaDataRegisterDTO` constraints are also enforced.
> * **Runtime enforcement verified (again):** `shenyu-admin/pom.xml` already depends on `spring-boot-starter-validation`, and `ExceptionHandlers.handleMethodArgumentNotValidException` (lines 93-101) converts violations into `ShenyuAdminResult.error` (HTTP 200, error code). So `@Valid` triggers real validation (HTTP 200 + code 500) rather than being a silent no-op.
> * **Test coverage strengthened since the dismissed review:** the new `testRegisterMetadataRejectsInvalidBody` in `ShenyuHttpRegistryControllerTest` posts `{}` and asserts rejection (`jsonPath("$.code").value(500)`) plus `verifyNoInteractions(publisher)` — validating enforcement end-to-end through the real `LocalValidatorFactoryBean`. This is a real functional test, an improvement over the prior declaration-only reflection test.
>
> ## Verdict
> Approve. No blocking issues.
>
> ## Non-blocking suggestions
> * Behavior change: payloads omitting required fields (e.g. `MetaDataRegisterDTO.ruleName`, `ApiDocRegisterDTO.eventType`/`httpMethod`) will now be rejected. This is the intended fix for #6713, but it may reject previously-accepted payloads from older clients.
> * `ShenyuClientHttpRegistryControllerValidationTest` still only asserts annotation _declarations_ via reflection; the functional `ShenyuHttpRegistryControllerTest` case above is the stronger coverage and is sufficient.
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: validate Shenyu client HTTP registry request bodies (#6713) [shenyu]
Posted by "juicewcode (via GitHub)" <gi...@apache.org>.
juicewcode commented on PR #7026:
URL: https://github.com/apache/shenyu/pull/7026#issuecomment-6251767391
@Aias00 Hi! After merging the protocol extraction from #7602, the registration DTOs now live in shenyu-register-protocol, which must remain JDK-only. Adding Jakarta Validation annotations to these DTOs violates that dependency boundary, so I would like to discuss where the HTTP request validation should live.
I see two options:
Option 1: An Admin-side Spring Validator. Keep the protocol DTOs free of validation annotations, implement the required-field checks in Admin, and register the validator through a controller-local @InitBinder. The existing @Valid @RequestBody parameters would trigger validation and return HTTP 400 before publishing invalid registrations. This avoids duplicate DTOs and mapping code, but requires maintaining explicit validation rules.
Option 2: Separate Admin request DTOs with Jakarta Validation annotations. Introduce Admin-owned request DTOs containing the validation annotations, validate them at the controller boundary, and map them to the protocol DTOs before publishing. This keeps annotation-based validation, but duplicates the request fields and adds mapping code that must stay aligned with the wire models. Both options keep shenyu-register-protocol JDK-only and can preserve the current required-field checks, including nested MCP metadata validation.
Would you prefer this approach?
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org