From 5f9a7277698726e5c1fbbdd7ec1355ec8c7c3e07 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Wed, 18 Oct 2023 08:43:48 +0100 Subject: [PATCH 1/2] refactor(Updated transfers inline with latest spec): --- .../conveyal/gtfs/loader/JdbcGtfsLoader.java | 2 +- .../java/com/conveyal/gtfs/loader/Table.java | 30 +++++---- .../com/conveyal/gtfs/model/Transfer.java | 62 ++++++++++++++----- src/test/java/com/conveyal/gtfs/GTFSTest.java | 19 +++++- src/test/resources/fake-agency/transfers.txt | 3 +- 5 files changed, 85 insertions(+), 31 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsLoader.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsLoader.java index b17a812c0..a2ed002f7 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsLoader.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsLoader.java @@ -161,8 +161,8 @@ public FeedLoadResult loadTables() { result.shapes = load(Table.SHAPES); result.stops = load(Table.STOPS); result.fareRules = load(Table.FARE_RULES); - result.transfers = load(Table.TRANSFERS); result.trips = load(Table.TRIPS); // refs routes + result.transfers = load(Table.TRANSFERS); // refs trips. result.frequencies = load(Table.FREQUENCIES); // refs trips result.stopTimes = load(Table.STOP_TIMES); result.translations = load(Table.TRANSLATIONS); diff --git a/src/main/java/com/conveyal/gtfs/loader/Table.java b/src/main/java/com/conveyal/gtfs/loader/Table.java index b3344c5c5..c3286f62d 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Table.java +++ b/src/main/java/com/conveyal/gtfs/loader/Table.java @@ -322,17 +322,6 @@ public Table (String name, Class entityClass, Requirement requ ).withParentTable(PATTERNS) .addPrimaryKeyNames("pattern_id", "stop_sequence"); - public static final Table TRANSFERS = new Table("transfers", Transfer.class, OPTIONAL, - // FIXME: Do we need an index on from_ and to_stop_id - new StringField("from_stop_id", REQUIRED).isReferenceTo(STOPS), - new StringField("to_stop_id", REQUIRED).isReferenceTo(STOPS), - new ShortField("transfer_type", REQUIRED, 3), - new StringField("min_transfer_time", OPTIONAL) - ).addPrimaryKey() - .keyFieldIsNotUnique() - .hasCompoundKey() - .addPrimaryKeyNames("from_stop_id", "to_stop_id"); - public static final Table TRIPS = new Table("trips", Trip.class, REQUIRED, new StringField("trip_id", REQUIRED), new StringField("route_id", REQUIRED).isReferenceTo(ROUTES).indexThisColumn(), @@ -351,6 +340,25 @@ public Table (String name, Class entityClass, Requirement requ ).addPrimaryKey() .addPrimaryKeyNames("trip_id"); + // Must come after TRIPS table to which it has references. + public static final Table TRANSFERS = new Table("transfers", Transfer.class, OPTIONAL, + // Conditionally required fields (from_stop_id, to_stop_id, from_trip_id and to_trip_id) are defined here as + // optional so as not to trigger required field checks as part of GTFS-lib validation. Correct validation of + // these fields will be managed by the MobilityData validator. + new StringField("from_stop_id", OPTIONAL).isReferenceTo(STOPS), + new StringField("to_stop_id", OPTIONAL).isReferenceTo(STOPS), + new StringField("from_route_id", OPTIONAL).isReferenceTo(ROUTES), + new StringField("to_route_id", OPTIONAL).isReferenceTo(ROUTES), + new StringField("from_trip_id", OPTIONAL).isReferenceTo(TRIPS), + new StringField("to_trip_id", OPTIONAL).isReferenceTo(TRIPS), + new ShortField("transfer_type", REQUIRED, 3), + new StringField("min_transfer_time", OPTIONAL) + ) + .addPrimaryKey() + .keyFieldIsNotUnique() + .hasCompoundKey() + .addPrimaryKeyNames("from_stop_id", "to_stop_id", "from_trip_id", "to_trip_id", "from_route_id", "to_route_id"); + // Must come after TRIPS and STOPS table to which it has references public static final Table STOP_TIMES = new Table("stop_times", StopTime.class, REQUIRED, new StringField("trip_id", REQUIRED).isReferenceTo(TRIPS), diff --git a/src/main/java/com/conveyal/gtfs/model/Transfer.java b/src/main/java/com/conveyal/gtfs/model/Transfer.java index 013007c2f..8ddfa1989 100644 --- a/src/main/java/com/conveyal/gtfs/model/Transfer.java +++ b/src/main/java/com/conveyal/gtfs/model/Transfer.java @@ -29,15 +29,18 @@ public void setStatementParameters(PreparedStatement statement, boolean setDefau if (!setDefaultId) statement.setInt(oneBasedIndex++, id); statement.setString(oneBasedIndex++, from_stop_id); statement.setString(oneBasedIndex++, to_stop_id); + statement.setString(oneBasedIndex++, from_trip_id); + statement.setString(oneBasedIndex++, to_trip_id); + statement.setString(oneBasedIndex++, from_route_id); + statement.setString(oneBasedIndex++, to_route_id); setIntParameter(statement, oneBasedIndex++, transfer_type); - setIntParameter(statement, oneBasedIndex++, min_transfer_time); + setIntParameter(statement, oneBasedIndex, min_transfer_time); } - // TODO: Add id method for Transfer. -// @Override -// public String getId() { -//// return trip_id; -// } + @Override + public String getId() { + return createId(from_stop_id, to_stop_id, from_trip_id, to_trip_id, from_route_id, to_route_id); + } public static class Loader extends Entity.Loader { @@ -54,24 +57,27 @@ protected boolean isRequired() { public void loadOneRow() throws IOException { Transfer tr = new Transfer(); tr.id = row + 1; // offset line number by 1 to account for 0-based row index - tr.from_stop_id = getStringField("from_stop_id", true); - tr.to_stop_id = getStringField("to_stop_id", true); - tr.transfer_type = getIntField("transfer_type", true, 0, 3); - tr.min_transfer_time = getIntField("min_transfer_time", false, 0, Integer.MAX_VALUE); + tr.from_stop_id = getStringField("from_stop_id", false); + tr.to_stop_id = getStringField("to_stop_id", false); tr.from_route_id = getStringField("from_route_id", false); tr.to_route_id = getStringField("to_route_id", false); tr.from_trip_id = getStringField("from_trip_id", false); tr.to_trip_id = getStringField("to_trip_id", false); + tr.transfer_type = getIntField("transfer_type", true, 0, 3); + tr.min_transfer_time = getIntField("min_transfer_time", false, 0, Integer.MAX_VALUE); - getRefField("from_stop_id", true, feed.stops); - getRefField("to_stop_id", true, feed.stops); + getRefField("from_stop_id", false, feed.stops); + getRefField("to_stop_id", false, feed.stops); getRefField("from_route_id", false, feed.routes); getRefField("to_route_id", false, feed.routes); getRefField("from_trip_id", false, feed.trips); getRefField("to_trip_id", false, feed.trips); tr.feed = feed; - feed.transfers.put(Long.toString(row), tr); + feed.transfers.put( + createId(tr.from_stop_id, tr.to_stop_id, tr.from_trip_id, tr.to_trip_id, tr.from_route_id, tr.to_route_id), + tr + ); } } @@ -83,13 +89,26 @@ public Writer (GTFSFeed feed) { @Override protected void writeHeaders() throws IOException { - writer.writeRecord(new String[] {"from_stop_id", "to_stop_id", "transfer_type", "min_transfer_time"}); + writer.writeRecord(new String[] { + "from_stop_id", + "to_stop_id", + "from_trip_id", + "to_trip_id", + "from_route_id", + "to_route_id", + "transfer_type", + "min_transfer_time" + }); } @Override protected void writeOneRow(Transfer t) throws IOException { writeStringField(t.from_stop_id); writeStringField(t.to_stop_id); + writeStringField(t.from_trip_id); + writeStringField(t.to_trip_id); + writeStringField(t.from_route_id); + writeStringField(t.to_route_id); writeIntField(t.transfer_type); writeIntField(t.min_transfer_time); endRecord(); @@ -99,7 +118,20 @@ protected void writeOneRow(Transfer t) throws IOException { protected Iterator iterator() { return feed.transfers.values().iterator(); } + } - + /** + * Transfer entries have no ID in GTFS so we define one based on the fields in the transfer entry. + */ + private static String createId( + String fromStopId, + String toStopId, + String fromTripId, + String toTripId, + String fromRouteId, + String toRouteId + ) { + return String.format("%s_%s_%s_%s_%s_%s", fromStopId, toStopId, fromTripId, toTripId, fromRouteId, toRouteId); } + } diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 19b0413d8..65e2aef29 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -16,7 +16,6 @@ import com.csvreader.CsvReader; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.Multimap; -import com.google.common.io.Files; import graphql.Assert; import org.apache.commons.io.FileUtils; import org.apache.commons.io.input.BOMInputStream; @@ -34,6 +33,7 @@ import java.io.InputStream; import java.io.PrintStream; import java.nio.charset.Charset; +import java.nio.file.Files; import java.sql.Connection; import java.sql.ResultSet; import java.sql.SQLException; @@ -246,12 +246,12 @@ public void canLoadFeedWithErrors () { * Tests whether or not "fake-agency" GTFS can be placed in a zipped subdirectory and loaded/exported successfully. */ @Test - public void canLoadAndExportSimpleAgencyInSubDirectory() { + public void canLoadAndExportSimpleAgencyInSubDirectory() throws IOException { String zipFileName = null; // Get filename for fake-agency resource String resourceFolder = TestUtils.getResourceFileName("fake-agency"); // Recursively copy folder into temp directory, which we zip up and run the integration test on. - File tempDir = Files.createTempDir(); + File tempDir = Files.createTempDirectory("").toFile(); tempDir.deleteOnExit(); File nestedDir = new File(TestUtils.fileNameWithDir(tempDir.getAbsolutePath(), "fake-agency")); LOG.info("Creating temp folder with nested subdirectory at {}", tempDir.getAbsolutePath()); @@ -1313,6 +1313,19 @@ private void assertThatPersistenceExpectationRecordWasFound( new RecordExpectation("bikes_allowed", 0), new RecordExpectation("wheelchair_accessible", 0) } + ), + new PersistenceExpectation( + "transfers", + new RecordExpectation[]{ + new RecordExpectation("from_stop_id", "4u6g"), + new RecordExpectation("to_stop_id", "johv"), + new RecordExpectation("from_trip_id", "a30277f8-e50a-4a85-9141-b1e0da9d429d"), + new RecordExpectation("to_trip_id", "frequency-trip"), + new RecordExpectation("from_route_id", "1"), + new RecordExpectation("to_route_id", "1"), + new RecordExpectation("transfer_type", "1"), + new RecordExpectation("min_transfer_time", "60") + } ) }; diff --git a/src/test/resources/fake-agency/transfers.txt b/src/test/resources/fake-agency/transfers.txt index 357103c47..271fb7193 100755 --- a/src/test/resources/fake-agency/transfers.txt +++ b/src/test/resources/fake-agency/transfers.txt @@ -1 +1,2 @@ -from_stop_id,to_stop_id,transfer_type,min_transfer_time +from_stop_id,to_stop_id,from_trip_id,to_trip_id,from_route_id,to_route_id,transfer_type,min_transfer_time +4u6g,johv,a30277f8-e50a-4a85-9141-b1e0da9d429d,frequency-trip,1,1,1,60 \ No newline at end of file From e5355d36e68731fa2d9c4a8e83d9f54b8cd02443 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Fri, 27 Oct 2023 10:53:43 +0100 Subject: [PATCH 2/2] refactor(Addressed PR feedback): Added an additional transfer minus optional fields --- src/main/java/com/conveyal/gtfs/model/Transfer.java | 8 ++------ src/test/resources/fake-agency/transfers.txt | 3 ++- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/model/Transfer.java b/src/main/java/com/conveyal/gtfs/model/Transfer.java index 8ddfa1989..187469a91 100644 --- a/src/main/java/com/conveyal/gtfs/model/Transfer.java +++ b/src/main/java/com/conveyal/gtfs/model/Transfer.java @@ -74,10 +74,7 @@ public void loadOneRow() throws IOException { getRefField("to_trip_id", false, feed.trips); tr.feed = feed; - feed.transfers.put( - createId(tr.from_stop_id, tr.to_stop_id, tr.from_trip_id, tr.to_trip_id, tr.from_route_id, tr.to_route_id), - tr - ); + feed.transfers.put(tr.getId(), tr); } } @@ -133,5 +130,4 @@ private static String createId( ) { return String.format("%s_%s_%s_%s_%s_%s", fromStopId, toStopId, fromTripId, toTripId, fromRouteId, toRouteId); } - -} +} \ No newline at end of file diff --git a/src/test/resources/fake-agency/transfers.txt b/src/test/resources/fake-agency/transfers.txt index 271fb7193..1296dea84 100755 --- a/src/test/resources/fake-agency/transfers.txt +++ b/src/test/resources/fake-agency/transfers.txt @@ -1,2 +1,3 @@ from_stop_id,to_stop_id,from_trip_id,to_trip_id,from_route_id,to_route_id,transfer_type,min_transfer_time -4u6g,johv,a30277f8-e50a-4a85-9141-b1e0da9d429d,frequency-trip,1,1,1,60 \ No newline at end of file +4u6g,johv,a30277f8-e50a-4a85-9141-b1e0da9d429d,frequency-trip,1,1,1,60 +4u6g,123,,,,,1,60 \ No newline at end of file