Validate schedule-for-stop date before the agency lookup - #1385
Validate schedule-for-stop date before the agency lookup#1385ARCoder181105 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe schedule-for-stop handler now validates the ChangesSchedule-for-stop validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Invalid schedule-for-stop dates now consistently return a date field error before lookup, while valid dates retain existing not-found behavior and agency-timezone resolution. The covered behavior is ready to merge. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
441f7eb to
cb3817f
Compare
The agency lookup ran first, so an unparseable date on an ID whose agency prefix does not exist answered 404 instead of the field error the reference server returns. The spec requires the field error to come back before any stop lookup is attempted. Check the date's format up front and keep resolving it to a service date below, where the agency's timezone is available.
cb3817f to
a495f99
Compare
|
@coderabbitai review |
|
burma-shave
left a comment
burma-shave
left a comment
There was a problem hiding this comment.
Two findings from an automated review, both confirmed against this PR's actual diff (git diff origin/main...HEAD). Left as inline comments below.
Note: the review also flagged three other issues (a duplicated route-reference builder, a routeless-stop early-return removal, and an unrelated test-file change) but those all point at code outside this PR's diff — pre-existing code this branch didn't touch — so they're left out here rather than posted as inline comments GitHub would reject.
| // ValidateServiceDate reports whether a service date parameter is parseable, in either | ||
| // of the forms ParseDate accepts. Handlers use it to reject a malformed date before | ||
| // looking up the agency whose timezone ParseDate then resolves the date against. | ||
| func ValidateServiceDate(date string) error { |
There was a problem hiding this comment.
ValidateServiceDate duplicates the pre-existing ValidateDate (line 95) instead of extending or replacing it. ValidateDate (YYYY-MM-DD only) appears unused anywhere in production code, so the file now carries two similarly-named date validators with overlapping scope — a future caller or bugfix can easily reach for the wrong one. Worth considering whether ValidateDate should be removed or merged into this new function instead of living alongside it.
| @@ -46,12 +56,10 @@ func (api *RestAPI) scheduleForStopHandler(w http.ResponseWriter, r *http.Reques | |||
|
|
|||
| if dateParam != "" { | |||
There was a problem hiding this comment.
This comment ("only fails on an unusable agency timezone") is inaccurate, and the branch below it looks unreachable now. ValidateServiceDate (line 36) already runs ParseDate(dateParam, time.UTC) and returns 400 on any format/bounds failure; neither of ParseDate's two success paths depends on the loc argument for success/failure, so once ValidateServiceDate has succeeded, ParseDate(dateParam, loc) here can't fail for any valid loc. A bad agency timezone is also already caught earlier by loadAgencyLocation (lines 48-52), before this second parse runs. Since this branch appears dead, could it be removed (or the comment corrected if there's a case I'm missing)?
There was a problem hiding this comment.
OBA API Change Review: PR #1385 / current branch — single endpoint
Input: Working tree on branch pr-1385; open PR #1385 found and local HEAD matches PR head SHA.
Stated goal: Fix #1382 — schedule-for-stop should return a 400 fieldErrors.date response for an unparseable date even when the agency prefix is unknown.
Scope: Single endpoint.
Affected endpoint(s): schedule-for-stop.
Changes: Both production and test changes.
Overview
What this change does
Before this PR, /api/where/schedule-for-stop/{id}.json split the combined stop ID, looked up the agency, and only parsed date after the agency lookup. That meant:
GET /schedule-for-stop/25_<known-stop>.json?date=garbagereturned the expected 400 field error.GET /schedule-for-stop/99_1001.json?date=garbage, where agency99does not exist, returned 404 before Maglev ever examined the bad date.
The PR adds utils.ValidateServiceDate, which delegates to the existing ParseDate, and calls it before the agency lookup. The later timezone-aware date resolution still happens after the agency is loaded. So the bad-date / unknown-agency case now returns HTTP 400 with fieldErrors.date, while valid-date unknown agency/stop cases still return 404.
Domain background
schedule-for-stop accepts a combined stop ID shaped like {agencyId}_{stopId} and an optional service-date parameter. The service date may be YYYY-MM-DD or a Unix millisecond timestamp. For YYYY-MM-DD, Maglev eventually needs the agency timezone to resolve “midnight on that date,” but it does not need the agency just to determine whether the string is parseable. The OBA spec requires malformed field values to produce structured field errors before resource lookup failures take precedence.
Details
### Goal checkGoal check: schedule-for-stop
Stated goal: malformed date should return 400 fieldErrors.date regardless of whether the agency/stop exists; valid-date unknown agency/stop should remain 404; agency timezone should still be used for real date resolution.
- ✓ Invalid date with unknown agency returns 400 before agency lookup.
- ✓ Invalid date with known agency still returns 400.
- ✓ Valid date with unknown agency still returns 404.
- ✓ Valid date with unknown stop still returns 404.
- ✓ Timezone-aware parsing remains after agency lookup.
Test coverage: Adequate for the central regression. The new test covers unknown-agency bad date, known-agency bad date, unknown-agency valid date, and unknown-stop valid date. Targeted test run passed:
go test -tags "sqlite_fts5 sqlite_math_functions" ./internal/restapi -run TestScheduleForStopHandlerDateValidationPrecedesLookup
Overall: fully closed.
Client impact
Caveat: client checkouts had anomalies: /workspace/wayfinder is on branch develop, and /workspace/maglev.wiki has an untracked file.
Client impact: schedule-for-stop
| Behaviour | Wayfinder/SDK | iOS | Android |
|---|---|---|---|
Invalid date + unknown agency changes from 404 to 400 fieldErrors.date |
Minimal/direct-call impact only. JS SDK has date?: string; Wayfinder passes date through from a date picker and does not intentionally send malformed dates. |
No routine impact. iOS builds yyyy-MM-dd from Date, so malformed dates are not normally sent. |
No routine impact. Android endpoint accepts date: String?, but normal callers would send valid dates. |
No response payload fields or success shapes change. The observable change is limited to an error-precedence edge case.
Spec check
Spec check: schedule-for-stop
- Consistent — The wiki spec’s Minimal Guarantees explicitly says: “When the date parameter is unrecognisable, a field-error response is returned before any stop lookup is attempted.”
- Consistent — Extension 2a specifies HTTP 400 with
fieldErrors.date. - Consistent — Valid-date unknown stop/agency remains 404 in Maglev, matching the existing Implementation Decision that Maglev intentionally corrects legacy Java’s null-body unknown-stop defect.
Deviation recording: No new deviation needed. This change implements existing Maglev spec behavior rather than introducing a new legacy divergence.
Overall: spec-consistent.
Summary
PR #1385 is a narrow schedule-for-stop error-precedence fix. It moves date-format validation ahead of agency lookup while preserving timezone-aware service-date resolution after the agency is known. The implementation matches the stated issue and the wiki spec, keeps valid unknown-resource behavior unchanged, and adds focused regression coverage.
|
@ARCoder181105 checking in on this stack. This base PR still has requested changes open, so we cannot sensibly review further up the stack yet. Could you please address the requested changes here first, then let us know when the base is ready? |
|
@ARCoder181105 checking in on this stack. This base PR still has requested changes open, so we cannot sensibly review further up the stack yet. Could you please address the requested changes here first, then let us know when the base is ready? |
|
2 participants
Fixes #1382
Stacked on #1384 — its two commits show up in this diff until it merges. Review the last commit here.
What changed
The agency lookup ran before the
dateparameter was parsed, so an unparseable date on an ID whose agency prefix does not exist answered 404 instead of a field error.The date's format is now checked up front, via a new
utils.ValidateServiceDatethat delegates toParseDateso the validator and the parser cannot drift apart. Resolving the date to a service date still happens after the agency lookup, where the agency's timezone is available.Spec
Minimal Guarantees: "When the date parameter is unrecognisable, a field-error response is returned before any stop lookup is attempted." Extension 2a specifies HTTP 400 with a
fieldErrors.datearray.Verified against
api.pugetsound.onebusaway.org:Tests
TestScheduleForStopHandlerDateValidationPrecedesLookupcovers unknown agency with a bad date, known agency with a bad date, and both unknown-agency and unknown-stop with a valid date still returning 404.Summary by CodeRabbit
Bug Fixes
Tests