TRT-2771: Return HTTP 400 for invalid filter fields instead of 500#3739
TRT-2771: Return HTTP 400 for invalid filter fields instead of 500#3739mstaeble wants to merge 3 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Skipping CI for Draft Pull Request. |
|
@mstaeble: This pull request references TRT-2771 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Tip For best results, initiate chat on the files or code changes.
|
mstaeble
force-pushed
the
trt-2771-filter-validation
branch
4 times, most recently
from
762ecc8 to
44414e9
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds shared error-classification helpers and updates API and server handlers to return HTTP 400 for request/parsing failures while routing other errors through a common JSON error response. It also promotes ChangesError handling and request classification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant IsBadRequestError
participant RespondWithError
participant failureResponseWithError
participant ResponseWriter
Client->>Handler: HTTP request
Handler->>Handler: parse request / build filters / query data
alt request parsing failure
Handler->>ResponseWriter: write 400 JSON error
else backend error
Handler->>RespondWithError: API handlers use shared responder
RespondWithError->>IsBadRequestError: classify err
IsBadRequestError-->>RespondWithError: true or false
RespondWithError->>ResponseWriter: write 400 or 500 JSON error
Handler->>failureResponseWithError: server handlers use shared failure helper
failureResponseWithError->>IsBadRequestError: classify err
IsBadRequestError-->>failureResponseWithError: true or false
failureResponseWithError->>ResponseWriter: write 400 or 500 JSON error
end
ResponseWriter-->>Client: HTTP response
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✨ Finishing Touches🧪 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/sippyserver/server.go (1)
549-568: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
failureResponseWithErrorhere
api.ListPayloadJobRunscan fail on the DB query as well as filter parsing, so this path should not always return 400 or echoerr.Error(). That keeps client errors as 400 while surfacing internal failures as 500 without leaking DB details.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/sippyserver/server.go` around lines 549 - 568, jsonListPayloadJobRuns should use failureResponseWithError instead of always returning a 400 and concatenating err.Error(), because api.ListPayloadJobRuns can fail for internal DB reasons as well as bad request filters. Keep FilterOptionsFromRequest handling for request parsing errors, but for the api.ListPayloadJobRuns call switch to failureResponseWithError so client-side issues still map to 400 while server/DB failures surface as 500 without leaking details.
🧹 Nitpick comments (2)
pkg/api/api.go (2)
28-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing unit tests for new exported helpers.
RespondWithErrorandIsBadRequestErrorare new exported functions with branching logic (pg error code vs. googleapi code) but no accompanying test file is included in this cohort. As per coding guidelines,**/*.{go,tsx,jsx}: "new Go functions and methods need unit tests". These are also pure/logic functions with no external client calls, so per the testing learning they should be straightforward to test directly without mocking storage clients.Could you confirm whether tests exist elsewhere in the stack, or add table-driven tests covering: nil err, generic error, wrapped
*pgconn.PgErrorwith code42703, wrapped*pgconn.PgErrorwith a different code, and wrapped*googleapi.Errorwith/without 400?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/api.go` around lines 28 - 52, Add unit tests for the new exported helpers in api.go. Cover RespondWithError and IsBadRequestError with table-driven cases for nil error, generic error, wrapped pgconn.PgError with code 42703 and a non-matching code, and wrapped googleapi.Error with 400 and a non-400 code. Use the function names RespondWithError and IsBadRequestError to locate the logic, and verify the HTTP status selection in RespondWithError matches the bad-request detection.Source: Coding guidelines
42-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOverly broad googleapi 400 classification risks masking real server bugs.
The Postgres branch narrowly targets SQLSTATE
42703(undefined column), but the googleapi branch treats anyCode == http.StatusBadRequestas a client error. BigQuery returns 400 for many distinct reasons beyond invalid filter columns, e.g. malformed generated SQL, type mismatches, or invalid query arguments. If a server-side bug generates an invalid BigQuery query, it will now surface to clients as a generic 400 "bad request" instead of a 500, hiding real defects from alerting/monitoring and misleading callers into thinking their input was invalid.Consider narrowing the googleapi check to the specific
reason(e.g.invalidQuerycombined with a message pattern for unknown columns) rather than matching on the HTTP status alone, mirroring the specificity used for the Postgres case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/api.go` around lines 42 - 52, The googleapi branch in IsBadRequestError is too broad because it maps every googleapi.Error with HTTP 400 to a client error. Narrow this check to the specific BigQuery error condition that indicates an unknown column, using the googleapi.Error details (such as reason and message) instead of status code alone, and keep the Postgres pgErr/pgUndefinedColumn logic unchanged so only genuinely invalid input is classified as bad request.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/api/releases.go`:
- Around line 332-336: The error response in PrintReleasesReport uses copied
job-run wording, so update the message built after
filter.FilterOptionsFromRequest to reference the releases report instead of “job
run report.” Keep the existing BadRequest handling, but change the text in the
RespondWithJSON payload to match the releases-report context in
PrintReleasesReport and avoid reusing PrintJobsReportFromDB phrasing.
In `@pkg/api/tests.go`:
- Line 6: Replace the remaining pkgerrors.Wrap usages in tests.go with stdlib
error wrapping using fmt.Errorf and %w, matching the repo guideline and
preserving the error chain. Update the affected wrapping sites in the test
helper/code paths that currently use pkgerrors.Wrap, keep the existing context
text, and if pkgerrors is no longer referenced afterward remove its import from
the file. Use the existing errors import and the relevant helper/function names
around the current wrap sites to locate each change.
In `@pkg/sippyserver/server.go`:
- Around line 454-461: The logic in failureResponseWithError duplicates
api.RespondWithError, including bad-request classification, error logging, and
JSON response construction. Update failureResponseWithError to delegate to
api.RespondWithError instead of re-implementing the behavior, using the existing
failureResponseWithError and api.RespondWithError symbols to keep response
handling centralized and avoid drift.
---
Outside diff comments:
In `@pkg/sippyserver/server.go`:
- Around line 549-568: jsonListPayloadJobRuns should use
failureResponseWithError instead of always returning a 400 and concatenating
err.Error(), because api.ListPayloadJobRuns can fail for internal DB reasons as
well as bad request filters. Keep FilterOptionsFromRequest handling for request
parsing errors, but for the api.ListPayloadJobRuns call switch to
failureResponseWithError so client-side issues still map to 400 while server/DB
failures surface as 500 without leaking details.
---
Nitpick comments:
In `@pkg/api/api.go`:
- Around line 28-52: Add unit tests for the new exported helpers in api.go.
Cover RespondWithError and IsBadRequestError with table-driven cases for nil
error, generic error, wrapped pgconn.PgError with code 42703 and a non-matching
code, and wrapped googleapi.Error with 400 and a non-400 code. Use the function
names RespondWithError and IsBadRequestError to locate the logic, and verify the
HTTP status selection in RespondWithError matches the bad-request detection.
- Around line 42-52: The googleapi branch in IsBadRequestError is too broad
because it maps every googleapi.Error with HTTP 400 to a client error. Narrow
this check to the specific BigQuery error condition that indicates an unknown
column, using the googleapi.Error details (such as reason and message) instead
of status code alone, and keep the Postgres pgErr/pgUndefinedColumn logic
unchanged so only genuinely invalid input is classified as bad request.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0c6db7fe-41eb-4672-97f0-4eaee6e70481
📒 Files selected for processing (6)
go.modpkg/api/api.gopkg/api/jobs.gopkg/api/releases.gopkg/api/tests.gopkg/sippyserver/server.go
mstaeble
force-pushed
the
trt-2771-filter-validation
branch
from
44414e9 to
906e4a2
Compare
API clients sending filter parameters with unrecognized column names caused database errors (Postgres SQLSTATE 42703, BigQuery HTTP 400) surfaced as HTTP 500 responses. This change catches those database errors at the HTTP response boundary and returns 400 instead of 500. Error details are logged server-side but not leaked to the client. Key changes: - Add IsBadRequestError() in pkg/api that detects Postgres undefined-column errors (SQLSTATE 42703) and BigQuery 400 errors - Add RespondWithError (pkg/api) and failureResponseWithError (pkg/sippyserver) that use IsBadRequestError to select 400 vs 500, log the full error server-side, and send only a generic message - Fix FilterOptionsFromRequest and ExtractFilters error responses from 500 to 400 across all handlers - Preserve error type chain through errors.Join in tests.go so pgconn.PgError is detectable by errors.As at the boundary Note: some pkg/api functions write HTTP responses directly rather than returning errors to the sippyserver handler layer. This creates two parallel error response paths (RespondWithError vs failureResponseWithError). Unifying this is left for a follow-up refactor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mstaeble
force-pushed
the
trt-2771-filter-validation
branch
from
906e4a2 to
f0f1d38
Compare
openshift-ci
Bot
added
the
ready-for-human-review
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
|
/lgtm |
openshift-ci
Bot
added
lgtm
sosiouxme
left a comment
sosiouxme
left a comment
There was a problem hiding this comment.
Much better this way, thanks
/lgtm
/hold
in case you want to address the nit first
openshift-ci
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mstaeble, neisw, sosiouxme The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
New changes are detected. LGTM label has been removed. |
|
/hold cancel Addressed nit from review. |
openshift-ci
Bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/api/api_test.go (1)
37-41: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding an
errors.Jointest case.The PR description states handlers use
errors.Jointo preserve the error type chain sopgconn.PgErrorcan still be detected witherrors.As.errors.Joinproduces an error implementingUnwrap() []error, whicherrors.Astraverses via a different code path than theUnwrap() errorchain tested by thefmt.Errorf("...: %w", ...)cases. Adding a case like the one below would verify this end-to-end behavior directly.♻️ Suggested additional test case
{ name: "wrapped googleapi 400 invalidQuery", err: fmt.Errorf("bq failed: %w", &googleapi.Error{ Code: 400, Errors: []googleapi.ErrorItem{{Reason: bqInvalidQuery}}, }), want: true, }, + { + name: "errors.Join wrapped pg undefined column", + err: errors.Join(fmt.Errorf("context"), &pgconn.PgError{Code: "42703"}), + want: true, + }, }Note: this would require adding
"errors"to the import block.Also applies to: 71-78
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/api_test.go` around lines 37 - 41, Add an `errors.Join`-based test case in `pkg/api/api_test.go` alongside the existing `Test...` cases that exercise `errors.As` detection for `pgconn.PgError`, so the suite covers the `Unwrap() []error` path as well as the current `fmt.Errorf("%w")` wrapping; update the import block to include `errors`, and mirror the existing wrapped-pg-error assertions in the relevant table entries referenced by the test helper names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/api/api_test.go`:
- Around line 37-41: Add an `errors.Join`-based test case in
`pkg/api/api_test.go` alongside the existing `Test...` cases that exercise
`errors.As` detection for `pgconn.PgError`, so the suite covers the `Unwrap()
[]error` path as well as the current `fmt.Errorf("%w")` wrapping; update the
import block to include `errors`, and mirror the existing wrapped-pg-error
assertions in the relevant table entries referenced by the test helper names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f37edb4e-1722-447c-9e8e-31ad27ea54d2
📒 Files selected for processing (7)
go.modpkg/api/api.gopkg/api/api_test.gopkg/api/jobs.gopkg/api/releases.gopkg/api/tests.gopkg/sippyserver/server.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/api/releases.go
- pkg/api/api.go
- pkg/api/tests.go
- pkg/api/jobs.go
- go.mod
- pkg/sippyserver/server.go
|
Scheduling required tests: |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Scheduling required tests: |
1 similar comment
|
Scheduling required tests: |
|
@mstaeble: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Summary
FilterOptionsFromRequestandExtractFilterserror responses from 500 to 400 across all handlerserrors.Joinsopgconn.PgErroris detectable byerrors.Asat the boundaryContext
API clients sending filter parameters with unrecognized column names (e.g.,
columnField: "component"on/api/tests) caused database errors surfaced as HTTP 500 responses. The database already validates column names, so we catch those errors at the boundary layer rather than maintaining a parallel field list.Follow-up
Some
pkg/apifunctions (e.g.,PrintTestsJSONFromDB) write HTTP responses directly rather than returning errors to thesippyserverhandler layer. This creates two parallel error response paths (RespondWithErrorvsfailureResponseWithError). Unifying this by having allpkg/apifunctions return errors and lettingsippyserverhandle all HTTP responses is left for a follow-up refactor.Test plan
make lintpassesmake testpasses (14924 Go tests, 19 JS tests, 54 Python tests)Manual test evidence (against staging DB)
Invalid filter field (
component) on/api/testsreturns 400:Valid filter field (
name) on/api/testsreturns 200:Invalid filter field (
timestamp) on/api/jobsreturns 400:🤖 Generated with Claude Code
Summary by CodeRabbit