Convert exception into data import issue

opentripplanner · Jun 19, 2024 · 98588b8 · 98588b8
1 parent 1ddadf2
commit 98588b8
Show file tree

Hide file tree

Showing 10 changed files with 69 additions and 37 deletions.
diff --git a/src/main/java/org/opentripplanner/graph_builder/issue/api/Issue.java b/src/main/java/org/opentripplanner/graph_builder/issue/api/Issue.java
@@ -1,5 +1,7 @@
 package org.opentripplanner.graph_builder.issue.api;
 
+import org.opentripplanner.framework.tostring.ToStringBuilder;
+
 /**
  * Generic issue type, which can be used to create issues.
  */
@@ -32,4 +34,13 @@ public String getType() {
   public String getMessage() {
     return String.format(message, arguments);
   }
+
+  @Override
+  public String toString() {
+    return ToStringBuilder
+      .of(this.getClass())
+      .addStr("type", type)
+      .addStr("message", getMessage())
+      .toString();
+  }
 }
diff --git a/src/main/java/org/opentripplanner/gtfs/mapping/AreaStopMapper.java b/src/main/java/org/opentripplanner/gtfs/mapping/AreaStopMapper.java
@@ -6,11 +6,14 @@
 import java.util.HashMap;
 import java.util.Map;
 import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.operation.valid.IsValidOp;
 import org.onebusaway.gtfs.model.Location;
 import org.opentripplanner.framework.collection.MapUtils;
 import org.opentripplanner.framework.geometry.GeometryUtils;
 import org.opentripplanner.framework.geometry.UnsupportedGeometryException;
 import org.opentripplanner.framework.i18n.NonLocalizedString;
+import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore;
+import org.opentripplanner.graph_builder.issue.api.Issue;
 import org.opentripplanner.transit.model.site.AreaStop;
 import org.opentripplanner.transit.service.StopModelBuilder;
 
