Merged
Conversation
📝 WalkthroughWalkthroughMajor version upgrade from v2 to v3 introducing a modular architecture: extracts validation into a separate Changes
Sequence DiagramssequenceDiagram
actor Client
participant Router
participant ParseRequest
participant DecodeRequestBody
participant BindPathParams
participant ValidateRequest
participant Handler
Client->>Router: HTTP Request
Router->>ParseRequest: ParseRequest[T](..., opts, pathParams)
ParseRequest->>DecodeRequestBody: Decode JSON body
alt Body Decode Success
DecodeRequestBody-->>ParseRequest: Decoded T
else Decode Error
DecodeRequestBody-->>ParseRequest: BodyDecodeError
ParseRequest->>Router: Write ProblemDetails (400/413)
Router-->>Client: Error Response
Note over ParseRequest: Return with error
end
ParseRequest->>BindPathParams: Bind path params
alt Binding Success
BindPathParams-->>ParseRequest: Populated T
else Binding Error
BindPathParams-->>ParseRequest: PathParamError
ParseRequest->>Router: Write ProblemDetails (400)
Router-->>Client: Error Response
Note over ParseRequest: Return with error
end
alt Not SkipValidation
ParseRequest->>ValidateRequest: Validate T
alt Validation Success
ValidateRequest-->>ParseRequest: nil
else Validation Failure
ValidateRequest-->>ParseRequest: ProblemDetails
ParseRequest->>Router: Write ProblemDetails (400+)
Router-->>Client: Error Response
Note over ParseRequest: Return with error
end
end
ParseRequest-->>Handler: Valid T, nil error
Handler->>Router: Process request
sequenceDiagram
actor Handler
participant Reply/Respond
participant ResponseBuilder
participant writeResponse
participant Client
Handler->>Reply/Respond: Reply() / Respond(data)
Reply/Respond-->>ResponseBuilder: ReplyBuilder / ResponseBuilder[T]
Handler->>ResponseBuilder: .Status(...).Meta(...).Header(...)
ResponseBuilder-->>Handler: Self (chainable)
Handler->>ResponseBuilder: .Write(w) / .OK(w) / .Created(w, location)
ResponseBuilder->>writeResponse: writeResponse(code, data, problem, meta, headers)
alt Problem Response (status >= 400)
writeResponse->>writeResponse: writeProblemDetail(...)
writeResponse->>Client: Content-Type: application/problem+json
writeResponse->>Client: Write ProblemDetails JSON
else Success Response
writeResponse->>writeResponse: Marshal Response[T]
writeResponse->>Client: Content-Type: application/json
writeResponse->>Client: Write { data, meta } JSON
end
writeResponse-->>Handler: Done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (4)
problem_config.go (1)
30-43: Redundant clone inmergeProblemConfig.
DefaultProblemConfig()already returns a deep clone, so the additionalmerged.Clone()on line 42 performs a second unnecessary deep copy on every merge call.♻️ Proposed simplification
func mergeProblemConfig(config *ProblemConfig) ProblemConfig { merged := DefaultProblemConfig() if config == nil { return merged } if config.BaseURL != "" { - merged.BaseURL = config.BaseURL + merged.BaseURL = strings.TrimRight(config.BaseURL, "/") } for key, value := range config.ErrorTypePaths { - merged.ErrorTypePaths[key] = value + merged.ErrorTypePaths[key] = normalizeProblemPath(value) } - return merged.Clone() + return merged }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@problem_config.go` around lines 30 - 43, The function mergeProblemConfig performs an unnecessary second deep copy by calling merged.Clone() before returning; since DefaultProblemConfig() already returns a deep clone, remove the redundant cloned return and return the merged variable directly. Update mergeProblemConfig (referencing the symbols mergeProblemConfig, DefaultProblemConfig, and merged.Clone) to eliminate the extra Clone call so the function returns merged without an additional deep copy.examples/stdmux/main.go (1)
34-38:StdMuxParamExtractorignoreskeyand only works for/submit/paths.The extractor hard-codes the
/submit/prefix and disregards thekeyargument, so ifParseRequestis ever called with additional path fields, every lookup returns the same suffix. Since this is example code the current behavior is fine for the demo, but a tiny switch onkey(or usingstrings.TrimPrefix) would make the example more instructive and future-proof.♻️ Suggested tweak
-func StdMuxParamExtractor(r *http.Request, key string) string { - // Remove "/submit/" (7 characters) from the URL path to get just the "id" - // Example: /submit/123 -> 123 - return r.URL.Path[len("/submit/"):] // Skip the "/submit/" part -} +func StdMuxParamExtractor(r *http.Request, key string) string { + switch key { + case "id": + return strings.TrimPrefix(r.URL.Path, "/submit/") + } + return "" +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/stdmux/main.go` around lines 34 - 38, StdMuxParamExtractor currently ignores the key parameter and always trims the hard-coded "/submit/" prefix; update StdMuxParamExtractor to use the provided key (or a switch on key) when extracting params — e.g., compute the expected prefix from key (like "/"+key+"/") and use strings.TrimPrefix or conditional logic to return the suffix only when that prefix matches, falling back to r.URL.Path or empty string otherwise; change references inside StdMuxParamExtractor to use the key variable rather than the literal "/submit/"..github/workflows/release.yml (1)
60-68: Gate thegithub-releasejob to tag-triggered events or provide explicittag_namefor manual dispatch.The workflow is triggered by both
pushwithv*tags andworkflow_dispatch. When triggered viaworkflow_dispatchon a non-tag ref, thegithub-releasejob will attempt to create a release against a non-existent tag (sincegithub.ref_namewill be a branch name, not a tag). Add an explicitif: github.ref_type == 'tag'condition to the job, or providetag_nameexplicitly in the action step. TheGITHUB_TOKENshould have sufficient permissions via the workflow'spermissions: contents: writesetting, but consider adding it explicitly for clarity if the release step fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 60 - 68, The github-release job currently runs for workflow_dispatch and push events and will error when dispatching on non-tag refs; update the github-release job (job name "github-release") to only run for tags by adding a job-level condition like if: github.ref_type == 'tag' OR, alternatively, pass an explicit tag to the softprops/action-gh-release step (the step that uses softprops/action-gh-release@v2) by supplying tag_name: ${{ github.ref_name }} so it doesn’t attempt to create a release for a branch; also ensure the workflow permissions include contents: write (or document adding it) so the GITHUB_TOKEN has rights to create releases.validation/playground/validator.go (1)
60-65: Return JSON field names in validation errors.The validation errors currently expose Go struct field names (e.g.,
Name) instead of the JSON field names sent by clients (e.g.,name). Examples likeexamples/restapi/main.goshow structs with both JSON and validation tags where field names differ. Register a JSON tag-name function soValidationErrorDetail.Fieldmatches the request payload structure and improves API error clarity.♻️ Proposed refactor
import ( "errors" "net/http" + "reflect" + "strings" playgroundvalidator "github.com/go-playground/validator/v10" "github.com/rluders/httpsuite/v3" ) @@ func New() *Validator { - return NewWithValidator(playgroundvalidator.New(), nil) + validate := playgroundvalidator.New() + validate.RegisterTagNameFunc(jsonFieldName) + return NewWithValidator(validate, nil) } @@ func NewWithValidator(validate *playgroundvalidator.Validate, problems *httpsuite.ProblemConfig) *Validator { if validate == nil { validate = playgroundvalidator.New() + validate.RegisterTagNameFunc(jsonFieldName) } @@ } + +func jsonFieldName(field reflect.StructField) string { + name := strings.Split(field.Tag.Get("json"), ",")[0] + if name == "-" { + return "" + } + if name != "" { + return name + } + return field.Name +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation/playground/validator.go` around lines 60 - 65, The ValidationErrorDetail currently uses Go struct field names because the validator instance hasn't been told to use JSON tag names; register a tag-name function on the validator (via validator.RegisterTagNameFunc) that returns the JSON tag (strings.Split(f.Tag.Get("json"), ",")[0]) and treats "-" as empty, ensure this registration happens when the validator is constructed (before any validation runs), and then the existing loop that builds errorDetails from validationErrors and httpsuite.ValidationErrorDetail.Field will produce JSON field names instead of Go struct names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/restapi/go.mod`:
- Line 17: Update the vulnerable dependency declaration "golang.org/x/crypto
v0.32.0" in go.mod to v0.45.0 or later; replace the version token for
golang.org/x/crypto with the new version, run `go get
golang.org/x/crypto@v0.45.0` (or newer) and then `go mod tidy` to update go.sum,
and re-run your vulnerability scanner to verify the issue is resolved.
In `@examples/restapi/main.go`:
- Around line 127-142: The offset computation can overflow because page and
page_size come from the query string; ensure you clamp/validate them and perform
the multiplication safely before casting to an index. Specifically, after
reading page and pageSize from readPositiveInt, enforce minimums (page >= 1,
pageSize >= 1) and compute the offset using a wider integer type (e.g., int64)
or check for overflow: if int64(page-1)*int64(pageSize) >= int64(len(users))
then set start = len(users) else set start = (page-1)*pageSize; then compute end
= min(len(users), start+pageSize) and use users[start:end] and NewPageMeta(page,
pageSize, len(users)).
In `@problem_details.go`:
- Around line 7-15: ProblemDetails currently marshals Extensions as a nested
"extensions" object; implement custom JSON marshaling on the ProblemDetails type
so Extensions are emitted as additional top-level members per RFC 9457. Add a
MarshalJSON (and optionally UnmarshalJSON) for ProblemDetails that builds a
map[string]interface{} with the core fields (Type, Title, Status, Detail,
Instance) only when non-empty, then range over p.Extensions and copy each
key/value into that map (overwriting duplicates if needed), and finally
json.Marshal that map; reference the ProblemDetails type and its Extensions
field when making the change.
In `@README.md`:
- Around line 208-213: The README contains absolute local filesystem links to
the examples (e.g., `/home/rluders/Projects/rluders/httpsuite/examples`,
`/home/rluders/Projects/rluders/httpsuite/examples/stdmux/main.go`,
`/home/rluders/Projects/rluders/httpsuite/examples/gorillamux/main.go`,
`/home/rluders/Projects/rluders/httpsuite/examples/chi/main.go`,
`/home/rluders/Projects/rluders/httpsuite/examples/restapi/main.go`); replace
them with repository-relative links like `examples/`, `examples/stdmux/main.go`,
`examples/gorillamux/main.go`, `examples/chi/main.go`, and
`examples/restapi/main.go` so links resolve on GitHub and in clones.
In `@request_bind_test.go`:
- Around line 3-7: Add the "context" import to request_bind_test.go and replace
every call to httptest.NewRequest(...) with
httptest.NewRequestWithContext(context.Background(), ...) used in the test
functions (look for the httptest.NewRequest calls in the file) so each test
request is context-aware; ensure all occurrences are updated (the import list
and all places creating requests).
In `@request_decode.go`:
- Around line 82-91: In the trailing-content check in request_decode.go (the
block that decodes into the local variable trailing using
decoder.Decode(&trailing)), replace the direct comparison err != io.EOF with an
errors.Is(err, io.EOF) check and add handling for *http.MaxBytesError: if
errors.Is(err, io.EOF) is false then if errors.As(err, &maxBytesErr) return a
BodyDecodeError with Kind BodyDecodeErrorBodyTooLarge, otherwise preserve the
existing behavior of returning BodyDecodeErrorMultipleDocuments; this change
should be applied to the trailing Decode() handling near the Decode(&trailing)
call and use the existing BodyDecodeError types and error wrapping patterns
already used for the first Decode() call.
In `@request_internal.go`:
- Around line 48-54: The branch handling BodyDecodeErrorBodyTooLarge currently
returns the generic status variable (400); update this branch so it returns HTTP
413 Payload Too Large instead. Locate the BodyDecodeErrorBodyTooLarge case in
request_internal.go and change the returned status to
http.StatusRequestEntityTooLarge (or the equivalent 413 constant) when calling
NewProblemDetails so the response and status code reflect the MaxBodyBytes
violation.
In `@response_builder.go`:
- Around line 55-63: The Headers merge currently calls b.Header(key, value)
which uses http.Header.Set and therefore overwrites earlier values for the same
header key; update both ReplyBuilder.Headers and ResponseBuilder.Headers to
append values instead of replacing them by using http.Header.Add (or the
builder's equivalent add method) for each value so multi-valued headers (e.g.,
Set-Cookie, Link, Vary, WWW-Authenticate) are preserved.
In `@response_meta.go`:
- Line 5: Remove the `omitempty` JSON tag from fields that can validly be
zero/false so those values are serialized; specifically update the struct tags
for Response[T].Data and CursorMeta.HasNext / CursorMeta.HasPrev to drop
`omitempty` so values like 0, false, and "" are preserved in JSON output, then
run tests or marshal examples to verify the Data and HasNext/HasPrev fields
appear when set to zero/false.
In `@response_write.go`:
- Around line 25-31: The code is returning raw serializer errors to clients via
NewProblemDetails(..., err.Error(), ...); instead replace the client-facing
detail with a generic message (e.g., "An unexpected error occurred") while
logging the original err for diagnostics, then call writeProblemDetail as
before; update the block that creates internalError (NewProblemDetails,
GetProblemTypeURL, internalError) to use the generic detail and ensure you log
err (using the existing logger or standard log) before returning the 500.
- Around line 55-58: The writeProblemDetail path currently applies headers and
calls w.WriteHeader before encoding the ProblemDetails JSON, so encoding
failures (e.g., from ProblemDetails.Extensions) cannot be recovered; change
writeProblemDetail to first marshal/encode the normalized ProblemDetails into a
buffer (mirroring writeResponse), check for encoding errors, and only then call
applyHeaders(w, headers), set the Content-Type and
w.WriteHeader(effectiveStatus), and write the buffered bytes; reference
functions/variables: writeProblemDetail, applyHeaders, writeResponse,
ProblemDetails.Extensions, normalized, effectiveStatus.
In `@validation/playground/go.mod`:
- Line 15: The go.mod entries pinning "golang.org/x/crypto v0.32.0" are
vulnerable; update each require line that references golang.org/x/crypto
(currently "golang.org/x/crypto v0.32.0") to a secure release such as v0.50.0
(or at minimum v0.45.0) so downstream consumers don’t inherit the vulnerable ssh
packages; edit the require statement in each impacted module (the lines
containing "golang.org/x/crypto v0.32.0") to the new version and run go mod tidy
to update go.sum accordingly.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 60-68: The github-release job currently runs for workflow_dispatch
and push events and will error when dispatching on non-tag refs; update the
github-release job (job name "github-release") to only run for tags by adding a
job-level condition like if: github.ref_type == 'tag' OR, alternatively, pass an
explicit tag to the softprops/action-gh-release step (the step that uses
softprops/action-gh-release@v2) by supplying tag_name: ${{ github.ref_name }} so
it doesn’t attempt to create a release for a branch; also ensure the workflow
permissions include contents: write (or document adding it) so the GITHUB_TOKEN
has rights to create releases.
In `@examples/stdmux/main.go`:
- Around line 34-38: StdMuxParamExtractor currently ignores the key parameter
and always trims the hard-coded "/submit/" prefix; update StdMuxParamExtractor
to use the provided key (or a switch on key) when extracting params — e.g.,
compute the expected prefix from key (like "/"+key+"/") and use
strings.TrimPrefix or conditional logic to return the suffix only when that
prefix matches, falling back to r.URL.Path or empty string otherwise; change
references inside StdMuxParamExtractor to use the key variable rather than the
literal "/submit/".
In `@problem_config.go`:
- Around line 30-43: The function mergeProblemConfig performs an unnecessary
second deep copy by calling merged.Clone() before returning; since
DefaultProblemConfig() already returns a deep clone, remove the redundant cloned
return and return the merged variable directly. Update mergeProblemConfig
(referencing the symbols mergeProblemConfig, DefaultProblemConfig, and
merged.Clone) to eliminate the extra Clone call so the function returns merged
without an additional deep copy.
In `@validation/playground/validator.go`:
- Around line 60-65: The ValidationErrorDetail currently uses Go struct field
names because the validator instance hasn't been told to use JSON tag names;
register a tag-name function on the validator (via
validator.RegisterTagNameFunc) that returns the JSON tag
(strings.Split(f.Tag.Get("json"), ",")[0]) and treats "-" as empty, ensure this
registration happens when the validator is constructed (before any validation
runs), and then the existing loop that builds errorDetails from validationErrors
and httpsuite.ValidationErrorDetail.Field will produce JSON field names instead
of Go struct names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c1d7169-dca6-4bad-bca8-c24e1e692c69
⛔ Files ignored due to path filters (6)
examples/chi/go.sumis excluded by!**/*.sumexamples/gorillamux/go.sumis excluded by!**/*.sumexamples/restapi/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumvalidation/playground/go.sumis excluded by!**/*.sum
📒 Files selected for processing (47)
.github/workflows/ci.yml.github/workflows/go.yml.github/workflows/release.ymlREADME.mdexamples/chi/go.modexamples/chi/main.goexamples/gorillamux/go.modexamples/gorillamux/main.goexamples/restapi/go.modexamples/restapi/main.goexamples/stdmux/go.modexamples/stdmux/main.gogo.modproblem_builder.goproblem_builder_test.goproblem_config.goproblem_config_test.goproblem_details.goproblem_details_test.goproblem_helpers.goproblem_helpers_test.gorequest.gorequest_bind.gorequest_bind_test.gorequest_decode.gorequest_decode_test.gorequest_internal.gorequest_test.gorequest_test_helpers_test.gorequest_types.gorequest_validate.gorequest_validate_test.goresponse.goresponse_benchmark_test.goresponse_builder.goresponse_builder_test.goresponse_helpers.goresponse_helpers_test.goresponse_meta.goresponse_meta_test.goresponse_test.goresponse_write.govalidation.govalidation/playground/go.modvalidation/playground/validator.govalidation/playground/validator_test.govalidation_test.go
💤 Files with no reviewable changes (3)
- .github/workflows/go.yml
- validation.go
- validation_test.go
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.
Summary
Verification
Summary by CodeRabbit
Release Notes
New Features
ProblemConfig.Refactor
ParseRequestsignature.Documentation
Chores