From a4d43a24ee157702b849090240c1ab4fb67a5edf Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Tue, 18 Dec 2018 12:27:00 -0500 Subject: [PATCH 01/11] fix(shape-edits): update trip#shape_id if pattern value changes refs conveyal/datatools-ui#385; refs #109 --- .../conveyal/gtfs/loader/JdbcTableWriter.java | 91 ++++++++++++++----- .../java/com/conveyal/gtfs/loader/Table.java | 4 +- 2 files changed, 69 insertions(+), 26 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java index 94f77418d..9a1c1a05d 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java @@ -77,9 +77,11 @@ public JdbcTableWriter ( // TODO: verify specTable.name is ok to use for constructing dynamic sql statements this.specTable = specTable; + // Below is a bit messy because the connection field on this class is set to final and we cannot set this to + // the optionally passed-in connection with the ternary statement unless connection1 already exists. Connection connection1; try { - connection1 = dataSource.getConnection(); + connection1 = this.dataSource.getConnection(); } catch (SQLException e) { e.printStackTrace(); connection1 = null; @@ -170,11 +172,34 @@ public String update(Integer id, String json, boolean autoCommit) throws SQLExce updateChildTable(childEntitiesArray, entityId, isCreating, referencingTable, connection); } } - // Iterate over table's fields and apply linked values to any tables - if ("routes".equals(specTable.name)) { - updateLinkedFields(specTable, jsonObject, "trips", "route_id", "wheelchair_accessible"); - } else if ("patterns".equals(specTable.name)) { - updateLinkedFields(specTable, jsonObject, "trips", "pattern_id", "direction_id"); + // Iterate over table's fields and apply linked values to any tables. This is to account for "exemplar" + // fields that exist in one place in our tables, but are duplicated in GTFS. For example, we have a + // Route#wheelchair_accessible field, which is used to set the Trip#wheelchair_accessible values for all + // trips on a route. + // NOTE: pattern_stops linked fields are updated in the updateChildTable method. + switch (specTable.name) { + case "routes": + updateLinkedFields( + specTable, + jsonObject, + "trips", + "route_id", + "wheelchair_accessible" + ); + break; + case "patterns": + updateLinkedFields( + specTable, + jsonObject, + "trips", + "pattern_id", + "direction_id", "shape_id" + ); + break; + default: + LOG.debug("No linked fields to update."); + // Do nothing. + break; } if (autoCommit) { // If nothing failed up to this point, it is safe to assume there were no problems updating/creating the @@ -205,6 +230,7 @@ public String update(Integer id, String json, boolean autoCommit) throws SQLExce * the related table (e.g., pattern_stop#timepoint should update all of its related stop_times). */ private void updateLinkedFields(Table referenceTable, ObjectNode jsonObject, String tableName, String keyField, String ...fieldNames) throws SQLException { + boolean updatingStopTimes = "stop_times".equals(tableName); // Collect fields, the JSON values for these fields, and the strings to add to the prepared statement into Lists. List fields = new ArrayList<>(); List values = new ArrayList<>(); @@ -216,12 +242,17 @@ private void updateLinkedFields(Table referenceTable, ObjectNode jsonObject, Str } String setFields = String.join(", ", fieldStrings); // If updating stop_times, use a more complex query that joins trips to stop_times in order to match on pattern_id - boolean updatingStopTimes = "stop_times".equals(tableName); Field orderField = updatingStopTimes ? referenceTable.getFieldForName(referenceTable.getOrderFieldName()) : null; String sql = updatingStopTimes - ? String.format("update %s.stop_times st set %s from %s.trips t " + - "where st.trip_id = t.trip_id AND t.%s = ? AND st.%s = ?", - tablePrefix, setFields, tablePrefix, keyField, orderField.name) + ? String.format( + "update %s.stop_times st set %s from %s.trips t " + + "where st.trip_id = t.trip_id AND t.%s = ? AND st.%s = ?", + tablePrefix, + setFields, + tablePrefix, + keyField, + orderField.name + ) : String.format("update %s.%s set %s where %s = ?", tablePrefix, tableName, setFields, keyField); // Prepare the statement and set statement parameters PreparedStatement statement = connection.prepareStatement(sql); @@ -371,8 +402,6 @@ private void updateChildTable(ArrayNode subEntities, Integer id, boolean isCreat // Get parent entity's key value keyValue = getValueForId(id, keyField.name, tablePrefix, specTable, connection); String childTableName = String.join(".", tablePrefix, subTable.name); - // FIXME: add check for pattern stop consistency. - // FIXME: re-order stop times if pattern stop order changes. // Reconciling pattern stops MUST happen before original pattern stops are deleted in below block (with // getUpdateReferencesSql) if ("pattern_stops".equals(subTable.name)) { @@ -390,15 +419,14 @@ private void updateChildTable(ArrayNode subEntities, Integer id, boolean isCreat } reconcilePatternStops(keyValue, newPatternStops, connection); } - // FIXME: allow shapes to be updated on pattern geometry change. 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. + // 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? @@ -436,7 +464,7 @@ private void updateChildTable(ArrayNode subEntities, Integer id, boolean isCreat // If handling first iteration, create the prepared statement (later iterations will add to batch). insertStatement = createPreparedUpdate(id, true, subEntity, subTable, connection, true); } - // Update linked stop times fields for updated pattern stop (e.g., timepoint, pickup/drop off type). + // Update linked stop times fields for each updated pattern stop (e.g., timepoint, pickup/drop off type). if ("pattern_stops".equals(subTable.name)) { updateLinkedFields( subTable, @@ -610,8 +638,13 @@ else if (i == newStops.size() - 1 || !originalStopIds.get(i).equals(newStops.get } // Increment sequences for stops that follow the inserted location (including the stop at the changed index). // NOTE: This should happen before the blank stop time insertion for logical consistency. - String updateSql = String.format("update %s.stop_times set stop_sequence = stop_sequence + 1 from %s.trips where stop_sequence >= %d AND %s", - tablePrefix, tablePrefix, differenceLocation, joinToTrips); + String updateSql = String.format( + "update %s.stop_times set stop_sequence = stop_sequence + 1 from %s.trips where stop_sequence >= %d AND %s", + tablePrefix, + tablePrefix, + differenceLocation, + joinToTrips + ); LOG.info(updateSql); PreparedStatement updateStatement = connection.prepareStatement(updateSql); int updated = updateStatement.executeUpdate(); @@ -638,13 +671,23 @@ else if (originalStopIds.size() == newStops.size() + 1) { } } // Delete stop at difference location - String deleteSql = String.format("delete from %s.stop_times using %s.trips where stop_sequence = %d AND %s", - tablePrefix, tablePrefix, differenceLocation, joinToTrips); + String deleteSql = String.format( + "delete from %s.stop_times using %s.trips where stop_sequence = %d AND %s", + tablePrefix, + tablePrefix, + differenceLocation, + joinToTrips + ); LOG.info(deleteSql); PreparedStatement deleteStatement = connection.prepareStatement(deleteSql); // Decrement all stops with sequence greater than difference location - String updateSql = String.format("update %s.stop_times set stop_sequence = stop_sequence - 1 from %s.trips where stop_sequence > %d AND %s", - tablePrefix, tablePrefix, differenceLocation, joinToTrips); + String updateSql = String.format( + "update %s.stop_times set stop_sequence = stop_sequence - 1 from %s.trips where stop_sequence > %d AND %s", + tablePrefix, + tablePrefix, + differenceLocation, + joinToTrips + ); LOG.info(updateSql); PreparedStatement updateStatement = connection.prepareStatement(updateSql); int deleted = deleteStatement.executeUpdate(); @@ -764,9 +807,7 @@ else if (originalStopIds.size() < newStops.size()) { insertBlankStopTimes(tripsForPattern, newStops, firstDifferentIndex, stopsToInsert, connection); } // ANY OTHER TYPE OF MODIFICATION IS NOT SUPPORTED - else { - throw new IllegalStateException(RECONCILE_STOPS_ERROR_MSG); - } + else throw new IllegalStateException(RECONCILE_STOPS_ERROR_MSG); } /** diff --git a/src/main/java/com/conveyal/gtfs/loader/Table.java b/src/main/java/com/conveyal/gtfs/loader/Table.java index 9c6b94b7b..0f2a91175 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Table.java +++ b/src/main/java/com/conveyal/gtfs/loader/Table.java @@ -163,6 +163,7 @@ public Table (String name, Class entityClass, Requirement requ new ColorField("route_text_color", OPTIONAL), // Editor fields below. new ShortField("publicly_visible", EDITOR, 1), + // wheelchair_accessible is an exemplar field applied to all trips on a route. new ShortField("wheelchair_accessible", EDITOR, 2).permitEmptyValue(), new IntegerField("route_sort_order", OPTIONAL, 0, Integer.MAX_VALUE), // Status values are In progress (0), Pending approval (1), and Approved (2). @@ -196,7 +197,8 @@ public Table (String name, Class entityClass, Requirement requ new StringField("pattern_id", REQUIRED), new StringField("route_id", REQUIRED).isReferenceTo(ROUTES), new StringField("name", OPTIONAL), - // Editor-specific fields + // Editor-specific fields. + // direction_id and shape_id are exemplar fields applied to all trips for a pattern. new ShortField("direction_id", EDITOR, 1), new ShortField("use_frequency", EDITOR, 1), new StringField("shape_id", EDITOR).isReferenceTo(SHAPES) From 07e0e29cc0d66638835f659e948305e17093d1d8 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Tue, 18 Dec 2018 13:48:06 -0500 Subject: [PATCH 02/11] style: add new lines to reduce line width --- .../conveyal/gtfs/loader/JdbcTableWriter.java | 128 +++++++++++++++--- 1 file changed, 109 insertions(+), 19 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java index 9a1c1a05d..6b793fdf8 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java @@ -229,7 +229,13 @@ public String update(Integer id, String json, boolean autoCommit) throws SQLExce * tables (for now just fields in trips and stop_times) where the reference table's value should take precedence over * the related table (e.g., pattern_stop#timepoint should update all of its related stop_times). */ - private void updateLinkedFields(Table referenceTable, ObjectNode jsonObject, String tableName, String keyField, String ...fieldNames) throws SQLException { + private void updateLinkedFields( + Table referenceTable, + ObjectNode jsonObject, + String tableName, + String keyField, + String ...fieldNames + ) throws SQLException { boolean updatingStopTimes = "stop_times".equals(tableName); // Collect fields, the JSON values for these fields, and the strings to add to the prepared statement into Lists. List fields = new ArrayList<>(); @@ -283,7 +289,14 @@ private void updateLinkedFields(Table referenceTable, ObjectNode jsonObject, Str * object here is provided as a positional argument (rather than provided via the JdbcTableWriter instance field) * because this method is used to update both the specTable for the primary entity and any relevant child entities. */ - private PreparedStatement createPreparedUpdate(Integer id, boolean isCreating, ObjectNode jsonObject, Table table, Connection connection, boolean batch) throws SQLException { + private PreparedStatement createPreparedUpdate( + Integer id, + boolean isCreating, + ObjectNode jsonObject, + Table table, + Connection connection, + boolean batch + ) throws SQLException { String statementString; if (isCreating) { statementString = table.generateInsertSql(tablePrefix, true); @@ -306,7 +319,12 @@ private PreparedStatement createPreparedUpdate(Integer id, boolean isCreating, O * taken from JSON. Note, string values are used here in order to take advantage of setParameter method on * individual fields, which handles parsing string and non-string values into the appropriate SQL field types. */ - private void setStatementParameters(ObjectNode jsonObject, Table table, PreparedStatement preparedStatement, Connection connection) throws SQLException { + private void setStatementParameters( + ObjectNode jsonObject, + Table table, + PreparedStatement preparedStatement, + Connection connection + ) throws SQLException { // JDBC SQL statements use a one-based index for setting fields/parameters List missingFieldNames = new ArrayList<>(); int index = 1; @@ -380,7 +398,13 @@ private void setStatementParameters(ObjectNode jsonObject, Table table, Prepared } if (missingFieldNames.size() > 0) { // String joinedFieldNames = missingFieldNames.stream().collect(Collectors.joining(", ")); - throw new SQLException(String.format("The following field(s) are missing from JSON %s object: %s", table.name, missingFieldNames.toString())); + throw new SQLException( + String.format( + "The following field(s) are missing from JSON %s object: %s", + table.name, + missingFieldNames.toString() + ) + ); } } @@ -486,13 +510,15 @@ private void updateChildTable(ArrayNode subEntities, Integer id, boolean isCreat boolean orderIsUnique = orderValues.add(orderValue); boolean valuesAreIncrementing = ++previousOrder == orderValue; if (!orderIsUnique || !valuesAreIncrementing) { - throw new SQLException(String.format( + throw new SQLException( + String.format( "%s %s values must be zero-based, unique, and incrementing. Entity at index %d had %s value of %d", subTable.name, orderFieldName, entityCount, previousOrder == 0 ? "non-zero" : !valuesAreIncrementing ? "non-incrementing" : "duplicate", - orderValue) + orderValue + ) ); } } @@ -814,7 +840,13 @@ else if (originalStopIds.size() < newStops.size()) { * Check the stops in the changed region to ensure they remain in the same order. If not, throw an exception to * cancel the transaction. */ - private static void verifyInteriorStopsAreUnchanged(List originalStopIds, List newStops, int firstDifferentIndex, int lastDifferentIndex, boolean movedRight) { + private static void verifyInteriorStopsAreUnchanged( + List originalStopIds, + List newStops, + int firstDifferentIndex, + int lastDifferentIndex, + boolean movedRight + ) { //Stops mapped to list of stop IDs simply for easier viewing/comparison with original IDs while debugging with // breakpoints. List newStopIds = newStops.stream().map(s -> s.stop_id).collect(Collectors.toList()); @@ -839,7 +871,13 @@ private static void verifyInteriorStopsAreUnchanged(List originalStopIds * You must call this method after updating sequences for any stop times following the starting stop sequence to * avoid overwriting these other stop times. */ - private void insertBlankStopTimes(List tripIds, List newStops, int startingStopSequence, int stopTimesToAdd, Connection connection) throws SQLException { + private void insertBlankStopTimes( + List tripIds, + List newStops, + int startingStopSequence, + int stopTimesToAdd, + Connection connection + ) throws SQLException { if (tripIds.isEmpty()) { // There is no need to insert blank stop times if there are no trips for the pattern. return; @@ -999,7 +1037,13 @@ private static long handleStatementExecution(PreparedStatement statement, boolea * * FIXME: add more detail/precise language on what this method actually does */ - private static void ensureReferentialIntegrity(Connection connection, ObjectNode jsonObject, String namespace, Table table, Integer id) throws SQLException { + private static void ensureReferentialIntegrity( + Connection connection, + ObjectNode jsonObject, + String namespace, + Table table, + Integer id + ) throws SQLException { final boolean isCreating = id == null; String keyField = table.getKeyFieldName(); String tableName = String.join(".", namespace, table.name); @@ -1075,7 +1119,12 @@ private static int getRowCount(String tableName, Connection connection) throws S /** * For some condition (where field = string value), return the set of unique int IDs for the records that match. */ - private static TIntSet getIdsForCondition(String tableName, String keyField, String keyValue, Connection connection) throws SQLException { + private static TIntSet getIdsForCondition( + String tableName, + String keyField, + String keyValue, + Connection connection + ) throws SQLException { String idCheckSql = String.format("select id from %s where %s = ?", tableName, keyField); // Create statement for counting rows selected PreparedStatement statement = connection.prepareStatement(idCheckSql); @@ -1149,7 +1198,13 @@ private static String getValueForId(int id, String fieldName, String namespace, * not necessarily delete a shape that is shared by multiple trips)? I think not because we are skipping foreign refs * found in the table for the entity being updated/deleted. [Leaving this comment in place for now though.] */ - private static void updateReferencingTables(String namespace, Table table, Connection connection, int id, String newKeyValue) throws SQLException { + private static void updateReferencingTables( + String namespace, + Table table, + Connection connection, + int id, + String newKeyValue + ) throws SQLException { Field keyField = table.getFieldForName(table.getKeyFieldName()); Class entityClass = table.getEntityClass(); // Determine method (update vs. delete) depending on presence of newKeyValue field. @@ -1206,10 +1261,18 @@ private static void updateReferencingTables(String namespace, Table table, Conne if (table.isCascadeDeleteRestricted()) { // The entity must not have any referencing entities in order to delete it. connection.rollback(); - // List idStrings = new ArrayList<>(); - // uniqueIds.forEach(uniqueId -> idStrings.add(String.valueOf(uniqueId))); - // String message = String.format("Cannot delete %s %s=%s. %d %s reference this %s (%s).", entityClass.getSimpleName(), keyField, keyValue, result, referencingTable.name, entityClass.getSimpleName(), String.join(",", idStrings)); - String message = String.format("Cannot delete %s %s=%s. %d %s reference this %s.", entityClass.getSimpleName(), keyField.name, keyValue, result, referencingTable.name, entityClass.getSimpleName()); + // List idStrings = new ArrayList<>(); + // uniqueIds.forEach(uniqueId -> idStrings.add(String.valueOf(uniqueId))); + // String message = String.format("Cannot delete %s %s=%s. %d %s reference this %s (%s).", entityClass.getSimpleName(), keyField, keyValue, result, referencingTable.name, entityClass.getSimpleName(), String.join(",", idStrings)); + String message = String.format( + "Cannot delete %s %s=%s. %d %s reference this %s.", + entityClass.getSimpleName(), + keyField.name, + keyValue, + result, + referencingTable.name, + entityClass.getSimpleName() + ); LOG.warn(message); throw new SQLException(message); } @@ -1226,12 +1289,23 @@ private static void updateReferencingTables(String namespace, Table table, Conne /** * Constructs SQL string based on method provided. */ - private static String getUpdateReferencesSql(SqlMethod sqlMethod, String refTableName, Field keyField, String keyValue, String newKeyValue) throws SQLException { + private static String getUpdateReferencesSql( + SqlMethod sqlMethod, + String refTableName, + Field keyField, + String keyValue, + String newKeyValue + ) throws SQLException { boolean isArrayField = keyField.getSqlType().equals(JDBCType.ARRAY); switch (sqlMethod) { case DELETE: if (isArrayField) { - return String.format("delete from %s where %s @> ARRAY['%s']::text[]", refTableName, keyField.name, keyValue); + return String.format( + "delete from %s where %s @> ARRAY['%s']::text[]", + refTableName, + keyField.name, + keyValue + ); } else { return String.format("delete from %s where %s = '%s'", refTableName, keyField.name, keyValue); } @@ -1240,9 +1314,25 @@ private static String getUpdateReferencesSql(SqlMethod sqlMethod, String refTabl // If the field to be updated is an array field (of which there are only text[] types in the db), // replace the old value with the new value using array contains clause. // FIXME This is probably horribly postgres specific. - return String.format("update %s set %s = array_replace(%s, '%s', '%s') where %s @> ARRAY['%s']::text[]", refTableName, keyField.name, keyField.name, keyValue, newKeyValue, keyField.name, keyValue); + return String.format( + "update %s set %s = array_replace(%s, '%s', '%s') where %s @> ARRAY['%s']::text[]", + refTableName, + keyField.name, + keyField.name, + keyValue, + newKeyValue, + keyField.name, + keyValue + ); } else { - return String.format("update %s set %s = '%s' where %s = '%s'", refTableName, keyField.name, newKeyValue, keyField.name, keyValue); + return String.format( + "update %s set %s = '%s' where %s = '%s'", + refTableName, + keyField.name, + newKeyValue, + keyField.name, + keyValue + ); } // case CREATE: // return String.format("insert into %s "); From f77397253dcfa340fbd7d231a9415996a4f73b57 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Tue, 18 Dec 2018 14:53:31 -0500 Subject: [PATCH 03/11] fix(editor): update stop_time travel times when frequency pattern is updated fixes #165 --- .../conveyal/gtfs/loader/JdbcTableWriter.java | 97 +++++++++++++++---- 1 file changed, 77 insertions(+), 20 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java index 6b793fdf8..bfed055c6 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java @@ -169,7 +169,14 @@ public String update(Integer id, String json, boolean autoCommit) throws SQLExce int entityId = isCreating ? (int) newId : id; // Cast child entities to array node to iterate over. ArrayNode childEntitiesArray = (ArrayNode)childEntities; - updateChildTable(childEntitiesArray, entityId, isCreating, referencingTable, connection); + updateChildTable( + childEntitiesArray, + entityId, + jsonObject.get("use_frequency").asBoolean(), + isCreating, + referencingTable, + connection + ); } } // Iterate over table's fields and apply linked values to any tables. This is to account for "exemplar" @@ -231,19 +238,19 @@ public String update(Integer id, String json, boolean autoCommit) throws SQLExce */ private void updateLinkedFields( Table referenceTable, - ObjectNode jsonObject, - String tableName, + ObjectNode exemplarEntity, + String linkedTableName, String keyField, - String ...fieldNames + String ...linkedFieldsToUpdate ) throws SQLException { - boolean updatingStopTimes = "stop_times".equals(tableName); + boolean updatingStopTimes = "stop_times".equals(linkedTableName); // Collect fields, the JSON values for these fields, and the strings to add to the prepared statement into Lists. List fields = new ArrayList<>(); List values = new ArrayList<>(); List fieldStrings = new ArrayList<>(); - for (String field : fieldNames) { + for (String field : linkedFieldsToUpdate) { fields.add(referenceTable.getFieldForName(field)); - values.add(jsonObject.get(field)); + values.add(exemplarEntity.get(field)); fieldStrings.add(String.format("%s = ?", field)); } String setFields = String.join(", ", fieldStrings); @@ -259,7 +266,7 @@ private void updateLinkedFields( keyField, orderField.name ) - : String.format("update %s.%s set %s where %s = ?", tablePrefix, tableName, setFields, keyField); + : String.format("update %s.%s set %s where %s = ?", tablePrefix, linkedTableName, setFields, keyField); // Prepare the statement and set statement parameters PreparedStatement statement = connection.prepareStatement(sql); int oneBasedIndex = 1; @@ -271,16 +278,16 @@ private void updateLinkedFields( else field.setParameter(statement, oneBasedIndex++, newValue); } // Set "where clause" with value for key field (e.g., set values where pattern_id = '3') - statement.setString(oneBasedIndex++, jsonObject.get(keyField).asText()); + statement.setString(oneBasedIndex++, exemplarEntity.get(keyField).asText()); if (updatingStopTimes) { // If updating stop times set the order field parameter (stop_sequence) - String orderValue = jsonObject.get(orderField.name).asText(); + String orderValue = exemplarEntity.get(orderField.name).asText(); orderField.setParameter(statement, oneBasedIndex++, orderValue); } // Log query, execute statement, and log result. LOG.debug(statement.toString()); int entitiesUpdated = statement.executeUpdate(); - LOG.debug("{} {} linked fields updated", entitiesUpdated, tableName); + LOG.debug("{} {} linked fields updated", entitiesUpdated, linkedTableName); } /** @@ -417,7 +424,14 @@ 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(ArrayNode subEntities, Integer id, boolean isCreatingNewEntity, Table subTable, Connection connection) throws SQLException, IOException { + private void updateChildTable( + ArrayNode subEntities, + Integer id, + boolean foreignReferenceIsFrequencyPattern, + boolean isCreatingNewEntity, + Table subTable, + Connection connection + ) throws SQLException, IOException { // Get parent table's key field Field keyField; String keyValue; @@ -464,6 +478,7 @@ private void updateChildTable(ArrayNode subEntities, Integer id, boolean isCreat int previousOrder = -1; TIntSet orderValues = new TIntHashSet(); Multimap referencesPerTable = HashMultimap.create(); + int cumulativeTravelTime = 0; for (JsonNode entityNode : subEntities) { // Cast entity node to ObjectNode to allow mutations (JsonNode is immutable). ObjectNode subEntity = (ObjectNode)entityNode; @@ -490,15 +505,22 @@ private void updateChildTable(ArrayNode subEntities, Integer id, boolean isCreat } // Update linked stop times fields for each updated pattern stop (e.g., timepoint, pickup/drop off type). if ("pattern_stops".equals(subTable.name)) { + if (foreignReferenceIsFrequencyPattern) { + // Update stop times linked to pattern stop if the pattern uses frequencies and accumulate time. + // Default travel and dwell time behave as "linked fields" for associated stop times. In other + // words, frequency trips in the editor must match the pattern stop travel times. + cumulativeTravelTime += updateStopTimesForPatternStop(subEntity, cumulativeTravelTime); + } + // These fields should be updated for all patterns (e.g., timepoint, pickup/drop off type). updateLinkedFields( - subTable, - subEntity, - "stop_times", - "pattern_id", - "timepoint", - "drop_off_type", - "pickup_type", - "shape_dist_traveled" + subTable, + subEntity, + "stop_times", + "pattern_id", + "timepoint", + "drop_off_type", + "pickup_type", + "shape_dist_traveled" ); } setStatementParameters(subEntity, subTable, insertStatement, connection); @@ -546,6 +568,41 @@ private void updateChildTable(ArrayNode subEntities, Integer id, boolean isCreat } } + /** + * Updates the stop times that reference the specified pattern stop. + * @param patternStop the pattern stop for which to update stop times + * @param previousTravelTime the travel time accumulated up to the previous stop_time's departure time (or the + * previous pattern stop's dwell time) + * @return the travel and dwell time added by this pattern stop + * @throws SQLException + */ + private int updateStopTimesForPatternStop(ObjectNode patternStop, int previousTravelTime) throws SQLException { + String sql = String.format( + "update %s.stop_times st set arrival_time = ?, departure_time = ? from %s.trips t " + + "where st.trip_id = t.trip_id AND t.%s = ? AND st.%s = ?", + tablePrefix, + tablePrefix, + "pattern_id", + "stop_sequence" + ); + // Prepare the statement and set statement parameters + PreparedStatement statement = connection.prepareStatement(sql); + int oneBasedIndex = 1; + int travelTime = patternStop.get("default_travel_time").asInt(); + int arrivalTime = previousTravelTime + travelTime; + statement.setInt(oneBasedIndex++, arrivalTime); + int dwellTime = patternStop.get("default_dwell_time").asInt(); + statement.setInt(oneBasedIndex++, arrivalTime + dwellTime); + // Set "where clause" with value for pattern_id and stop_sequence + statement.setString(oneBasedIndex++, patternStop.get("pattern_id").asText()); + statement.setInt(oneBasedIndex++, patternStop.get("stop_sequence").asInt()); + // Log query, execute statement, and log result. + LOG.debug(statement.toString()); + int entitiesUpdated = statement.executeUpdate(); + LOG.debug("{} stop_time arrivals/departures updated", entitiesUpdated); + return travelTime + dwellTime; + } + /** * Checks that a set of string references to a set of reference tables are all valid. For each set of references * mapped to a reference table, the method queries for all of the references. If there are any references that were From 903aebd0d7602091acd4ad4d2215271921ea1e5d Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Tue, 18 Dec 2018 15:00:27 -0500 Subject: [PATCH 04/11] refactor(editor): fix NPE --- src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java index bfed055c6..7a3c1b571 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java @@ -169,10 +169,12 @@ public String update(Integer id, String json, boolean autoCommit) throws SQLExce int entityId = isCreating ? (int) newId : id; // Cast child entities to array node to iterate over. ArrayNode childEntitiesArray = (ArrayNode)childEntities; + boolean foreignReferenceIsFrequencyPattern = jsonObject.has("use_frequency") && + jsonObject.get("use_frequency").asBoolean(); updateChildTable( childEntitiesArray, entityId, - jsonObject.get("use_frequency").asBoolean(), + foreignReferenceIsFrequencyPattern, isCreating, referencingTable, connection From a9ad0f6fe55aa1b404bdb4ca54260ea7e11dc19b Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Tue, 18 Dec 2018 15:07:01 -0500 Subject: [PATCH 05/11] refactor: change method arg from Integer -> int --- src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java index 7a3c1b571..51aed6726 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java @@ -428,7 +428,7 @@ private void setStatementParameters( */ private void updateChildTable( ArrayNode subEntities, - Integer id, + int id, boolean foreignReferenceIsFrequencyPattern, boolean isCreatingNewEntity, Table subTable, From 040a9f8b7317e48a5091a4717b23b215d828f302 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Thu, 3 Jan 2019 13:08:36 -0500 Subject: [PATCH 06/11] test(JdbcTableWriter): add CRUD tests for trips and basic create test for frequency --- .../gtfs/graphql/GraphQLGtfsSchema.java | 2 +- .../gtfs/loader/JDBCTableWriterTest.java | 185 ++++++++++++++++-- .../com/conveyal/gtfs/util/CalendarDTO.java | 16 ++ .../com/conveyal/gtfs/util/FrequencyDTO.java | 9 + .../com/conveyal/gtfs/util/PatternDTO.java | 13 ++ .../conveyal/gtfs/util/PatternStopDTO.java | 20 ++ .../com/conveyal/gtfs/util/ShapePointDTO.java | 6 + .../java/com/conveyal/gtfs/util/StopDTO.java | 17 ++ .../com/conveyal/gtfs/util/StopTimeDTO.java | 26 +++ .../java/com/conveyal/gtfs/util/TripDTO.java | 18 ++ .../java/com/conveyal/gtfs/util/UtilTest.java | 7 +- 11 files changed, 300 insertions(+), 19 deletions(-) create mode 100644 src/test/java/com/conveyal/gtfs/util/CalendarDTO.java create mode 100644 src/test/java/com/conveyal/gtfs/util/FrequencyDTO.java create mode 100644 src/test/java/com/conveyal/gtfs/util/PatternDTO.java create mode 100644 src/test/java/com/conveyal/gtfs/util/PatternStopDTO.java create mode 100644 src/test/java/com/conveyal/gtfs/util/ShapePointDTO.java create mode 100644 src/test/java/com/conveyal/gtfs/util/StopDTO.java create mode 100644 src/test/java/com/conveyal/gtfs/util/StopTimeDTO.java create mode 100644 src/test/java/com/conveyal/gtfs/util/TripDTO.java diff --git a/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java b/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java index b5702b85e..d0ee71372 100644 --- a/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java +++ b/src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java @@ -381,6 +381,7 @@ public class GraphQLGtfsSchema { .field(MapFetcher.field("shape_dist_traveled", GraphQLFloat)) .field(MapFetcher.field("drop_off_type", GraphQLInt)) .field(MapFetcher.field("pickup_type", GraphQLInt)) + .field(MapFetcher.field("stop_sequence", GraphQLInt)) .field(MapFetcher.field("timepoint", GraphQLInt)) // FIXME: This will only returns a list with one stop entity (unless there is a referential integrity issue) // Should this be modified to be an object, rather than a list? @@ -390,7 +391,6 @@ public class GraphQLGtfsSchema { .dataFetcher(new JDBCFetcher("stops", "stop_id")) .build() ) - .field(MapFetcher.field("stop_sequence", GraphQLInt)) .build(); /** diff --git a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java index 44a418b4a..57c09b21e 100644 --- a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java +++ b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java @@ -1,11 +1,20 @@ 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.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.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import org.junit.AfterClass; import org.junit.BeforeClass; @@ -18,12 +27,19 @@ import java.sql.Connection; import java.sql.ResultSet; import java.sql.SQLException; +import java.util.UUID; import static com.conveyal.gtfs.GTFS.createDataSource; import static com.conveyal.gtfs.GTFS.makeSnapshot; import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertThat; +/** + * This class contains CRUD tests for {@link JdbcTableWriter} (i.e., editing GTFS entities in the RDBMS). Set up + * consists of creating a scratch database and an empty feed snapshot, which is the necessary starting condition + * for building a GTFS feed from scratch. It then runs the various CRUD tests and finishes by dropping the database + * (even if tests fail). + */ public class JDBCTableWriterTest { private static final Logger LOG = LoggerFactory.getLogger(JDBCTableWriterTest.class); @@ -120,6 +136,10 @@ public void canCreateUpdateAndDeleteFeedInfoEntities() throws IOException, SQLEx )); } + /** + * Ensure that potentially malicious SQL injection is sanitized properly during create operations. + * TODO: We might should perform this check on multiple entities and for update and/or delete operations. + */ @Test public void canPreventSQLInjection() throws IOException, SQLException, InvalidNamespaceException { // create new object to be saved @@ -246,23 +266,8 @@ public void canCreateUpdateAndDeleteRoutes() throws IOException, SQLException, I final Class 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); + RouteDTO createdRoute = createSimpleTestRoute(routeId, "RTA", "500", "Hollingsworth", 3); // make sure saved data matches expected data assertThat(createdRoute.route_id, equalTo(routeId)); @@ -307,6 +312,154 @@ public void canCreateUpdateAndDeleteRoutes() throws IOException, SQLException, I )); } + /** + * Creates a simple route for testing. + */ + private static RouteDTO createSimpleTestRoute(String routeId, String agencyId, String shortName, String longName, int routeType) throws InvalidNamespaceException, IOException, SQLException { + RouteDTO input = new RouteDTO(); + input.route_id = routeId; + input.agency_id = agencyId; + // Empty value should be permitted for transfers and transfer_duration + input.route_short_name = shortName; + input.route_long_name = longName; + input.route_type = routeType; + // convert object to json and save it + JdbcTableWriter createTableWriter = createTestTableWriter(Table.ROUTES); + String output = createTableWriter.create(mapper.writeValueAsString(input), true); + LOG.info("create {} output:", Table.ROUTES.name); + LOG.info(output); + // parse output + return mapper.readValue(output, RouteDTO.class); + } + + /** + * 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 { + // 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[]{}; + // Write the pattern to the database + JdbcTableWriter createPatternWriter = createTestTableWriter(Table.PATTERNS); + String output = createPatternWriter.create(mapper.writeValueAsString(input), true); + LOG.info("create {} output:", Table.PATTERNS.name); + LOG.info(output); + // Parse output + return mapper.readValue(output, PatternDTO.class); + } + + /** + * Checks that creating, updating, and deleting a trip functions properly. As a part of this test, a service calendar, + * stops, a route, and a pattern are all created because they are necessary for a trip to be stored. This also tests + * updating a pattern with pattern stops and creating a trip with a frequency entry. + */ + @Test + public void canCreateUpdateAndDeleteTrips() throws IOException, SQLException, InvalidNamespaceException { + // Store Table and Class values for use in test. + final Table tripsTable = Table.TRIPS; + // Create service calendar and stops, both of which are necessary to construct a complete pattern. + String serviceId = "1"; + createWeekdayCalendar(serviceId, "20180103", "20180104"); + String firstStopId = "1"; + String lastStopId = "2"; + createSimpleStop(firstStopId, "First Stop", 34.2222, -87.333); + createSimpleStop(lastStopId, "Last Stop", 34.2233, -87.334); + String routeId = "700"; + String patternId = UUID.randomUUID().toString(); + PatternDTO createdPattern = createSimpleRouteAndPattern(routeId, patternId, "The Loop"); + // Update pattern with pattern stops and TODO shape points + JdbcTableWriter patternUpdater = createTestTableWriter(Table.PATTERNS); + createdPattern.pattern_stops = new PatternStopDTO[]{ + new PatternStopDTO(patternId, firstStopId, 0), + new PatternStopDTO(patternId, lastStopId, 1) + }; + patternUpdater.update(createdPattern.id, mapper.writeValueAsString(createdPattern), true); + // Create new trip for the pattern + JdbcTableWriter createTripWriter = createTestTableWriter(tripsTable); + TripDTO tripInput = new TripDTO(); + tripInput.pattern_id = createdPattern.pattern_id; + tripInput.route_id = routeId; + tripInput.service_id = serviceId; + tripInput.stop_times = new StopTimeDTO[]{ + new StopTimeDTO(firstStopId, 0, 0, 0), + new StopTimeDTO(lastStopId, 60, 60, 1) + }; + // FIXME: Adding a frequency entry to this trip should not be permitted because the pattern has useFrequency set + // to false (0). + FrequencyDTO frequency = new FrequencyDTO(); + int startTime = 6 * 60 * 60; + frequency.start_time = startTime; + frequency.end_time = 9 * 60 * 60; + frequency.headway_secs = 15 * 60; + tripInput.frequencies = new FrequencyDTO[]{frequency}; + String createTripOutput = createTripWriter.create(mapper.writeValueAsString(tripInput), true); + TripDTO createdTrip = mapper.readValue(createTripOutput, TripDTO.class); + // Update trip + // TODO: Add update and delete tests for updating pattern stops, stop_times, and frequencies. + String updatedTripId = "100A"; + createdTrip.trip_id = updatedTripId; + JdbcTableWriter updateTripWriter = createTestTableWriter(tripsTable); + String updateTripOutput = updateTripWriter.update(tripInput.id, mapper.writeValueAsString(createdTrip), true); + TripDTO updatedTrip = mapper.readValue(updateTripOutput, TripDTO.class); + // Check that saved data matches expected data + assertThat(updatedTrip.frequencies[0].start_time, equalTo(startTime)); + assertThat(updatedTrip.trip_id, equalTo(updatedTripId)); + // Delete trip record + JdbcTableWriter deleteTripWriter = createTestTableWriter(tripsTable); + int deleteOutput = deleteTripWriter.delete( + createdTrip.id, + true + ); + LOG.info("deleted {} records from {}", deleteOutput, tripsTable.name); + // Check that route record does not exist in DB + assertThatSqlQueryYieldsZeroRows( + String.format( + "select * from %s.%s where id=%d", + testNamespace, + tripsTable.name, + createdTrip.id + )); + } + + private static StopDTO createSimpleStop(String stopId, String stopName, double latitude, double longitude) throws InvalidNamespaceException, IOException, SQLException { + JdbcTableWriter createStopWriter = new JdbcTableWriter(Table.STOPS, testDataSource, testNamespace); + StopDTO input = new StopDTO(); + input.stop_id = stopId; + input.stop_name = stopName; + input.stop_lat = latitude; + input.stop_lon = longitude; + String output = createStopWriter.create(mapper.writeValueAsString(input), true); + LOG.info("create {} output:", Table.STOPS.name); + LOG.info(output); + return mapper.readValue(output, StopDTO.class); + } + + private static CalendarDTO createWeekdayCalendar(String serviceId, String startDate, String endDate) throws IOException, SQLException, InvalidNamespaceException { + JdbcTableWriter createCalendarWriter = new JdbcTableWriter(Table.CALENDAR, testDataSource, testNamespace); + CalendarDTO calendarInput = new CalendarDTO(); + calendarInput.service_id = serviceId; + calendarInput.monday = 1; + calendarInput.tuesday = 1; + calendarInput.wednesday = 1; + calendarInput.thursday = 1; + calendarInput.friday = 1; + calendarInput.saturday = 0; + calendarInput.sunday = 0; + calendarInput.start_date = startDate; + calendarInput.end_date = endDate; + String output = createCalendarWriter.create(mapper.writeValueAsString(calendarInput), true); + LOG.info("create {} output:", Table.CALENDAR.name); + LOG.info(output); + return mapper.readValue(output, CalendarDTO.class); + } + @AfterClass public static void tearDownClass() { TestUtils.dropDB(testDBName); diff --git a/src/test/java/com/conveyal/gtfs/util/CalendarDTO.java b/src/test/java/com/conveyal/gtfs/util/CalendarDTO.java new file mode 100644 index 000000000..169ffa5ac --- /dev/null +++ b/src/test/java/com/conveyal/gtfs/util/CalendarDTO.java @@ -0,0 +1,16 @@ +package com.conveyal.gtfs.util; + +public class CalendarDTO { + public Integer id; + public String service_id; + public Integer monday; + public Integer tuesday; + public Integer wednesday; + public Integer thursday; + public Integer friday; + public Integer saturday; + public Integer sunday; + public String start_date; + public String end_date; + public String description; +} diff --git a/src/test/java/com/conveyal/gtfs/util/FrequencyDTO.java b/src/test/java/com/conveyal/gtfs/util/FrequencyDTO.java new file mode 100644 index 000000000..a59617b4a --- /dev/null +++ b/src/test/java/com/conveyal/gtfs/util/FrequencyDTO.java @@ -0,0 +1,9 @@ +package com.conveyal.gtfs.util; + +public class FrequencyDTO { + public String trip_id; + public Integer start_time; + public Integer end_time; + public Integer headway_secs; + public Integer exact_times; +} diff --git a/src/test/java/com/conveyal/gtfs/util/PatternDTO.java b/src/test/java/com/conveyal/gtfs/util/PatternDTO.java new file mode 100644 index 000000000..eb78e516d --- /dev/null +++ b/src/test/java/com/conveyal/gtfs/util/PatternDTO.java @@ -0,0 +1,13 @@ +package com.conveyal.gtfs.util; + +public class PatternDTO { + public Integer id; + public String pattern_id; + public String shape_id; + public String route_id; + public Integer direction_id; + public Integer use_frequency; + public String name; + public PatternStopDTO[] pattern_stops; + public ShapePointDTO[] shapes; +} \ No newline at end of file diff --git a/src/test/java/com/conveyal/gtfs/util/PatternStopDTO.java b/src/test/java/com/conveyal/gtfs/util/PatternStopDTO.java new file mode 100644 index 000000000..b6daf838d --- /dev/null +++ b/src/test/java/com/conveyal/gtfs/util/PatternStopDTO.java @@ -0,0 +1,20 @@ +package com.conveyal.gtfs.util; + +public class PatternStopDTO { + public Integer id; + public String pattern_id; + public String stop_id; + public Integer default_travel_time; + public Integer default_dwell_time; + public Double shape_dist_traveled; + public Integer drop_off_type; + public Integer pickup_type; + public Integer stop_sequence; + public Integer timepoint; + + public PatternStopDTO (String patternId, String stopId, int stopSequence) { + pattern_id = patternId; + stop_id = stopId; + stop_sequence = stopSequence; + } +} diff --git a/src/test/java/com/conveyal/gtfs/util/ShapePointDTO.java b/src/test/java/com/conveyal/gtfs/util/ShapePointDTO.java new file mode 100644 index 000000000..05ed6d6da --- /dev/null +++ b/src/test/java/com/conveyal/gtfs/util/ShapePointDTO.java @@ -0,0 +1,6 @@ +package com.conveyal.gtfs.util; + +// TODO add fields +public class ShapePointDTO { + +} diff --git a/src/test/java/com/conveyal/gtfs/util/StopDTO.java b/src/test/java/com/conveyal/gtfs/util/StopDTO.java new file mode 100644 index 000000000..4ca821e59 --- /dev/null +++ b/src/test/java/com/conveyal/gtfs/util/StopDTO.java @@ -0,0 +1,17 @@ +package com.conveyal.gtfs.util; + +public class StopDTO { + public Integer id; + public String stop_id; + public String stop_name; + public String stop_code; + public String stop_desc; + public Double stop_lon; + public Double stop_lat; + public String zone_id; + public String stop_url; + public String stop_timezone; + public String parent_station; + public Integer location_type; + public Integer wheelchair_boarding; +} diff --git a/src/test/java/com/conveyal/gtfs/util/StopTimeDTO.java b/src/test/java/com/conveyal/gtfs/util/StopTimeDTO.java new file mode 100644 index 000000000..bfbc56424 --- /dev/null +++ b/src/test/java/com/conveyal/gtfs/util/StopTimeDTO.java @@ -0,0 +1,26 @@ +package com.conveyal.gtfs.util; + +public class StopTimeDTO { + public String trip_id; + public String stop_id; + public Integer stop_sequence; + public Integer arrival_time; + public Integer departure_time; + public String stop_headsign; + public Integer timepoint; + public Integer drop_off_type; + public Integer pickup_type; + public Double shape_dist_traveled; + + /** + * Empty constructor for deserialization + */ + public StopTimeDTO () {} + + public StopTimeDTO (String stopId, Integer arrivalTime, Integer departureTime, Integer stopSequence) { + stop_id = stopId; + arrival_time = arrivalTime; + departure_time = departureTime; + stop_sequence = stopSequence; + } +} diff --git a/src/test/java/com/conveyal/gtfs/util/TripDTO.java b/src/test/java/com/conveyal/gtfs/util/TripDTO.java new file mode 100644 index 000000000..ccd43a7e0 --- /dev/null +++ b/src/test/java/com/conveyal/gtfs/util/TripDTO.java @@ -0,0 +1,18 @@ +package com.conveyal.gtfs.util; + +public class TripDTO { + public Integer id; + public String trip_id; + public String trip_headsign; + public String trip_short_name; + public String block_id; + public Integer direction_id; + public String route_id; + public String service_id; + public Integer wheelchair_accessible; + public Integer bikes_allowed; + public String shape_id; + public String pattern_id; + public StopTimeDTO[] stop_times; + public FrequencyDTO[] frequencies; +} diff --git a/src/test/java/com/conveyal/gtfs/util/UtilTest.java b/src/test/java/com/conveyal/gtfs/util/UtilTest.java index c2c6ccd36..982ea31a4 100644 --- a/src/test/java/com/conveyal/gtfs/util/UtilTest.java +++ b/src/test/java/com/conveyal/gtfs/util/UtilTest.java @@ -15,8 +15,11 @@ */ public class UtilTest { - @Before // setup() - public void before() throws Exception { + /** + * Ensure locale (for {@link #canHumanize()}) is set correctly before doing anything else. + */ + @Before + public void before() { java.util.Locale.setDefault(java.util.Locale.US); } From 667d3fc8b0f1f00ed24a5a08593152ecf3ce76a1 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Thu, 3 Jan 2019 13:09:00 -0500 Subject: [PATCH 07/11] refactor: address PR comments about inlining string literals --- src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java | 6 ++---- 1 file changed, 2 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 51aed6726..90e6f2abd 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java @@ -581,11 +581,9 @@ private void updateChildTable( private int updateStopTimesForPatternStop(ObjectNode patternStop, int previousTravelTime) throws SQLException { String sql = String.format( "update %s.stop_times st set arrival_time = ?, departure_time = ? from %s.trips t " + - "where st.trip_id = t.trip_id AND t.%s = ? AND st.%s = ?", + "where st.trip_id = t.trip_id AND t.pattern_id = ? AND st.stop_sequence = ?", tablePrefix, - tablePrefix, - "pattern_id", - "stop_sequence" + tablePrefix ); // Prepare the statement and set statement parameters PreparedStatement statement = connection.prepareStatement(sql); From 55f2d879082c8ff5306f7dca616e936154d1ad89 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Thu, 3 Jan 2019 15:47:16 -0500 Subject: [PATCH 08/11] refactor(writer): fix check for useFrequency when updating patterns or trips This refactor includes a check for Pattern#useFrequency when either a pattern is updated or trips are updated in order to appropriately trigger other behaviors (for example, keeping stop times in sync with patter stops travel times or disallowing frequency entries for timetable-based patterns). fixes #173 --- .../conveyal/gtfs/loader/JdbcTableWriter.java | 37 ++++++++++++++----- .../gtfs/loader/JDBCTableWriterTest.java | 9 +++-- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java index 90e6f2abd..4b341081e 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java @@ -169,12 +169,27 @@ public String update(Integer id, String json, boolean autoCommit) throws SQLExce int entityId = isCreating ? (int) newId : id; // Cast child entities to array node to iterate over. ArrayNode childEntitiesArray = (ArrayNode)childEntities; - boolean foreignReferenceIsFrequencyPattern = jsonObject.has("use_frequency") && - jsonObject.get("use_frequency").asBoolean(); + boolean referencedPatternUsesFrequencies = false; + // If an entity references a pattern (e.g., pattern stop or trip), determine whether the pattern uses + // frequencies because this impacts update behaviors, for example whether stop times are kept in + // sync with default travel times or whether frequencies are allowed to be nested with a JSON trip. + if (jsonObject.has("pattern_id") && !jsonObject.get("pattern_id").isNull()) { + PreparedStatement statement = connection.prepareStatement(String.format( + "select use_frequency from %s.%s where pattern_id = ?", + tablePrefix, + Table.PATTERNS.name + )); + statement.setString(1, jsonObject.get("pattern_id").asText()); + LOG.info(statement.toString()); + ResultSet selectResults = statement.executeQuery(); + while (selectResults.next()) { + referencedPatternUsesFrequencies = selectResults.getBoolean(1); + } + } updateChildTable( childEntitiesArray, entityId, - foreignReferenceIsFrequencyPattern, + referencedPatternUsesFrequencies, isCreating, referencingTable, connection @@ -429,7 +444,7 @@ private void setStatementParameters( private void updateChildTable( ArrayNode subEntities, int id, - boolean foreignReferenceIsFrequencyPattern, + boolean referencedPatternUsesFrequencies, boolean isCreatingNewEntity, Table subTable, Connection connection @@ -442,9 +457,13 @@ private void updateChildTable( // Get parent entity's key value 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. + throw new IllegalStateException("Cannot create or update frequency entries for a timetable-based pattern."); + } // Reconciling pattern stops MUST happen before original pattern stops are deleted in below block (with // getUpdateReferencesSql) - if ("pattern_stops".equals(subTable.name)) { + if (Table.PATTERN_STOP.name.equals(subTable.name)) { List newPatternStops = new ArrayList<>(); // Clean up pattern stop ID fields (passed in as string ID from datatools-ui to avoid id collision) for (JsonNode node : subEntities) { @@ -507,7 +526,7 @@ private void updateChildTable( } // Update linked stop times fields for each updated pattern stop (e.g., timepoint, pickup/drop off type). if ("pattern_stops".equals(subTable.name)) { - if (foreignReferenceIsFrequencyPattern) { + if (referencedPatternUsesFrequencies) { // Update stop times linked to pattern stop if the pattern uses frequencies and accumulate time. // Default travel and dwell time behave as "linked fields" for associated stop times. In other // words, frequency trips in the editor must match the pattern stop travel times. @@ -1232,11 +1251,11 @@ private static String getValueForId(int id, String fieldName, String namespace, LOG.info(selectIdSql); Statement selectIdStatement = connection.createStatement(); ResultSet selectResults = selectIdStatement.executeQuery(selectIdSql); - String keyValue = null; + String value = null; while (selectResults.next()) { - keyValue = selectResults.getString(1); + value = selectResults.getString(1); } - return keyValue; + return value; } /** diff --git a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java index 57c09b21e..8fee6c2e7 100644 --- a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java +++ b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java @@ -374,13 +374,16 @@ public void canCreateUpdateAndDeleteTrips() throws IOException, SQLException, In String routeId = "700"; String patternId = UUID.randomUUID().toString(); PatternDTO createdPattern = createSimpleRouteAndPattern(routeId, patternId, "The Loop"); - // Update pattern with pattern stops and TODO shape points + // TODO Test that a frequency trip entry cannot be added for a timetable-based pattern. + // Update pattern with pattern stops, set to use frequencies, and TODO shape points JdbcTableWriter patternUpdater = createTestTableWriter(Table.PATTERNS); + createdPattern.use_frequency = 1; createdPattern.pattern_stops = new PatternStopDTO[]{ new PatternStopDTO(patternId, firstStopId, 0), new PatternStopDTO(patternId, lastStopId, 1) }; - patternUpdater.update(createdPattern.id, mapper.writeValueAsString(createdPattern), true); + String updatedPatternOutput = patternUpdater.update(createdPattern.id, mapper.writeValueAsString(createdPattern), true); + LOG.info("Updated pattern output: {}", updatedPatternOutput); // Create new trip for the pattern JdbcTableWriter createTripWriter = createTestTableWriter(tripsTable); TripDTO tripInput = new TripDTO(); @@ -391,8 +394,6 @@ public void canCreateUpdateAndDeleteTrips() throws IOException, SQLException, In new StopTimeDTO(firstStopId, 0, 0, 0), new StopTimeDTO(lastStopId, 60, 60, 1) }; - // FIXME: Adding a frequency entry to this trip should not be permitted because the pattern has useFrequency set - // to false (0). FrequencyDTO frequency = new FrequencyDTO(); int startTime = 6 * 60 * 60; frequency.start_time = startTime; From 6e9042f33fb3dc78c926b466c71303bf1dae30d0 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Thu, 3 Jan 2019 16:31:08 -0500 Subject: [PATCH 09/11] test(JdbcTableWriter): improve writer tests for frequencies --- .../gtfs/loader/JDBCTableWriterTest.java | 102 ++++++++++-------- 1 file changed, 60 insertions(+), 42 deletions(-) diff --git a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java index 8fee6c2e7..46966c1ba 100644 --- a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java +++ b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java @@ -14,7 +14,6 @@ import com.conveyal.gtfs.util.StopDTO; import com.conveyal.gtfs.util.StopTimeDTO; import com.conveyal.gtfs.util.TripDTO; -import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import org.junit.AfterClass; import org.junit.BeforeClass; @@ -27,7 +26,6 @@ import java.sql.Connection; import java.sql.ResultSet; import java.sql.SQLException; -import java.util.UUID; import static com.conveyal.gtfs.GTFS.createDataSource; import static com.conveyal.gtfs.GTFS.makeSnapshot; @@ -47,6 +45,9 @@ public class JDBCTableWriterTest { private static String testDBName; private static DataSource testDataSource; private static String testNamespace; + private static String simpleServiceId = "1"; + private static String firstStopId = "1"; + private static String lastStopId = "2"; private static final ObjectMapper mapper = new ObjectMapper(); private static JdbcTableWriter createTestTableWriter (Table table) throws InvalidNamespaceException { @@ -54,8 +55,8 @@ private static JdbcTableWriter createTestTableWriter (Table table) throws Invali } @BeforeClass - public static void setUpClass() throws SQLException { - // create a new database + public static void setUpClass() throws SQLException, IOException, InvalidNamespaceException { + // Create a new database testDBName = TestUtils.generateNewDB(); String dbConnectionUrl = String.format("jdbc:postgresql://localhost/%s", testDBName); testDataSource = createDataSource (dbConnectionUrl, null, null); @@ -67,10 +68,13 @@ public static void setUpClass() throws SQLException { "snapshot_of varchar)"); connection.commit(); LOG.info("feeds table created"); - - // create an empty snapshot to create a new namespace and all the tables + // Create an empty snapshot to create a new namespace and all the tables FeedLoadResult result = makeSnapshot(null, testDataSource); 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); } @Test @@ -313,7 +317,7 @@ public void canCreateUpdateAndDeleteRoutes() throws IOException, SQLException, I } /** - * Creates a simple route for testing. + * Create and store a simple route for testing. */ private static RouteDTO createSimpleTestRoute(String routeId, String agencyId, String shortName, String longName, int routeType) throws InvalidNamespaceException, IOException, SQLException { RouteDTO input = new RouteDTO(); @@ -356,50 +360,38 @@ private static PatternDTO createSimpleRouteAndPattern(String routeId, String pat } /** - * Checks that creating, updating, and deleting a trip functions properly. As a part of this test, a service calendar, - * stops, a route, and a pattern are all created because they are necessary for a trip to be stored. This also tests - * updating a pattern with pattern stops and creating a trip with a frequency entry. + * Test that a frequency trip entry CANNOT be added for a timetable-based pattern. Expects an exception to be thrown. + */ + @Test(expected = IllegalStateException.class) + public void cannotCreateFrequencyForTimetablePattern() throws InvalidNamespaceException, IOException, SQLException { + PatternDTO simplePattern = createSimpleRouteAndPattern("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); + } + + /** + * 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. */ @Test - public void canCreateUpdateAndDeleteTrips() throws IOException, SQLException, InvalidNamespaceException { + public void canCreateFrequencyForFrequencyPattern() throws IOException, SQLException, InvalidNamespaceException { // Store Table and Class values for use in test. final Table tripsTable = Table.TRIPS; - // Create service calendar and stops, both of which are necessary to construct a complete pattern. - String serviceId = "1"; - createWeekdayCalendar(serviceId, "20180103", "20180104"); - String firstStopId = "1"; - String lastStopId = "2"; - createSimpleStop(firstStopId, "First Stop", 34.2222, -87.333); - createSimpleStop(lastStopId, "Last Stop", 34.2233, -87.334); - String routeId = "700"; - String patternId = UUID.randomUUID().toString(); - PatternDTO createdPattern = createSimpleRouteAndPattern(routeId, patternId, "The Loop"); - // TODO Test that a frequency trip entry cannot be added for a timetable-based pattern. + int startTime = 6 * 60 * 60; + PatternDTO simplePattern = createSimpleRouteAndPattern("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 JdbcTableWriter patternUpdater = createTestTableWriter(Table.PATTERNS); - createdPattern.use_frequency = 1; - createdPattern.pattern_stops = new PatternStopDTO[]{ - new PatternStopDTO(patternId, firstStopId, 0), - new PatternStopDTO(patternId, lastStopId, 1) + simplePattern.use_frequency = 1; + simplePattern.pattern_stops = new PatternStopDTO[]{ + new PatternStopDTO(simplePattern.pattern_id, firstStopId, 0), + new PatternStopDTO(simplePattern.pattern_id, lastStopId, 1) }; - String updatedPatternOutput = patternUpdater.update(createdPattern.id, mapper.writeValueAsString(createdPattern), true); + String updatedPatternOutput = patternUpdater.update(simplePattern.id, mapper.writeValueAsString(simplePattern), true); LOG.info("Updated pattern output: {}", updatedPatternOutput); // Create new trip for the pattern - JdbcTableWriter createTripWriter = createTestTableWriter(tripsTable); - TripDTO tripInput = new TripDTO(); - tripInput.pattern_id = createdPattern.pattern_id; - tripInput.route_id = routeId; - tripInput.service_id = serviceId; - tripInput.stop_times = new StopTimeDTO[]{ - new StopTimeDTO(firstStopId, 0, 0, 0), - new StopTimeDTO(lastStopId, 60, 60, 1) - }; - FrequencyDTO frequency = new FrequencyDTO(); - int startTime = 6 * 60 * 60; - frequency.start_time = startTime; - frequency.end_time = 9 * 60 * 60; - frequency.headway_secs = 15 * 60; - tripInput.frequencies = new FrequencyDTO[]{frequency}; String createTripOutput = createTripWriter.create(mapper.writeValueAsString(tripInput), true); TripDTO createdTrip = mapper.readValue(createTripOutput, TripDTO.class); // Update trip @@ -429,6 +421,29 @@ public void canCreateUpdateAndDeleteTrips() throws IOException, SQLException, In )); } + /** + * Construct (without writing to the database) a trip with a frequency entry. + */ + private TripDTO constructFrequencyTrip(String patternId, String routeId, int startTime) { + TripDTO tripInput = new TripDTO(); + tripInput.pattern_id = patternId; + tripInput.route_id = routeId; + tripInput.service_id = simpleServiceId; + tripInput.stop_times = new StopTimeDTO[]{ + new StopTimeDTO(firstStopId, 0, 0, 0), + new StopTimeDTO(lastStopId, 60, 60, 1) + }; + FrequencyDTO frequency = new FrequencyDTO(); + frequency.start_time = startTime; + frequency.end_time = 9 * 60 * 60; + frequency.headway_secs = 15 * 60; + tripInput.frequencies = new FrequencyDTO[]{frequency}; + return tripInput; + } + + /** + * Create and store a simple stop entity. + */ private static StopDTO createSimpleStop(String stopId, String stopName, double latitude, double longitude) throws InvalidNamespaceException, IOException, SQLException { JdbcTableWriter createStopWriter = new JdbcTableWriter(Table.STOPS, testDataSource, testNamespace); StopDTO input = new StopDTO(); @@ -442,6 +457,9 @@ private static StopDTO createSimpleStop(String stopId, String stopName, double l return mapper.readValue(output, StopDTO.class); } + /** + * Create and store a simple calendar that runs on each weekday. + */ private static CalendarDTO createWeekdayCalendar(String serviceId, String startDate, String endDate) throws IOException, SQLException, InvalidNamespaceException { JdbcTableWriter createCalendarWriter = new JdbcTableWriter(Table.CALENDAR, testDataSource, testNamespace); CalendarDTO calendarInput = new CalendarDTO(); From 0fcf662c193844e12e3cafd8c9fb47edf609f5d4 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Thu, 3 Jan 2019 16:49:00 -0500 Subject: [PATCH 10/11] refactor: fix comment language from bad copypasta --- src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java index 46966c1ba..0665ad28e 100644 --- a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java +++ b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java @@ -411,7 +411,7 @@ public void canCreateFrequencyForFrequencyPattern() throws IOException, SQLExcep true ); LOG.info("deleted {} records from {}", deleteOutput, tripsTable.name); - // Check that route record does not exist in DB + // Check that record does not exist in DB assertThatSqlQueryYieldsZeroRows( String.format( "select * from %s.%s where id=%d", From 37fb468f60278eb63456ca3ccec73814aad62bf3 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Fri, 4 Jan 2019 10:40:27 -0500 Subject: [PATCH 11/11] test(writer): rename frequency trip crud test --- src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java index 0665ad28e..592874ae5 100644 --- a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java +++ b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java @@ -375,7 +375,7 @@ public void cannotCreateFrequencyForTimetablePattern() throws InvalidNamespaceEx * which is a prerequisite for creating a frequency trip with stop times. */ @Test - public void canCreateFrequencyForFrequencyPattern() throws IOException, SQLException, InvalidNamespaceException { + public void canCreateUpdateAndDeleteFrequencyTripForFrequencyPattern() throws IOException, SQLException, InvalidNamespaceException { // Store Table and Class values for use in test. final Table tripsTable = Table.TRIPS; int startTime = 6 * 60 * 60;