Fix segfault: guard nil OBA SDK responses (null JSON body)#3
Conversation
The OBA REST server can return a literal JSON `null` body with HTTP 200.
The go-sdk decodes that into a (nil, nil) response — a nil pointer with a
nil error — so checks that dereferenced `resp.Data...` after only an
`err != nil` check panicked with SIGSEGV (reported on
`oba-validator -json example_configs/kcm.json`).
Add a nil-response guard to every in-check SDK call (vehicle, endpoint,
alert, trip-update checks). Severity follows the evidence-based model,
matching each call's existing empty/error sibling:
- per-stop/per-vehicle cross-refs Warn (unconfirmed)
- vehicles-for-agency Fails (feed proves vehicles exist, API returned none)
- the basic-endpoints smoke test Fails (null core endpoint = server fault)
Also add a `withReason` helper so a null-body Fail no longer renders a
dangling colon ("current-time failed: ").
|
Warning Review limit reached
More reviews will be available in 3 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
✨ 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 |
SonarQube flagged 24.5% duplication on new code. Remove the duplicated blocks: - Production: extract `queryArrivals`, shared by the alert and trip-update cross-reference checks, replacing the identical error/null-response guard each had inline. - Tests: add shared helpers (`arrivalsClient`, `endpointsClient`, `vehicleClient`, `baseSrc`, `alertSrcForStop`, `tripUpdateSrcForStop`, `assertFirstStatus`), collapse the three vehicle null-response tests into one table test, and route the happy-path/empty/404 tests through the same builders instead of repeating httptest handlers and SourceContext literals. Behavior unchanged; full suite green, `dupl -threshold 100` reports zero clone groups in the package.
|
1 participant
Summary
nullbody with HTTP 200 for some queries (confirmed live ontrips-for-location.json). The go-sdk decodesnullinto a**T, leaving the response(nil, nil)— a nil pointer with a nil error. Every check guarded onlyerr != nil, then dereferencedresp.Data...→ SIGSEGV. It was intermittent because the crashing entity depends on the live feed at fetch time.check_vehicles.go,check_endpoints.go,check_alerts.go,check_tripupdates.go(the pre-fetchedvc.Agencieswas already guarded).Warn(unconfirmed);vehicles-for-agencyand thebasic-endpointssmoke testFail(the feed/endpoint proves data the API is missing).withReasonso a null-bodyFailno longer renders a dangling colon ("current-time failed: ").Test plan
go vet ./...clean, fullgo test ./...greenFail+ no dangling colonWarn(not the "absent"Fail)oba-validator -json example_configs/kcm.jsonno longer panics; emitted the real warningtrips-for-location returned a null response near vehicle "11743"in the wildFaildetection still works (live run flagged a realtrip-for-vehicleAPI-vs-feed trip mismatch → exit 1)Review notes (deliberate decisions, flagging for visibility)
vehicles-for-agencynull →Fail, but the other cross-ref null cases →Warn. Intentional: that call is the agency-wide roster whose empty-list branch alreadyFails, and a null is the same evidence (feed proves vehicles exist). The per-stop/per-vehicle cross-refs stayWarnbecause their empties are genuinely unconfirmed (wrong agency prefix, vehicle moved, no current trip)..Data/.Entry/.Listare value types, so once the top-level pointer is non-nil the downstream derefs are safe.🤖 Generated with Claude Code