Skip to content

Commit 2fe37d9

Browse files
authored
fix(api): improve feedback record validation errors (#67)
* chore: address makefile, and validation error from qa finding * fix: address qa review findings * chore: fix pr findings * chore: fix test
1 parent 30895da commit 2fe37d9

11 files changed

Lines changed: 501 additions & 45 deletions

File tree

Makefile

Lines changed: 73 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
.PHONY: all test help tests tests-coverage check-coverage build build-api build-worker build-backfill-embeddings run run-worker run-backfill-embeddings init-db clean docker-up docker-down docker-clean deps install-tools fmt lint lint-new lint-openapi dev-setup test-all test-unit schemathesis install-hooks migrate-status migrate-validate river-migrate
1+
.PHONY: all test help tests tests-coverage check-coverage build build-api build-worker build-backfill-embeddings run run-api run-worker run-backfill-embeddings init-db clean docker-up docker-down docker-clean deps install-tools fmt lint lint-new lint-openapi dev-setup test-all test-unit schemathesis install-hooks migrate-status migrate-validate river-migrate
22

33
# Aliases for checkmake/lint expectations
44
all: build
@@ -13,8 +13,9 @@ help:
1313
@echo " make build-api - Build hub-api only (bin/hub-api)"
1414
@echo " make build-worker - Build hub-worker only (bin/hub-worker)"
1515
@echo " make build-backfill-embeddings - Build the backfill-embeddings command"
16-
@echo " make run - Run the API server (hub-api)"
17-
@echo " make run-worker - Run the worker (hub-worker)"
16+
@echo " make run - Run River migrations, then hub-api and hub-worker"
17+
@echo " make run-api - Run the API server only (hub-api)"
18+
@echo " make run-worker - Run the worker only (hub-worker)"
1819
@echo " make run-backfill-embeddings - Run the backfill-embeddings command (enqueues embedding jobs; loads .env)"
1920
@echo " make test-unit - Run unit tests (fast, no database)"
2021
@echo " make tests - Run integration tests"
@@ -109,13 +110,78 @@ run-backfill-embeddings:
109110
@if [ ! -f .env ]; then echo "Error: .env file required. Copy .env.example to .env and configure."; exit 1; fi && \
110111
(set -a && . ./.env && set +a && go run ./cmd/backfill-embeddings)
111112

112-
# Run the API server (hub-api).
113+
define RUN_LOCAL_APP
114+
set -Eeuo pipefail
115+
worker_pid=""
116+
api_pid=""
117+
started_pid=""
118+
state_dir="$$(mktemp -d "$${TMPDIR:-/tmp}/hub-run.XXXXXX")"
119+
event_pipe="$$state_dir/process-events"
120+
mkfifo "$$event_pipe"
121+
stop_process() {
122+
pid="$${1:-}"
123+
name="$$2"
124+
if [ -z "$$pid" ] || ! kill -0 "$$pid" 2>/dev/null; then return; fi
125+
echo "Stopping $$name..."
126+
pkill -TERM -P "$$pid" 2>/dev/null || true
127+
kill -TERM "$$pid" 2>/dev/null || true
128+
wait "$$pid" 2>/dev/null || true
129+
}
130+
cleanup() {
131+
status=$$?
132+
trap - INT TERM EXIT
133+
stop_process "$$api_pid" "hub-api"
134+
stop_process "$$worker_pid" "hub-worker"
135+
rm -rf "$$state_dir"
136+
exit "$$status"
137+
}
138+
run_and_report() {
139+
name="$$1"
140+
shift
141+
(set +e; "$$@"; status=$$?; printf '%s %s\n' "$$name" "$$status" >"$$event_pipe" || true; exit "$$status") &
142+
started_pid=$$!
143+
}
144+
trap cleanup EXIT
145+
trap 'echo "Stopping hub-api and hub-worker..."; exit 130' INT
146+
trap 'echo "Stopping hub-api and hub-worker..."; exit 143' TERM
147+
echo "Starting hub-worker..."
148+
run_and_report hub-worker go run ./cmd/worker
149+
worker_pid="$$started_pid"
150+
echo "Starting hub-api..."
151+
run_and_report hub-api go run ./cmd/api
152+
api_pid="$$started_pid"
153+
read -r exited_process exit_status <"$$event_pipe"
154+
case "$$exited_process" in
155+
hub-worker)
156+
echo "hub-worker exited unexpectedly with status $$exit_status; stopping hub-api."
157+
stop_process "$$api_pid" "hub-api"
158+
if [ "$$exit_status" -eq 0 ]; then exit 1; fi
159+
exit "$$exit_status"
160+
;;
161+
hub-api)
162+
echo "hub-api exited with status $$exit_status; stopping hub-worker."
163+
stop_process "$$worker_pid" "hub-worker"
164+
exit "$$exit_status"
165+
;;
166+
*)
167+
echo "Unknown process event from local runner: $$exited_process" >&2
168+
exit 1
169+
;;
170+
esac
171+
endef
172+
export RUN_LOCAL_APP
173+
174+
# Run the full local app: apply River migrations, then supervise hub-worker and hub-api together.
113175
# Config: .env if present, else environment variables; env vars override .env. Copy .env.example to .env or set env vars.
114-
run:
176+
run: river-migrate
177+
@bash -c "$$RUN_LOCAL_APP"
178+
179+
# Run the API server only (hub-api).
180+
run-api:
115181
@echo "Starting hub-api..."
116182
go run ./cmd/api
117183

