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/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java index 94f77418d..4b341081e 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; @@ -167,14 +169,61 @@ 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); + 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, + referencedPatternUsesFrequencies, + 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 @@ -204,25 +253,37 @@ 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 exemplarEntity, + String linkedTableName, + String keyField, + String ...linkedFieldsToUpdate + ) throws SQLException { + 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); // 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.%s set %s where %s = ?", tablePrefix, tableName, setFields, keyField); + ? 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, linkedTableName, setFields, keyField); // Prepare the statement and set statement parameters PreparedStatement statement = connection.prepareStatement(sql); int oneBasedIndex = 1; @@ -234,16 +295,16 @@ private void updateLinkedFields(Table referenceTable, ObjectNode jsonObject, Str 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); } /** @@ -252,7 +313,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); @@ -275,7 +343,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; @@ -349,7 +422,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() + ) + ); } } @@ -362,7 +441,14 @@ private void setStatementParameters(ObjectNode jsonObject, Table table, Prepared * 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, + int id, + boolean referencedPatternUsesFrequencies, + boolean isCreatingNewEntity, + Table subTable, + Connection connection + ) throws SQLException, IOException { // Get parent table's key field Field keyField; String keyValue; @@ -371,11 +457,13 @@ 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. + 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) { @@ -390,15 +478,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? @@ -412,6 +499,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; @@ -436,17 +524,24 @@ 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)) { + 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. + 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); @@ -458,13 +553,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 + ) ); } } @@ -492,6 +589,39 @@ 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.pattern_id = ? AND st.stop_sequence = ?", + tablePrefix, + tablePrefix + ); + // 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 @@ -610,8 +740,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 +773,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,16 +909,20 @@ 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); } /** * 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()); @@ -798,7 +947,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; @@ -958,7 +1113,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); @@ -1034,7 +1195,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); @@ -1085,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; } /** @@ -1108,7 +1274,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. @@ -1165,10 +1337,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); } @@ -1185,12 +1365,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); } @@ -1199,9 +1390,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 "); 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) diff --git a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java index 44a418b4a..592874ae5 100644 --- a/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java +++ b/src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java @@ -1,11 +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.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.databind.ObjectMapper; import org.junit.AfterClass; import org.junit.BeforeClass; @@ -24,6 +32,12 @@ 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); @@ -31,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 { @@ -38,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); @@ -51,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 @@ -120,6 +140,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 +270,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 +316,169 @@ public void canCreateUpdateAndDeleteRoutes() throws IOException, SQLException, I )); } + /** + * 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(); + 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); + } + + /** + * 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 canCreateUpdateAndDeleteFrequencyTripForFrequencyPattern() throws IOException, SQLException, InvalidNamespaceException { + // 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"); + 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); + 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(simplePattern.id, mapper.writeValueAsString(simplePattern), true); + LOG.info("Updated pattern output: {}", updatedPatternOutput); + // Create new trip for the pattern + 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 record does not exist in DB + assertThatSqlQueryYieldsZeroRows( + String.format( + "select * from %s.%s where id=%d", + testNamespace, + tripsTable.name, + createdTrip.id + )); + } + + /** + * 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(); + 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); + } + + /** + * 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(); + 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); }