From ac8e4f866aed838ff1757dd667db81dd3e1126fe Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Tue, 21 May 2019 10:17:20 -0400 Subject: [PATCH 1/5] fix(ReferenceTracker): fix false positive ref int. for calendar_date#service_id fix #231 --- .../java/com/conveyal/gtfs/loader/ReferenceTracker.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/ReferenceTracker.java b/src/main/java/com/conveyal/gtfs/loader/ReferenceTracker.java index 41a29273b..303f55c9a 100644 --- a/src/main/java/com/conveyal/gtfs/loader/ReferenceTracker.java +++ b/src/main/java/com/conveyal/gtfs/loader/ReferenceTracker.java @@ -116,13 +116,16 @@ public Set checkReferencesAndUniqueness(String keyValue, int lineN if (isOrderField) { duplicateIdError.setSequence(value); } errors.add(duplicateIdError); } - } else if (field.name.equals(keyField) && !field.isForeignReference()) { + } else if ( + field.name.equals(keyField) && + (!field.isForeignReference() || Table.CALENDAR_DATES.name.equals(table.name)) + ) { // We arrive here if the field is not a foreign reference and not the unique key field // on the table (e.g., shape_pt_sequence), but is still a key on the table. For // example, this is where we add shape_id from the shapes table, so that when we // check the referential integrity of trips#shape_id, we know that the shape_id // exists in the shapes table. It also handles tracking calendar_dates#service_id values. - this.transitIds.add(uniqueId); + listOfUniqueIds.add(uniqueId); } return errors; } From e44f6ed8eee1a6cd6224b959ddd67130c6a5b274 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Tue, 21 May 2019 10:57:38 -0400 Subject: [PATCH 2/5] fix(StopTimes): add dropoff/pick up type = 3 (coordinate with driver) --- src/main/java/com/conveyal/gtfs/loader/Table.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/Table.java b/src/main/java/com/conveyal/gtfs/loader/Table.java index d744fae63..4866640d7 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Table.java +++ b/src/main/java/com/conveyal/gtfs/loader/Table.java @@ -286,12 +286,12 @@ public Table (String name, Class entityClass, Requirement requ // FIXME: Do we need an index on stop_id new StringField("stop_id", REQUIRED).isReferenceTo(STOPS), // .indexThisColumn(), - // TODO verify that we have a special check for arrival and departure times first and last stop_time in a trip, which are reqiured + // TODO verify that we have a special check for arrival and departure times first and last stop_time in a trip, which are required new TimeField("arrival_time", OPTIONAL), new TimeField("departure_time", OPTIONAL), new StringField("stop_headsign", OPTIONAL), - new ShortField("pickup_type", OPTIONAL, 2), - new ShortField("drop_off_type", OPTIONAL, 2), + new ShortField("pickup_type", OPTIONAL, 3), + new ShortField("drop_off_type", OPTIONAL, 3), new DoubleField("shape_dist_traveled", OPTIONAL, 0, Double.POSITIVE_INFINITY, 2), new ShortField("timepoint", OPTIONAL, 1), new IntegerField("fare_units_traveled", EXTENSION) // OpenOV NL extension From 1bf8b4db929321ecb93c2c8ac828831e11fc51cc Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Tue, 21 May 2019 16:11:08 -0400 Subject: [PATCH 3/5] fix(editor): fix issue w empty fare_attributes#transfers val conveyal/datatools-ui#346 --- .../conveyal/gtfs/loader/JdbcTableWriter.java | 6 ++++- .../java/com/conveyal/gtfs/dto/FareDTO.java | 3 ++- .../gtfs/loader/JDBCTableWriterTest.java | 23 +++++++++++++++++-- 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java index bc5fbac84..89674bff9 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java @@ -424,7 +424,11 @@ private void setStatementParameters( } field.setParameter(preparedStatement, index, String.join(",", values)); } else { - field.setParameter(preparedStatement, index, value.asText()); + String text = value.asText(); + // If the field is a ShortField and the string is empty, set value to null. Otherwise, set + // parameter with string. + if (field instanceof ShortField && text.isEmpty()) field.setNull(preparedStatement, index); + else field.setParameter(preparedStatement, index, text); } } } catch (StorageException e) { diff --git a/src/test/java/com/conveyal/gtfs/dto/FareDTO.java b/src/test/java/com/conveyal/gtfs/dto/FareDTO.java index 5aec354c3..081e385fa 100644 --- a/src/test/java/com/conveyal/gtfs/dto/FareDTO.java +++ b/src/test/java/com/conveyal/gtfs/dto/FareDTO.java @@ -13,7 +13,8 @@ public class FareDTO { public Double price; public String currency_type; public Integer payment_method; - public Integer transfers; + // transfers is a string because we need to be able to pass empty strings to the JdbcTableWriter + public String transfers; public String agency_id; public Integer transfer_duration; public FareRuleDTO[] fare_rules; diff --git a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java index 12649e636..a2885869b 100644 --- a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java +++ b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java @@ -18,6 +18,7 @@ import com.conveyal.gtfs.dto.StopTimeDTO; import com.conveyal.gtfs.dto.TripDTO; import com.fasterxml.jackson.databind.ObjectMapper; +import org.hamcrest.Matchers; import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.Test; @@ -209,8 +210,8 @@ public void canCreateUpdateAndDeleteFares() throws IOException, SQLException, In fareInput.price = 2.50; fareInput.agency_id = "RTA"; fareInput.payment_method = 0; - // Empty value should be permitted for transfers and transfer_duration - fareInput.transfers = null; + // Empty string value or null should be permitted for transfers and transfer_duration + fareInput.transfers = ""; fareInput.transfer_duration = null; FareRuleDTO fareRuleInput = new FareRuleDTO(); // Fare ID should be assigned to "child entity" by editor automatically. @@ -235,9 +236,19 @@ public void canCreateUpdateAndDeleteFares() throws IOException, SQLException, In assertThat(createdFare.fare_id, equalTo(fareId)); assertThat(createdFare.fare_rules[0].fare_id, equalTo(fareId)); + // Ensure transfers value is null. + ResultSet resultSet = getResultSetForId(createdFare.id, Table.FARE_ATTRIBUTES, "transfers"); + while (resultSet.next()) { + // We must use getObject because getInt will always return 0 for a null value. + Object value = resultSet.getObject(1); + LOG.info("fare#transfers {}", value); + assertThat(value, Matchers.nullValue()); + } + // try to update record String updatedFareId = "3B"; createdFare.fare_id = updatedFareId; + createdFare.transfers = "0"; // covert object to json and save it JdbcTableWriter updateTableWriter = createTestTableWriter(fareTable); @@ -255,6 +266,14 @@ public void canCreateUpdateAndDeleteFares() throws IOException, SQLException, In assertThat(updatedFareDTO.fare_id, equalTo(updatedFareId)); assertThat(updatedFareDTO.fare_rules[0].fare_id, equalTo(updatedFareId)); + // Ensure transfers value is updated correctly. + ResultSet updatedResult = getResultSetForId(createdFare.id, Table.FARE_ATTRIBUTES, "transfers"); + while (updatedResult.next()) { + Object value = updatedResult.getObject(1); + LOG.info("fare#transfers {}", value); + assertThat(value, equalTo(0)); + } + // try to delete record JdbcTableWriter deleteTableWriter = createTestTableWriter(fareTable); int deleteOutput = deleteTableWriter.delete( From d405a20fee57e9f7e3af76b2eb36bab7a6e4210d Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Tue, 21 May 2019 16:35:33 -0400 Subject: [PATCH 4/5] test(JdbcTableWriterTest): improve CRUD test for fares --- .../gtfs/loader/JDBCTableWriterTest.java | 32 +++++++++++++------ 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java index a2885869b..d0d88c4dd 100644 --- a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java +++ b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java @@ -18,6 +18,7 @@ import com.conveyal.gtfs.dto.StopTimeDTO; import com.conveyal.gtfs.dto.TripDTO; import com.fasterxml.jackson.databind.ObjectMapper; +import org.hamcrest.Matcher; import org.hamcrest.Matchers; import org.junit.AfterClass; import org.junit.BeforeClass; @@ -236,13 +237,18 @@ public void canCreateUpdateAndDeleteFares() throws IOException, SQLException, In assertThat(createdFare.fare_id, equalTo(fareId)); assertThat(createdFare.fare_rules[0].fare_id, equalTo(fareId)); - // Ensure transfers value is null. - ResultSet resultSet = getResultSetForId(createdFare.id, Table.FARE_ATTRIBUTES, "transfers"); + // Ensure transfers value is null to check database integrity. + ResultSet resultSet = getResultSetForId(createdFare.id, Table.FARE_ATTRIBUTES); while (resultSet.next()) { - // We must use getObject because getInt will always return 0 for a null value. - Object value = resultSet.getObject(1); - LOG.info("fare#transfers {}", value); - assertThat(value, Matchers.nullValue()); + // We must match against null value for transfers because the database stored value will + // not be an empty string, but null. + assertResultValue(resultSet, "transfers", Matchers.nullValue()); + assertResultValue(resultSet, "fare_id", equalTo(fareInput.fare_id)); + assertResultValue(resultSet, "currency_type", equalTo(fareInput.currency_type)); + assertResultValue(resultSet, "price", equalTo(fareInput.price)); + assertResultValue(resultSet, "agency_id", equalTo(fareInput.agency_id)); + assertResultValue(resultSet, "payment_method", equalTo(fareInput.payment_method)); + assertResultValue(resultSet, "transfer_duration", equalTo(fareInput.transfer_duration)); } // try to update record @@ -266,12 +272,11 @@ public void canCreateUpdateAndDeleteFares() throws IOException, SQLException, In assertThat(updatedFareDTO.fare_id, equalTo(updatedFareId)); assertThat(updatedFareDTO.fare_rules[0].fare_id, equalTo(updatedFareId)); - // Ensure transfers value is updated correctly. + // Ensure transfers value is updated correctly to check database integrity. ResultSet updatedResult = getResultSetForId(createdFare.id, Table.FARE_ATTRIBUTES, "transfers"); while (updatedResult.next()) { - Object value = updatedResult.getObject(1); - LOG.info("fare#transfers {}", value); - assertThat(value, equalTo(0)); + assertResultValue(resultSet, "transfers", equalTo(0)); + assertResultValue(resultSet, "fare_id", equalTo(createdFare.fare_id)); } // try to delete record @@ -719,6 +724,13 @@ private ResultSet getResultSetForId(int id, Table table, String... columns) thro return testDataSource.getConnection().prepareStatement(sql).executeQuery(); } + /** + * Asserts that a given value for the specified field in result set matches provided matcher. + */ + private void assertResultValue(ResultSet resultSet, String field, Matcher matcher) throws SQLException { + assertThat(resultSet.getObject(field), matcher); + } + private void assertThatSqlQueryYieldsRowCount(String sql, int expectedRowCount) throws SQLException { LOG.info(sql); int recordCount = 0; From df0d06d76588c66b5c5c74455ffe625927e2c3f1 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Tue, 21 May 2019 16:58:56 -0400 Subject: [PATCH 5/5] test(JDBCTableWriterTest): fix broken fare test --- .../gtfs/loader/JDBCTableWriterTest.java | 29 +++++++------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java index d0d88c4dd..5ca031da1 100644 --- a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java +++ b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java @@ -166,7 +166,7 @@ public void canCreateUpdateAndDeleteFeedInfoEntities() throws IOException, SQLEx LOG.info("deleted {} records from {}", deleteOutput, feedInfoTable.name); // make sure record does not exist in DB - assertThatSqlQueryYieldsZeroRows(getAllColumnsForId(createdFeedInfo.id, feedInfoTable)); + assertThatSqlQueryYieldsZeroRows(getColumnsForId(createdFeedInfo.id, feedInfoTable)); } /** @@ -273,10 +273,10 @@ public void canCreateUpdateAndDeleteFares() throws IOException, SQLException, In assertThat(updatedFareDTO.fare_rules[0].fare_id, equalTo(updatedFareId)); // Ensure transfers value is updated correctly to check database integrity. - ResultSet updatedResult = getResultSetForId(createdFare.id, Table.FARE_ATTRIBUTES, "transfers"); + ResultSet updatedResult = getResultSetForId(createdFare.id, Table.FARE_ATTRIBUTES); while (updatedResult.next()) { - assertResultValue(resultSet, "transfers", equalTo(0)); - assertResultValue(resultSet, "fare_id", equalTo(createdFare.fare_id)); + assertResultValue(updatedResult, "transfers", equalTo(0)); + assertResultValue(updatedResult, "fare_id", equalTo(createdFare.fare_id)); } // try to delete record @@ -288,10 +288,10 @@ public void canCreateUpdateAndDeleteFares() throws IOException, SQLException, In LOG.info("deleted {} records from {}", deleteOutput, fareTable.name); // make sure fare_attributes record does not exist in DB - assertThatSqlQueryYieldsZeroRows(getAllColumnsForId(createdFare.id, fareTable)); + assertThatSqlQueryYieldsZeroRows(getColumnsForId(createdFare.id, fareTable)); // make sure fare_rules record does not exist in DB - assertThatSqlQueryYieldsZeroRows(getAllColumnsForId(createdFare.fare_rules[0].id, Table.FARE_RULES)); + assertThatSqlQueryYieldsZeroRows(getColumnsForId(createdFare.fare_rules[0].id, Table.FARE_RULES)); } @Test @@ -339,7 +339,7 @@ public void canCreateUpdateAndDeleteRoutes() throws IOException, SQLException, I LOG.info("deleted {} records from {}", deleteOutput, routeTable.name); // make sure route record does not exist in DB - assertThatSqlQueryYieldsZeroRows(getAllColumnsForId(createdRoute.id, routeTable)); + assertThatSqlQueryYieldsZeroRows(getColumnsForId(createdRoute.id, routeTable)); } /** @@ -400,7 +400,7 @@ public void canCreateUpdateAndDeleteScheduleExceptions() throws IOException, SQL ); LOG.info("deleted {} records from {}", deleteOutput, scheduleExceptionTable.name); // Make sure route record does not exist in DB. - assertThatSqlQueryYieldsZeroRows(getAllColumnsForId(scheduleException.id, scheduleExceptionTable)); + assertThatSqlQueryYieldsZeroRows(getColumnsForId(scheduleException.id, scheduleExceptionTable)); } /** @@ -468,13 +468,13 @@ public void shouldDeleteReferencingTripsAndStopTimesOnPatternDelete() throws IOE JdbcTableWriter createTripWriter = createTestTableWriter(Table.TRIPS); String createdTripOutput = createTripWriter.create(mapper.writeValueAsString(tripInput), true); TripDTO createdTrip = mapper.readValue(createdTripOutput, TripDTO.class); - assertThatSqlQueryYieldsRowCount(getAllColumnsForId(createdTrip.id, Table.TRIPS), 1); + assertThatSqlQueryYieldsRowCount(getColumnsForId(createdTrip.id, Table.TRIPS), 1); // Delete pattern record JdbcTableWriter deletePatternWriter = createTestTableWriter(Table.PATTERNS); int deleteOutput = deletePatternWriter.delete(pattern.id, true); LOG.info("deleted {} records from {}", deleteOutput, Table.PATTERNS.name); // Check that pattern record does not exist in DB - assertThatSqlQueryYieldsZeroRows(getAllColumnsForId(pattern.id, Table.PATTERNS)); + assertThatSqlQueryYieldsZeroRows(getColumnsForId(pattern.id, Table.PATTERNS)); // Check that trip records for pattern do not exist in DB assertThatSqlQueryYieldsZeroRows( String.format( @@ -700,7 +700,7 @@ private static String newUUID() { private String getColumnsForId(int id, Table table, String... columns) throws SQLException { String sql = String.format( "select %s from %s.%s where id=%d", - String.join(", ", columns), + columns.length > 0 ? String.join(", ", columns) : "*", testNamespace, table.name, id @@ -709,13 +709,6 @@ private String getColumnsForId(int id, Table table, String... columns) throws SQ return sql; } - /** - * Constructs SQL query for the specified ID for all table columns and returns the resulting result set. - */ - private String getAllColumnsForId(int id, Table table) throws SQLException { - return getColumnsForId(id, table, "*"); - } - /** * Executes SQL query for the specified ID and columns and returns the resulting result set. */