opentripplanner / OpenTripPlanner

An open source multi-modal trip planner
http://www.opentripplanner.org
Other
2.18k stars 1.02k forks source link

Require valid polygons for AreaStop #5915

Closed leonardehrenfried closed 3 months ago

leonardehrenfried commented 3 months ago

Summary

For Netex it is required that the imported polygons for AreaStops are valid and not (for example) self-intersecting.

So far GTFS allowed you to import invalid polygons, which mostly worked, but sometimes didn't. As it was not spelled out in the GTFS spec, I am currently proposing a clarification in the spec to change this: https://github.com/google/transit/pull/476 It contains also a good example why invalid polygons are problematic.

For this reason I'm proposing that all AreaStop geometries be valid and the check can happen in the constructor.

This PR also renames the GTFS mapper since the entity it is mapping to has been renamed quite a while ago. I'm also removing an old integration test that contained an invalid polygon.

Issue

https://github.com/opentripplanner/OpenTripPlanner/pull/5457

Unit tests

Added.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 69.43%. Comparing base (67e6510) to head (ccbce81). Report is 50 commits behind head on dev-2.x.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev-2.x #5915 +/- ## ============================================= - Coverage 69.46% 69.43% -0.03% + Complexity 17068 17059 -9 ============================================= Files 1928 1934 +6 Lines 73580 73620 +40 Branches 7550 7541 -9 ============================================= + Hits 51109 51117 +8 - Misses 19847 19874 +27 - Partials 2624 2629 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

vpaturet commented 3 months ago

what happens when the polygon is not closed? In this case a Polygon instance cannot even be created.

  public static final Polygon NON_CLOSED = FAC.createPolygon(
    new Coordinate[] {
      Coordinates.of(59.62575084033623, 6.3023991052849),
      Coordinates.of(59.62883380609349, 6.289718020117876),
      Coordinates.of(59.6346950024935, 6.293494451572027),
    }
  );

I assume this gets caught in the OneBusAway code. Does it get mapped to an issue as well?

leonardehrenfried commented 3 months ago

@vpaturet I've now added that adds an issue and returns null when the polygon is not a closed ring. That will probably fail with a confusing NPE elsewhere.

vpaturet commented 3 months ago

Indeed that will fail with NPE here for instance: https://github.com/opentripplanner/OpenTripPlanner/blob/c0ebcbef4a88af8752ad3dd26bc866932edd5bf2/src/main/java/org/opentripplanner/transit/model/site/GroupStopBuilder.java#L73 and in other places. Trading an IllegalArgumentException for a NullPointerException does not really help.