Remove additional panics in request path - #625
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes several production-observed panic(err) sites from request paths and replaces them with structured error propagation so malformed inputs, bad rows, or Checker outages don’t crash requests.
Changes:
- Convert
model.WithPermsto return(context.Context, error)and makeAddPermsreturn a JSON 500 on Checker failure instead of panicking. - Propagate dataloader batch errors via per-key
errs[]entries rather than panicking. - Convert mutation
scanColand trip query construction (tripSelect) to return errors instead of panicking.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| server/model/perm_filter.go | Switch WithPerms to error-returning API; AddPerms now fails closed with JSON 500 response. |
| server/model/perm_filter_test.go | Update WithPerms call sites and add tests for AddPerms failure/success behavior. |
| server/gql/loaders.go | Replace panic on batch query failure with per-index error propagation. |
| server/finders/dbfinder/trip.go | Make tripSelect return (builder, error) and return structured error on invalid RelativeDate. |
| server/finders/dbfinder/mutations.go | Replace panic in scan paths with an error accumulator pattern via scanCol(..., errp *error). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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
Converts four production-observed
panic(err)sites in request paths to structured error returns so a bad row, malformed enum input, or checker outage no longer takes down the request. Closes interline-io/tlv2#337.Dataloader batch
paramGroupQueryinserver/gql/loaders.gono longer panics when the inner query function returns an error. The error is spread across every input index in the failing param group via the existingerrs[]slice; dataloader consumers surface it as a per-key error.WithPerms / AddPerms
model.WithPermsreturns(context.Context, error)instead of panicking when the Checker can't resolve the caller's permissions. The returned context is the original ctx on error, with no merged PermFilter applied.AddPermsHTTP middleware logs and writes a JSON 500 viainternal/util.WriteJsonErroron Checker failure and does not call the downstream handler. Previously this path panicked.checkActive's nil-Checker panic is preserved (it's a config-time misconfiguration, not a runtime error).Mutations
scanColinserver/finders/dbfinder/mutations.gotakes a non-nil*erroraccumulator instead of panicking on Scan failure. Once*errpis set, subsequent scanCol calls short-circuit, so callers chain several and inspect a single error at the end.scanColskips its internal append whencolnameis empty, so the existing sites that pre-append the column name explicitly (parent_station, level_id, from_stop_id, to_stop_id) keep working — they pass""to get only the Scan side effect.Stop,StopExternalReference,Pathway,Level) declarevar err errorand propagate it.Trips
tripSelectinserver/finders/dbfinder/trip.goreturns(sq.SelectBuilder, error)instead of panicking on aRelativeDateenum-parse failure. The three callers (FindTrips,TripsByRouteIDs,TripsByFeedVersionIDs) propagate the error.Tests
TestAddPerms_CheckerErrorReturns500andTestAddPerms_SuccessForwardsToDownstreaminserver/model/perm_filter_test.golock in the middleware's fail-closed 500 behavior and the happy-path context propagation (stdlibhttptest, no DB).WithPermstest sites updated to consume the new(ctx, error)return.Behavior changes to be aware of
RelativeDateenum values now return a structured GraphQL error instead of crashing the request.scanColScan failures now surface as wrapped errors (scan <col>: <inner>) on the mutation response instead of crashing.Test plan
go test ./server/...RelativeDateenum and confirm a structured GraphQL error is returned rather than a 500 / crash./queryreturns a 500 JSON body and the request log shows theAddPerms: failed to resolve caller permissionserror.