Merge pull request #234 from conveyal/dev

Bug fix release

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) {

src/main/java/com/conveyal/gtfs/loader/ReferenceTracker.java

@@ -116,13 +116,16 @@ public Set<NewGTFSError> 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;
     }

src/main/java/com/conveyal/gtfs/loader/Table.java

@@ -286,12 +286,12 @@ public Table (String name, Class<? extends Entity> 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

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;

src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java

@@ -18,6 +18,8 @@
 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;
 import org.junit.Test;
@@ -164,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));
     }
 
     /**
@@ -209,8 +211,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 +237,24 @@ 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 to check database integrity.
+        ResultSet resultSet = getResultSetForId(createdFare.id, Table.FARE_ATTRIBUTES);
+        while (resultSet.next()) {
+            // 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
         String updatedFareId = "3B";
         createdFare.fare_id = updatedFareId;
+        createdFare.transfers = "0";
 
         // covert object to json and save it
         JdbcTableWriter updateTableWriter = createTestTableWriter(fareTable);
@@ -255,6 +272,13 @@ 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 to check database integrity.
+        ResultSet updatedResult = getResultSetForId(createdFare.id, Table.FARE_ATTRIBUTES);
+        while (updatedResult.next()) {
+            assertResultValue(updatedResult, "transfers", equalTo(0));
+            assertResultValue(updatedResult, "fare_id", equalTo(createdFare.fare_id));
+        }
+
         // try to delete record
         JdbcTableWriter deleteTableWriter = createTestTableWriter(fareTable);
         int deleteOutput = deleteTableWriter.delete(
@@ -264,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
@@ -315,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));
     }
 
     /**
@@ -376,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));
     }
 
     /**
@@ -444,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(
@@ -676,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
@@ -685,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.
      */
@@ -700,6 +717,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;