From b2e67553196f5383ed60c327fa911b513aab67fc Mon Sep 17 00:00:00 2001 From: miles-grant-ibi Date: Mon, 25 Dec 2023 17:53:33 -0500 Subject: [PATCH 1/7] correct shared stops validator behaviors --- .../jobs/validation/SharedStopsValidator.java | 71 +++++++++++++------ 1 file changed, 50 insertions(+), 21 deletions(-) diff --git a/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java b/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java index d608fe249..209aefd4b 100644 --- a/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java +++ b/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java @@ -13,10 +13,8 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.HashMap; import java.util.HashSet; import java.util.List; -import java.util.Set; public class SharedStopsValidator extends FeedValidator { @@ -58,11 +56,40 @@ public void validate() { CsvReader configReader = CsvReader.parse(config); - // TODO: pull indicies from the CSV header - final int STOP_GROUP_ID_INDEX = 0; - final int STOP_ID_INDEX = 2; - final int IS_PRIMARY_INDEX = 3; - final int FEED_ID_INDEX = 1; + int STOP_GROUP_ID_INDEX = -1; + int STOP_ID_INDEX = -1; + int IS_PRIMARY_INDEX = -1; + int FEED_ID_INDEX = -1; + + try { + configReader.setHeaders(new String[]{"stop_group_id", "feed_id", "stop_id", "is_primary"}); + String[] headers = configReader.getHeaders(); + for (int i = 0; i < headers.length; i++) { + String header = headers[i]; + switch(header) { + case "stop_group_id": + STOP_GROUP_ID_INDEX = i; + break; + case "feed_id": + FEED_ID_INDEX = i; + break; + case "stop_id": + STOP_ID_INDEX = i; + break; + case "is_primary": + IS_PRIMARY_INDEX = i; + break; + default: + throw new RuntimeException("shared_stops.csv contained invalid headers!"); + } + } + + if (STOP_GROUP_ID_INDEX==-1 || FEED_ID_INDEX==-1 || STOP_ID_INDEX==-1 || IS_PRIMARY_INDEX==-1) { + throw new RuntimeException("shared_stops.csv is missing headers!"); + } + } catch (IOException e) { + throw new RuntimeException("shared_stops.csv was invalid! " + e.toString()); + } // Build list of stop Ids. List stopIds = new ArrayList<>(); @@ -74,7 +101,7 @@ public void validate() { } // Initialize hashmaps to hold duplication info - HashMap> stopGroupStops = new HashMap<>(); + HashSet seenStopIds = new HashSet<>(); HashSet stopGroupsWithPrimaryStops = new HashSet<>(); try { @@ -88,26 +115,28 @@ public void validate() { continue; } - stopGroupStops.putIfAbsent(stopGroupId, new HashSet<>()); - Set seenStopIds = stopGroupStops.get(stopGroupId); - // Check for SS_01 (stop id appearing in multiple stop groups) + // Make sure this error is only returned if we are inside the feed that is being checked if (seenStopIds.contains(stopId)) { - registerError(stops - .stream() - .filter(stop -> stop.stop_id.equals(stopId)) - .findFirst() - .orElse(stops.get(0)), NewGTFSErrorType.MULTIPLE_SHARED_STOPS_GROUPS - ); + if (this.feedId.equals(sharedStopFeedId)) { + registerError(stops + .stream() + .filter(stop -> stop.stop_id.equals(stopId)) + .findFirst() + .orElse(new Stop()), NewGTFSErrorType.MULTIPLE_SHARED_STOPS_GROUPS + ); + } } else { seenStopIds.add(stopId); } // Check for SS_02 (multiple primary stops per stop group) - if (stopGroupsWithPrimaryStops.contains(stopGroupId)) { - registerError(NewGTFSError.forFeed(NewGTFSErrorType.SHARED_STOP_GROUP_MUTLIPLE_PRIMARY_STOPS, stopGroupId)); - } else if (configReader.get(IS_PRIMARY_INDEX).equals("true")) { - stopGroupsWithPrimaryStops.add(stopGroupId); + if (configReader.get(IS_PRIMARY_INDEX).equals("1") || configReader.get(IS_PRIMARY_INDEX).equals("true")) { + if (stopGroupsWithPrimaryStops.contains(stopGroupId)) { + registerError(NewGTFSError.forFeed(NewGTFSErrorType.SHARED_STOP_GROUP_MUTLIPLE_PRIMARY_STOPS, stopGroupId)); + } else { + stopGroupsWithPrimaryStops.add(stopGroupId); + } } // Check for SS_03 (stop_id referenced doesn't exist) From 6ad9abfce4f3fd21df1f0dbf74e2e2876dbb1e1b Mon Sep 17 00:00:00 2001 From: miles-grant-ibi Date: Mon, 25 Dec 2023 17:56:49 -0500 Subject: [PATCH 2/7] fix formatting --- .../datatools/manager/jobs/validation/SharedStopsValidator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java b/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java index 209aefd4b..d06dd0729 100644 --- a/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java +++ b/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java @@ -84,7 +84,7 @@ public void validate() { } } - if (STOP_GROUP_ID_INDEX==-1 || FEED_ID_INDEX==-1 || STOP_ID_INDEX==-1 || IS_PRIMARY_INDEX==-1) { + if (STOP_GROUP_ID_INDEX == -1 || FEED_ID_INDEX == -1 || STOP_ID_INDEX == -1 || IS_PRIMARY_INDEX == -1) { throw new RuntimeException("shared_stops.csv is missing headers!"); } } catch (IOException e) { From 8eabd138e54ce55e4ba6c4d1cfc42450bf4b02bf Mon Sep 17 00:00:00 2001 From: miles-grant-ibi Date: Fri, 5 Jan 2024 20:44:40 +0100 Subject: [PATCH 3/7] refactor: address pr feedback --- .../manager/jobs/validation/SharedStopsValidator.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java b/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java index d06dd0729..83d4dd0d7 100644 --- a/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java +++ b/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java @@ -62,11 +62,13 @@ public void validate() { int FEED_ID_INDEX = -1; try { - configReader.setHeaders(new String[]{"stop_group_id", "feed_id", "stop_id", "is_primary"}); + configReader.readRecord(); + + configReader.setHeaders(configReader.getRawRecord().split(",")); String[] headers = configReader.getHeaders(); for (int i = 0; i < headers.length; i++) { String header = headers[i]; - switch(header) { + switch (header) { case "stop_group_id": STOP_GROUP_ID_INDEX = i; break; @@ -88,7 +90,7 @@ public void validate() { throw new RuntimeException("shared_stops.csv is missing headers!"); } } catch (IOException e) { - throw new RuntimeException("shared_stops.csv was invalid! " + e.toString()); + throw new RuntimeException("shared_stops.csv was invalid!", e); } // Build list of stop Ids. @@ -118,7 +120,7 @@ public void validate() { // Check for SS_01 (stop id appearing in multiple stop groups) // Make sure this error is only returned if we are inside the feed that is being checked if (seenStopIds.contains(stopId)) { - if (this.feedId.equals(sharedStopFeedId)) { + if (feedId.equals(sharedStopFeedId)) { registerError(stops .stream() .filter(stop -> stop.stop_id.equals(stopId)) From a55e37e864e61e08cef8c97f1cee55ae59b41acb Mon Sep 17 00:00:00 2001 From: miles-grant-ibi Date: Fri, 5 Jan 2024 21:00:07 +0100 Subject: [PATCH 4/7] deps: update gtfs-lib --- pom.xml | 2 +- .../datatools/manager/jobs/validation/SharedStopsValidator.java | 2 +- .../java/com/conveyal/datatools/manager/models/FeedVersion.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pom.xml b/pom.xml index 31fbf057d..3e478f354 100644 --- a/pom.xml +++ b/pom.xml @@ -272,7 +272,7 @@ com.github.ibi-group gtfs-lib - 3fa0da9 + 761456fdc66bc1b72fc860b5e5853b0f2904e009 diff --git a/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java b/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java index 83d4dd0d7..b185d0f87 100644 --- a/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java +++ b/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java @@ -135,7 +135,7 @@ public void validate() { // Check for SS_02 (multiple primary stops per stop group) if (configReader.get(IS_PRIMARY_INDEX).equals("1") || configReader.get(IS_PRIMARY_INDEX).equals("true")) { if (stopGroupsWithPrimaryStops.contains(stopGroupId)) { - registerError(NewGTFSError.forFeed(NewGTFSErrorType.SHARED_STOP_GROUP_MUTLIPLE_PRIMARY_STOPS, stopGroupId)); + registerError(NewGTFSError.forFeed(NewGTFSErrorType.SHARED_STOP_GROUP_MULTIPLE_PRIMARY_STOPS, stopGroupId)); } else { stopGroupsWithPrimaryStops.add(stopGroupId); } diff --git a/src/main/java/com/conveyal/datatools/manager/models/FeedVersion.java b/src/main/java/com/conveyal/datatools/manager/models/FeedVersion.java index 060f8589b..071ff9556 100644 --- a/src/main/java/com/conveyal/datatools/manager/models/FeedVersion.java +++ b/src/main/java/com/conveyal/datatools/manager/models/FeedVersion.java @@ -517,7 +517,7 @@ public boolean hasBlockingIssuesForPublishing() { NewGTFSErrorType.TABLE_IN_SUBDIRECTORY, NewGTFSErrorType.WRONG_NUMBER_OF_FIELDS, NewGTFSErrorType.MULTIPLE_SHARED_STOPS_GROUPS, - NewGTFSErrorType.SHARED_STOP_GROUP_MUTLIPLE_PRIMARY_STOPS + NewGTFSErrorType.SHARED_STOP_GROUP_MULTIPLE_PRIMARY_STOPS )); } From 7521cc7b5a40d52717e60dce4f9766b8d2358063 Mon Sep 17 00:00:00 2001 From: miles-grant-ibi Date: Thu, 11 Jan 2024 21:09:47 +0200 Subject: [PATCH 5/7] extract testable functionality to unit tests --- .../jobs/validation/SharedStopsValidator.java | 67 ++++++++++++------- .../validation/SharedStopsValidatorTest.java | 51 ++++++++++++++ .../canHandleVariousCorrectCSV-0.json | 6 ++ .../canHandleVariousCorrectCSV-1.json | 6 ++ .../canHandleVariousCorrectCSV-2.json | 6 ++ 5 files changed, 113 insertions(+), 23 deletions(-) create mode 100644 src/test/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest.java create mode 100644 src/test/resources/snapshots/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest/canHandleVariousCorrectCSV-0.json create mode 100644 src/test/resources/snapshots/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest/canHandleVariousCorrectCSV-1.json create mode 100644 src/test/resources/snapshots/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest/canHandleVariousCorrectCSV-2.json diff --git a/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java b/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java index b185d0f87..dfc795879 100644 --- a/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java +++ b/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java @@ -13,9 +13,17 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; +enum SharedStopsHeader { + STOP_GROUP_ID_INDEX, + STOP_ID_INDEX, + IS_PRIMARY_INDEX, + FEED_ID_INDEX +} public class SharedStopsValidator extends FeedValidator { private static final Logger LOG = LoggerFactory.getLogger(SharedStopsValidator.class); @@ -47,52 +55,62 @@ public SharedStopsValidator(Feed feed, SQLErrorStorage errorStorage, Project pro this.feedId = feedId; } - @Override - public void validate() { - String config = project.sharedStopsConfig; - if (config == null) { - return; - } - - CsvReader configReader = CsvReader.parse(config); + public static String[] getHeaders(CsvReader configReader) throws IOException { + configReader.setHeaders(configReader.getRawRecord().split(",")); + return configReader.getHeaders(); + } - int STOP_GROUP_ID_INDEX = -1; - int STOP_ID_INDEX = -1; - int IS_PRIMARY_INDEX = -1; - int FEED_ID_INDEX = -1; + public static Map getHeaderIncedes(CsvReader configReader) { + HashMap map = new HashMap(); try { configReader.readRecord(); + String[] headers = getHeaders(configReader); - configReader.setHeaders(configReader.getRawRecord().split(",")); - String[] headers = configReader.getHeaders(); for (int i = 0; i < headers.length; i++) { String header = headers[i]; switch (header) { case "stop_group_id": - STOP_GROUP_ID_INDEX = i; + map.put(SharedStopsHeader.STOP_GROUP_ID_INDEX, i); break; case "feed_id": - FEED_ID_INDEX = i; + map.put(SharedStopsHeader.FEED_ID_INDEX, i); break; case "stop_id": - STOP_ID_INDEX = i; + map.put(SharedStopsHeader.STOP_ID_INDEX, i); break; case "is_primary": - IS_PRIMARY_INDEX = i; + map.put(SharedStopsHeader.IS_PRIMARY_INDEX, i); break; default: throw new RuntimeException("shared_stops.csv contained invalid headers!"); } } - if (STOP_GROUP_ID_INDEX == -1 || FEED_ID_INDEX == -1 || STOP_ID_INDEX == -1 || IS_PRIMARY_INDEX == -1) { + // TODO: some way to generate this from the enum? + if (!map.containsKey(SharedStopsHeader.STOP_GROUP_ID_INDEX) || + !map.containsKey(SharedStopsHeader.FEED_ID_INDEX) || + !map.containsKey(SharedStopsHeader.STOP_ID_INDEX) || + !map.containsKey(SharedStopsHeader.IS_PRIMARY_INDEX)) { throw new RuntimeException("shared_stops.csv is missing headers!"); } } catch (IOException e) { throw new RuntimeException("shared_stops.csv was invalid!", e); } + return map; + } + + @Override + public void validate() { + String config = project.sharedStopsConfig; + if (config == null) { + return; + } + + CsvReader configReader = CsvReader.parse(config); + + // Build list of stop Ids. List stopIds = new ArrayList<>(); List stops = new ArrayList<>(); @@ -102,15 +120,18 @@ public void validate() { stopIds.add(stop.stop_id); } + Map indeces = getHeaderIncedes(configReader); + // Initialize hashmaps to hold duplication info HashSet seenStopIds = new HashSet<>(); HashSet stopGroupsWithPrimaryStops = new HashSet<>(); try { while (configReader.readRecord()) { - String stopGroupId = configReader.get(STOP_GROUP_ID_INDEX); - String stopId = configReader.get(STOP_ID_INDEX); - String sharedStopFeedId = configReader.get(FEED_ID_INDEX); + // TODO: Avoid casting here? It must be possible with the enums + String stopGroupId = configReader.get((Integer) indeces.get(SharedStopsHeader.STOP_GROUP_ID_INDEX)); + String stopId = configReader.get((Integer) indeces.get(SharedStopsHeader.STOP_ID_INDEX)); + String sharedStopFeedId = configReader.get((Integer) indeces.get(SharedStopsHeader.FEED_ID_INDEX)); if (stopId.equals("stop_id")) { // Swallow header row @@ -133,7 +154,7 @@ public void validate() { } // Check for SS_02 (multiple primary stops per stop group) - if (configReader.get(IS_PRIMARY_INDEX).equals("1") || configReader.get(IS_PRIMARY_INDEX).equals("true")) { + if (configReader.get((Integer) indeces.get(SharedStopsHeader.IS_PRIMARY_INDEX)).equals("1") || configReader.get((Integer) indeces.get(SharedStopsHeader.IS_PRIMARY_INDEX)).equals("true")) { if (stopGroupsWithPrimaryStops.contains(stopGroupId)) { registerError(NewGTFSError.forFeed(NewGTFSErrorType.SHARED_STOP_GROUP_MULTIPLE_PRIMARY_STOPS, stopGroupId)); } else { diff --git a/src/test/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest.java b/src/test/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest.java new file mode 100644 index 000000000..9ef157393 --- /dev/null +++ b/src/test/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest.java @@ -0,0 +1,51 @@ +package com.conveyal.datatools.manager.jobs.validation; + +import com.conveyal.datatools.UnitTest; +import com.csvreader.CsvReader; +import org.junit.jupiter.api.Test; + +import java.util.Map; + +import static com.zenika.snapshotmatcher.SnapshotMatcher.matchesSnapshot; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +public class SharedStopsValidatorTest extends UnitTest { + private void attemptToParseInvalidCSV(String csv, String expectedError) { + CsvReader configReader = CsvReader.parse(csv); + Exception exception = assertThrows(RuntimeException.class, () -> { + SharedStopsValidator.getHeaderIncedes(configReader); + }); + + String error = exception.getMessage(); + assertEquals(expectedError, error); + } + + private Map parseValidCSV(String csv) { + CsvReader configReader = CsvReader.parse(csv); + return SharedStopsValidator.getHeaderIncedes(configReader); + } + + @Test + void canHandleIncorrectCsv() { + String invalid = "shared_stops.csv contained invalid headers!"; + String missing = "shared_stops.csv is missing headers!"; + + attemptToParseInvalidCSV("this is a great string, but it's not a shared_stops csv!", invalid); + attemptToParseInvalidCSV("", invalid); + attemptToParseInvalidCSV("is_primary,stop_id", missing); + attemptToParseInvalidCSV("is_primary,feed_id,,stop_group_id", invalid); + attemptToParseInvalidCSV("is_primary,feed_id,stop_group_id", missing); + attemptToParseInvalidCSV("feed_id, is_primary, stop_group_id,stop_id", invalid); + } + + @Test + void canHandleVariousCorrectCSV() { + assertThat(parseValidCSV("is_primary,feed_id,stop_id,stop_group_id"), matchesSnapshot()); + assertThat(parseValidCSV("feed_id,stop_id,stop_group_id,is_primary"), matchesSnapshot()); + + // Only last is handled + assertThat(parseValidCSV("feed_id,is_primary,stop_group_id,stop_id,feed_id"), matchesSnapshot()); + } +} diff --git a/src/test/resources/snapshots/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest/canHandleVariousCorrectCSV-0.json b/src/test/resources/snapshots/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest/canHandleVariousCorrectCSV-0.json new file mode 100644 index 000000000..efafa6fbc --- /dev/null +++ b/src/test/resources/snapshots/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest/canHandleVariousCorrectCSV-0.json @@ -0,0 +1,6 @@ +{ + "STOP_GROUP_ID_INDEX" : 3, + "STOP_ID_INDEX" : 2, + "IS_PRIMARY_INDEX" : 0, + "FEED_ID_INDEX" : 1 +} \ No newline at end of file diff --git a/src/test/resources/snapshots/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest/canHandleVariousCorrectCSV-1.json b/src/test/resources/snapshots/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest/canHandleVariousCorrectCSV-1.json new file mode 100644 index 000000000..0a5c3f7a5 --- /dev/null +++ b/src/test/resources/snapshots/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest/canHandleVariousCorrectCSV-1.json @@ -0,0 +1,6 @@ +{ + "STOP_GROUP_ID_INDEX" : 2, + "STOP_ID_INDEX" : 1, + "IS_PRIMARY_INDEX" : 3, + "FEED_ID_INDEX" : 0 +} \ No newline at end of file diff --git a/src/test/resources/snapshots/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest/canHandleVariousCorrectCSV-2.json b/src/test/resources/snapshots/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest/canHandleVariousCorrectCSV-2.json new file mode 100644 index 000000000..351f7a0d6 --- /dev/null +++ b/src/test/resources/snapshots/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest/canHandleVariousCorrectCSV-2.json @@ -0,0 +1,6 @@ +{ + "STOP_GROUP_ID_INDEX" : 2, + "STOP_ID_INDEX" : 3, + "IS_PRIMARY_INDEX" : 1, + "FEED_ID_INDEX" : 4 +} \ No newline at end of file From a2bdbff0f2a0127f532b6b1220227f8b49b8ba31 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Fri, 12 Jan 2024 10:53:33 +0000 Subject: [PATCH 6/7] refactor(Updates to enum use and parameterized unit tests): --- .../jobs/validation/SharedStopsHeader.java | 37 ++++++++ .../jobs/validation/SharedStopsValidator.java | 91 ++++++++----------- .../validation/SharedStopsValidatorTest.java | 68 ++++++++------ 3 files changed, 116 insertions(+), 80 deletions(-) create mode 100644 src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsHeader.java diff --git a/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsHeader.java b/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsHeader.java new file mode 100644 index 000000000..9d63ae7ce --- /dev/null +++ b/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsHeader.java @@ -0,0 +1,37 @@ +package com.conveyal.datatools.manager.jobs.validation; + +import java.util.Map; + +public enum SharedStopsHeader { + + STOP_GROUP_ID_INDEX("stop_group_id"), + + STOP_ID_INDEX("stop_id"), + + IS_PRIMARY_INDEX("is_primary"), + + FEED_ID_INDEX("feed_id"); + + public final String headerName; + + SharedStopsHeader(String name) { + this.headerName = name; + } + + public String getHeaderName() { + return headerName; + } + + public static SharedStopsHeader fromValue(String headerName) { + for (SharedStopsHeader sharedStopsHeader : SharedStopsHeader.values()) { + if (sharedStopsHeader.getHeaderName().equalsIgnoreCase(headerName)) { + return sharedStopsHeader; + } + } + throw new UnsupportedOperationException(String.format("Unknown shared stops header: %s", headerName)); + } + + public static boolean hasRequiredHeaders(Map headerIndices) { + return headerIndices.size() == SharedStopsHeader.values().length; + } +} diff --git a/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java b/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java index dfc795879..a99e686f2 100644 --- a/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java +++ b/src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java @@ -13,17 +13,13 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.HashMap; +import java.util.EnumMap; import java.util.HashSet; import java.util.List; import java.util.Map; -enum SharedStopsHeader { - STOP_GROUP_ID_INDEX, - STOP_ID_INDEX, - IS_PRIMARY_INDEX, - FEED_ID_INDEX -} +import static com.conveyal.datatools.manager.jobs.validation.SharedStopsHeader.STOP_GROUP_ID_INDEX; +import static com.conveyal.datatools.manager.jobs.validation.SharedStopsHeader.STOP_ID_INDEX; public class SharedStopsValidator extends FeedValidator { private static final Logger LOG = LoggerFactory.getLogger(SharedStopsValidator.class); @@ -48,6 +44,7 @@ public SharedStopsValidator buildSharedStopsValidator(Feed feed, SQLErrorStorage } return new SharedStopsValidator(feed, errorStorage, this.project, this.feedId); } + public SharedStopsValidator(Feed feed, SQLErrorStorage errorStorage, Project project, String feedId) { super(feed, errorStorage); this.feed = feed; @@ -60,8 +57,8 @@ public static String[] getHeaders(CsvReader configReader) throws IOException { return configReader.getHeaders(); } - public static Map getHeaderIncedes(CsvReader configReader) { - HashMap map = new HashMap(); + public static Map getHeaderIndices(CsvReader configReader) { + Map headerIndices = new EnumMap<>(SharedStopsHeader.class); try { configReader.readRecord(); @@ -69,36 +66,29 @@ public static Map getHeaderIncedes(CsvReader configR for (int i = 0; i < headers.length; i++) { String header = headers[i]; - switch (header) { - case "stop_group_id": - map.put(SharedStopsHeader.STOP_GROUP_ID_INDEX, i); + switch (SharedStopsHeader.fromValue(header)) { + case STOP_GROUP_ID_INDEX: + headerIndices.put(STOP_GROUP_ID_INDEX, i); break; - case "feed_id": - map.put(SharedStopsHeader.FEED_ID_INDEX, i); + case FEED_ID_INDEX: + headerIndices.put(SharedStopsHeader.FEED_ID_INDEX, i); break; - case "stop_id": - map.put(SharedStopsHeader.STOP_ID_INDEX, i); + case STOP_ID_INDEX: + headerIndices.put(STOP_ID_INDEX, i); break; - case "is_primary": - map.put(SharedStopsHeader.IS_PRIMARY_INDEX, i); + case IS_PRIMARY_INDEX: + headerIndices.put(SharedStopsHeader.IS_PRIMARY_INDEX, i); break; - default: - throw new RuntimeException("shared_stops.csv contained invalid headers!"); } } - // TODO: some way to generate this from the enum? - if (!map.containsKey(SharedStopsHeader.STOP_GROUP_ID_INDEX) || - !map.containsKey(SharedStopsHeader.FEED_ID_INDEX) || - !map.containsKey(SharedStopsHeader.STOP_ID_INDEX) || - !map.containsKey(SharedStopsHeader.IS_PRIMARY_INDEX)) { + if (!SharedStopsHeader.hasRequiredHeaders(headerIndices)) { throw new RuntimeException("shared_stops.csv is missing headers!"); } } catch (IOException e) { throw new RuntimeException("shared_stops.csv was invalid!", e); } - - return map; + return headerIndices; } @Override @@ -110,7 +100,6 @@ public void validate() { CsvReader configReader = CsvReader.parse(config); - // Build list of stop Ids. List stopIds = new ArrayList<>(); List stops = new ArrayList<>(); @@ -120,41 +109,42 @@ public void validate() { stopIds.add(stop.stop_id); } - Map indeces = getHeaderIncedes(configReader); - - // Initialize hashmaps to hold duplication info + // Initialize hashmaps to hold duplication info. HashSet seenStopIds = new HashSet<>(); HashSet stopGroupsWithPrimaryStops = new HashSet<>(); try { + Map headerIndices = getHeaderIndices(configReader); while (configReader.readRecord()) { - // TODO: Avoid casting here? It must be possible with the enums - String stopGroupId = configReader.get((Integer) indeces.get(SharedStopsHeader.STOP_GROUP_ID_INDEX)); - String stopId = configReader.get((Integer) indeces.get(SharedStopsHeader.STOP_ID_INDEX)); - String sharedStopFeedId = configReader.get((Integer) indeces.get(SharedStopsHeader.FEED_ID_INDEX)); + String stopGroupId = configReader.get(headerIndices.get(STOP_GROUP_ID_INDEX)); + String stopId = configReader.get(headerIndices.get(STOP_ID_INDEX)); + String sharedStopFeedId = configReader.get(headerIndices.get(SharedStopsHeader.FEED_ID_INDEX)); - if (stopId.equals("stop_id")) { - // Swallow header row + if (stopId.equals(STOP_ID_INDEX.headerName)) { + // Swallow header row. continue; } - // Check for SS_01 (stop id appearing in multiple stop groups) - // Make sure this error is only returned if we are inside the feed that is being checked + // Check for SS_01 (stop id appearing in multiple stop groups). + // Make sure this error is only returned if we are inside the feed that is being checked. if (seenStopIds.contains(stopId)) { if (feedId.equals(sharedStopFeedId)) { registerError(stops - .stream() - .filter(stop -> stop.stop_id.equals(stopId)) - .findFirst() - .orElse(new Stop()), NewGTFSErrorType.MULTIPLE_SHARED_STOPS_GROUPS + .stream() + .filter(stop -> stop.stop_id.equals(stopId)) + .findFirst() + .orElse(new Stop()), NewGTFSErrorType.MULTIPLE_SHARED_STOPS_GROUPS ); } } else { seenStopIds.add(stopId); } - // Check for SS_02 (multiple primary stops per stop group) - if (configReader.get((Integer) indeces.get(SharedStopsHeader.IS_PRIMARY_INDEX)).equals("1") || configReader.get((Integer) indeces.get(SharedStopsHeader.IS_PRIMARY_INDEX)).equals("true")) { + // Check for SS_02 (multiple primary stops per stop group). + if ( + configReader.get(headerIndices.get(SharedStopsHeader.IS_PRIMARY_INDEX)).equals("1") || + configReader.get(headerIndices.get(SharedStopsHeader.IS_PRIMARY_INDEX)).equals("true") + ) { if (stopGroupsWithPrimaryStops.contains(stopGroupId)) { registerError(NewGTFSError.forFeed(NewGTFSErrorType.SHARED_STOP_GROUP_MULTIPLE_PRIMARY_STOPS, stopGroupId)); } else { @@ -162,17 +152,16 @@ public void validate() { } } - // Check for SS_03 (stop_id referenced doesn't exist) - // Make sure this error is only returned if we are inside the feed that is being checked + // Check for SS_03 (stop_id referenced doesn't exist). + // Make sure this error is only returned if we are inside the feed that is being checked. if (feedId.equals(sharedStopFeedId) && !stopIds.contains(stopId)) { registerError(NewGTFSError.forFeed(NewGTFSErrorType.SHARED_STOP_GROUP_ENTITY_DOES_NOT_EXIST, stopId)); } } - } catch (IOException e) { LOG.error(e.toString()); } - finally { + } catch (IOException e) { + LOG.error("Unable to validate shared stops.", e); + } finally { configReader.close(); } - - } } diff --git a/src/test/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest.java b/src/test/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest.java index 9ef157393..b8a944f71 100644 --- a/src/test/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest.java +++ b/src/test/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidatorTest.java @@ -2,50 +2,60 @@ import com.conveyal.datatools.UnitTest; import com.csvreader.CsvReader; -import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; -import java.util.Map; +import java.util.stream.Stream; import static com.zenika.snapshotmatcher.SnapshotMatcher.matchesSnapshot; import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; -public class SharedStopsValidatorTest extends UnitTest { - private void attemptToParseInvalidCSV(String csv, String expectedError) { - CsvReader configReader = CsvReader.parse(csv); - Exception exception = assertThrows(RuntimeException.class, () -> { - SharedStopsValidator.getHeaderIncedes(configReader); - }); +class SharedStopsValidatorTest extends UnitTest { - String error = exception.getMessage(); - assertEquals(expectedError, error); + @ParameterizedTest + @MethodSource("createCSVHeaders") + void canHandleVariousCorrectCSV(String headers) { + assertThat(SharedStopsValidator.getHeaderIndices(CsvReader.parse(headers)), matchesSnapshot()); } - private Map parseValidCSV(String csv) { - CsvReader configReader = CsvReader.parse(csv); - return SharedStopsValidator.getHeaderIncedes(configReader); + private static Stream createCSVHeaders() { + return Stream.of( + "is_primary,feed_id,stop_id,stop_group_id", + "feed_id,stop_id,stop_group_id,is_primary", + "feed_id,is_primary,stop_group_id,stop_id,feed_id" + ); } - @Test - void canHandleIncorrectCsv() { - String invalid = "shared_stops.csv contained invalid headers!"; - String missing = "shared_stops.csv is missing headers!"; + @ParameterizedTest + @MethodSource("createInvalidCSVArguments") + void attemptToParseInvalidCSV(InvalidCSVArgument invalidCSVArgument) { + CsvReader configReader = CsvReader.parse(invalidCSVArgument.csv); + Exception exception = assertThrows(RuntimeException.class, () -> SharedStopsValidator.getHeaderIndices(configReader)); + assertEquals(invalidCSVArgument.expectedError, exception.getMessage()); + } - attemptToParseInvalidCSV("this is a great string, but it's not a shared_stops csv!", invalid); - attemptToParseInvalidCSV("", invalid); - attemptToParseInvalidCSV("is_primary,stop_id", missing); - attemptToParseInvalidCSV("is_primary,feed_id,,stop_group_id", invalid); - attemptToParseInvalidCSV("is_primary,feed_id,stop_group_id", missing); - attemptToParseInvalidCSV("feed_id, is_primary, stop_group_id,stop_id", invalid); + private static Stream createInvalidCSVArguments() { + String invalid = "Unknown shared stops header: "; + String missing = "shared_stops.csv is missing headers!"; + return Stream.of( + new InvalidCSVArgument("this is a great string, but it's not a shared_stops csv!", invalid + "this is a great string"), + new InvalidCSVArgument("", invalid), + new InvalidCSVArgument("is_primary,stop_id", missing), + new InvalidCSVArgument("is_primary,feed_id,,stop_group_id", invalid), + new InvalidCSVArgument("is_primary,feed_id,stop_group_id", missing), + new InvalidCSVArgument("feed_id, is_primary, stop_group_id,stop_id", invalid + " is_primary") + ); } - @Test - void canHandleVariousCorrectCSV() { - assertThat(parseValidCSV("is_primary,feed_id,stop_id,stop_group_id"), matchesSnapshot()); - assertThat(parseValidCSV("feed_id,stop_id,stop_group_id,is_primary"), matchesSnapshot()); + private static class InvalidCSVArgument { + public String csv; + public String expectedError; - // Only last is handled - assertThat(parseValidCSV("feed_id,is_primary,stop_group_id,stop_id,feed_id"), matchesSnapshot()); + public InvalidCSVArgument(String csv, String expectedError) { + this.csv = csv; + this.expectedError = expectedError; + } } } From 509cec48d558adcca7a97d8a963f7b051c42d7e1 Mon Sep 17 00:00:00 2001 From: miles-grant-ibi Date: Tue, 16 Jan 2024 10:12:54 -0500 Subject: [PATCH 7/7] update ci node --- .github/workflows/maven.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index 7fe151840..2616dcec9 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -33,10 +33,10 @@ jobs: java-version: 19 distribution: 'temurin' # Install node 14 for running e2e tests (and for maven-semantic-release). - - name: Use Node.js 18.x + - name: Use Node.js 20.x uses: actions/setup-node@v1 with: - node-version: 18.x + node-version: 20.x - name: Start MongoDB uses: supercharge/mongodb-github-action@1.3.0 with: