Skip to content

Don't expect every pattern in a route to have the specified stop with constrained transfers - #3868

Merged
optionsome merged 2 commits into
opentripplanner:dev-2.xfrom
HSLdevcom:fix-gtfs-constrained-transfer
Feb 10, 2022
Merged

Don't expect every pattern in a route to have the specified stop with constrained transfers#3868
optionsome merged 2 commits into
opentripplanner:dev-2.xfrom
HSLdevcom:fix-gtfs-constrained-transfer

Conversation

@optionsome

Copy link
Copy Markdown
Member

Summary

Adds validation for indexing constrained transfers so that it can handle situations where a route doesn't have a stop specified in a transfer in every pattern.

Issue

closes #3867

Unit tests

Should they be added?

Code style

Have you followed the suggested code style?
Yes

Documentation

not needed

Changelog

From title

@optionsome
optionsome requested a review from a team as a code owner February 4, 2022 19:53
@optionsome optionsome added the !Bug Apply to issues describing a bug and PRs witch fixes it. label Feb 4, 2022
@optionsome

Copy link
Copy Markdown
Member Author

I'm not sure if I like the way I've fixed this issue. However, this was the easiest way to fix it but I'm more than happy to change it to something else. Below are some alternative ways to do it:

  • Validate that the stop exists in the pattern before fetching its index
  • Return -1 as index if it doesn't exist on the pattern and check that it's not -1 before adding the transfer point. This would either require to change the existing uses of the function that currently throws the exception or implement another version of it that doesn't throw an exception-
  • Change the exception to be less generic

@leonardehrenfried

Copy link
Copy Markdown
Member

I would also prefer a solution that doesn't use exceptions as a control flow statements.

Is this due to invalid input data or is this a legal state that we have to take into account when building the index?

@optionsome

Copy link
Copy Markdown
Member Author

This is a legal state and it's even expected to happen quite often so logging when it happens doesn't really make sense

@optionsome
optionsome force-pushed the fix-gtfs-constrained-transfer branch from f862ac5 to 263cd75 Compare February 8, 2022 15:19
@optionsome

Copy link
Copy Markdown
Member Author

I rewrote it by returning -1 when stop is not on pattern. Should I add tests for this somewhere?

@t2gran t2gran added this to the 2.1 milestone Feb 8, 2022
Comment thread src/main/java/org/opentripplanner/model/StopPattern.java
@leonardehrenfried

Copy link
Copy Markdown
Member

Wasn't there any calling code that needed to be updated?

@optionsome

Copy link
Copy Markdown
Member Author

Wasn't there any calling code that needed to be updated?

Didn't seem like so. Most places called different method for finding stop index in the stop pattern.

@optionsome
optionsome merged commit 3525d4d into opentripplanner:dev-2.x Feb 10, 2022
@optionsome
optionsome deleted the fix-gtfs-constrained-transfer branch February 10, 2022 07:30
t2gran pushed a commit that referenced this pull request Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Bug Apply to issues describing a bug and PRs witch fixes it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Timed transfer between two routes on a pair of stops crashes OTP when loading graph

4 participants