@@ -19,9 +22,11 @@ public class AreaStopMapper {
 
   private final Map<Location, AreaStop> mappedLocations = new HashMap<>();
   private final StopModelBuilder stopModelBuilder;
+  private final DataImportIssueStore issueStore;
 
-  public AreaStopMapper(StopModelBuilder stopModelBuilder) {
+  public AreaStopMapper(StopModelBuilder stopModelBuilder, DataImportIssueStore issueStore) {
     this.stopModelBuilder = stopModelBuilder;
+    this.issueStore = issueStore;
   }
 
   Collection<AreaStop> map(Collection<Location> allLocations) {
@@ -36,9 +41,24 @@ AreaStop map(Location orginal) {
   private AreaStop doMap(Location gtfsLocation) {
     var name = NonLocalizedString.ofNullable(gtfsLocation.getName());
     try {
+      var id = mapAgencyAndId(gtfsLocation.getId());
       Geometry geometry = GeometryUtils.convertGeoJsonToJtsGeometry(gtfsLocation.getGeometry());
+      var isValidOp = new IsValidOp(geometry);
+      if (!isValidOp.isValid()) {
+        var error = isValidOp.getValidationError();
+        issueStore.add(
+          Issue.issue(
+            "InvalidFlexAreaGeometry",
+            "GTFS flex location %s has an invalid geometry: %s at (lat: %s, lon: %s)",
+            id,
+            error.getMessage(),
+            error.getCoordinate().y,
+            error.getCoordinate().x
+          )
+        );
+      }
       return stopModelBuilder
-        .areaStop(mapAgencyAndId(gtfsLocation.getId()))
+        .areaStop(id)
         .withName(name)
         .withUrl(NonLocalizedString.ofNullable(gtfsLocation.getUrl()))
         .withDescription(NonLocalizedString.ofNullable(gtfsLocation.getDescription()))

diff --git a/src/main/java/org/opentripplanner/gtfs/mapping/GTFSToOtpTransitServiceMapper.java b/src/main/java/org/opentripplanner/gtfs/mapping/GTFSToOtpTransitServiceMapper.java
@@ -110,7 +110,7 @@ public GTFSToOtpTransitServiceMapper(
     entranceMapper = new EntranceMapper(translationHelper, stationLookup);
     pathwayNodeMapper = new PathwayNodeMapper(translationHelper, stationLookup);
     boardingAreaMapper = new BoardingAreaMapper(translationHelper, stopLookup);
-    areaStopMapper = new AreaStopMapper(builder.stopModel());
+    areaStopMapper = new AreaStopMapper(builder.stopModel(), issueStore);
     locationGroupMapper = new LocationGroupMapper(stopMapper, areaStopMapper, builder.stopModel());
     // the use of stop areas were reverted in the spec
     // this code will go away, please migrate now!

diff --git a/src/main/java/org/opentripplanner/transit/model/site/AreaStop.java b/src/main/java/org/opentripplanner/transit/model/site/AreaStop.java
@@ -6,7 +6,6 @@
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import org.locationtech.jts.geom.Geometry;
-import org.locationtech.jts.operation.valid.IsValidOp;
 import org.opentripplanner.framework.geometry.WgsCoordinate;
 import org.opentripplanner.framework.i18n.I18NString;
 import org.opentripplanner.framework.i18n.NonLocalizedString;
@@ -53,17 +52,7 @@ public class AreaStop
     this.url = builder.url();
     this.zoneId = builder.zoneId();
     this.geometry = builder.geometry();
-    if (!this.geometry.isValid()) {
-      var error = new IsValidOp(this.geometry).getValidationError();
-      throw new IllegalArgumentException(
-        "Polygon geometry for AreaStop %s is invalid: %s at (lat: %s, lon: %s)".formatted(
-            getId(),
-            error.getMessage(),
-            error.getCoordinate().y,
-            error.getCoordinate().x
-          )
-      );
-    }
+
     this.centroid = Objects.requireNonNull(builder.centroid());
   }
 

diff --git a/src/test/java/org/opentripplanner/gtfs/mapping/AreaStopMapperTest.java b/src/test/java/org/opentripplanner/gtfs/mapping/AreaStopMapperTest.java
@@ -2,8 +2,8 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertThrows;
 
+import java.util.List;
 import java.util.stream.Stream;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
@@ -13,6 +13,9 @@
 import org.onebusaway.gtfs.model.AgencyAndId;
 import org.onebusaway.gtfs.model.Location;
 import org.opentripplanner._support.geometry.Polygons;
+import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore;
+import org.opentripplanner.graph_builder.issue.api.Issue;
+import org.opentripplanner.graph_builder.issue.service.DefaultDataImportIssueStore;
 import org.opentripplanner.transit.service.StopModel;
 
 class AreaStopMapperTest {
@@ -24,9 +27,9 @@ static Stream<Arguments> testCases() {
   @ParameterizedTest(name = "a name of <{0}> should set bogusName={1}")
   @MethodSource("testCases")
   void testMapping(String name, boolean isBogusName) {
-    final var gtfsLocation = getLocation(name, Polygons.OSLO);
+    var gtfsLocation = getLocation(name, Polygons.OSLO);
 
-    var mapper = new AreaStopMapper(StopModel.of());
+    var mapper = new AreaStopMapper(StopModel.of(), DataImportIssueStore.NOOP);
     var flexLocation = mapper.map(gtfsLocation);
 
     assertEquals(isBogusName, flexLocation.hasFallbackName());
@@ -39,8 +42,22 @@ void invalidPolygon() {
 
     var gtfsLocation = getLocation("invalid", selfIntersecting);
 
-    var mapper = new AreaStopMapper(StopModel.of());
-    assertThrows(IllegalArgumentException.class, () -> mapper.map(gtfsLocation));
+    var issueStore = new DefaultDataImportIssueStore();
+    var mapper = new AreaStopMapper(StopModel.of(), issueStore);
+
+    mapper.map(gtfsLocation);
+
+    assertEquals(
+      List
+        .of(
+          Issue.issue(
+            "InvalidFlexAreaGeometry",
+            "GTFS flex location 1:zone-3 has an invalid geometry: Self-intersection at (lat: 1.0, lon: 2.0)"
+          )
+        )
+        .toString(),
+      issueStore.listIssues().toString()
+    );
   }
 
   private static Location getLocation(String name, Polygon polygon) {

diff --git a/src/test/java/org/opentripplanner/gtfs/mapping/LocationGroupMapperTest.java b/src/test/java/org/opentripplanner/gtfs/mapping/LocationGroupMapperTest.java
@@ -9,6 +9,7 @@
 import org.onebusaway.gtfs.model.AgencyAndId;
 import org.onebusaway.gtfs.model.LocationGroup;
 import org.onebusaway.gtfs.model.Stop;
+import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore;
 import org.opentripplanner.transit.model.framework.FeedScopedId;
 import org.opentripplanner.transit.model.site.StopLocation;
 import org.opentripplanner.transit.service.StopModel;
@@ -22,7 +23,7 @@ void map() {
     var builder = StopModel.of();
     var mapper = new LocationGroupMapper(
       new StopMapper(new TranslationHelper(), id -> null, builder),
-      new AreaStopMapper(builder),
+      new AreaStopMapper(builder, DataImportIssueStore.NOOP),
       builder
     );
 

diff --git a/src/test/java/org/opentripplanner/gtfs/mapping/StopAreaMapperTest.java b/src/test/java/org/opentripplanner/gtfs/mapping/StopAreaMapperTest.java
@@ -1,6 +1,7 @@
 package org.opentripplanner.gtfs.mapping;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.opentripplanner.graph_builder.issue.api.DataImportIssueStore.NOOP;
 
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -23,7 +24,7 @@ class StopAreaMapperTest {
   void map() {
     var stopModel = StopModel.of();
     var stopMapper = new StopMapper(new TranslationHelper(), ignored -> null, stopModel);
-    var locationMapper = new AreaStopMapper(stopModel);
+    var locationMapper = new AreaStopMapper(stopModel, NOOP);
     var mapper = new StopAreaMapper(stopMapper, locationMapper, stopModel);
 
     var area = new Area();

diff --git a/src/test/java/org/opentripplanner/gtfs/mapping/StopTimeMapperTest.java b/src/test/java/org/opentripplanner/gtfs/mapping/StopTimeMapperTest.java
@@ -76,7 +76,10 @@ public class StopTimeMapperTest {
     stopModelBuilder
   );
   private final BookingRuleMapper bookingRuleMapper = new BookingRuleMapper();
-  private final AreaStopMapper areaStopMapper = new AreaStopMapper(stopModelBuilder);
+  private final AreaStopMapper areaStopMapper = new AreaStopMapper(
+    stopModelBuilder,
+    DataImportIssueStore.NOOP
+  );
   private final LocationGroupMapper locationGroupMapper = new LocationGroupMapper(
     stopMapper,
     areaStopMapper,

diff --git a/src/test/java/org/opentripplanner/gtfs/mapping/TransferMapperTest.java b/src/test/java/org/opentripplanner/gtfs/mapping/TransferMapperTest.java
@@ -46,7 +46,10 @@ public class TransferMapperTest {
   );
   private static final BookingRuleMapper BOOKING_RULE_MAPPER = new BookingRuleMapper();
 
-  private static final AreaStopMapper LOCATION_MAPPER = new AreaStopMapper(STOP_MODEL_BUILDER);
+  private static final AreaStopMapper LOCATION_MAPPER = new AreaStopMapper(
+    STOP_MODEL_BUILDER,
+    ISSUE_STORE
+  );
 
   private static final LocationGroupMapper LOCATION_GROUP_MAPPER = new LocationGroupMapper(
     STOP_MAPPER,

diff --git a/src/test/java/org/opentripplanner/transit/model/site/AreaStopTest.java b/src/test/java/org/opentripplanner/transit/model/site/AreaStopTest.java
@@ -4,7 +4,6 @@
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotSame;
 import static org.junit.jupiter.api.Assertions.assertSame;
-import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import org.junit.jupiter.api.Test;
@@ -82,16 +81,4 @@ void sameAs() {
       subject.sameAs(subject.copy().withGeometry(GeometryUtils.makeLineString(0, 0, 0, 2)).build())
     );
   }
-
-  @Test
-  void invalidGeometry() {
-    var ex = assertThrows(
-      IllegalArgumentException.class,
-      () -> areaStopBuilder().withGeometry(Polygons.SELF_INTERSECTING).build()
-    );
-    assertEquals(
-      "Polygon geometry for AreaStop F:1 is invalid: Self-intersection at (lat: 1.0, lon: 2.0)",
-      ex.getMessage()
-    );
-  }
 }