118-
# Run the worker (hub-worker). Config: .env if present, else env vars (same as run). Requires DATABASE_URL; API_KEY not required.
184+
# Run the worker only (hub-worker). Config: .env if present, else env vars (same as run). Requires DATABASE_URL; API_KEY not required.
119185
run-worker:
120186
@echo "Starting hub-worker..."
121187
go run ./cmd/worker
@@ -258,7 +324,7 @@ install-hooks:
258324
dev-setup: docker-up deps install-tools init-db install-hooks
259325
@echo "Development environment ready!"
260326
@echo "Set API_KEY environment variable for authentication"
261-
@echo "Run 'make run' to start the API server"
327+
@echo "Run 'make run' to apply River migrations and start hub-api with hub-worker"
262328

263329
# Run Schemathesis API tests (all phases for thorough local testing)
264330
# Phases: examples (schema examples), coverage (boundary values), stateful (API sequences), fuzzing (random)

README.md

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -92,14 +92,13 @@ For a local Hub setup:
9292
```bash
9393
cp .env.example .env
9494
make dev-setup
95-
make river-migrate
9695
make run
9796
```
9897

99-
Run `make run-worker` in a separate terminal when you need webhook delivery or
100-
embedding workers. `make dev-setup` runs the Hub schema migrations through
101-
`make init-db`; `make river-migrate` applies River queue migrations needed by
102-
worker-backed jobs.
98+
`make run` applies River queue migrations and starts both `hub-api` and
99+
`hub-worker` for local webhook delivery and embedding jobs. Use `make run-api`
100+
or `make run-worker` only when you intentionally want to run one process on its
101+
own. `make dev-setup` runs the Hub schema migrations through `make init-db`.
103102

104103
For the full Formbricks XM Suite UI, use the
105104
[`formbricks/formbricks`](https://github.com/formbricks/formbricks) repository

internal/api/handlers/feedback_records_handler.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ func (h *FeedbackRecordsHandler) Create(w http.ResponseWriter, r *http.Request)
4646
decoder.DisallowUnknownFields()
4747

4848
if err := decoder.Decode(&req); err != nil {
49-
response.RespondBadRequest(w, response.JSONDecodeErrorDetail(err))
49+
response.RespondInvalidRequestBody(w, err)
5050

5151
return
5252
}
@@ -167,7 +167,7 @@ func (h *FeedbackRecordsHandler) Update(w http.ResponseWriter, r *http.Request)
167167
decoder.DisallowUnknownFields()
168168

169169
if err := decoder.Decode(&req); err != nil {
170-
response.RespondBadRequest(w, response.JSONDecodeErrorDetail(err))
170+
response.RespondInvalidRequestBody(w, err)
171171

172172
return
173173
}

internal/api/handlers/feedback_records_handler_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"github.com/stretchr/testify/assert"
1313
"github.com/stretchr/testify/require"
1414

15+
"github.com/formbricks/hub/internal/api/response"
1516
"github.com/formbricks/hub/internal/huberrors"
1617
"github.com/formbricks/hub/internal/models"
1718
)
@@ -112,6 +113,40 @@ func TestFeedbackRecordsHandler_Create(t *testing.T) {
112113
assert.Equal(t, "org-123", got.TenantID)
113114
})
114115

