Merge pull request #133 from conveyal/fix-fare-rule-no-route

Editor: Allow null values for foreign ref fields or required fields where permitted

Author: Landon Reed

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

@@ -176,8 +176,7 @@ private TableLoadResult copy (Table table, boolean createIndexes) {
      */
     private TableLoadResult createScheduleExceptionsTable() {
         // check to see if the schedule_exceptions table exists
-        boolean scheduleExceptionsTableExists = feedIdToSnapshot != null &&
-            tableExists(feedIdToSnapshot, "schedule_exceptions");
+        boolean scheduleExceptionsTableExists = tableExists(feedIdToSnapshot, "schedule_exceptions");
         String scheduleExceptionsTableName = tablePrefix + "schedule_exceptions";
 
         if (scheduleExceptionsTableExists) {
@@ -271,7 +270,11 @@ private TableLoadResult createScheduleExceptionsTable() {
 
                 // determine if we appear to be working with a calendar_dates-only feed.
                 // If so, we must also add dummy entries to the calendar table
-                if (!tableExists(feedIdToSnapshot, "calendar") && calendarDatesReader.getRowCount() > 0) {
+                if (
+                    feedIdToSnapshot != null &&
+                    !tableExists(feedIdToSnapshot, "calendar") &&
+                    calendarDatesReader.getRowCount() > 0
+                ) {
                     sql = String.format(
                         "insert into %s (service_id, description, start_date, end_date, " +
                             "monday, tuesday, wednesday, thursday, friday, saturday, sunday)" +
@@ -322,6 +325,8 @@ private TableLoadResult createScheduleExceptionsTable() {
      * Helper method to determine if a table exists within a namespace.
      */
     private boolean tableExists(String namespace, String tableName) {
+        // Preempt SQL check with null check of either namespace or table name.
+        if (namespace == null || tableName == null) return false;
         try {
             // This statement is postgres-specific.
             PreparedStatement tableExistsStatement = connection.prepareStatement(

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

@@ -292,10 +292,13 @@ private void setStatementParameters(ObjectNode jsonObject, Table table, Prepared
                 continue;
             }
             JsonNode value = jsonObject.get(field.name);
-//            LOG.info("{}={}", field.name, value);
+            LOG.debug("{}={}", field.name, value);
             try {
                 if (value == null || value.isNull()) {
-                    if (field.isRequired()) {
+                    if (field.isRequired() && !field.isEmptyValuePermitted()) {
+                        // Only register the field as missing if the value is null, the field is required, and empty
+                        // values are not permitted. For example, a null value for fare_attributes#transfers should not
+                        // trigger a missing field exception.
                         missingFieldNames.add(field.name);
                         continue;
                     }
@@ -318,7 +321,7 @@ private void setStatementParameters(ObjectNode jsonObject, Table table, Prepared
                     // FIXME: This is a hack to get arrival and departure time into the right format. Because the UI
                     // currently returns them as seconds since midnight rather than the Field-defined format HH:MM:SS.
                     try {
-                        if (value == null ||value.isNull()) {
+                        if (value == null || value.isNull()) {
                             if (field.isRequired()) {
                                 missingFieldNames.add(field.name);
                                 continue;
@@ -421,7 +424,11 @@ private void updateChildTable(ArrayNode subEntities, Integer id, boolean isCreat
             // field statement above.
             for (Field field : subTable.specFields()) {
                 if (field.referenceTable != null && !field.referenceTable.name.equals(specTable.name)) {
-                    referencesPerTable.put(field.referenceTable, subEntity.get(field.name).asText());
+                    JsonNode refValueNode = subEntity.get(field.name);
+                    // Skip over references that are null but not required (e.g., route_id in fare_rules).
+                    if (refValueNode.isNull() && !field.isRequired()) continue;
+                    String refValue = refValueNode.asText();
+                    referencesPerTable.put(field.referenceTable, refValue);
                 }
             }
             // Insert new sub-entity.

src/test/java/com/conveyal/gtfs/GTFSTest.java

@@ -33,7 +33,7 @@
 import static org.junit.Assert.assertThat;
 
 /**
- * A test suite for the GTFS Class
+ * A test suite for the {@link GTFS} Class.
  */
 public class GTFSTest {
     private final ByteArrayOutputStream outContent = new ByteArrayOutputStream();
@@ -570,7 +570,9 @@ private void assertThatPersistenceExpectationRecordWasFound(int numRecordsSearch
         );
     }
 
-    // a helper class to verify that data got stored in a particular table
+    /**
+     * A helper class to verify that data got stored in a particular table.
+     */
     private class PersistenceExpectation {
         public String tableName;
         // each persistence expectation has an array of record expectations which all must reference a single row
@@ -590,7 +592,9 @@ private enum ExpectedFieldType {
         DOUBLE, STRING
     }
 
-    // a helper class to verify that data got stored in a particular record
+    /**
+     * A helper class to verify that data got stored in a particular record.
+     */
     private class RecordExpectation {
         public double acceptedDelta;
         public double doubleExpectation;

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

@@ -1,8 +1,11 @@
 package com.conveyal.gtfs.loader;
 
 import com.conveyal.gtfs.TestUtils;
+import com.conveyal.gtfs.util.FareDTO;
+import com.conveyal.gtfs.util.FareRuleDTO;
 import com.conveyal.gtfs.util.FeedInfoDTO;
 import com.conveyal.gtfs.util.InvalidNamespaceException;
+import com.conveyal.gtfs.util.RouteDTO;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
@@ -28,14 +31,19 @@ public class JDBCTableWriterTest {
     private static String testDBName;
     private static DataSource testDataSource;
     private static String testNamespace;
+    private static final ObjectMapper mapper = new ObjectMapper();
+
+    private static JdbcTableWriter createTestTableWriter (Table table) throws InvalidNamespaceException {
+        return new JdbcTableWriter(table, testDataSource, testNamespace);
+    }
 
     @BeforeClass
     public static void setUpClass() throws SQLException {
         // create a new database
         testDBName = TestUtils.generateNewDB();
         String dbConnectionUrl = String.format("jdbc:postgresql://localhost/%s", testDBName);
         testDataSource = createDataSource (dbConnectionUrl, null, null);
-        LOG.info("creating feeds table because it isn't automtically generated unless you import a feed");
+        LOG.info("creating feeds table because it isn't automatically generated unless you import a feed");
         Connection connection = testDataSource.getConnection();
         connection.createStatement()
             .execute("create table if not exists feeds (namespace varchar primary key, md5 varchar, " +
@@ -50,8 +58,10 @@ public static void setUpClass() throws SQLException {
     }
 
     @Test
-    public void canCreateUpdateAndDeleteFeedinfoEntities () throws IOException, SQLException, InvalidNamespaceException {
-        ObjectMapper mapper = new ObjectMapper();
+    public void canCreateUpdateAndDeleteFeedInfoEntities() throws IOException, SQLException, InvalidNamespaceException {
+        // Store Table and Class values for use in test.
+        final Table feedInfoTable = Table.FEED_INFO;
+        final Class<FeedInfoDTO> feedInfoDTOClass = FeedInfoDTO.class;
 
         // create new object to be saved
         FeedInfoDTO feedInfoInput = new FeedInfoDTO();
@@ -63,13 +73,13 @@ public void canCreateUpdateAndDeleteFeedinfoEntities () throws IOException, SQLE
         feedInfoInput.default_route_type = "3";
 
         // convert object to json and save it
-        JdbcTableWriter createTableWriter = new JdbcTableWriter(Table.FEED_INFO, testDataSource, testNamespace);
+        JdbcTableWriter createTableWriter = createTestTableWriter(feedInfoTable);
         String createOutput = createTableWriter.create(mapper.writeValueAsString(feedInfoInput), true);
-        LOG.info("create output:");
+        LOG.info("create {} output:", feedInfoTable.name);
         LOG.info(createOutput);
 
         // parse output
-        FeedInfoDTO createdFeedInfo = mapper.readValue(createOutput, FeedInfoDTO.class);
+        FeedInfoDTO createdFeedInfo = mapper.readValue(createOutput, feedInfoDTOClass);
 
         // make sure saved data matches expected data
         assertThat(createdFeedInfo.feed_publisher_name, equalTo(publisherName));
@@ -79,45 +89,39 @@ public void canCreateUpdateAndDeleteFeedinfoEntities () throws IOException, SQLE
         createdFeedInfo.feed_publisher_name = updatedPublisherName;
 
         // covert object to json and save it
-        JdbcTableWriter updateTableWriter = new JdbcTableWriter(Table.FEED_INFO, testDataSource, testNamespace);
+        JdbcTableWriter updateTableWriter = createTestTableWriter(feedInfoTable);
         String updateOutput = updateTableWriter.update(
             createdFeedInfo.id,
             mapper.writeValueAsString(createdFeedInfo),
             true
         );
-        LOG.info("update output:");
+        LOG.info("update {} output:", feedInfoTable.name);
         LOG.info(updateOutput);
 
-        FeedInfoDTO updatedFeedInfoDTO = mapper.readValue(updateOutput, FeedInfoDTO.class);
+        FeedInfoDTO updatedFeedInfoDTO = mapper.readValue(updateOutput, feedInfoDTOClass);
 
         // make sure saved data matches expected data
         assertThat(updatedFeedInfoDTO.feed_publisher_name, equalTo(updatedPublisherName));
 
         // try to delete record
-        JdbcTableWriter deleteTableWriter = new JdbcTableWriter(Table.FEED_INFO, testDataSource, testNamespace);
+        JdbcTableWriter deleteTableWriter = createTestTableWriter(feedInfoTable);
         int deleteOutput = deleteTableWriter.delete(
             createdFeedInfo.id,
             true
         );
-        LOG.info("delete output:");
-        LOG.info(updateOutput);
+        LOG.info("deleted {} records from {}", deleteOutput, feedInfoTable.name);
 
         // make sure record does not exist in DB
-        String sql = String.format(
+        assertThatSqlQueryYieldsZeroRows(String.format(
             "select * from %s.%s where id=%d",
             testNamespace,
-            Table.FEED_INFO.name,
+            feedInfoTable.name,
             createdFeedInfo.id
-        );
-        LOG.info(sql);
-        ResultSet rs = testDataSource.getConnection().prepareStatement(sql).executeQuery();
-        assertThat(rs.getFetchSize(), equalTo(0));
+        ));
     }
 
     @Test
     public void canPreventSQLInjection() throws IOException, SQLException, InvalidNamespaceException {
-        ObjectMapper mapper = new ObjectMapper();
-
         // create new object to be saved
         FeedInfoDTO feedInfoInput = new FeedInfoDTO();
         String publisherName = "' OR 1 = 1; SELECT '1";
@@ -128,7 +132,7 @@ public void canPreventSQLInjection() throws IOException, SQLException, InvalidNa
         feedInfoInput.default_route_type = "3";
 
         // convert object to json and save it
-        JdbcTableWriter createTableWriter = new JdbcTableWriter(Table.FEED_INFO, testDataSource, testNamespace);
+        JdbcTableWriter createTableWriter = createTestTableWriter(Table.FEED_INFO);
         String createOutput = createTableWriter.create(mapper.writeValueAsString(feedInfoInput), true);
         LOG.info("create output:");
         LOG.info(createOutput);
@@ -140,6 +144,169 @@ public void canPreventSQLInjection() throws IOException, SQLException, InvalidNa
         assertThat(createdFeedInfo.feed_publisher_name, equalTo(publisherName));
     }
 
+    @Test
+    public void canCreateUpdateAndDeleteFares() throws IOException, SQLException, InvalidNamespaceException {
+        // Store Table and Class values for use in test.
+        final Table fareTable = Table.FARE_ATTRIBUTES;
+        final Class<FareDTO> fareDTOClass = FareDTO.class;
+
+        // create new object to be saved
+        FareDTO fareInput = new FareDTO();
+        String fareId = "2A";
+        fareInput.fare_id = fareId;
+        fareInput.currency_type = "USD";
+        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;
+        fareInput.transfer_duration = null;
+        FareRuleDTO fareRuleInput = new FareRuleDTO();
+        // Fare ID should be assigned to "child entity" by editor automatically.
+        fareRuleInput.fare_id = null;
+        fareRuleInput.route_id = null;
+        // FIXME There is currently no check for valid zone_id values in contains_id, origin_id, and destination_id.
+        fareRuleInput.contains_id = "any";
+        fareRuleInput.origin_id = "value";
+        fareRuleInput.destination_id = "permitted";
+        fareInput.fare_rules = new FareRuleDTO[]{fareRuleInput};
+
+        // convert object to json and save it
+        JdbcTableWriter createTableWriter = createTestTableWriter(fareTable);
+        String createOutput = createTableWriter.create(mapper.writeValueAsString(fareInput), true);
+        LOG.info("create {} output:", fareTable.name);
+        LOG.info(createOutput);
+
+        // parse output
+        FareDTO createdFare = mapper.readValue(createOutput, fareDTOClass);
+
+        // make sure saved data matches expected data
+        assertThat(createdFare.fare_id, equalTo(fareId));
+        assertThat(createdFare.fare_rules[0].fare_id, equalTo(fareId));
+
+        // try to update record
+        String updatedFareId = "3B";
+        createdFare.fare_id = updatedFareId;
+
+        // covert object to json and save it
+        JdbcTableWriter updateTableWriter = createTestTableWriter(fareTable);
+        String updateOutput = updateTableWriter.update(
+                createdFare.id,
+                mapper.writeValueAsString(createdFare),
+                true
+        );
+        LOG.info("update {} output:", fareTable.name);
+        LOG.info(updateOutput);
+
+        FareDTO updatedFareDTO = mapper.readValue(updateOutput, fareDTOClass);
+
+        // make sure saved data matches expected data
+        assertThat(updatedFareDTO.fare_id, equalTo(updatedFareId));
+        assertThat(updatedFareDTO.fare_rules[0].fare_id, equalTo(updatedFareId));
+
+        // try to delete record
+        JdbcTableWriter deleteTableWriter = createTestTableWriter(fareTable);
+        int deleteOutput = deleteTableWriter.delete(
+                createdFare.id,
+                true
+        );
+        LOG.info("deleted {} records from {}", deleteOutput, fareTable.name);
+
+        // make sure fare_attributes record does not exist in DB
+        assertThatSqlQueryYieldsZeroRows(String.format(
+                "select * from %s.%s where id=%d",
+                testNamespace,
+                fareTable.name,
+                createdFare.id
+        ));
+
+        // make sure fare_rules record does not exist in DB
+        assertThatSqlQueryYieldsZeroRows(String.format(
+                "select * from %s.%s where id=%d",
+                testNamespace,
+                Table.FARE_RULES.name,
+                createdFare.fare_rules[0].id
+        ));
+    }
+
+    private void assertThatSqlQueryYieldsZeroRows(String sql) throws SQLException {
+        assertThatSqlQueryYieldsRowCount(sql, 0);
+    }
+
+    private void assertThatSqlQueryYieldsRowCount(String sql, int expectedRowCount) throws SQLException {
+        LOG.info(sql);
+        ResultSet resultSet = testDataSource.getConnection().prepareStatement(sql).executeQuery();
+        assertThat(resultSet.getFetchSize(), equalTo(expectedRowCount));
+    }
+
+    @Test
+    public void canCreateUpdateAndDeleteRoutes() throws IOException, SQLException, InvalidNamespaceException {
+        // Store Table and Class values for use in test.
+        final Table routeTable = Table.ROUTES;
+        final Class<RouteDTO> routeDTOClass = RouteDTO.class;
+
+        // create new object to be saved
+        RouteDTO routeInput = new RouteDTO();
+        String routeId = "500";
+        routeInput.route_id = routeId;
+        routeInput.agency_id = "RTA";
+        // Empty value should be permitted for transfers and transfer_duration
+        routeInput.route_short_name = "500";
+        routeInput.route_long_name = "Hollingsworth";
+        routeInput.route_type = 3;
+
+        // convert object to json and save it
+        JdbcTableWriter createTableWriter = createTestTableWriter(routeTable);
+        String createOutput = createTableWriter.create(mapper.writeValueAsString(routeInput), true);
+        LOG.info("create {} output:", routeTable.name);
+        LOG.info(createOutput);
+
+        // parse output
+        RouteDTO createdRoute = mapper.readValue(createOutput, routeDTOClass);
+
+        // make sure saved data matches expected data
+        assertThat(createdRoute.route_id, equalTo(routeId));
+        // TODO: Verify with a SQL query that the database now contains the created data (we may need to use the same
+        //       db connection to do this successfully?)
+
+        // try to update record
+        String updatedRouteId = "600";
+        createdRoute.route_id = updatedRouteId;
+
+        // covert object to json and save it
+        JdbcTableWriter updateTableWriter = createTestTableWriter(routeTable);
+        String updateOutput = updateTableWriter.update(
+                createdRoute.id,
+                mapper.writeValueAsString(createdRoute),
+                true
+        );
+        LOG.info("update {} output:", routeTable.name);
+        LOG.info(updateOutput);
+
+        RouteDTO updatedRouteDTO = mapper.readValue(updateOutput, routeDTOClass);
+
+        // make sure saved data matches expected data
+        assertThat(updatedRouteDTO.route_id, equalTo(updatedRouteId));
+        // TODO: Verify with a SQL query that the database now contains the updated data (we may need to use the same
+        //       db connection to do this successfully?)
+
+        // try to delete record
+        JdbcTableWriter deleteTableWriter = createTestTableWriter(routeTable);
+        int deleteOutput = deleteTableWriter.delete(
+                createdRoute.id,
+                true
+        );
+        LOG.info("deleted {} records from {}", deleteOutput, routeTable.name);
+
+        // make sure route record does not exist in DB
+        assertThatSqlQueryYieldsZeroRows(String.format(
+                "select * from %s.%s where id=%d",
+                testNamespace,
+                routeTable.name,
+                createdRoute.id
+        ));
+    }
+
     @AfterClass
     public static void tearDownClass() {
         TestUtils.dropDB(testDBName);