fix(api): improve feedback record validation errors#67
Conversation
✱ Stainless preview buildsThis PR will update the
|
WalkthroughThis pull request makes changes across build configuration, documentation, and API error handling. The Makefile adds a new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openapi.yaml (1)
204-215:⚠️ Potential issue | 🟡 MinorUpdate the validation examples to match runtime payloads.
The
validation_errorsamples still usebody.*locations and generic detail text, butinternal/api/validation/validation.gonow emits JSON-tag field names and formatted validation messages. Please refresh these examples so the docs match the API.Suggested doc fix
- location: "body.field_id" + location: "field_id" - location: "body.value_text" + location: "value_text"Also applies to: 618-629
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openapi.yaml` around lines 204 - 215, The OpenAPI validation_error example uses legacy "body.*" locations and generic detail; update the example under the validation_error schema to match the runtime output produced by internal/api/validation/validation.go by using the JSON-tag field names (e.g., "field_id" not "body.field_id"), formatting the detail to a runtime-style summary, and making each errors[] entry use the same structure the validator emits (location: "field_id", message: formatted validation message as produced by the validator, and value: null); update the duplicate block referenced similarly at the other occurrence so both examples mirror the actual JSON emitted by the validation package.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 113-133: The Makefile run target currently backgrounds go run
./cmd/worker which hides worker crashes and triggers checkmake's maxbodylength;
replace this by extracting the orchestration into a small wrapper script that
supervises the worker process (restarting it on unexpected exit and forwarding
INT/TERM/EXIT signals), then runs go run ./cmd/api in the foreground; update the
run target to call river-migrate and then invoke that wrapper (instead of
backgrounding go run ./cmd/worker and duplicating long trap logic), referencing
the existing run target, go run ./cmd/worker and go run ./cmd/api so the wrapper
performs supervision and graceful shutdown while keeping the Makefile short.
---
Outside diff comments:
In `@openapi.yaml`:
- Around line 204-215: The OpenAPI validation_error example uses legacy "body.*"
locations and generic detail; update the example under the validation_error
schema to match the runtime output produced by
internal/api/validation/validation.go by using the JSON-tag field names (e.g.,
"field_id" not "body.field_id"), formatting the detail to a runtime-style
summary, and making each errors[] entry use the same structure the validator
emits (location: "field_id", message: formatted validation message as produced
by the validator, and value: null); update the duplicate block referenced
similarly at the other occurrence so both examples mirror the actual JSON
emitted by the validation package.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1107dea8-012c-43e0-8487-d24b9d048f6e
📒 Files selected for processing (9)
MakefileREADME.mdinternal/api/handlers/feedback_records_handler.gointernal/api/handlers/feedback_records_handler_test.gointernal/api/response/response.gointernal/api/response/response_test.gointernal/api/validation/validation.gointernal/models/feedback_records.goopenapi.yaml
BhagyaAmarasinghe
left a comment
BhagyaAmarasinghe
left a comment
There was a problem hiding this comment.
looks good, only 1 small change
2 participants
What does this PR do?
Fixes ENG-791 and ENG-797.
This PR improves feedback record validation errors and local development startup:
field_typevalues inPOST /v1/feedback-recordsabout:blankproblem types with stable Hub problem type URIsmake runso local development runs River migrations and starts bothhub-apiandhub-workermake run-apifor API-only local runsExample invalid
field_typeresponse:{ "type": "https://hub.formbricks.com/problems/validation-error", "title": "Validation Error", "status": 400, "detail": "field_type has invalid value \"textt\"; must be one of: text, categorical, nps, csat, ces, rating, number, boolean, date", "errors": [ { "location": "field_type", "message": "field_type has invalid value \"textt\"; must be one of: text, categorical, nps, csat, ces, rating, number, boolean, date", "value": "textt" } ] }How should this be tested?
make lintgo test ./internal/... ./pkg/...make -n runGET /healthreturned200POST /v1/feedback-recordswithfield_type: "textt"returned400with detailedvalidation-errorPOST /v1/feedback-recordswithfield_type: "text"returned201Checklist
Required
make buildmake tests(integration tests intests/)make fmtandmake lint; no new warningsgit pull origin mainmigrations/with goose annotations and ranmake migrate-validateAppreciated
make testsor API contract workflow)docs/if changes were necessarymake tests-coveragefor meaningful logic changes