feat(validation): field-level validation, return 422 on failure#259
feat(validation): field-level validation, return 422 on failure#259nanotaboada merged 1 commit intomasterfrom
Conversation
WalkthroughThis PR implements input validation for player request payloads using Gin's validator framework. It adds binding tags to the Player struct defining required fields and value ranges, modifies controller handlers to distinguish validation errors (422) from JSON parse errors (400), adds comprehensive test coverage for validation scenarios, and regenerates Swagger documentation. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Controller as Player Controller
participant Binder as ShouldBindJSON
participant Validator as go-playground/validator
Client->>Controller: POST/PUT with JSON payload
Controller->>Binder: ShouldBindJSON(&player)
alt Valid JSON, Invalid Fields
Binder->>Validator: Validate struct tags
Validator-->>Binder: ValidationErrors
Binder-->>Controller: validator.ValidationErrors
Controller-->>Client: 422 Unprocessable Entity
else Malformed JSON
Binder-->>Controller: JSON parse error (not ValidationErrors)
Controller-->>Client: 400 Bad Request
else Valid JSON, Valid Fields
Binder-->>Controller: nil error
Controller->>Controller: Process request (POST/PUT logic)
Controller-->>Client: 200/201/404
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ 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 |
90ccdf1 to
b0f32be
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #259 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 130 141 +11
=========================================
+ Hits 130 141 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/player_controller.go`:
- Around line 69-75: The request handler uses context.ShouldBindJSON(&player)
but doesn't enforce the Content-Type header, allowing non-application/json
requests (e.g., text/plain) with JSON bodies to slip through; before calling
context.ShouldBindJSON (in the player binding blocks where
context.ShouldBindJSON(&player) is used) check the request Content-Type (e.g.,
context.GetHeader("Content-Type") or context.ContentType()) and ensure it is
application/json (or starts with "application/json", case-insensitive); if not,
immediately respond with http.StatusBadRequest and do not attempt binding. Apply
the same Content-Type guard to both binding sites (the one at the first
ShouldBindJSON(&player) and the second occurrence around lines 206-213).
In `@tests/main_test.go`:
- Around line 211-242: The test table in tests/main_test.go (the cases slice
built using MakeNonexistentPlayer and modified FirstName/SquadNumber) lacks
wrong-JSON-type payloads; add entries that send incorrect types (e.g., set
squadNumber to the JSON string "23" and set firstName to a non-string like 123)
by building those bodies via json.Marshal of a map or by creating a copy of
MakeNonexistentPlayer and injecting wrong-typed fields, then assert the same
UnprocessableEntity response as for the other validation cases; mirror these
additions in the other similar test suite mentioned (the other cases block) to
ensure type-validation coverage.
- Around line 220-221: The test builders currently call json.Marshal(p) and
discard the error (b, _ := json.Marshal(p)) which hides serialization failures;
update each builder used in the POST and PUT validation tests to handle the
error instead of ignoring it—either return an (string, error) from the builder
or accept *testing.T and call t.Fatalf/t.Fatalf-like helper on marshal error;
update callers accordingly for the six occurrences (the three POST builders
around the snippet with json.Marshal(p) and the three PUT builders later) so
that any json.Marshal failure surfaces and fails the test rather than producing
a silent invalid payload.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c54f8e52-b867-4dad-9ab0-c8643f1dc548
⛔ Files ignored due to path filters (3)
docs/docs.gois excluded by!**/docs/docs.godocs/swagger.jsonis excluded by!**/docs/swagger.jsondocs/swagger.yamlis excluded by!**/docs/swagger.yaml
📒 Files selected for processing (5)
CHANGELOG.mdcontroller/player_controller.gomodel/player_model.gotests/main_test.gotests/player_fake.go



Summary
bindingstruct tags toPlayerfor field-level validation (requiredon 7 string fields,min=1,max=99onsquadNumber,omitemptyonmiddleName,binding:"-"onID)BindJSONwithShouldBindJSONin POST and PUT handlers; useserrors.As(err, &validator.ValidationErrors{})to return422 Unprocessable Entityfor validation failures and400 Bad Requestfor malformed JSON@Failure 422Swagger annotations on POST and PUT; regenerates docsTestRequestPOSTPlayersValidationResponseStatusUnprocessableEntityandTestRequestPUTPlayerBySquadNumberValidationResponseStatusUnprocessableEntity(3 subtests each); updatesMakeUnknownPlayer()to carry all required fieldsCloses #257
Closes #253
Test plan
go test ./...)service,controller,routego vetandgolangci-lintclean🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes