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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
