Copy shape on update

src/main/java/com/conveyal/gtfs/graphql/fetchers/FeedFetcher.java

@@ -34,7 +34,7 @@ public Map<String, Object> get (DataFetchingEnvironment environment) {
         try {
             connection = GTFSGraphQL.getConnection();
             Statement statement = connection.createStatement();
-            LOG.info("SQL: {}", sqlBuilder.toString());
+            LOG.debug("SQL: {}", sqlBuilder.toString());
             if (statement.execute(sqlBuilder.toString())) {
                 ResultSet resultSet = statement.getResultSet();
                 ResultSetMetaData meta = resultSet.getMetaData();

src/main/java/com/conveyal/gtfs/graphql/fetchers/JDBCFetcher.java

@@ -341,7 +341,7 @@ List<Map<String, Object>> getResults (
             }
             // This logging produces a lot of noise during testing due to large numbers of joined sub-queries
 //            LOG.info("table name={}", tableName);
-            LOG.info("SQL: {}", preparedStatement.toString());
+            LOG.debug("SQL: {}", preparedStatement.toString());
             if (preparedStatement.execute()) {
                 ResultSet resultSet = preparedStatement.getResultSet();
                 ResultSetMetaData meta = resultSet.getMetaData();
@@ -363,7 +363,7 @@ List<Map<String, Object>> getResults (
         } finally {
             DbUtils.closeQuietly(connection);
         }
-        LOG.info("Result size: {}", results.size());
+        LOG.debug("Result size: {}", results.size());
         // Return a List of Maps, one Map for each row in the result.
         return results;
     }

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

@@ -186,14 +186,18 @@ public String update(Integer id, String json, boolean autoCommit) throws SQLExce
                             referencedPatternUsesFrequencies = selectResults.getBoolean(1);
                         }
                     }
-                    updateChildTable(
+                    String keyValue = updateChildTable(
                         childEntitiesArray,
                         entityId,
                         referencedPatternUsesFrequencies,
                         isCreating,
                         referencingTable,
                         connection
                     );
+                    // Ensure JSON return object is updated with referencing table's (potentially) new key value.
+                    // Currently, the only case where an update occurs is when a referenced shape is referenced by other
+                    // patterns.
+                    jsonObject.put(referencingTable.getKeyFieldName(), keyValue);
                 }
             }
             // Iterate over table's fields and apply linked values to any tables. This is to account for "exemplar"
