Skip to content

Commit b14015c

Browse files
author
Landon Reed
authored
Merge pull request #192 from conveyal/dev
Bug Fix Release
2 parents c1d41d8 + 658e115 commit b14015c

26 files changed

+247
-89
lines changed

.travis.yml

+14-1
Original file line numberDiff line numberDiff line change
@@ -67,4 +67,17 @@ after_success:
6767
- semantic-release --prepare @conveyal/maven-semantic-release --publish @semantic-release/github,@conveyal/maven-semantic-release --verify-conditions @semantic-release/github,@conveyal/maven-semantic-release --verify-release @conveyal/maven-semantic-release --use-conveyal-workflow --dev-branch=dev
6868
# run codecov after semantic-release because maven-semantic-release creates extra commits that
6969
# codecov will need a report on to reference in future PRs to the release branch
70-
- bash <(curl -s https://codecov.io/bash)
70+
# this first codecov run will upload a report associated with the commit set through Travis CI environment variables
71+
- bash <(curl -s https://codecov.io/bash)
72+
# Since the above codecov run is associated with the commit that initiated the Travis build, the report will not be
73+
# associated with the commits that maven-semantic-release performed (if it ended up creating a release and the two
74+
# commits that were a part of that workflow). Therefore, if on master codecov needs to be ran two more times to create
75+
# codecov reports for the commits made by maven-semantic-release.
76+
# See https://github.com/conveyal/gtfs-lib/issues/193.
77+
#
78+
# The git commands get the commit hash of the HEAD commit and the commit just before HEAD.
79+
- |
80+
if [[ "$TRAVIS_BRANCH" = "master" ]]; then
81+
bash <(curl -s https://codecov.io/bash) -C "$(git rev-parse HEAD)"
82+
bash <(curl -s https://codecov.io/bash) -C "$(git rev-parse HEAD^)"
83+
fi

src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,7 @@ public class GraphQLGtfsSchema {
263263
.field(MapFetcher.field("route_url"))
264264
.field(MapFetcher.field("route_branding_url"))
265265
// TODO route_type as enum or int
266-
.field(MapFetcher.field("route_type"))
266+
.field(MapFetcher.field("route_type", GraphQLInt))
267267
.field(MapFetcher.field("route_color"))
268268
.field(MapFetcher.field("route_text_color"))
269269
// FIXME ˇˇ Editor fields that should perhaps be moved elsewhere.

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ public Map<String, Object> get (DataFetchingEnvironment environment) {
3434
try {
3535
connection = GTFSGraphQL.getConnection();
3636
Statement statement = connection.createStatement();
37-
LOG.info("SQL: {}", sqlBuilder.toString());
37+
LOG.debug("SQL: {}", sqlBuilder.toString());
3838
if (statement.execute(sqlBuilder.toString())) {
3939
ResultSet resultSet = statement.getResultSet();
4040
ResultSetMetaData meta = resultSet.getMetaData();

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ List<Map<String, Object>> getResults (
341341
}
342342
// This logging produces a lot of noise during testing due to large numbers of joined sub-queries
343343
// LOG.info("table name={}", tableName);
344-
LOG.info("SQL: {}", preparedStatement.toString());
344+
LOG.debug("SQL: {}", preparedStatement.toString());
345345
if (preparedStatement.execute()) {
346346
ResultSet resultSet = preparedStatement.getResultSet();
347347
ResultSetMetaData meta = resultSet.getMetaData();
@@ -363,7 +363,7 @@ List<Map<String, Object>> getResults (
363363
} finally {
364364
DbUtils.closeQuietly(connection);
365365
}
366-
LOG.info("Result size: {}", results.size());
366+
LOG.debug("Result size: {}", results.size());
367367
// Return a List of Maps, one Map for each row in the result.
368368
return results;
369369
}

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

+3-1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import org.slf4j.LoggerFactory;
1212

1313
import javax.sql.DataSource;
14+
import java.io.File;
1415
import java.io.FileOutputStream;
1516
import java.io.FilterOutputStream;
1617
import java.io.IOException;
@@ -325,7 +326,8 @@ private void cleanUpZipFile() {
325326
zip_properties.put("create", "false");
326327

327328
// Specify the path to the ZIP File that you want to read as a File System
328-
URI zip_disk = URI.create("jar:file://" + outFile);
329+
// (File#toURI allows this to work across different operating systems, including Windows)
330+
URI zip_disk = URI.create("jar:" + new File(outFile).toURI());
329331

330332
// Create ZIP file System
331333
try (FileSystem fileSystem = FileSystems.newFileSystem(zip_disk, zip_properties)) {

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

+67-19
Original file line numberDiff line numberDiff line change
@@ -186,14 +186,18 @@ public String update(Integer id, String json, boolean autoCommit) throws SQLExce
186186
referencedPatternUsesFrequencies = selectResults.getBoolean(1);
187187
}
188188
}
189-
updateChildTable(
189+
String keyValue = updateChildTable(
190190
childEntitiesArray,
191191
entityId,
192192
referencedPatternUsesFrequencies,
193193
isCreating,
194194
referencingTable,
195195
connection
196196
);
197+
// Ensure JSON return object is updated with referencing table's (potentially) new key value.
198+
// Currently, the only case where an update occurs is when a referenced shape is referenced by other
199+
// patterns.
200+
jsonObject.put(referencingTable.getKeyFieldName(), keyValue);
197201
}
198202
}
199203
// Iterate over table's fields and apply linked values to any tables. This is to account for "exemplar"
@@ -441,21 +445,19 @@ private void setStatementParameters(
441445
* have a hierarchical relationship.
442446
* FIXME develop a better way to update tables with foreign keys to the table being updated.
443447
*/
444-
private void updateChildTable(
448+
private String updateChildTable(
445449
ArrayNode subEntities,
446450
int id,
447451
boolean referencedPatternUsesFrequencies,
448452
boolean isCreatingNewEntity,
449453
Table subTable,
450454
Connection connection
451455
) throws SQLException, IOException {
452-
// Get parent table's key field
453-
Field keyField;
454-
String keyValue;
455-
// Primary key fields are always referenced by foreign key fields with the same name.
456-
keyField = specTable.getFieldForName(subTable.getKeyFieldName());
456+
// Get parent table's key field. Primary key fields are always referenced by foreign key fields with the same
457+
// name.
458+
Field keyField = specTable.getFieldForName(subTable.getKeyFieldName());
457459
// Get parent entity's key value
458-
keyValue = getValueForId(id, keyField.name, tablePrefix, specTable, connection);
460+
String keyValue = getValueForId(id, keyField.name, tablePrefix, specTable, connection);
459461
String childTableName = String.join(".", tablePrefix, subTable.name);
460462
if (!referencedPatternUsesFrequencies && subTable.name.equals(Table.FREQUENCIES.name) && subEntities.size() > 0) {
461463
// Do not permit the illegal state where frequency entries are being added/modified for a timetable pattern.
@@ -479,17 +481,44 @@ private void updateChildTable(
479481
reconcilePatternStops(keyValue, newPatternStops, connection);
480482
}
481483
if (!isCreatingNewEntity) {
482-
// Delete existing sub-entities for given entity ID if the parent entity is not being newly created.
483-
String deleteSql = getUpdateReferencesSql(SqlMethod.DELETE, childTableName, keyField, keyValue, null);
484-
LOG.info(deleteSql);
485-
Statement statement = connection.createStatement();
486-
// FIXME: Use copy on update for a pattern's shape instead of deleting the previous shape and replacing it.
487-
// This would better account for GTFS data loaded from a file where multiple patterns reference a single
488-
// shape.
489-
int result = statement.executeUpdate(deleteSql);
490-
LOG.info("Deleted {} {}", result, subTable.name);
491-
// FIXME: are there cases when an update should not return zero?
492-
// if (result == 0) throw new SQLException("No stop times found for trip ID");
484+
// If not creating a new entity, we will delete the child entities (e.g., shape points or pattern stops) and
485+
// regenerate them anew to avoid any messiness that we may encounter with update statements.
486+
if (Table.SHAPES.name.equals(subTable.name)) {
487+
// Check how many patterns are referencing the same shape_id to determine if we should copy on update.
488+
String patternsForShapeIdSql = String.format("select id from %s.patterns where shape_id = ?", tablePrefix);
489+
PreparedStatement statement = connection.prepareStatement(patternsForShapeIdSql);
490+
statement.setString(1, keyValue);
491+
LOG.info(statement.toString());
492+
ResultSet resultSet = statement.executeQuery();
493+
int patternsForShapeId = 0;
494+
while (resultSet.next()) {
495+
patternsForShapeId++;
496+
}
497+
if (patternsForShapeId > 1) {
498+
// Use copy on update for pattern shape if a single shape is being used for multiple patterns because
499+
// we do not want edits for a given pattern (for example, a short run) to impact the shape for another
500+
// pattern (perhaps a long run that extends farther). Note: this behavior will have the side effect of
501+
// creating potentially redundant shape information, but this is better than accidentally butchering the
502+
// shapes for other patterns.
503+
LOG.info("More than one pattern references shape_id: {}", keyValue);
504+
keyValue = UUID.randomUUID().toString();
505+
LOG.info("Creating new shape_id ({}) for pattern id={}.", keyValue, id);
506+
// Update pattern#shape_id with new value. Note: shape_point#shape_id values are coerced to new
507+
// value further down in this function.
508+
String updatePatternShapeIdSql = String.format("update %s.patterns set shape_id = ? where id = ?", tablePrefix);
509+
PreparedStatement updateStatement = connection.prepareStatement(updatePatternShapeIdSql);
510+
updateStatement.setString(1, keyValue);
511+
updateStatement.setInt(2, id);
512+
LOG.info(updateStatement.toString());
513+
updateStatement.executeUpdate();
514+
} else {
515+
// If only one pattern references this shape, delete the existing shape points to start anew.
516+
deleteChildEntities(subTable, keyField, keyValue);
517+
}
518+
} else {
519+
// If not handling shape points, delete the child entities and create them anew.
520+
deleteChildEntities(subTable, keyField, keyValue);
521+
}
493522
}
494523
int entityCount = 0;
495524
PreparedStatement insertStatement = null;
@@ -587,6 +616,25 @@ private void updateChildTable(
587616
} else {
588617
LOG.info("No inserts to execute. Empty array found in JSON for child table {}", childTableName);
589618
}
619+
// Return key value in the case that it was updated (the only case for this would be if the shape was referenced
620+
// by multiple patterns).
621+
return keyValue;
622+
}
623+
624+
/**
625+
* Delete existing sub-entities for given key value for when an update to the parent entity is made (i.e., the parent
626+
* entity is not being newly created). Examples of sub-entities include stop times for trips, pattern stops for a
627+
* pattern, or shape points (for a pattern in our model).
628+
*/
629+
private void deleteChildEntities(Table childTable, Field keyField, String keyValue) throws SQLException {
630+
String childTableName = String.join(".", tablePrefix, childTable.name);
631+
String deleteSql = getUpdateReferencesSql(SqlMethod.DELETE, childTableName, keyField, keyValue, null);
632+
LOG.info(deleteSql);
633+
Statement deleteStatement = connection.createStatement();
634+
int result = deleteStatement.executeUpdate(deleteSql);
635+
LOG.info("Deleted {} {}", result, childTable.name);
636+
// FIXME: are there cases when an update should not return zero?
637+
// if (result == 0) throw new SQLException("No stop times found for trip ID");
590638
}
591639

592640
/**

src/main/java/com/conveyal/gtfs/model/Pattern.java

+7-3
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ public String getId () {
4343
public LineString geometry;
4444
public String name;
4545
public String route_id;
46+
public int direction_id = INT_MISSING;
4647
public static Joiner joiner = Joiner.on("-").skipNulls();
4748
public String feed_id;
4849

@@ -78,6 +79,7 @@ public Pattern (List<String> orderedStops, Collection<Trip> trips, LineString pa
7879
// Patterns have one and only one route.
7980
// FIXME are we certain we're only passing in trips on one route? or are we losing information here?
8081
this.route_id = exemplarTrip.route_id;
82+
this.direction_id = exemplarTrip.direction_id;
8183

8284
// A name is assigned to this pattern based on the headsign, short name, direction ID or stop IDs.
8385
// This is not at all guaranteed to be unique, it's just to help identify the pattern.
@@ -112,9 +114,11 @@ public void setStatementParameters(PreparedStatement statement, boolean setDefau
112114
statement.setString(oneBasedIndex++, route_id);
113115
statement.setString(oneBasedIndex++, name);
114116
// Editor-specific fields
115-
setIntParameter(statement, oneBasedIndex++, 0);
116-
setIntParameter(statement, oneBasedIndex++, 0);
117+
setIntParameter(statement, oneBasedIndex++, direction_id);
118+
// Note: pattern#use_frequency is set in JdbcGtfsSnapshotter here:
119+
// https://github.com/conveyal/gtfs-lib/blob/0c6aca98a83d534853b74011e6cc7bf376592581/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java#L196-L211
120+
setIntParameter(statement, oneBasedIndex++, INT_MISSING); // use_frequency
117121
// FIXME: Shape set might be null?
118-
statement.setString(7, associatedShapes.iterator().next());
122+
statement.setString(oneBasedIndex++, associatedShapes.iterator().next());
119123
}
120124
}

src/main/java/com/conveyal/gtfs/validator/PatternFinderValidator.java

+13-21
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,8 @@ public void complete(ValidationResult validationResult) {
7777
Connection connection = null;
7878
try {
7979
// TODO this assumes gtfs-lib is using an SQL database and not a MapDB.
80-
// Maybe we should just create patterns in a separate step, but that would mean iterating over the stop_times twice.
80+
// Maybe we should just create patterns in a separate step, but that would mean iterating over the
81+
// stop_times twice.
8182
LOG.info("Creating pattern and pattern stops tables.");
8283
connection = feed.getConnection();
8384
Statement statement = connection.createStatement();
@@ -86,20 +87,18 @@ public void complete(ValidationResult validationResult) {
8687
String patternStopsTableName = feed.tablePrefix + "pattern_stops";
8788
statement.execute(String.format("alter table %s add column pattern_id varchar", tripsTableName));
8889
// FIXME: Here we're creating a pattern table that has an integer ID field (similar to the other GTFS tables)
89-
// AND a varchar pattern_id with essentially the same value cast to a string. Perhaps the pattern ID should
90-
// be a UUID or something, just to better distinguish it from the int ID?
91-
statement.execute(String.format("create table %s (id serial, pattern_id varchar, " +
92-
"route_id varchar, name varchar, shape_id varchar)", patternsTableName));
93-
// FIXME: Use patterns table?
94-
// Table patternsTable = new Table(patternsTableName, Pattern.class, Requirement.EDITOR, Table.PATTERNS.fields);
90+
// AND a varchar pattern_id with essentially the same value cast to a string. Perhaps the pattern ID should
91+
// be a UUID or something, just to better distinguish it from the int ID?
92+
Table patternsTable = new Table(patternsTableName, Pattern.class, Requirement.EDITOR, Table.PATTERNS.fields);
9593
Table patternStopsTable = new Table(patternStopsTableName, PatternStop.class, Requirement.EDITOR,
9694
Table.PATTERN_STOP.fields);
97-
String insertPatternStopSql = patternStopsTable.generateInsertSql(true);
98-
// Create pattern stops table with serial ID
95+
// Create pattern and pattern stops table, each with serial ID fields.
96+
patternsTable.createSqlTable(connection, null, true);
9997
patternStopsTable.createSqlTable(connection, null, true);
100-
PreparedStatement insertPatternStatement = connection.prepareStatement(
101-
String.format("insert into %s values (DEFAULT, ?, ?, ?, ?)", patternsTableName)
102-
);
98+
// Generate prepared statements for inserts.
99+
String insertPatternSql = patternsTable.generateInsertSql(true);
100+
String insertPatternStopSql = patternStopsTable.generateInsertSql(true);
101+
PreparedStatement insertPatternStatement = connection.prepareStatement(insertPatternSql);
103102
BatchTracker patternTracker = new BatchTracker("pattern", insertPatternStatement);
104103
PreparedStatement insertPatternStopStatement = connection.prepareStatement(insertPatternStopSql);
105104
BatchTracker patternStopTracker = new BatchTracker("pattern stop", insertPatternStopStatement);
@@ -119,18 +118,11 @@ public void complete(ValidationResult validationResult) {
119118
statement.execute(createTempSql);
120119
patternForTripsFileStream = new PrintStream(new BufferedOutputStream(new FileOutputStream(tempPatternForTripsTextFile)));
121120
}
122-
// TODO update to use batch trackers
123121
for (Map.Entry<TripPatternKey, Pattern> entry : patterns.entrySet()) {
124122
Pattern pattern = entry.getValue();
125-
// LOG.info("Batching pattern {}", pattern.pattern_id);
123+
LOG.debug("Batching pattern {}", pattern.pattern_id);
126124
TripPatternKey key = entry.getKey();
127-
// First, create a pattern relation.
128-
insertPatternStatement.setString(1, pattern.pattern_id);
129-
insertPatternStatement.setString(2, pattern.route_id);
130-
insertPatternStatement.setString(3, pattern.name);
131-
// FIXME: This could be null...
132-
String shapeId = pattern.associatedShapes.iterator().next();
133-
insertPatternStatement.setString(4, shapeId);
125+
pattern.setStatementParameters(insertPatternStatement, true);
134126
patternTracker.addBatch();
135127
// Construct pattern stops based on values in trip pattern key.
136128
// FIXME: Use pattern stops table here?

src/test/java/com/conveyal/gtfs/util/CalendarDTO.java renamed to src/test/java/com/conveyal/gtfs/dto/CalendarDTO.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package com.conveyal.gtfs.util;
1+
package com.conveyal.gtfs.dto;
22

33
public class CalendarDTO {
44
public Integer id;

src/test/java/com/conveyal/gtfs/util/FareDTO.java renamed to src/test/java/com/conveyal/gtfs/dto/FareDTO.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package com.conveyal.gtfs.util;
1+
package com.conveyal.gtfs.dto;
22

33
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
44

src/test/java/com/conveyal/gtfs/util/FareRuleDTO.java renamed to src/test/java/com/conveyal/gtfs/dto/FareRuleDTO.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package com.conveyal.gtfs.util;
1+
package com.conveyal.gtfs.dto;
22

33
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
44

src/test/java/com/conveyal/gtfs/util/FeedInfoDTO.java renamed to src/test/java/com/conveyal/gtfs/dto/FeedInfoDTO.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package com.conveyal.gtfs.util;
1+
package com.conveyal.gtfs.dto;
22

33
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
44

src/test/java/com/conveyal/gtfs/util/FrequencyDTO.java renamed to src/test/java/com/conveyal/gtfs/dto/FrequencyDTO.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package com.conveyal.gtfs.util;
1+
package com.conveyal.gtfs.dto;
22

33
public class FrequencyDTO {
44
public String trip_id;

src/test/java/com/conveyal/gtfs/util/PatternDTO.java renamed to src/test/java/com/conveyal/gtfs/dto/PatternDTO.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package com.conveyal.gtfs.util;
1+
package com.conveyal.gtfs.dto;
22

33
public class PatternDTO {
44
public Integer id;

src/test/java/com/conveyal/gtfs/util/PatternStopDTO.java renamed to src/test/java/com/conveyal/gtfs/dto/PatternStopDTO.java

+4-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package com.conveyal.gtfs.util;
1+
package com.conveyal.gtfs.dto;
22

33
public class PatternStopDTO {
44
public Integer id;
@@ -12,6 +12,9 @@ public class PatternStopDTO {
1212
public Integer stop_sequence;
1313
public Integer timepoint;
1414

15+
/** Empty constructor for deserialization */
16+
public PatternStopDTO() {}
17+
1518
public PatternStopDTO (String patternId, String stopId, int stopSequence) {
1619
pattern_id = patternId;
1720
stop_id = stopId;

src/test/java/com/conveyal/gtfs/util/RouteDTO.java renamed to src/test/java/com/conveyal/gtfs/dto/RouteDTO.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package com.conveyal.gtfs.util;
1+
package com.conveyal.gtfs.dto;
22

33
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
44

0 commit comments

Comments
 (0)