feat: implemented GET /api/where/route-details/{id} endpoint - #1222
feat: implemented GET /api/where/route-details/{id} endpoint#1222RaginiSharma01 wants to merge 11 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesAdds route-details response models and a handler, registers Route details API
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant routeDetailsHandler
participant GTFSDatabase
participant APIResponse
Client->>routeDetailsHandler: GET route-details/{id}
routeDetailsHandler->>GTFSDatabase: Load agency, route, services, stops, and alerts
GTFSDatabase-->>routeDetailsHandler: Route data and active services
routeDetailsHandler->>APIResponse: Build RouteDetailsEntry and ReferencesModel
APIResponse-->>Client: JSON route details
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
RaginiSharma01
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/models/route_details.go`:
- Around line 4-12: Add constructors for AgencyAndId and RouteDetailsEntry
following the repository’s existing model-constructor patterns, accepting each
struct’s required fields and returning initialized instances. Keep the JSON
field mappings and existing struct fields unchanged.
In `@internal/restapi/route_details_handler_test.go`:
- Around line 103-110: Update the reference assertion in the test to compare
each route’s ID with testdata.Route1.ID, rather than accepting any non-empty ID.
Keep the existing found flag and final assertion so the test specifically
verifies the requested route remains referenced without active trips.
- Around line 151-155: Update the route details test around callAPIHandler to
assert Cache-Control exactly equals "public, max-age=300" rather than only
checking presence. Reissue the request using the returned ETag in If-None-Match
and assert the response status is http.StatusNotModified.
In `@internal/restapi/route_details_handler.go`:
- Line 14: Reduce cognitive complexity in RestAPI.routeDetailsHandler by
extracting its validation and date-loading logic into focused helper functions,
and moving response-reference assembly into a separate helper. Keep
routeDetailsHandler responsible only for orchestration while preserving the
existing validation, loading, and response behavior.
- Line 78: Update the processRouteStops invocation in the route details handler
to provide all arguments required by its current signature, preserving the
existing ctx, agencyID, routeID, serviceIDs, and false values and adding the
missing argument in the correct position.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fa3faab7-66b9-4f87-b288-5617bca84a06
📒 Files selected for processing (4)
internal/models/route_details.gointernal/restapi/route_details_handler.gointernal/restapi/route_details_handler_test.gointernal/restapi/routes.go
|
Cool!! Great Work.. @RaginiSharma01 I will take a look on that the CI is majorly failing due to openapi.yaml (i think so havent look at that just a gist) few questions: as i can see i think there is a lot of refactoring is to be done can you do this from ur side (dont scare of CI we will update the openapi.yaml in sdk-config it will pass most as far as i think) like address the coderabbit comment and sonarcloud issues they are usually simple... also is the response is matching with the prod endpoint which i mentioned in the issues desc (like is there similarity ) |
|
Thanks @ARCoder181105!, :)
|
999696d to
edc946e
Compare
Extract date parsing and reference assembly into helpers to reduce cognitive complexity. Add constructors for AgencyAndId and RouteDetailsEntry models. Fix route details test to assert specific testdata Route ID and proper ETag cache headers.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/models/route_details.go (1)
4-26: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove undocumented exports. The new declarations do not meet the required exported-Go documentation convention.
internal/models/route_details.go#L4-L26: add identifier-leading doc comments for both models and both constructors.internal/restapi/route_details_handler_test.go#L13-L20: renameRouteDetailsResponseto unexportedrouteDetailsResponse, or add a doc comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/models/route_details.go` around lines 4 - 26, Document the exported AgencyAndId, NewAgencyAndId, RouteDetailsEntry, and NewRouteDetailsEntry declarations in internal/models/route_details.go with identifier-leading Go doc comments. In internal/restapi/route_details_handler_test.go, rename RouteDetailsResponse to routeDetailsResponse or add an identifier-leading doc comment.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/restapi/route_details_handler_test.go`:
- Around line 152-172: Replace the manual httptest.NewServer setup in the route
details test with the established serveApiAndRetrieveEndpoint helper. Extend
that helper only as needed to support sending the If-None-Match request header,
while preserving the existing ETag, cache-control, and 304 response assertions.
- Around line 157-171: In the HTTP response test flow, replace the non-fatal
error assertions after http.Get and http.DefaultClient.Do with fatal checks that
stop execution before resp or resp2 are dereferenced. Preserve the existing
response assertions and cleanup for successful requests.
---
Outside diff comments:
In `@internal/models/route_details.go`:
- Around line 4-26: Document the exported AgencyAndId, NewAgencyAndId,
RouteDetailsEntry, and NewRouteDetailsEntry declarations in
internal/models/route_details.go with identifier-leading Go doc comments. In
internal/restapi/route_details_handler_test.go, rename RouteDetailsResponse to
routeDetailsResponse or add an identifier-leading doc comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 16f6cf4d-86d4-4939-a09e-c1f65fa1c579
📒 Files selected for processing (3)
internal/models/route_details.gointernal/restapi/route_details_handler.gointernal/restapi/route_details_handler_test.go
Extend serveApiAndRetrieveEndpoint to accept request headers and use it in caching tests instead of manual httptest.NewServer setup. Add identifier-leading Go doc comments for exported types and functions in models/route_details.go. Rename RouteDetailsResponse to routeDetailsResponse.
|
Hi @RaginiSharma01 ! Thanks again for the great work on this PR. Just wanted to give you a quick status update on my end:
I'll get back to you soon with the results of the testing and any final updates. Thanks! |
The legacy Java API always includes polylines in the route-details endpoint and explicitly provides a "heuristic" stop grouping fallback. Client applications rely on this precise structural shape. This change enables polylines during stop processing and duplicates the standard direction groupings, relabeling the copies as "heuristic", achieving full 1-to-1 structural parity with the production API. Unit tests are updated to assert the presence of both grouping types.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/restapi/route_details_handler_test.go`:
- Around line 43-45: Extend the success test assertions for entry.StopGroupings
to verify that at least one grouping has a non-empty polylines/map-shape
payload, while preserving the existing type assertions for “heuristic” and
“direction”.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b96044fb-7f78-44b3-a552-2b444f7650b5
📒 Files selected for processing (4)
internal/models/route_details.gointernal/restapi/http_test.gointernal/restapi/route_details_handler.gointernal/restapi/route_details_handler_test.go
The legacy Java API includes an empty "subGroups" array in the route details response for each stop group. This field was previously omitted in Maglev because it was not defined in the model. This change adds the SubGroups field to the StopGroup struct and explicitly initializes it as an empty slice during stop group construction. This ensures it marshals to an empty array rather than null, achieving full parity with the production API shape.
Address final SonarCloud and CodeRabbit review feedback. - Refactored routeDetailsHandler to extract database lookups (GetAgency and GetRoute) into discrete helper functions (getAgencyForHandler and getRouteForHandler), reducing Cognitive Complexity from 16 to 10 (well within the limit of 15). - Expanded TestRouteDetailsHandler_Success to assert that at least one stop group within the groupings contains a non-empty polylines array, ensuring spatial payloads are correctly mapped.
|
I pushed a few cleanup commits to your branch to bring us to 100% API parity. So much work T_T Changes added to your branch:
Final tasks to wrap up this PR:
Let me know if you need help... |
API Parity VerificationI just ran the parity tests against multiple GTFS feeds comparing the Maglev local response to the legacy Java production response. Everything looks incredibly clean! The structural schema perfectly matches Java . Any minor discrepancies in polylines or route references are purely due to expected dataset differences between the test feeds and production, not the codebase logic. We are in great shape... |
|
@ARCoder181105 , Thank you for this ;) .. |
|
|
Hi @ARCoder181105 — I've made both fixes you asked for: and also tested via postman |
|
Hey @ARCoder181105 , i just wanted to ask, that i had raised my pr with the fixes could you please review it. |
|
Hi @RaginiSharma01 will get back as soon as possible... |
2 participants
#1040
PR description:
Added a new endpoint: GET /api/where/route-details/{id}. It returns the stops and route shape for a specific route, so client apps can draw maps of the route.
What I changed
Which this i tested the new route on postman
response.json
Summary by CodeRabbit
/api/where/route-details/{id}that returns stop groupings (with polylines and heuristic groupings) plus reference data, optionally including situation/alert references.timeorserviceDate(via existing behavior).time, missing/invalid API keys, and dates with no active service/trips.ETag/Cache-Controlwith304 Not Modified,includeReferences=false, and deterministic reference ordering.