Require valid polygons for AreaStop

[leonardehrenfried]

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: google/transit#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

#5457

Unit tests

Added.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

@@              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     

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

[vpaturet]

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]

@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]

Indeed that will fail with NPE here for instance:

OpenTripPlanner/src/main/java/org/opentripplanner/transit/model/site/GroupStopBuilder.java

Line 73 in c0ebcbe

location.getStopType() == StopType.REGULAR ||

and in other places.
Trading an IllegalArgumentException for a NullPointerException does not really help.

[optionsome]

This class has weird naming but I guess that's not our fault.

[optionsome]

So are there some geometries which don't throw an exception but are still considered invalid by the validator? Do these cause issues during runtime?

[leonardehrenfried]

The routing sometimes (mostly?) works anyway but the vector tiles library refuses to serialise these geometries.

[optionsome]

Can these be called open polygons? I tried quickly reading about this and maybe the open is a bit vague so lets stick with nonClosed.

[leonardehrenfried]

We talked about this last week and the handling of the incomplete polygons was removed from this PR.