Repository navigation
Add a parser for obacloud's bundle-inputs manifest - #1814
aaronbrethorst wants to merge 5 commits into
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
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. |
|
The description follows the PR template. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
99d7f53 to
69c18d4
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
github-actions
Bot
added
the
follows-template
69c18d4 to
5967bf9
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
5967bf9 to
6b398a6
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
burma-shave
left a comment
burma-shave
left a comment
There was a problem hiding this comment.
Reviewed at 6b398a6. This looks good.
- Reusing
appconf.ValidateStaticFeedskeeps a manifest list and a config list under the same rules. - Requiring http(s) for feed and consolidation URLs in a remote manifest is the right restriction.
- The read is well guarded: a size cap, a check for trailing data, and a regular-file check for local paths, so a FIFO can't hang startup.
- The tests cover each rule, and CI is green.
I have one small inline nit, about byteSize not being range-checked. The signed-URL point (Fetch errors include the full source URL) is raised on #1815, where that URL gets logged and dumped.
Generated by Claude Code
| if !appconf.IsHTTPURL(feed.URL) { | ||
| return fmt.Errorf("url %q must be an http:// or https:// URL", feed.URL) | ||
| } | ||
| if feed.SHA256 != "" && !isSHA256Hex(feed.SHA256) { |
There was a problem hiding this comment.
Nit: sha256 gets a format check, but byteSize doesn't, so a negative value parses fine. Nothing reads it yet. Since this PR is defining the format rules, it might be worth rejecting byteSize < 0 here, rather than leaving it to the phase C download check to handle a nonsense value.
Generated by Claude Code
There was a problem hiding this comment.
Done in b0cf517. A negative byteSize is now rejected next to the sha256 check.
The bundle-inputs manifest yields a static feed list too, and it must pass the same rules as a configured one: unique ids, unique default agencies without '_', and valid fields in each feed. Exporting ValidateStaticFeeds lets the manifest reuse those checks instead of repeating them. validateStaticFeeds keeps the config-only rules (an empty list) and calls it with the key the feeds came from. IsHTTPURL replaces the http/https prefix test that the feed url check and gtfs.Config.isLocalFile each wrote out, so the manifest code can use it too. Behavior is unchanged.
obacloud publishes a version 1 bundle-inputs.json that lists the per-agency static feeds an OBA server loads, in load order, with an optional stop consolidation URL. Maglev needs to read it to run in manifest mode. Parse decodes the manifest and checks the manifest format: version 1, at least one feed, an id and defaultAgencyId on each feed, and a 64-character hex sha256 when one is given. Feed and consolidation URLs must be http(s), so a manifest served by another host cannot point the server at local files. The feeds then go through appconf.ValidateStaticFeeds, the same checks a configured feed list gets, so ids and defaultAgencyIds are unique and agency ids have no '_'. StaticFeeds maps the feeds onto appconf.GtfsStaticFeed. SHA256 and ByteSize stay on the manifest feed for the download check that comes later.
Fetch reads the manifest from an http(s) URL or a local path and parses it. Errors name the source so a startup failure points at the manifest that broke it. The HTTP code lives in the package rather than reusing the static GTFS download in internal/gtfs: that one is tied to gtfs.Config's auth header and gtfstidy, and the manifest needs neither. A manifest is a few kilobytes, so the body is capped at 1 MiB and the request times out after 30 seconds.
Parse now rejects data after the first JSON value, so a concatenated or partly overwritten manifest is an error instead of silently using its first object. A local manifest path must be a regular file, as LoadFromFile already requires of the config file; a directory or FIFO fails up front instead of hanging or producing a confusing JSON error.
sha256 already had a format check, but byteSize did not, so a manifest with a negative size parsed cleanly. Nothing reads the size yet; the format rules belong here rather than in the later download check.
6b398a6 to
b0cf517
Compare
|
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Part of #1976
Stacked on #1813. Phase A.2 of the multi-feed static GTFS spec (#1811).
What it does
Adds
internal/bundleinputs, a package that reads thebundle-inputs.jsonmanifest (version 1) that obacloud publishes to list the per-agency feeds an OneBusAway server should load.Parsedecodes and validates a manifest;Fetchreads one from anhttps://URL or a local file.Manifest.StaticFeeds()maps each entry onto agtfs-static-feedsitem (id,url,default-agency-id).sha256andbyteSizeare parsed and kept for later download verification;stopConsolidationUrlis parsed for phase D.Validation that applies to every feed list, wherever it came from (unique ids, unique default agencies, no
_in agency ids, the per-feed checks), is shared with the file config:appconf.ValidateStaticFeedsis exported and the manifest calls it. The manifest adds only its own format rules: version 1, a non-empty list,idanddefaultAgencyIdrequired, feed and consolidation URLs must be http(s) (a remote manifest must not name local files), and a well-formedsha256.appconf.IsHTTPURLreplaces three copies of the same prefix check.Nothing reads the package yet; the config key that uses it is the next PR in the stack.
Root cause
Production deployments get their feed list from obacloud, not from a hand-written config. Maglev had no way to read that contract.
Testing
mainbefore this change: n/a (new package). Table-driven tests cover a golden manifest fixture, every validation rule (including a duplicatedefaultAgencyIdfailing through the shared validator), trailing data after the JSON value, andFetchover HTTP, from a local file, a 404, an over-size body, a directory and a missing file.go vetwith both tag sets,make testandgo fmtpass.Checklist
appconfexports; the rest is tests and a fixture.mainand passes on this branch.gtfsdb/(db.go,models.go,query.sql.go) ortestdata/openapi.yml(synced from upstream).r.Context()to DB/service calls, notcontext.Background().testdata/openapi.yml, has an issue open on OneBusAway/sdk-config, or is outside the SDK and the description says why.Notes for reviewers
rawGtfsDataininternal/gtfs/static.go. Sharing them is left for phase C, because the streaming import stack rewrites that fetch.default-agency-id) because they come from the shared validator.