Skip to content

Validate stop id in Transit leg reference - #5440

Merged
vpaturet merged 3 commits into
opentripplanner:dev-2.xfrom
entur:validate_stop_in_scheduled_transit_leg_reference
Oct 24, 2023
Merged

Validate stop id in Transit leg reference#5440
vpaturet merged 3 commits into
opentripplanner:dev-2.xfrom
entur:validate_stop_in_scheduled_transit_leg_reference

Conversation

@vpaturet

Copy link
Copy Markdown
Contributor

Summary

As detailed in #5423, transit leg references currently refer to stop position, not stop id: if the trip pattern is changed by a real-time update, the leg reference could point to the wrong physical stop.

This PR verifies that the referenced stop positions still refer to the same stop ids in the actual pattern.
As an exception, the reference is considered valid if the referenced stop is different but belongs to the same parent station: this covers for example the case of a last-minute platform change in a train station that typically does not affect the validity of the leg.

This PR modifies the structure of the serialized leg id by adding new fields, but guarantees that existing (legacy) references will be deserialized successfully.

Issue

Partially addresses #5423

Unit tests

Added unit tests

Documentation

No

@vpaturet vpaturet added the !Improvement A functional improvement or micro feature label Oct 20, 2023
@vpaturet vpaturet self-assigned this Oct 20, 2023
@vpaturet
vpaturet force-pushed the validate_stop_in_scheduled_transit_leg_reference branch from dc73bdd to fff75a1 Compare October 20, 2023 08:33
@codecov

codecov Bot commented Oct 20, 2023

Copy link
Copy Markdown

Codecov Report

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

Comparison is base (c44b66c) 66.67% compared to head (562609b) 66.81%.
Report is 75 commits behind head on dev-2.x.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5440      +/-   ##
=============================================
+ Coverage      66.67%   66.81%   +0.13%     
- Complexity     15396    15470      +74     
=============================================
  Files           1794     1798       +4     
  Lines          69683    69808     +125     
  Branches        7345     7360      +15     
=============================================
+ Hits           46462    46641     +179     
+ Misses         20758    20711      -47     
+ Partials        2463     2456       -7     
Files Coverage Δ
...pentripplanner/model/plan/ScheduledTransitLeg.java 90.57% <100.00%> (+0.13%) ⬆️
...nner/model/plan/legreference/LegReferenceType.java 88.88% <77.77%> (+11.96%) ⬆️
...lan/legreference/ScheduledTransitLegReference.java 81.66% <77.27%> (-2.55%) ⬇️
...odel/plan/legreference/LegReferenceSerializer.java 63.15% <57.89%> (-9.92%) ⬇️

... and 29 files with indirect coverage changes

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

@vpaturet
vpaturet force-pushed the validate_stop_in_scheduled_transit_leg_reference branch from fff75a1 to c2be864 Compare October 20, 2023 08:44
@vpaturet
vpaturet marked this pull request as ready for review October 20, 2023 08:55
@vpaturet
vpaturet requested a review from a team as a code owner October 20, 2023 08:55

@leonardehrenfried leonardehrenfried left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please have a look at my comments.

@vpaturet

vpaturet commented Oct 20, 2023

Copy link
Copy Markdown
Contributor Author

Added a more robust versioning logic. This will make it easier to implement further modifications in the leg serialized id structure

ScheduledTransitLegReference.class,
LegReferenceSerializer::writeScheduledTransitLegV2,
LegReferenceSerializer::readScheduledTransitLegV2
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The design is a bit awkward. But, we can leave it for now. I would love to make a common TokenSerializer witch encapsulate the read/write operations, version and base64 encoding. This should be used in paging as well. I think I will create a task for it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See issue #5451

@vpaturet
vpaturet requested a review from t2gran October 24, 2023 10:44
@vpaturet
vpaturet merged commit 8394b7c into opentripplanner:dev-2.x Oct 24, 2023
@vpaturet
vpaturet deleted the validate_stop_in_scheduled_transit_leg_reference branch October 24, 2023 12:07
t2gran pushed a commit that referenced this pull request Oct 24, 2023
@t2gran t2gran added this to the 2.5 (next release) milestone Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Improvement A functional improvement or micro feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants