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 index 794d9785934..24beb48e845 100644 --- 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 index e42d7414620..b2d66bdb5c4 100644 --- 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 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 map(Collection 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 index 88a32c0f95e..afbb105aa27 100644 --- 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 index cf0ac3d0b3f..64fafcb58b3 100644 --- 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 index a1a03226014..a5df87bc4ce 100644 --- 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 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 index 513c96edd13..20cc574c383 100644 --- 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 index d96d447ca5e..a68a9682371 100644 --- 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 index ebeaccbda6a..413a7f651a2 100644 --- 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 index f58355dbd84..bace306745b 100644 --- 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 index b7a772e0cb5..a36b6611368 100644 --- 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() - ); - } }