@@ -441,21 +445,19 @@ private void setStatementParameters(
      * have a hierarchical relationship.
      * FIXME develop a better way to update tables with foreign keys to the table being updated.
      */
-    private void updateChildTable(
+    private String updateChildTable(
         ArrayNode subEntities,
         int id,
         boolean referencedPatternUsesFrequencies,
         boolean isCreatingNewEntity,
         Table subTable,
         Connection connection
     ) throws SQLException, IOException {
-        // Get parent table's key field
-        Field keyField;
-        String keyValue;
-        // Primary key fields are always referenced by foreign key fields with the same name.
-        keyField = specTable.getFieldForName(subTable.getKeyFieldName());
+        // Get parent table's key field. Primary key fields are always referenced by foreign key fields with the same
+        // name.
+        Field keyField = specTable.getFieldForName(subTable.getKeyFieldName());
         // Get parent entity's key value
-        keyValue = getValueForId(id, keyField.name, tablePrefix, specTable, connection);
+        String keyValue = getValueForId(id, keyField.name, tablePrefix, specTable, connection);
         String childTableName = String.join(".", tablePrefix, subTable.name);
         if (!referencedPatternUsesFrequencies && subTable.name.equals(Table.FREQUENCIES.name) && subEntities.size() > 0) {
             // Do not permit the illegal state where frequency entries are being added/modified for a timetable pattern.
@@ -479,17 +481,44 @@ private void updateChildTable(
             reconcilePatternStops(keyValue, newPatternStops, connection);
         }
         if (!isCreatingNewEntity) {
-            // Delete existing sub-entities for given entity ID if the parent entity is not being newly created.
-            String deleteSql = getUpdateReferencesSql(SqlMethod.DELETE, childTableName, keyField, keyValue, null);
-            LOG.info(deleteSql);
-            Statement statement = connection.createStatement();
-            // FIXME: Use copy on update for a pattern's shape instead of deleting the previous shape and replacing it.
-            //   This would better account for GTFS data loaded from a file where multiple patterns reference a single
-            //   shape.
-            int result = statement.executeUpdate(deleteSql);
-            LOG.info("Deleted {} {}", result, subTable.name);
-            // FIXME: are there cases when an update should not return zero?
-//            if (result == 0) throw new SQLException("No stop times found for trip ID");
+            // If not creating a new entity, we will delete the child entities (e.g., shape points or pattern stops) and
+            // regenerate them anew to avoid any messiness that we may encounter with update statements.
+            if (Table.SHAPES.name.equals(subTable.name)) {
+                // Check how many patterns are referencing the same shape_id to determine if we should copy on update.
+                String patternsForShapeIdSql = String.format("select id from %s.patterns where shape_id = ?", tablePrefix);
+                PreparedStatement statement = connection.prepareStatement(patternsForShapeIdSql);
+                statement.setString(1, keyValue);
+                LOG.info(statement.toString());
+                ResultSet resultSet = statement.executeQuery();
+                int patternsForShapeId = 0;
+                while (resultSet.next()) {
+                    patternsForShapeId++;
+                }
+                if (patternsForShapeId > 1) {
+                    // Use copy on update for pattern shape if a single shape is being used for multiple patterns because
+                    // we do not want edits for a given pattern (for example, a short run) to impact the shape for another
+                    // pattern (perhaps a long run that extends farther). Note: this behavior will have the side effect of
+                    // creating potentially redundant shape information, but this is better than accidentally butchering the
+                    // shapes for other patterns.
+                    LOG.info("More than one pattern references shape_id: {}", keyValue);
+                    keyValue = UUID.randomUUID().toString();
+                    LOG.info("Creating new shape_id ({}) for pattern id={}.", keyValue, id);
+                    // Update pattern#shape_id with new value. Note: shape_point#shape_id values are coerced to new
+                    // value further down in this function.
+                    String updatePatternShapeIdSql = String.format("update %s.patterns set shape_id = ? where id = ?", tablePrefix);
+                    PreparedStatement updateStatement = connection.prepareStatement(updatePatternShapeIdSql);
+                    updateStatement.setString(1, keyValue);
+                    updateStatement.setInt(2, id);
+                    LOG.info(updateStatement.toString());
+                    updateStatement.executeUpdate();
+                } else {
+                    // If only one pattern references this shape, delete the existing shape points to start anew.
+                    deleteChildEntities(subTable, keyField, keyValue);
+                }
+            } else {
+                // If not handling shape points, delete the child entities and create them anew.
+                deleteChildEntities(subTable, keyField, keyValue);
+            }
         }
         int entityCount = 0;
         PreparedStatement insertStatement = null;
@@ -587,6 +616,25 @@ private void updateChildTable(
         } else {
             LOG.info("No inserts to execute. Empty array found in JSON for child table {}", childTableName);
         }
+        // Return key value in the case that it was updated (the only case for this would be if the shape was referenced
+        // by multiple patterns).
+        return keyValue;
+    }
+
+    /**
+     * Delete existing sub-entities for given key value for when an update to the parent entity is made (i.e., the parent
+     * entity is not being newly created). Examples of sub-entities include stop times for trips, pattern stops for a
+     * pattern, or shape points (for a pattern in our model).
+     */
+    private void deleteChildEntities(Table childTable, Field keyField, String keyValue) throws SQLException {
+        String childTableName = String.join(".", tablePrefix, childTable.name);
+        String deleteSql = getUpdateReferencesSql(SqlMethod.DELETE, childTableName, keyField, keyValue, null);
+        LOG.info(deleteSql);
+        Statement deleteStatement = connection.createStatement();
+        int result = deleteStatement.executeUpdate(deleteSql);
+        LOG.info("Deleted {} {}", result, childTable.name);
+        // FIXME: are there cases when an update should not return zero?
+        //   if (result == 0) throw new SQLException("No stop times found for trip ID");
     }
 
     /**

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

@@ -30,7 +30,10 @@
 import static com.conveyal.gtfs.GTFS.createDataSource;
 import static com.conveyal.gtfs.GTFS.makeSnapshot;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.not;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
 
 /**
  * This class contains CRUD tests for {@link JdbcTableWriter} (i.e., editing GTFS entities in the RDBMS). Set up
@@ -48,6 +51,11 @@ public class JDBCTableWriterTest {
     private static String simpleServiceId = "1";
     private static String firstStopId = "1";
     private static String lastStopId = "2";
+    private static double firstStopLat = 34.2222;
+    private static double firstStopLon = -87.333;
+    private static double lastStopLat = 34.2233;
+    private static double lastStopLon = -87.334;
+    private static String sharedShapeId = "shared_shape_id";
     private static final ObjectMapper mapper = new ObjectMapper();
 
     private static JdbcTableWriter createTestTableWriter (Table table) throws InvalidNamespaceException {
@@ -73,8 +81,8 @@ public static void setUpClass() throws SQLException, IOException, InvalidNamespa
         testNamespace = result.uniqueIdentifier;
         // Create a service calendar and two stops, both of which are necessary to perform pattern and trip tests.
         createWeekdayCalendar(simpleServiceId, "20180103", "20180104");
-        createSimpleStop(firstStopId, "First Stop", 34.2222, -87.333);
-        createSimpleStop(lastStopId, "Last Stop", 34.2233, -87.334);
+        createSimpleStop(firstStopId, "First Stop", firstStopLat, firstStopLon);
+        createSimpleStop(lastStopId, "Last Stop", lastStopLat, lastStopLon);
     }
 
     @Test
@@ -341,17 +349,25 @@ private static RouteDTO createSimpleTestRoute(String routeId, String agencyId, S
     /**
      * Creates a pattern by first creating a route and then a pattern for that route.
      */
-    private static PatternDTO createSimpleRouteAndPattern(String routeId, String patternId, String name) throws InvalidNamespaceException, SQLException, IOException {
+    private static PatternDTO createRouteAndSimplePattern(String routeId, String patternId, String name) throws InvalidNamespaceException, SQLException, IOException {
+        return createRouteAndPattern(routeId, patternId, name, null, new ShapePointDTO[]{}, new PatternStopDTO[]{}, 0);
+    }
+
+    /**
+     * Creates a pattern by first creating a route and then a pattern for that route.
+     */
+    private static PatternDTO createRouteAndPattern(String routeId, String patternId, String name, String shapeId, ShapePointDTO[] shapes, PatternStopDTO[] patternStops, int useFrequency) throws InvalidNamespaceException, SQLException, IOException {
         // Create new route
         createSimpleTestRoute(routeId, "RTA", "500", "Hollingsworth", 3);
         // Create new pattern for route
         PatternDTO input = new PatternDTO();
         input.pattern_id = patternId;
         input.route_id = routeId;
         input.name = name;
-        input.use_frequency = 0;
-        input.shapes = new ShapePointDTO[]{};
-        input.pattern_stops = new PatternStopDTO[]{};
+        input.use_frequency = useFrequency;
+        input.shape_id = shapeId;
+        input.shapes = shapes;
+        input.pattern_stops = patternStops;
         // Write the pattern to the database
         JdbcTableWriter createPatternWriter = createTestTableWriter(Table.PATTERNS);
         String output = createPatternWriter.create(mapper.writeValueAsString(input), true);
@@ -366,12 +382,57 @@ private static PatternDTO createSimpleRouteAndPattern(String routeId, String pat
      */
     @Test(expected = IllegalStateException.class)
     public void cannotCreateFrequencyForTimetablePattern() throws InvalidNamespaceException, IOException, SQLException {
-        PatternDTO simplePattern = createSimpleRouteAndPattern("900", "8", "The Loop");
+        PatternDTO simplePattern = createRouteAndSimplePattern("900", "8", "The Loop");
         TripDTO tripInput = constructFrequencyTrip(simplePattern.pattern_id, simplePattern.route_id, 6 * 60 * 60);
         JdbcTableWriter createTripWriter = createTestTableWriter(Table.TRIPS);
         createTripWriter.create(mapper.writeValueAsString(tripInput), true);
     }
 
+    /**
+     * When multiple patterns reference a single shape_id, the returned JSON from an update to any of these patterns
+     * (whether the shape points were updated or not) should have a new shape_id because of the "copy on update" logic
+     * that ensures the shared shape is not modified.
+     */
+    @Test
+    public void shouldChangeShapeIdOnPatternUpdate() throws IOException, SQLException, InvalidNamespaceException {
+        String patternId = "10";
+        ShapePointDTO[] shapes = new ShapePointDTO[]{
+            new ShapePointDTO(2, 0.0, sharedShapeId, firstStopLat, firstStopLon, 0),
+            new ShapePointDTO(2, 150.0, sharedShapeId, lastStopLat, lastStopLon, 1)
+        };
+        PatternStopDTO[] patternStops = new PatternStopDTO[]{
+            new PatternStopDTO(patternId, firstStopId, 0),
+            new PatternStopDTO(patternId, lastStopId, 1)
+        };
+        PatternDTO simplePattern = createRouteAndPattern("1001", patternId, "The Line", sharedShapeId, shapes, patternStops, 0);
+        assertThat(simplePattern.shape_id, equalTo(sharedShapeId));
+        // Create pattern with shared shape. Note: typically we would encounter shared shapes on imported feeds (e.g.,
+        // BART), but this should simulate the situation well enough.
+        String secondPatternId = "11";
+        patternStops[0].pattern_id = secondPatternId;
+        patternStops[1].pattern_id = secondPatternId;
+        PatternDTO patternWithSharedShape = createRouteAndPattern("1002", secondPatternId, "The Line 2", sharedShapeId, shapes, patternStops, 0);
+        // Verify that shape_id is shared.
+        assertThat(patternWithSharedShape.shape_id, equalTo(sharedShapeId));
+        // Update any field on one of the patterns.
+        JdbcTableWriter patternUpdater = createTestTableWriter(Table.PATTERNS);
+        patternWithSharedShape.name = "The shape_id should update";
+        String sharedPatternOutput = patternUpdater.update(patternWithSharedShape.id, mapper.writeValueAsString(patternWithSharedShape), true);
+        // The output should contain a new backend-generated shape_id.
+        PatternDTO updatedSharedPattern = mapper.readValue(sharedPatternOutput, PatternDTO.class);
+        LOG.info("Updated pattern output: {}", sharedPatternOutput);
+        String newShapeId = updatedSharedPattern.shape_id;
+        assertThat(newShapeId, not(equalTo(sharedShapeId)));
+        // Ensure that pattern record in database reflects updated shape ID.
+        assertThatSqlQueryYieldsRowCount(String.format(
+            "select * from %s.%s where shape_id='%s' and pattern_id='%s'",
+            testNamespace,
+            Table.PATTERNS.name,
+            newShapeId,
+            secondPatternId
+        ), 1);
+    }
+
     /**
      * Checks that creating a frequency trip functions properly. This also updates the pattern to include pattern stops,
      * which is a prerequisite for creating a frequency trip with stop times.
@@ -381,7 +442,7 @@ public void canCreateUpdateAndDeleteFrequencyTripForFrequencyPattern() throws IO
         // Store Table and Class values for use in test.
         final Table tripsTable = Table.TRIPS;
         int startTime = 6 * 60 * 60;
-        PatternDTO simplePattern = createSimpleRouteAndPattern("1000", "9", "The Line");
+        PatternDTO simplePattern = createRouteAndSimplePattern("1000", "9", "The Line");
         TripDTO tripInput = constructFrequencyTrip(simplePattern.pattern_id, simplePattern.route_id, startTime);
         JdbcTableWriter createTripWriter = createTestTableWriter(tripsTable);
         // Update pattern with pattern stops, set to use frequencies, and TODO shape points

src/test/java/com/conveyal/gtfs/util/PatternStopDTO.java

@@ -12,6 +12,9 @@ public class PatternStopDTO {
     public Integer stop_sequence;
     public Integer timepoint;
 
+    /** Empty constructor for deserialization */
+    public PatternStopDTO() {}
+
     public PatternStopDTO (String patternId, String stopId, int stopSequence) {
         pattern_id = patternId;
         stop_id = stopId;

src/test/java/com/conveyal/gtfs/util/ShapePointDTO.java

@@ -2,5 +2,22 @@
 
 // TODO add fields
 public class ShapePointDTO {
+    public Integer point_type;
+    public Double shape_dist_traveled;
+    public String shape_id;
+    public Double shape_pt_lat;
+    public Double shape_pt_lon;
+    public Integer shape_pt_sequence;
 
+    /** Empty constructor for deserialization */
+    public ShapePointDTO() {}
+
+    public ShapePointDTO(Integer point_type, Double shape_dist_traveled, String shape_id, Double shape_pt_lat, Double shape_pt_lon, Integer shape_pt_sequence) {
+        this.point_type = point_type;
+        this.shape_dist_traveled = shape_dist_traveled;
+        this.shape_id = shape_id;
+        this.shape_pt_lat = shape_pt_lat;
+        this.shape_pt_lon = shape_pt_lon;
+        this.shape_pt_sequence = shape_pt_sequence;
+    }
 }