116+
t.Run("invalid field_type returns field-level problem details", func(t *testing.T) {
117+
mock := &mockFeedbackRecordsService{}
118+
handler := NewFeedbackRecordsHandler(mock)
119+
120+
body := []byte(`{
121+
"source_type": "survey",
122+
"field_id": "q1",
123+
"field_type": "textt",
124+
"tenant_id": "tenant-123",
125+
"submission_id": "submission-123"
126+
}`)
127+
req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "http://test/v1/feedback-records", bytes.NewReader(body))
128+
rec := httptest.NewRecorder()
129+
130+
handler.Create(rec, req)
131+
132+
assert.Equal(t, http.StatusBadRequest, rec.Code)
133+
assert.Contains(t, rec.Header().Get("Content-Type"), "application/problem+json")
134+
135+
var problem response.ProblemDetails
136+
137+
err := json.Unmarshal(rec.Body.Bytes(), &problem)
138+
require.NoError(t, err)
139+
140+
assert.Equal(t, response.ProblemTypeValidationError, problem.Type)
141+
assert.NotEqual(t, "about:blank", problem.Type)
142+
assert.Equal(t, "Validation Error", problem.Title)
143+
require.Len(t, problem.Errors, 1)
144+
assert.Equal(t, "field_type", problem.Errors[0].Location)
145+
assert.Equal(t, "textt", problem.Errors[0].Value)
146+
assert.Contains(t, problem.Errors[0].Message, "text")
147+
assert.Contains(t, problem.Errors[0].Message, "date")
148+
})
149+
115150
t.Run("service validation error returns bad request", func(t *testing.T) {
116151
mock := &mockFeedbackRecordsService{
117152
createFunc: func(_ context.Context, _ *models.CreateFeedbackRecordRequest) (*models.FeedbackRecord, error) {

internal/api/response/response.go

Lines changed: 118 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,31 @@ import (
1010
"strings"
1111

1212
"github.com/iancoleman/strcase"
13+
14+
"github.com/formbricks/hub/internal/models"
15+
)
16+
17+
const (
18+
// ProblemTypeBadRequest identifies malformed request problems.
19+
ProblemTypeBadRequest = "https://hub.formbricks.com/problems/bad-request"
20+
// ProblemTypeValidationError identifies request validation problems.
21+
ProblemTypeValidationError = "https://hub.formbricks.com/problems/validation-error"
22+
// ProblemTypeClientError identifies unclassified client-side request problems.
23+
ProblemTypeClientError = "https://hub.formbricks.com/problems/client-error"
24+
// ProblemTypeUnauthorized identifies authentication problems.
25+
ProblemTypeUnauthorized = "https://hub.formbricks.com/problems/unauthorized"
26+
// ProblemTypeNotFound identifies missing resource problems.
27+
ProblemTypeNotFound = "https://hub.formbricks.com/problems/not-found"
28+
// ProblemTypeConflict identifies resource conflict problems.
29+
ProblemTypeConflict = "https://hub.formbricks.com/problems/conflict"
30+
// ProblemTypeForbidden identifies authorization problems.
31+
ProblemTypeForbidden = "https://hub.formbricks.com/problems/forbidden"
32+
// ProblemTypeMethodNotAllowed identifies unsupported HTTP method problems.
33+
ProblemTypeMethodNotAllowed = "https://hub.formbricks.com/problems/method-not-allowed"
34+
// ProblemTypeServiceUnavailable identifies temporary dependency problems.
35+
ProblemTypeServiceUnavailable = "https://hub.formbricks.com/problems/service-unavailable"
36+
// ProblemTypeInternalServerError identifies unexpected server problems.
37+
ProblemTypeInternalServerError = "https://hub.formbricks.com/problems/internal-server-error"
1338
)
1439

1540
// ErrorDetail represents a single error detail in RFC 7807 Problem Details.
@@ -32,12 +57,47 @@ type ProblemDetails struct {
3257
// RespondError writes an RFC 7807 Problem Details error response.
3358
func RespondError(w http.ResponseWriter, statusCode int, title, detail string) {
3459
problem := ProblemDetails{
35-
Type: "about:blank",
60+
Type: problemTypeForStatus(statusCode),
3661
Title: title,
3762
Status: statusCode,
3863
Detail: detail,
3964
}
4065

66+
respondProblem(w, statusCode, problem)
67+
}
68+
69+
// RespondInvalidRequestBody writes a 400 response for JSON request body decoding failures.
70+
func RespondInvalidRequestBody(w http.ResponseWriter, err error) {
71+
problemType := jsonDecodeProblemType(err)
72+
73+
problem := ProblemDetails{
74+
Type: problemType,
75+
Title: jsonDecodeProblemTitle(problemType),
76+
Status: http.StatusBadRequest,
77+
Detail: JSONDecodeErrorDetail(err),
78+
Errors: JSONDecodeErrorDetails(err),
79+
}
80+
81+
respondProblem(w, http.StatusBadRequest, problem)
82+
}
83+
84+
func jsonDecodeProblemType(err error) string {
85+
if _, ok := invalidFieldTypeErrorDetail(err); ok {
86+
return ProblemTypeValidationError
87+
}
88+
89+
return ProblemTypeBadRequest
90+
}
91+
92+
func jsonDecodeProblemTitle(problemType string) string {
93+
if problemType == ProblemTypeValidationError {
94+
return "Validation Error"
95+
}
96+
97+
return "Bad Request"
98+
}
99+
100+
func respondProblem(w http.ResponseWriter, statusCode int, problem ProblemDetails) {
41101
w.Header().Set("Content-Type", "application/problem+json")
42102
w.WriteHeader(statusCode)
43103

@@ -46,6 +106,33 @@ func RespondError(w http.ResponseWriter, statusCode int, title, detail string) {
46106
}
47107
}
48108

109+
func problemTypeForStatus(statusCode int) string {
110+
switch statusCode {
111+
case http.StatusBadRequest:
112+
return ProblemTypeBadRequest
113+
case http.StatusUnauthorized:
114+
return ProblemTypeUnauthorized
115+
case http.StatusForbidden:
116+
return ProblemTypeForbidden
117+
case http.StatusMethodNotAllowed:
118+
return ProblemTypeMethodNotAllowed
119+
case http.StatusNotFound:
120+
return ProblemTypeNotFound
121+
case http.StatusConflict:
122+
return ProblemTypeConflict
123+
case http.StatusServiceUnavailable:
124+
return ProblemTypeServiceUnavailable
125+
case http.StatusInternalServerError:
126+
return ProblemTypeInternalServerError
127+
default:
128+
if statusCode >= http.StatusBadRequest && statusCode < http.StatusInternalServerError {
129+
return ProblemTypeClientError
130+
}
131+
132+
return ProblemTypeInternalServerError
133+
}
134+
}
135+
49136
// RespondBadRequest writes a 400 Bad Request error response.
50137
func RespondBadRequest(w http.ResponseWriter, detail string) {
51138
RespondError(w, http.StatusBadRequest, "Bad Request", detail)
@@ -59,6 +146,10 @@ func JSONDecodeErrorDetail(err error) string {
59146
return "Invalid request body"
60147
}
61148

149+
if detail, ok := invalidFieldTypeErrorDetail(err); ok {
150+
return detail.Message
151+
}
152+
62153
var syntaxErr *json.SyntaxError
63154
if errors.As(err, &syntaxErr) {
64155
return "Invalid JSON: " + err.Error()
@@ -78,6 +169,32 @@ func JSONDecodeErrorDetail(err error) string {
78169
return "Invalid request body"
79170
}
80171

172+
// JSONDecodeErrorDetails returns field-level details for JSON request body decoding failures.
173+
func JSONDecodeErrorDetails(err error) []ErrorDetail {
174+
if detail, ok := invalidFieldTypeErrorDetail(err); ok {
175+
return []ErrorDetail{detail}
176+
}
177+
178+
return nil
179+
}
180+
181+
func invalidFieldTypeErrorDetail(err error) (ErrorDetail, bool) {
182+
var invalidFieldType *models.InvalidFieldTypeError
183+
if !errors.As(err, &invalidFieldType) {
184+
return ErrorDetail{}, false
185+
}
186+
187+
return ErrorDetail{
188+
Location: "field_type",
189+
Message: fmt.Sprintf(
190+
"field_type has invalid value %q; must be one of: %s",
191+
invalidFieldType.Value,
192+
models.ValidFieldTypeValuesString(),
193+
),
194+
Value: invalidFieldType.Value,
195+
}, true
196+
}
197+
81198
// fieldNameForAPI converts a struct field path (e.g. "TenantID" or "X.Y") to API-style snake_case.
82199
func fieldNameForAPI(fieldPath string) string {
83200
if i := strings.LastIndex(fieldPath, "."); i >= 0 && i+1 < len(fieldPath) {

0 commit comments

Comments
 (0)