Skip to content

fix: require transfers stop ids when transfer_type is empty - #2177

Open
yasumorishima wants to merge 2 commits into
MobilityData:masterfrom
yasumorishima:fix/transfers-empty-transfer-type
Open

fix: require transfers stop ids when transfer_type is empty#2177
yasumorishima wants to merge 2 commits into
MobilityData:masterfrom
yasumorishima:fix/transfers-empty-transfer-type

Conversation

@yasumorishima

Copy link
Copy Markdown

Summary:

Closes #2105.

The spec change in google/transit@a806576 (google/transit#591) made transfers.from_stop_id and transfers.to_stop_id Required if transfer_type is empty, 0, 1, 2 or 3 — previously the reference only required them for 1, 2 and 3.

TransferStopIdsConditionalValidator wraps its per-row check in if (transfer.hasTransferType()), so rows whose transfer_type is empty are skipped entirely and the newly required case is never validated. An empty transfer_type is a recommended transfer point, which is the same as transfer_type=0 — a value the validator already treats as requiring both stop ids.

This drops that guard so every row is checked. In-seat transfer types (4 and 5) keep their exemption through the existing isTransferTypeInSeat check, so the only behavior that changes is the empty case.

Expected behavior:

transfers.txt row before after
transfer_type empty, no from_stop_id / to_stop_id no notice two missing_required_field notices, one per field
transfer_type empty, both stop ids present no notice no notice
transfer_type 03, stop ids missing two notices unchanged
transfer_type 4 or 5, stop ids missing no notice unchanged

No new notice type is introduced — this reuses missing_required_field — so RULES.md needs no change.

Two unit tests are added for the empty transfer_type case, which the existing tests never covered (every existing case calls setTransferType()).

One note on CI: I ran the checks on my fork first. Format code, Test Package Document (test on Java 17 for both ubuntu-latest and windows-latest) and Package Installers are green. Rule acceptance tests cannot run on a fork — its get-reports matrix requires the ubuntu-latest-4-cores runner label, which forks do not have — so that comparison needs to run here. Since this rule newly fires on real feeds that leave transfer_type empty, the acceptance report is worth a look before merging.

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the unit tests with gradle test to make sure you didn't break anything
  • Add or update any needed documentation to the repo — no documentation change needed, see above
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues
  • Include screenshot(s) showing how this pull request works and fixes the issue(s) — not applicable to a validation rule; the added unit tests show the behavior instead

@CLAassistant

CLAassistant commented Aug 26, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@emmambd
emmambd requested a review from skalexch August 27, 2026 16:24

@skalexch skalexch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good on my side!

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.

Require from_stop_id and to_stop_id in recommended transfer point transfers (empty or 0)

3 participants