Skip to content

Fix crash on itineraries with frequencies where distanceMeters was not computed #6580

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

Merged
merged 8 commits into from
Apr 3, 2025

Conversation

epascal
Copy link
Contributor

@epascal epascal commented Mar 27, 2025

PR Instructions

When creating a pull request, please follow the format below. For each section, replace the
guidance text with your own text, keeping the section heading. If you have nothing to say in a
particular section, you can completely delete the section including its heading to indicate that you
have taken the requested steps. None of these instructions or the guidance text (non-heading text)
should be present in the submitted PR. These sections serve as a checklist: when you have replaced
or deleted all of them, the PR is considered complete. As of 2021, most regular OTP contributors
participate in our twice-weekly conference calls. For all but the simplest and smallest PRs,
participation in these discussions is necessary to facilitate the review and merge process. Other
developers can ask questions and provide immediate feedback on technical design and code style, and
resolve any concerns about long term maintenance and comprehension of new code.

Summary

When I was computing itineraries based on frequencies rather that stop times in GTFS there was an error saying distanceMeters was missing

Issue

Closes #6522

Unit tests

Write a few words on how the new code is tested.

  • Were unit tests added/updated?
    No
  • Was any manual verification done?
    Yes
  • Any observations on changes to performance?
    None
  • Was the code designed so it is unit testable?
    No change needed but a new unit test could be created
  • Were any tests applied to the smallest appropriate unit?
    No
  • Do all tests
    pass the continuous integration service
    Yes

Documentation

  • Have you added documentation in code covering design and rationale behind the code?
    No
  • Were all non-trivial public classes and methods documented with Javadoc?
    No
  • Were any new configuration options added? If so were the tables in
    the configuration documentation updated?

Changelog

The changelog file
is generated from the pull-request title, make sure the title describe the feature or issue fixed.
To exclude the PR from the changelog add the label skip changelog to the PR.

Bumping the serialization version id

If you have made changes to the way the routing graph is serialized, for example by renaming a field
in one of the edges, then you must add the label bump serialization id to the PR. With this label
Github Actions will increase the field otp.serialization.version.id in pom.xml.

@epascal epascal requested a review from a team as a code owner March 27, 2025 17:10
@@ -0,0 +1,76 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

There is a lot of stuff in this PR that is not Java code. Can you fix that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, that's why I was hesitating to push there is only one file changed that matter, do I need to make a new change request once I have fixed the branch ?

Copy link
Member

Choose a reason for hiding this comment

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

no, you can just do git push --force on your branch.

@scottgrunerud
Copy link

Thank you for putting up this PR can't wait for it to merge, @leonardehrenfried. After this is merged, do you know how soon the fix will be available on Docker Hub?

@VillePihlava
Copy link
Contributor

@epascal you should do this to remove the unnecessary files from the commit history:

  1. Make a local backup branch in order to accidentally not destroy your changes
  2. git checkout Fix-issue-#6569
  3. git reset <hash of commit before your changes>
    • You can use git log to view your commit history
  4. git add application/src/main/java/org/opentripplanner/routing/algorithm/mapping/RaptorPathToItineraryMapper.java
  5. git commit -m "your message here"
  6. git push --force

@epascal
Copy link
Contributor Author

epascal commented Mar 29, 2025

I removed the unrelated files, is it ok for you

@VillePihlava
Copy link
Contributor

I'm not sure what the policy has been for these situations in this project. @leonardehrenfried is keeping the commit history ok, or should it be cleaned up?

Copy link

codecov bot commented Mar 29, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.29%. Comparing base (1732221) to head (a79a766).
Report is 48 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...algorithm/mapping/RaptorPathToItineraryMapper.java 66.66% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6580      +/-   ##
=============================================
- Coverage      70.29%   70.29%   -0.01%     
+ Complexity     18408    18407       -1     
=============================================
  Files           2088     2088              
  Lines          77488    77489       +1     
  Branches        7871     7871              
=============================================
- Hits           54470    54469       -1     
- Misses         20243    20244       +1     
- Partials        2775     2776       +1     

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@leonardehrenfried
Copy link
Member

I'm not sure what the policy has been for these situations in this project. @leonardehrenfried is keeping the commit history ok, or should it be cleaned up?

We don't have a formal policy. If it's deemed "too ugly" then we just squash-merge the PR.

@optionsome optionsome self-requested a review April 1, 2025 08:36
@leonardehrenfried
Copy link
Member

leonardehrenfried commented Apr 2, 2025

Reminder for whoever merges this: use Github's squash functionality.

@optionsome optionsome added this to the 2.8 (next release) milestone Apr 3, 2025
@optionsome optionsome merged commit 3be47b6 into opentripplanner:dev-2.x Apr 3, 2025
6 checks passed
t2gran pushed a commit that referenced this pull request Apr 3, 2025
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.

NullPointerException when fetching distanceMeters in GTFS GraphQL API
5 participants