diff --git a/src/main/java/com/conveyal/gtfs/graphql/fetchers/FeedFetcher.java b/src/main/java/com/conveyal/gtfs/graphql/fetchers/FeedFetcher.java index 5b5386529..cf8f2ade0 100644 --- a/src/main/java/com/conveyal/gtfs/graphql/fetchers/FeedFetcher.java +++ b/src/main/java/com/conveyal/gtfs/graphql/fetchers/FeedFetcher.java @@ -34,7 +34,7 @@ public Map 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(); diff --git a/src/main/java/com/conveyal/gtfs/graphql/fetchers/JDBCFetcher.java b/src/main/java/com/conveyal/gtfs/graphql/fetchers/JDBCFetcher.java index 3eac44404..d0a3e8806 100644 --- a/src/main/java/com/conveyal/gtfs/graphql/fetchers/JDBCFetcher.java +++ b/src/main/java/com/conveyal/gtfs/graphql/fetchers/JDBCFetcher.java @@ -341,7 +341,7 @@ List> 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> 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; } diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java index 4b341081e..67168e7d7 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java @@ -186,7 +186,7 @@ public String update(Integer id, String json, boolean autoCommit) throws SQLExce referencedPatternUsesFrequencies = selectResults.getBoolean(1); } } - updateChildTable( + String keyValue = updateChildTable( childEntitiesArray, entityId, referencedPatternUsesFrequencies, @@ -194,6 +194,10 @@ public String update(Integer id, String json, boolean autoCommit) throws SQLExce 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,7 +445,7 @@ 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, @@ -449,13 +453,11 @@ private void updateChildTable( 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"); } /** diff --git a/src/test/java/com/conveyal/gtfs/util/CalendarDTO.java b/src/test/java/com/conveyal/gtfs/dto/CalendarDTO.java similarity index 92% rename from src/test/java/com/conveyal/gtfs/util/CalendarDTO.java rename to src/test/java/com/conveyal/gtfs/dto/CalendarDTO.java index 169ffa5ac..f948c082c 100644 --- a/src/test/java/com/conveyal/gtfs/util/CalendarDTO.java +++ b/src/test/java/com/conveyal/gtfs/dto/CalendarDTO.java @@ -1,4 +1,4 @@ -package com.conveyal.gtfs.util; +package com.conveyal.gtfs.dto; public class CalendarDTO { public Integer id; diff --git a/src/test/java/com/conveyal/gtfs/util/FareDTO.java b/src/test/java/com/conveyal/gtfs/dto/FareDTO.java similarity index 95% rename from src/test/java/com/conveyal/gtfs/util/FareDTO.java rename to src/test/java/com/conveyal/gtfs/dto/FareDTO.java index 889e328e3..5aec354c3 100644 --- a/src/test/java/com/conveyal/gtfs/util/FareDTO.java +++ b/src/test/java/com/conveyal/gtfs/dto/FareDTO.java @@ -1,4 +1,4 @@ -package com.conveyal.gtfs.util; +package com.conveyal.gtfs.dto; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; diff --git a/src/test/java/com/conveyal/gtfs/util/FareRuleDTO.java b/src/test/java/com/conveyal/gtfs/dto/FareRuleDTO.java similarity index 92% rename from src/test/java/com/conveyal/gtfs/util/FareRuleDTO.java rename to src/test/java/com/conveyal/gtfs/dto/FareRuleDTO.java index a5fb475d1..b2611583e 100644 --- a/src/test/java/com/conveyal/gtfs/util/FareRuleDTO.java +++ b/src/test/java/com/conveyal/gtfs/dto/FareRuleDTO.java @@ -1,4 +1,4 @@ -package com.conveyal.gtfs.util; +package com.conveyal.gtfs.dto; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; diff --git a/src/test/java/com/conveyal/gtfs/util/FeedInfoDTO.java b/src/test/java/com/conveyal/gtfs/dto/FeedInfoDTO.java similarity index 93% rename from src/test/java/com/conveyal/gtfs/util/FeedInfoDTO.java rename to src/test/java/com/conveyal/gtfs/dto/FeedInfoDTO.java index a7b109d31..0ccbbd942 100644 --- a/src/test/java/com/conveyal/gtfs/util/FeedInfoDTO.java +++ b/src/test/java/com/conveyal/gtfs/dto/FeedInfoDTO.java @@ -1,4 +1,4 @@ -package com.conveyal.gtfs.util; +package com.conveyal.gtfs.dto; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; diff --git a/src/test/java/com/conveyal/gtfs/util/FrequencyDTO.java b/src/test/java/com/conveyal/gtfs/dto/FrequencyDTO.java similarity index 85% rename from src/test/java/com/conveyal/gtfs/util/FrequencyDTO.java rename to src/test/java/com/conveyal/gtfs/dto/FrequencyDTO.java index a59617b4a..c4650490b 100644 --- a/src/test/java/com/conveyal/gtfs/util/FrequencyDTO.java +++ b/src/test/java/com/conveyal/gtfs/dto/FrequencyDTO.java @@ -1,4 +1,4 @@ -package com.conveyal.gtfs.util; +package com.conveyal.gtfs.dto; public class FrequencyDTO { public String trip_id; diff --git a/src/test/java/com/conveyal/gtfs/util/PatternDTO.java b/src/test/java/com/conveyal/gtfs/dto/PatternDTO.java similarity index 90% rename from src/test/java/com/conveyal/gtfs/util/PatternDTO.java rename to src/test/java/com/conveyal/gtfs/dto/PatternDTO.java index eb78e516d..1a060f8d3 100644 --- a/src/test/java/com/conveyal/gtfs/util/PatternDTO.java +++ b/src/test/java/com/conveyal/gtfs/dto/PatternDTO.java @@ -1,4 +1,4 @@ -package com.conveyal.gtfs.util; +package com.conveyal.gtfs.dto; public class PatternDTO { public Integer id; diff --git a/src/test/java/com/conveyal/gtfs/util/PatternStopDTO.java b/src/test/java/com/conveyal/gtfs/dto/PatternStopDTO.java similarity index 82% rename from src/test/java/com/conveyal/gtfs/util/PatternStopDTO.java rename to src/test/java/com/conveyal/gtfs/dto/PatternStopDTO.java index b6daf838d..12888f4d6 100644 --- a/src/test/java/com/conveyal/gtfs/util/PatternStopDTO.java +++ b/src/test/java/com/conveyal/gtfs/dto/PatternStopDTO.java @@ -1,4 +1,4 @@ -package com.conveyal.gtfs.util; +package com.conveyal.gtfs.dto; public class PatternStopDTO { public Integer id; @@ -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; diff --git a/src/test/java/com/conveyal/gtfs/util/RouteDTO.java b/src/test/java/com/conveyal/gtfs/dto/RouteDTO.java similarity index 96% rename from src/test/java/com/conveyal/gtfs/util/RouteDTO.java rename to src/test/java/com/conveyal/gtfs/dto/RouteDTO.java index c5868f592..33f6fa49d 100644 --- a/src/test/java/com/conveyal/gtfs/util/RouteDTO.java +++ b/src/test/java/com/conveyal/gtfs/dto/RouteDTO.java @@ -1,4 +1,4 @@ -package com.conveyal.gtfs.util; +package com.conveyal.gtfs.dto; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; diff --git a/src/test/java/com/conveyal/gtfs/dto/ShapePointDTO.java b/src/test/java/com/conveyal/gtfs/dto/ShapePointDTO.java new file mode 100644 index 000000000..b7b5210f6 --- /dev/null +++ b/src/test/java/com/conveyal/gtfs/dto/ShapePointDTO.java @@ -0,0 +1,23 @@ +package com.conveyal.gtfs.dto; + +// 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; + } +} diff --git a/src/test/java/com/conveyal/gtfs/util/StopDTO.java b/src/test/java/com/conveyal/gtfs/dto/StopDTO.java similarity index 92% rename from src/test/java/com/conveyal/gtfs/util/StopDTO.java rename to src/test/java/com/conveyal/gtfs/dto/StopDTO.java index 4ca821e59..21ec9e4fa 100644 --- a/src/test/java/com/conveyal/gtfs/util/StopDTO.java +++ b/src/test/java/com/conveyal/gtfs/dto/StopDTO.java @@ -1,4 +1,4 @@ -package com.conveyal.gtfs.util; +package com.conveyal.gtfs.dto; public class StopDTO { public Integer id; diff --git a/src/test/java/com/conveyal/gtfs/util/StopTimeDTO.java b/src/test/java/com/conveyal/gtfs/dto/StopTimeDTO.java similarity index 95% rename from src/test/java/com/conveyal/gtfs/util/StopTimeDTO.java rename to src/test/java/com/conveyal/gtfs/dto/StopTimeDTO.java index bfbc56424..9e385e3ca 100644 --- a/src/test/java/com/conveyal/gtfs/util/StopTimeDTO.java +++ b/src/test/java/com/conveyal/gtfs/dto/StopTimeDTO.java @@ -1,4 +1,4 @@ -package com.conveyal.gtfs.util; +package com.conveyal.gtfs.dto; public class StopTimeDTO { public String trip_id; diff --git a/src/test/java/com/conveyal/gtfs/util/TripDTO.java b/src/test/java/com/conveyal/gtfs/dto/TripDTO.java similarity index 93% rename from src/test/java/com/conveyal/gtfs/util/TripDTO.java rename to src/test/java/com/conveyal/gtfs/dto/TripDTO.java index ccd43a7e0..587c83e01 100644 --- a/src/test/java/com/conveyal/gtfs/util/TripDTO.java +++ b/src/test/java/com/conveyal/gtfs/dto/TripDTO.java @@ -1,4 +1,4 @@ -package com.conveyal.gtfs.util; +package com.conveyal.gtfs.dto; public class TripDTO { public Integer id; diff --git a/src/test/java/com/conveyal/gtfs/dto/package-info.java b/src/test/java/com/conveyal/gtfs/dto/package-info.java new file mode 100644 index 000000000..546d39afc --- /dev/null +++ b/src/test/java/com/conveyal/gtfs/dto/package-info.java @@ -0,0 +1,18 @@ +/** + * This package contains Data Transfer Objects that are solely used for efficient serialization and deserialization of + * data into and out of JSON strings in unit tests, which the {@link com.conveyal.gtfs.loader.JdbcTableWriter#update(java.lang.Integer, java.lang.String, boolean)} + * and {@link com.conveyal.gtfs.loader.JdbcTableWriter#create(java.lang.String, boolean)} methods take as input in order + * to modify or create GTFS entities. These DTOs are not used internally by the JdbcTableWriter in favor of simply + * deserializing JSON into {@link com.fasterxml.jackson.databind.JsonNode} objects and validating the fields using + * {@link com.conveyal.gtfs.loader.Table} definitions. This approach was chosen because it allows the update and create + * methods to generically process the JSON and update the data based on the rules and relations defined by the Tables. + * + * The value/wisdom of maintaining these objects for tests alone has been posed and is up for debate, but they do offer + * an advantage over generating JSON with Jackson because JdbcTableWriter expects input JSON to have a value defined for + * each field for a given entity (even if it is just null). Using Jackson for this purpose would become verbose. They + * also might be better suited for tests than {@link com.conveyal.gtfs.model} objects because they do not use primitive + * types, which allows for testing bad inputs (though as of 2019/02/13 we are not testing for bad inputs AFAIK). + */ + +package com.conveyal.gtfs.dto; + diff --git a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java index c00c59857..b95253689 100644 --- a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java +++ b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java @@ -1,19 +1,19 @@ package com.conveyal.gtfs.loader; import com.conveyal.gtfs.TestUtils; -import com.conveyal.gtfs.util.CalendarDTO; -import com.conveyal.gtfs.util.FareDTO; -import com.conveyal.gtfs.util.FareRuleDTO; -import com.conveyal.gtfs.util.FeedInfoDTO; -import com.conveyal.gtfs.util.FrequencyDTO; +import com.conveyal.gtfs.dto.CalendarDTO; +import com.conveyal.gtfs.dto.FareDTO; +import com.conveyal.gtfs.dto.FareRuleDTO; +import com.conveyal.gtfs.dto.FeedInfoDTO; +import com.conveyal.gtfs.dto.FrequencyDTO; import com.conveyal.gtfs.util.InvalidNamespaceException; -import com.conveyal.gtfs.util.PatternDTO; -import com.conveyal.gtfs.util.PatternStopDTO; -import com.conveyal.gtfs.util.RouteDTO; -import com.conveyal.gtfs.util.ShapePointDTO; -import com.conveyal.gtfs.util.StopDTO; -import com.conveyal.gtfs.util.StopTimeDTO; -import com.conveyal.gtfs.util.TripDTO; +import com.conveyal.gtfs.dto.PatternDTO; +import com.conveyal.gtfs.dto.PatternStopDTO; +import com.conveyal.gtfs.dto.RouteDTO; +import com.conveyal.gtfs.dto.ShapePointDTO; +import com.conveyal.gtfs.dto.StopDTO; +import com.conveyal.gtfs.dto.StopTimeDTO; +import com.conveyal.gtfs.dto.TripDTO; import com.fasterxml.jackson.databind.ObjectMapper; import org.junit.AfterClass; import org.junit.BeforeClass; @@ -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,7 +349,14 @@ 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 @@ -349,9 +364,10 @@ private static PatternDTO createSimpleRouteAndPattern(String routeId, String pat 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 diff --git a/src/test/java/com/conveyal/gtfs/util/ShapePointDTO.java b/src/test/java/com/conveyal/gtfs/util/ShapePointDTO.java deleted file mode 100644 index 05ed6d6da..000000000 --- a/src/test/java/com/conveyal/gtfs/util/ShapePointDTO.java +++ /dev/null @@ -1,6 +0,0 @@ -package com.conveyal.gtfs.util; - -// TODO add fields -public class ShapePointDTO { - -}