diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index e04f2c0f8..33a87a6a8 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -29,11 +29,11 @@ jobs: uses: actions/setup-java@v1 with: java-version: 1.8 - # Install node 18+ for running maven-semantic-release. - - name: Use Node.js 18.X + # Install node 20.11.0 (LTS) for running maven-semantic-release. + - name: Use Node.js 20.X uses: actions/setup-node@v1 with: - node-version: 18 + node-version: 20.11.0 - name: Install maven-semantic-release # FIXME: Enable cache for node packages (add package.json?) run: | @@ -64,10 +64,10 @@ jobs: # # The git commands get the commit hash of the HEAD commit and the commit just before HEAD. # Install node 14+ for running maven-semantic-release. - - name: Use Node.js 18.X + - name: Use Node.js 20.X uses: actions/setup-node@v1 with: - node-version: 18 + node-version: 20.11.0 - name: Run maven-semantic-release env: GH_TOKEN: ${{ secrets.GH_TOKEN }} diff --git a/src/main/java/com/conveyal/gtfs/PatternBuilder.java b/src/main/java/com/conveyal/gtfs/PatternBuilder.java index c3361e5ca..64d422941 100644 --- a/src/main/java/com/conveyal/gtfs/PatternBuilder.java +++ b/src/main/java/com/conveyal/gtfs/PatternBuilder.java @@ -20,7 +20,6 @@ import java.sql.SQLException; import java.sql.Statement; import java.util.Map; -import java.util.Set; import static com.conveyal.gtfs.loader.JdbcGtfsLoader.copyFromFile; import static com.conveyal.gtfs.model.Entity.INT_MISSING; @@ -40,7 +39,7 @@ public PatternBuilder(Feed feed) throws SQLException { connection = feed.getConnection(); } - public void create(Map patterns, Set patternIdsLoadedFromFile) { + public void create(Map patterns, boolean usePatternsFromFeed) { String patternsTableName = feed.getTableNameWithSchemaPrefix("patterns"); String tripsTableName = feed.getTableNameWithSchemaPrefix("trips"); String patternStopsTableName = feed.getTableNameWithSchemaPrefix("pattern_stops"); @@ -53,13 +52,14 @@ public void create(Map patterns, Set patternIds LOG.info("Creating pattern and pattern stops tables."); Statement statement = connection.createStatement(); statement.execute(String.format("alter table %s add column pattern_id varchar", tripsTableName)); - if (patternIdsLoadedFromFile.isEmpty()) { - // No patterns were loaded from file so the pattern table has not previously been created. + if (!usePatternsFromFeed) { + // If no patterns were loaded from file, create the pattern table. Conversely, if the patterns loaded + // from file have been superseded by generated patterns, recreate the table to start afresh. patternsTable.createSqlTable(connection, null, true); } patternStopsTable.createSqlTable(connection, null, true); try (PrintStream patternForTripsFileStream = createTempPatternForTripsTable(tempPatternForTripsTextFile, statement)) { - processPatternAndPatternStops(patternsTable, patternStopsTable, patternForTripsFileStream, patterns, patternIdsLoadedFromFile); + processPatternAndPatternStops(patternsTable, patternStopsTable, patternForTripsFileStream, patterns, usePatternsFromFeed); } updateTripPatternIds(tempPatternForTripsTextFile, statement, tripsTableName); createIndexes(statement, patternsTableName, patternStopsTableName, tripsTableName); @@ -80,7 +80,7 @@ private void processPatternAndPatternStops( Table patternStopsTable, PrintStream patternForTripsFileStream, Map patterns, - Set patternIdsLoadedFromFile + boolean usePatternsFromFeed ) throws SQLException { // Generate prepared statements for inserts. String insertPatternSql = patternsTable.generateInsertSql(true); @@ -90,7 +90,7 @@ private void processPatternAndPatternStops( for (Map.Entry entry : patterns.entrySet()) { Pattern pattern = entry.getValue(); LOG.debug("Batching pattern {}", pattern.pattern_id); - if (!patternIdsLoadedFromFile.contains(pattern.pattern_id)) { + if (!usePatternsFromFeed) { // Only insert the pattern if it has not already been imported from file. pattern.setStatementParameters(insertPatternStatement, true); patternTracker.addBatch(); diff --git a/src/main/java/com/conveyal/gtfs/PatternFinder.java b/src/main/java/com/conveyal/gtfs/PatternFinder.java index d251b6820..488d01616 100644 --- a/src/main/java/com/conveyal/gtfs/PatternFinder.java +++ b/src/main/java/com/conveyal/gtfs/PatternFinder.java @@ -61,10 +61,20 @@ public void processTrip(Trip trip, Iterable orderedStopTimes) { * Once all trips have been processed, call this method to produce the final Pattern objects representing all the * unique sequences of stops encountered. Returns map of patterns to their keys so that downstream functions can * make use of trip pattern keys for constructing pattern stops or other derivative objects. + * + * There is no viable relationship between patterns that are loaded from a feed (patternsFromFeed) and patterns + * generated here. Process ordering is used to update the pattern id and name if patterns from a feed are available. + * E.g. The first pattern loaded from a feed will be used to updated the first pattern created here. */ - public Map createPatternObjects(Map stopById, SQLErrorStorage errorStorage) { + public Map createPatternObjects( + Map stopById, + List patternsFromFeed, + SQLErrorStorage errorStorage + ) { // Make pattern ID one-based to avoid any JS type confusion between an ID of zero vs. null value. int nextPatternId = 1; + int patternsFromFeedIndex = 0; + boolean usePatternsFromFeed = canUsePatternsFromFeed(patternsFromFeed); // Create an in-memory list of Patterns because we will later rename them before inserting them into storage. // Use a LinkedHashMap so we can retrieve the entrySets later in the order of insertion. Map patterns = new LinkedHashMap<>(); @@ -72,8 +82,13 @@ public Map createPatternObjects(Map stopB for (TripPatternKey key : tripsForPattern.keySet()) { Collection trips = tripsForPattern.get(key); Pattern pattern = new Pattern(key.stops, trips, null); - // Overwrite long UUID with sequential integer pattern ID - pattern.pattern_id = Integer.toString(nextPatternId++); + if (usePatternsFromFeed) { + pattern.pattern_id = patternsFromFeed.get(patternsFromFeedIndex).pattern_id; + pattern.name = patternsFromFeed.get(patternsFromFeedIndex).name; + } else { + // Overwrite long UUID with sequential integer pattern ID + pattern.pattern_id = Integer.toString(nextPatternId++); + } // FIXME: Should associated shapes be a single entry? pattern.associatedShapes = new HashSet<>(); trips.stream().forEach(trip -> pattern.associatedShapes.add(trip.shape_id)); @@ -87,13 +102,26 @@ public Map createPatternObjects(Map stopB .setBadValue(pattern.associatedShapes.toString())); } patterns.put(key, pattern); + patternsFromFeedIndex++; + } + if (!usePatternsFromFeed) { + // Name patterns before storing in SQL database if they have not already been provided with a feed. + renamePatterns(patterns.values(), stopById); } - // Name patterns before storing in SQL database. - renamePatterns(patterns.values(), stopById); LOG.info("Total patterns: {}", tripsForPattern.keySet().size()); return patterns; } + /** + * If there is a difference in the number of patterns provided by a feed and the number of patterns generated here, + * the patterns provided by the feed are rejected. + */ + public boolean canUsePatternsFromFeed(List patternsFromFeed) { + boolean usePatternsFromFeed = patternsFromFeed.size() == tripsForPattern.keySet().size(); + LOG.info("Using patterns from feed: {}", usePatternsFromFeed); + return usePatternsFromFeed; + } + /** * Destructively rename the supplied collection of patterns. * This process requires access to all the stops in the feed. diff --git a/src/main/java/com/conveyal/gtfs/validator/PatternFinderValidator.java b/src/main/java/com/conveyal/gtfs/validator/PatternFinderValidator.java index 88fe62b9f..f9c7dfa3f 100644 --- a/src/main/java/com/conveyal/gtfs/validator/PatternFinderValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/PatternFinderValidator.java @@ -4,40 +4,20 @@ import com.conveyal.gtfs.PatternFinder; import com.conveyal.gtfs.TripPatternKey; import com.conveyal.gtfs.error.SQLErrorStorage; -import com.conveyal.gtfs.loader.BatchTracker; import com.conveyal.gtfs.loader.Feed; -import com.conveyal.gtfs.loader.Requirement; -import com.conveyal.gtfs.loader.Table; import com.conveyal.gtfs.model.Pattern; -import com.conveyal.gtfs.model.PatternStop; import com.conveyal.gtfs.model.Route; import com.conveyal.gtfs.model.Stop; import com.conveyal.gtfs.model.StopTime; import com.conveyal.gtfs.model.Trip; -import org.apache.commons.dbutils.DbUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.io.BufferedOutputStream; -import java.io.File; -import java.io.FileOutputStream; -import java.io.IOException; -import java.io.PrintStream; -import java.sql.Connection; -import java.sql.PreparedStatement; import java.sql.SQLException; -import java.sql.Statement; -import java.util.Collections; +import java.util.ArrayList; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; - -import static com.conveyal.gtfs.loader.JdbcGtfsLoader.copyFromFile; -import static com.conveyal.gtfs.model.Entity.INT_MISSING; -import static com.conveyal.gtfs.model.Entity.setDoubleParameter; -import static com.conveyal.gtfs.model.Entity.setIntParameter; /** * Groups trips together into "patterns" that share the same sequence of stops. @@ -72,9 +52,9 @@ public void validateTrip (Trip trip, Route route, List stopTimes, List */ @Override public void complete(ValidationResult validationResult) { - Set patternIds = new HashSet<>(); + List patternsFromFeed = new ArrayList<>(); for(Pattern pattern : feed.patterns) { - patternIds.add(pattern.pattern_id); + patternsFromFeed.add(pattern); } LOG.info("Finding patterns..."); Map stopById = new HashMap<>(); @@ -82,9 +62,8 @@ public void complete(ValidationResult validationResult) { stopById.put(stop.stop_id, stop); } // Although patterns may have already been loaded from file, the trip patterns are still required. - Map patterns = patternFinder.createPatternObjects(stopById, errorStorage); - patternBuilder.create(patterns, patternIds); + Map patterns = patternFinder.createPatternObjects(stopById, patternsFromFeed, errorStorage); + patternBuilder.create(patterns, patternFinder.canUsePatternsFromFeed(patternsFromFeed)); } - } diff --git a/src/test/java/com/conveyal/gtfs/validator/PatternFinderValidatorTest.java b/src/test/java/com/conveyal/gtfs/validator/PatternFinderValidatorTest.java new file mode 100644 index 000000000..d2b8a3022 --- /dev/null +++ b/src/test/java/com/conveyal/gtfs/validator/PatternFinderValidatorTest.java @@ -0,0 +1,121 @@ +package com.conveyal.gtfs.validator; + +import com.conveyal.gtfs.GTFSFeed; +import com.conveyal.gtfs.TestUtils; +import com.conveyal.gtfs.loader.EntityPopulator; +import com.conveyal.gtfs.loader.FeedLoadResult; +import com.conveyal.gtfs.loader.JDBCTableReader; +import com.conveyal.gtfs.loader.Table; +import com.conveyal.gtfs.model.Pattern; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import javax.sql.DataSource; +import java.io.IOException; +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; + +import static com.conveyal.gtfs.GTFS.load; +import static com.conveyal.gtfs.GTFS.validate; +import static com.conveyal.gtfs.TestUtils.getResourceFileName; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.greaterThan; + + +class PatternFinderValidatorTest { + + private static String testDBName; + + private static DataSource testDataSource; + + @BeforeAll + public static void setUpClass() { + // create a new database + testDBName = TestUtils.generateNewDB(); + String dbConnectionUrl = String.format("jdbc:postgresql://localhost/%s", testDBName); + testDataSource = TestUtils.createTestDataSource(dbConnectionUrl); + } + + @AfterAll + public static void tearDownClass() { + TestUtils.dropDB(testDBName); + } + + @Test + void canUseFeedPatterns() throws SQLException { + String fileName = getResourceFileName("real-world-gtfs-feeds/RABA.zip"); + FeedLoadResult feedLoadResult = load(fileName, testDataSource); + String testNamespace = feedLoadResult.uniqueIdentifier; + validate(testNamespace, testDataSource); + checkPatternStopsAgainstFeedPatterns(fileName, testNamespace); + } + + /** + * Remove one pattern from the feed so that there is a mismatch between the patterns loaded and the patterns + * generated. This will result in the generated patterns taking precedence over the loaded patterns. + */ + @Test + void canRevertToGeneratedPatterns() throws SQLException { + String fileName = getResourceFileName("real-world-gtfs-feeds/RABA.zip"); + FeedLoadResult feedLoadResult = load(fileName, testDataSource); + String testNamespace = feedLoadResult.uniqueIdentifier; + String patternIdToExclude = "2k3j"; + executeSqlStatement(String.format( + "delete from %s where pattern_id = '%s'", + String.format("%s.%s", testNamespace, Table.PATTERNS.name), + patternIdToExclude + )); + validate(testNamespace, testDataSource); + JDBCTableReader patterns = new JDBCTableReader(Table.PATTERNS, + testDataSource, + testNamespace + ".", + EntityPopulator.PATTERN + ); + for (Pattern pattern : patterns.getAllOrdered()) { + assertThatSqlQueryYieldsRowCountGreaterThanZero(generateSql(testNamespace, pattern.pattern_id)); + } + } + + @Test + void canUseGeneratedPatterns() throws SQLException, IOException { + String zipFileName = TestUtils.zipFolderFiles("fake-agency", true); + FeedLoadResult feedLoadResult = load(zipFileName, testDataSource); + String testNamespace = feedLoadResult.uniqueIdentifier; + validate(testNamespace, testDataSource); + checkPatternStopsAgainstFeedPatterns(zipFileName, testNamespace); + } + + private void checkPatternStopsAgainstFeedPatterns(String zipFileName, String testNamespace) throws SQLException { + GTFSFeed feed = GTFSFeed.fromFile(zipFileName); + for (String key : feed.patterns.keySet()) { + Pattern pattern = feed.patterns.get(key); + assertThatSqlQueryYieldsRowCountGreaterThanZero(generateSql(testNamespace, pattern.pattern_id)); + } + } + + private String generateSql(String testNamespace, String patternId) { + return String.format( + "select * from %s where pattern_id = '%s'", + String.format("%s.%s", testNamespace, Table.PATTERN_STOP.name), + patternId + ); + } + + private void assertThatSqlQueryYieldsRowCountGreaterThanZero(String sql) throws SQLException { + int recordCount = 0; + ResultSet rs = testDataSource.getConnection().prepareStatement(sql).executeQuery(); + while (rs.next()) recordCount++; + assertThat(recordCount, greaterThan(0)); + } + + private void executeSqlStatement(String sql) throws SQLException { + Connection connection = testDataSource.getConnection(); + Statement statement = connection.createStatement(); + statement.execute(sql); + statement.close(); + connection.commit(); + } +} \ No newline at end of file diff --git a/src/test/resources/real-world-gtfs-feeds/RABA.zip b/src/test/resources/real-world-gtfs-feeds/RABA.zip new file mode 100644 index 000000000..894d2f043 Binary files /dev/null and b/src/test/resources/real-world-gtfs-feeds/RABA.zip differ