Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue where stop points are sometimes added twice to index #5552

Conversation

habrahamsson-skanetrafiken
Copy link
Contributor

Summary

When netex data contains duplicate JourneyPatterns the scheduledStopPointsIndex will get duplicates of all the stop points for those JourneyPatterns during graph build. When the graph is loaded this will result in strange IndexOutOfBoundsExceptions in the TransferMapper.

Issue

NetexMapper uses a Multimap for the scheduledStopPointsIndex. But the semantics of a multimap is that putting to a key that already exists will extend any existing data that has already been added. This is not really the behaviour we want in this case.

This PR fixes this issue by replacing the Multimap with a Map<List>. This way we will replace the current values when calling putAll().

I suppose that duplicate JourneyPatterns could cause all kinds of other problems, so to solve the underlying issue we should really add some validation of the netex data for this. But I think these changes are worthwhile anyway, since it makes the code less brittle.

Unit tests

There aren't any tests for this code that I could find, and writing them feels a bit out of scope for this PR.

Documentation

Nothing needed.

Bumping the serialization version id

Nope

This only happens when there are duplicate JourneyPattern in the netex
files.
@habrahamsson-skanetrafiken habrahamsson-skanetrafiken requested a review from a team as a code owner December 6, 2023 15:00
Copy link

codecov bot commented Dec 6, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (a4eb912) 67.19% compared to head (a66b534) 67.18%.
Report is 1 commits behind head on dev-2.x.

Files Patch % Lines
.../opentripplanner/netex/mapping/TransferMapper.java 0.00% 12 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5552      +/-   ##
=============================================
- Coverage      67.19%   67.18%   -0.01%     
- Complexity     16040    16041       +1     
=============================================
  Files           1850     1850              
  Lines          70921    70922       +1     
  Branches        7410     7409       -1     
=============================================
- Hits           47653    47652       -1     
- Misses         20801    20802       +1     
- Partials        2467     2468       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@t2gran t2gran added this to the 2.5 (next release) milestone Dec 6, 2023
@t2gran t2gran added the Bug label Dec 6, 2023
@t2gran
Copy link
Member

t2gran commented Dec 7, 2023

When a duplicate is encountered it should result in an issue being added to the issue report. It might/might not be the case here, can you test if that is the case @jtorin ? If not we should add that, you may merge this PR and add another on or just append a commit this this - it is up to you.

@habrahamsson-skanetrafiken
Copy link
Contributor Author

@t2gran That's a good point. I'll merge this and add that in a separate PR.

@habrahamsson-skanetrafiken habrahamsson-skanetrafiken merged commit aa34cde into opentripplanner:dev-2.x Dec 12, 2023
5 checks passed
t2gran pushed a commit that referenced this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants