fix: bind only the request body in echo v5 strict handlers#2444
Merged
Conversation
The echo v5 strict handler template used ctx.Bind, which binds path values (and, for some methods, query parameters) into the request body struct before binding the body itself, so a path parameter sharing a name with a body field would overwrite it. Generated code now type-switches on the binder installed on the Echo instance: with the default binder, only the body is bound via the package-level echo.BindBody; a custom binder is deferred to entirely, since the echo.Binder interface does not expose body-only binding. Strict mode for echo v5 previously had no test coverage, so this also adds internal/test/servers/strict/echo5, mirroring the echo v4 strict server tests against the shared strict-schema spec. This is a follow-up to PR #2063, which fixed the same problem for echo v4, and extends the fix for issue #1757 to echo v5.
Contributor
Greptile SummaryThis PR fixes issue #1757 for the echo v5 strict server backend:
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to the echo v5 strict template and adds test infrastructure; no existing generated API surfaces are altered. The template change is a direct, well-understood parallel to the already-merged echo v4 fix. The io.EOF guard for optional bodies is present, all backends implement the new endpoint, the regression test correctly targets the overwrite scenario, and generated file diffs are proportional to the schema addition. No files require special attention.
|
| Filename | Overview |
|---|---|
| pkg/codegen/templates/strict/strict-echo5.tmpl | Core fix: replaces ctx.Bind with echo.BindBody (package-level, body-only) for the DefaultBinder path, and adds the io.EOF guard for optional JSON bodies — aligns with the echo v4 template's approach |
| internal/test/servers/strict/strict-schema.yaml | Adds /same-name-param-and-body-property/{name} endpoint with a sameName schema that carries a param:"name" extra tag — exactly the construction that triggers the overwrite bug under echo's full Bind |
| internal/test/servers/strict/strict_test.go | Adds TestEcho5Server wiring and a SameNameParamAndBodyProperty regression sub-test (shared across all backends via testImpl) verifying that path-param "path-value" is not copied into the body's Name field |
| internal/test/servers/strict/echo5/server.gen.go | New generated file for the echo v5 strict test server; SameNameParamAndBodyProperty correctly uses echo.BindBody + io.EOF guard for the optional body; proportional to the schema addition |
| internal/test/servers/strict/echo5/server.go | New echo v5 strict-server test implementation; mirrors other backends' server.go; SameNameParamAndBodyProperty echoes the body back, correctly returning an empty response when Body is nil |
| internal/test/servers/strict/chi/server.gen.go | Regenerated to add SameNameParamAndBodyProperty; change is proportional and consistent with the spec addition, no unrelated drift detected |
| internal/test/servers/strict/client/client.gen.go | Adds SameName type and SameNameParamAndBodyProperty client methods; change is additive and consistent with the new schema endpoint |
Reviews (2): Last reviewed commit: "fix: bring echo v5 optional-body handlin..." | Re-trigger Greptile
…add issue #1757 regression test Two review follow-ups: Add the optional-body guard from the echo v4 strict template to the echo v5 template, so a binding error caused by an empty body (errors.Is(err, io.EOF), which unwraps through echo v5's ErrBadRequest.Wrap) is tolerated when the request body is not required, leaving the body pointer nil instead of failing the request. Add a targeted regression test for issue #1757 to the shared strict server test suite: a new endpoint whose path parameter shares its name with a request body property, exercised by testImpl across every framework. The body property carries a param tag via x-oapi-codegen-extra-tags, which is what makes Echo's full Bind copy a same-named path parameter into the body struct — Echo's binder skips fields without param/query/form tags, so plain generated structs need an extra tag to be affected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The echo v5 strict handler template used ctx.Bind, which binds path values (and, for some methods, query parameters) into the request body struct before binding the body itself, so a path parameter sharing a name with a body field would overwrite it.
Generated code now type-switches on the binder installed on the Echo instance: with the default binder, only the body is bound via the package-level echo.BindBody; a custom binder is deferred to entirely, since the echo.Binder interface does not expose body-only binding.
Strict mode for echo v5 previously had no test coverage, so this also adds internal/test/servers/strict/echo5, mirroring the echo v4 strict server tests against the shared strict-schema spec.
This is a follow-up to PR #2063, which fixed the same problem for echo v4, and extends the fix for issue #1757 to echo v5.