Fix quay duplicates overwriting stop index

[bas-hbt]

Issue

closes #4949

[codecov]

Codecov Report

Patch coverage: 96.42% and no project coverage change.

Comparison is base (279b058) 64.06% compared to head (284c5f5) 64.07%.

❗ Current head 284c5f5 differs from pull request most recent head 4130fde. Consider uploading reports for the commit 4130fde to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             dev-2.x    #4964   +/-   ##
==========================================
  Coverage      64.06%   64.07%           
- Complexity     13579    13589   +10     
==========================================
  Files           1675     1675           
  Lines          66229    66243   +14     
  Branches        7147     7147           
==========================================
+ Hits           42429    42443   +14     
- Misses         21428    21429    +1     
+ Partials        2372     2371    -1     

Impacted Files	Coverage Δ
.../org/opentripplanner/netex/mapping/QuayMapper.java	89.28% <95.00%> (+3.57%)	⬆️
...org/opentripplanner/netex/mapping/NetexMapper.java	82.30% <100.00%> (+0.07%)	⬆️
...g/opentripplanner/netex/mapping/StationMapper.java	64.40% <100.00%> (+2.58%)	⬆️
...ripplanner/netex/mapping/StopAndStationMapper.java	85.36% <100.00%> (+0.18%)	⬆️
...ripplanner/transit/model/framework/EntityById.java	92.59% <100.00%> (+0.28%)	⬆️

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[leonardehrenfried]

The commit list contains some which are not part of the PR. Can you please rebase to clean this up?

[leonardehrenfried]

I think you made it worse. Can you please use git rebase?

Alternatively, we can also squash everything into a single commit when we merge this.

[vpaturet]

I miss the big picture, could you add a few words in the PR description to explain what was wrong and how you solved it?

[vpaturet]

I cannot see any code path that leads to a null quay. You could remove the null check and annotate the method parameter with a Nonnull annotation

[loj-hbt]

I miss the big picture, could you add a few words in the PR description to explain what was wrong and how you solved it?

The problem was that our data contains no shared data for stations and stops so that all of these are declared in multiple files. This lead to a situation where boarding and alighting was only possible for the last line referencing this quay that was read.
Whenever a quay was declared another time, a new RegularStop with a new index got added to the map in the StopModelBuilder. The old index was no longer valid but was still referenced from trips. We fixed this by checking if the Quay already existed and returning the corresponding reference if present.
We did the same for the Station although that did not cause the initial problem. This makes sure that all child stops get referenced correctly.

[vpaturet]

The rest of the codebase uses @javax.annotation.Nonnull rather than @jakarta.validation.constraints.NotNull

[loj-hbt]

Oh good point, I will change that

[leonardehrenfried]

Can you resolve the conflicts?

[loj-hbt]

Well it looks like I broke it again

[leonardehrenfried]

In this case you could have just merged dev-2.x. We don't require a linear history.

[2martens]

The hickup was caused by the following:

• local branch was rebased successfully onto dev-2.x
• remote branch on GitHub still contained the previous version
• local changes (history changes) were attempted to push (not force-push), Git rejects
• subsequently a merge occurred with the remote branch and the result pushed
• consequence: branch-off point remained the same as the remote branch had before and all commits added to dev-2.x. in the meantime were now part of the branch history

Solution: merge was reverted locally and then correctly force-pushed. History should now be clean.

[leonardehrenfried]

Thanks for solving this.

Something to remember for the future: it's preferable to not rebase/force-push after the review has started because this means the "review state" (which files have been changed since you last viewed) is wiped out.