Skip to content

fix(api): respect includeReferences query parameter #1049

Open
ARCoder181105 wants to merge 5 commits into
OneBusAway:mainfrom
ARCoder181105:fix/1047-include-references
Open

fix(api): respect includeReferences query parameter #1049
ARCoder181105 wants to merge 5 commits into
OneBusAway:mainfrom
ARCoder181105:fix/1047-include-references

Conversation

@ARCoder181105

@ARCoder181105 ARCoder181105 commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Description
This PR resolves the spec gap where the includeReferences query parameter was being ignored, causing the API to unconditionally fetch and populate reference entities.

When includeReferences=false is requested, the stop_handler now short-circuits the database queries for routes, agencies, stops, and situations. It returns the required references block with empty arrays, fulfilling the specification requirements and improving endpoint performance.

Changes Made

  • Wrapped the reference population logic in stop_handler.go inside a ShouldIncludeReferences(r) condition.
  • Added TestStopHandler_IncludeReferences to verify that passing includeReferences=false successfully bypasses the reference lookups without affecting the main entry payload.

Closes: #1047

Summary by CodeRabbit

  • New Features

    • The stop endpoint supports an includeReferences query parameter to control whether related references (routes, agencies, parent-stop route references, and alert situations) are populated.
    • When includeReferences=false the response still contains a references object but its reference arrays are empty.
  • Tests

    • Added test coverage verifying includeReferences behavior (references populated by default; empty arrays when false).

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@ARCoder181105, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 24 minutes and 57 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 00920343-5d93-4981-b302-5552c408946a

📥 Commits

Reviewing files that changed from the base of the PR and between 99539b5 and d7d87d2.

📒 Files selected for processing (2)
  • internal/restapi/route_handler.go
  • internal/restapi/stop_handler.go
📝 Walkthrough

Walkthrough

The stop handler now respects the includeReferences query flag: when includeReferences=false the response returns the same stop entry but the references block is present with all arrays empty; when true (default) references are populated as before.

Changes

Stop Handler includeReferences Query Support

Layer / File(s) Summary
Conditional reference population logic
internal/restapi/stop_handler.go
stopHandler initializes an empty references struct and gates all reference enrichment—routes, agencies, parent stop, and alert situations—behind ShouldIncludeReferences(r). When the guard is false the handler returns empty reference arrays.
Test coverage for includeReferences behavior
internal/restapi/stop_handler_test.go
New TestStopHandler_IncludeReferences validates both paths: default inclusion populates agencies and routes, and includeReferences=false returns 200 OK with the core entry unchanged and all reference arrays empty.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • OneBusAway/maglev#1024: Touches parent-stop references/RouteIDs generation which is affected by conditional references population.
  • OneBusAway/maglev#1019: Introduces/uses ShouldIncludeReferences(r) for conditional references in other handlers.
  • OneBusAway/maglev#999: Adds tests around references.stops contents that relate to this conditional population change.

Suggested reviewers

  • burma-shave
  • aaronbrethorst
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(api): respect includeReferences query parameter' clearly and concisely describes the main change: making the API respect the includeReferences query parameter that was previously being ignored.
Linked Issues check ✅ Passed The PR successfully addresses all coding requirements from issue #1047: it extracts and respects the includeReferences parameter, conditionally populates references based on ShouldIncludeReferences(r), and includes comprehensive unit tests verifying correct behavior.
Out of Scope Changes check ✅ Passed All changes are directly scoped to resolving issue #1047: modifications to stop_handler.go conditionally populate references, and new test coverage validates the includeReferences parameter behavior without introducing unrelated changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/stop_handler_test.go`:
- Around line 534-539: The test omits asserting that references.stopTimes is
empty for the includeReferences=false case; add an assertion like
assert.Empty(t, modelFalse.Data.References.StopTimes, "StopTimes must be empty
when includeReferences=false") in the same block alongside the existing
assertions (referencing modelFalse.Data.References.StopTimes) so the test
verifies all reference arrays per the OpenAPI spec.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b7fe59ba-4a3e-4ac5-abed-4413ace1391f

📥 Commits

Reviewing files that changed from the base of the PR and between b491551 and bb42224.

📒 Files selected for processing (2)
  • internal/restapi/stop_handler.go
  • internal/restapi/stop_handler_test.go

Comment thread internal/restapi/stop_handler_test.go
@ARCoder181105

Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/stop_handler.go`:
- Around line 87-103: The includeReferences=true branch in the stop handler
currently ignores errors from GetAgency and returns 200 with partial data;
update the code that calls s.db.GetAgency (in the stop handler function handling
includeReferences) to check the returned error: if err == sql.ErrNoRows return
the same NotFound/404 response used elsewhere, if err != nil return an
operational error/500 (or propagate via the existing error helper), and only add
agency references to the response when GetAgency succeeds; mirror the explicit
error handling used by the other handlers instead of swallowing failures.
- Around line 85-103: references.Agencies is iterated directly causing
nondeterministic ordering; to fix, in the handler that builds stop response
(look for references.Agencies usage in stop_handler.go) first collect the map
keys into a slice, sort that slice (e.g., sort.Strings or appropriate
comparator), then iterate the sorted keys to fetch values and append to the
response so the resulting agencies list is stable across requests; update the
loop that currently ranges over references.Agencies to use the sorted key slice
and append in that deterministic order.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: eed2e230-8243-4b66-b820-1267c2c623e9

📥 Commits

Reviewing files that changed from the base of the PR and between b491551 and 99539b5.

📒 Files selected for processing (2)
  • internal/restapi/stop_handler.go
  • internal/restapi/stop_handler_test.go

Comment thread internal/restapi/stop_handler.go
Comment thread internal/restapi/stop_handler.go
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

spec-gap: includeReferences query parameter is ignored

1 participant