Fix feed version by ID download#655
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the REST feed-version download endpoints to support integer database IDs (in addition to existing string keys) by correcting GraphQL query variable nullability and adjusting the “found” check logic for feed-version lookups.
Changes:
- Make
$feed_onestop_id/$feed_version_sha1GraphQL variables nullable so requests that only supplyidspass GraphQL validation. - Fix
feedVersionDownloadHandlerto treat a feed version as found when the lookup returns a non-empty sha1 (instead of comparing sha1 to the request key). - Add tests that resolve stable IDs via GraphQL at runtime and exercise both download endpoints using integer IDs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/rest/feed_version_download.go | Makes GraphQL query variables nullable and fixes feed-version “found” logic so integer-ID download paths work. |
| server/rest/feed_version_download_test.go | Adds a GraphQL-based ID resolver helper and new subtests to validate downloading by integer ID. |
💡 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
Fixes the feed version download REST endpoints so they work when given an integer database ID instead of a string key. Both
/feed_versions/{key}/downloadand/feeds/{key}/download_latest_feed_versionalready branch on whether the path parameter parses as an integer, sending it as the GraphQLidsvariable instead of the sha1/onestop_id key — but the integer path was broken in two ways, so downloads by ID always failed.GraphQL variable nullability
The lookup queries declared their key variables as non-nullable (
$feed_version_sha1: String!,$feed_onestop_id: String!). When downloading by integer ID the handler leaves those variables unset, which fails GraphQL validation before the query ever runs. Both are now nullable (String), so a request that supplies onlyidsis valid.Found-check no longer assumes the key is the sha1
feedVersionDownloadHandlerdecided a feed version was found by comparing the returnedsha1against the request key (fvsha1 == key). That holds when the caller passes a sha1, but never when the caller passes an integer ID, so the handler returned 404 for valid IDs. It now treats the lookup as found whenever the query returns a non-empty sha1 (fvsha1 != ""). The latest-feed-version handler already used a gjson existence check and only needed the nullability fix.Tests
Adds a
resolveIDhelper that posts a GraphQL query as admin and extracts an id via gjson, since fixture serial ids are not stable across runs. New subtests inTestFeedVersionDownloadRequestandTestFeedDownloadLatestRequestresolve the feed version / feed id, hit the download endpoints by integer id, and assert a 200 with the expected 59324-byte body.Test plan
GET /feed_versions/{id}/downloadas admin; confirm a 200 and the full GTFS zip body.GET /feeds/{id}/download_latest_feed_versionas admin; confirm a 200 and the full zip body.