From 78afddb4e1de31b8173b71a5a579984204017a18 Mon Sep 17 00:00:00 2001 From: Merlin Unterfinger Date: Wed, 5 Jun 2024 10:16:07 +0200 Subject: [PATCH 1/4] FIX: NAV-90 - Forbid nullable on gtfs builder for add stops; If the parent_station column is mapped, then there will always be a string value, but most probably an empty one. --- .../schedule/model/GtfsScheduleBuilder.java | 42 +++++++++++++------ 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/src/main/java/ch/naviqore/gtfs/schedule/model/GtfsScheduleBuilder.java b/src/main/java/ch/naviqore/gtfs/schedule/model/GtfsScheduleBuilder.java index 7a7898e4..f9b8966c 100644 --- a/src/main/java/ch/naviqore/gtfs/schedule/model/GtfsScheduleBuilder.java +++ b/src/main/java/ch/naviqore/gtfs/schedule/model/GtfsScheduleBuilder.java @@ -50,14 +50,24 @@ public GtfsScheduleBuilder addAgency(String id, String name, String url, String return this; } - public GtfsScheduleBuilder addStop(String id, String name, @Nullable String parentStop, double lat, double lon) { + public GtfsScheduleBuilder addStop(String id, String name, double lat, double lon) { + addStop(id, name, lat, lon, null); + return this; + } + + public GtfsScheduleBuilder addStop(String id, String name, double lat, double lon, String parentStopId) { checkNotBuilt(); if (stops.containsKey(id)) { throw new IllegalArgumentException("Stop " + id + " already exists"); } log.debug("Adding stop {}", id); Stop stop = new Stop(id, name, new GeoCoordinate(lat, lon)); - parents.computeIfAbsent(parentStop, ignored -> new ArrayList<>()).add(stop); + + // only add stop id if it is not a blank string + if (!parentStopId.isEmpty()) { + parents.computeIfAbsent(parentStopId, ignored -> new ArrayList<>()).add(stop); + } + stops.put(id, stop); return this; } @@ -171,18 +181,9 @@ public GtfsScheduleBuilder addTransfer(String fromStopId, String toStopId, Trans */ public GtfsSchedule build() { checkNotBuilt(); - log.info("Building schedule with {} stops, {} routes and {} trips", stops.size(), routes.size(), trips.size()); - // set parents for child - parents.forEach((parentId, children) -> { - Stop parent = stops.get(parentId); - if (parent == null) { - log.warn("Parent {} does not exist", parentId); - } else { - parent.setChildren(children); - children.forEach(child -> child.setParent(parent)); - } - }); + log.info("Building schedule with {} stops, {} routes and {} trips", stops.size(), routes.size(), trips.size()); + setParentChildrenStopRelations(); // initialize: make immutable and resize arrays to capacity trips.values().parallelStream().forEach(Initializable::initialize); @@ -197,6 +198,21 @@ public GtfsSchedule build() { return schedule; } + private void setParentChildrenStopRelations() { + parents.forEach((parentId, children) -> { + Stop parent = stops.get(parentId); + + if (parent == null) { + throw new IllegalStateException("Parent stop " + parentId + " for children " + children.stream() + .map(Stop::getId) + .toList() + " does not exist."); + } + + parent.setChildren(children); + children.forEach(child -> child.setParent(parent)); + }); + } + /** * Resets the builder to its initial state, allowing it to be reused. */ From 92a3f1056cb9557a39fe009c61ed47ad9a38c9d7 Mon Sep 17 00:00:00 2001 From: Merlin Unterfinger Date: Wed, 5 Jun 2024 10:19:08 +0200 Subject: [PATCH 2/4] FIX: NAV-90 - Forbid nullable on gtfs builder for add stops; If the parent_station column is mapped, then there will always be a string value, but most probably an empty one. --- .../ch/naviqore/gtfs/schedule/model/GtfsScheduleBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/ch/naviqore/gtfs/schedule/model/GtfsScheduleBuilder.java b/src/main/java/ch/naviqore/gtfs/schedule/model/GtfsScheduleBuilder.java index f9b8966c..c7c3a967 100644 --- a/src/main/java/ch/naviqore/gtfs/schedule/model/GtfsScheduleBuilder.java +++ b/src/main/java/ch/naviqore/gtfs/schedule/model/GtfsScheduleBuilder.java @@ -51,7 +51,7 @@ public GtfsScheduleBuilder addAgency(String id, String name, String url, String } public GtfsScheduleBuilder addStop(String id, String name, double lat, double lon) { - addStop(id, name, lat, lon, null); + addStop(id, name, lat, lon, ""); return this; } From 176d1126332cd7652f67cfaa0af967a9aa356efb Mon Sep 17 00:00:00 2001 From: Merlin Unterfinger Date: Wed, 5 Jun 2024 10:19:54 +0200 Subject: [PATCH 3/4] FIX: NAV-90 - Adjust parser to check for empty strings in parent stop id --- .../ch/naviqore/gtfs/schedule/GtfsScheduleParser.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/java/ch/naviqore/gtfs/schedule/GtfsScheduleParser.java b/src/main/java/ch/naviqore/gtfs/schedule/GtfsScheduleParser.java index 6e0076d4..4bc2cfb0 100644 --- a/src/main/java/ch/naviqore/gtfs/schedule/GtfsScheduleParser.java +++ b/src/main/java/ch/naviqore/gtfs/schedule/GtfsScheduleParser.java @@ -81,13 +81,13 @@ private void parseCalendarDate(CSVRecord record) { } private void parseStop(CSVRecord record) { - String parentId = null; if (record.isMapped("parent_station")) { - parentId = record.get("parent_station"); + builder.addStop(record.get("stop_id"), record.get("stop_name"), Double.parseDouble(record.get("stop_lat")), + Double.parseDouble(record.get("stop_lon")), record.get("parent_station").trim()); + } else { + builder.addStop(record.get("stop_id"), record.get("stop_name"), Double.parseDouble(record.get("stop_lat")), + Double.parseDouble(record.get("stop_lon"))); } - builder.addStop(record.get("stop_id"), record.get("stop_name"), parentId, - Double.parseDouble(record.get("stop_lat")), Double.parseDouble(record.get("stop_lon"))); - } private void parseRoute(CSVRecord record) { From b71ce916b3ecc9881a0913af911d48683d7ff6a3 Mon Sep 17 00:00:00 2001 From: Merlin Unterfinger Date: Wed, 5 Jun 2024 10:29:53 +0200 Subject: [PATCH 4/4] FIX: NAV-90 - Remove parent ids in tests --- .../naviqore/gtfs/schedule/model/GtfsScheduleTestBuilder.java | 2 +- .../impl/transfer/SameStationTransferGeneratorTest.java | 4 ++-- .../service/impl/transfer/WalkTransferGeneratorTest.java | 3 +-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/test/java/ch/naviqore/gtfs/schedule/model/GtfsScheduleTestBuilder.java b/src/test/java/ch/naviqore/gtfs/schedule/model/GtfsScheduleTestBuilder.java index dab2f3fc..ef47eb88 100644 --- a/src/test/java/ch/naviqore/gtfs/schedule/model/GtfsScheduleTestBuilder.java +++ b/src/test/java/ch/naviqore/gtfs/schedule/model/GtfsScheduleTestBuilder.java @@ -177,7 +177,7 @@ private void addStops(Route route) { for (String stopId : route.stops) { if (!addedStops.contains(stopId)) { Stop stop = STOPS.get(stopId); - builder.addStop(stop.id, stop.id, stop.id, stop.lat, stop.lon); + builder.addStop(stop.id, stop.id, stop.lat, stop.lon); addedStops.add(stopId); } } diff --git a/src/test/java/ch/naviqore/service/impl/transfer/SameStationTransferGeneratorTest.java b/src/test/java/ch/naviqore/service/impl/transfer/SameStationTransferGeneratorTest.java index bc1c8a44..a23099e5 100644 --- a/src/test/java/ch/naviqore/service/impl/transfer/SameStationTransferGeneratorTest.java +++ b/src/test/java/ch/naviqore/service/impl/transfer/SameStationTransferGeneratorTest.java @@ -38,8 +38,8 @@ class CreateTransfers { @BeforeEach void setUp() { GtfsScheduleBuilder builder = GtfsSchedule.builder(); - builder.addStop("stop1", "Zürich, Stadelhofen", "stop1", 47.366542, 8.548384); - builder.addStop("stop2", "Zürich, Opernhaus", "stop2", 47.365030, 8.547976); + builder.addStop("stop1", "Zürich, Stadelhofen", 47.366542, 8.548384); + builder.addStop("stop2", "Zürich, Opernhaus", 47.365030, 8.547976); schedule = builder.build(); } diff --git a/src/test/java/ch/naviqore/service/impl/transfer/WalkTransferGeneratorTest.java b/src/test/java/ch/naviqore/service/impl/transfer/WalkTransferGeneratorTest.java index d1034838..2bbc356d 100644 --- a/src/test/java/ch/naviqore/service/impl/transfer/WalkTransferGeneratorTest.java +++ b/src/test/java/ch/naviqore/service/impl/transfer/WalkTransferGeneratorTest.java @@ -92,8 +92,7 @@ private static void assertTransfersGenerated(List tr void setUp() { GtfsScheduleBuilder builder = GtfsSchedule.builder(); TEST_STOPS.values() - .forEach(stopData -> builder.addStop(stopData.id(), stopData.name(), stopData.id(), stopData.lat(), - stopData.lon())); + .forEach(stopData -> builder.addStop(stopData.id(), stopData.name(), stopData.lat(), stopData.lon())); schedule = builder.build(); spatialIndex = new KDTreeBuilder().addLocations(schedule.getStops().values()).build(); generator = new WalkTransferGenerator(DEFAULT_CALCULATOR, DEFAULT_MINIMUM_TRANSFER_TIME,