Merge pull request #213 from conveyal/dev

Release

.github/CODEOWNERS

@@ -0,0 +1,4 @@
+# See https://help.github.com/articles/about-codeowners/
+
+# A Conveyal employee is required to approve PR merges
+* @conveyal/employees

.github/issue_template.md

@@ -0,0 +1,17 @@
+_**NOTE:** This issue system is intended for reporting bugs and tracking progress in software development. Although this software is licensed with an open-source license, any issue opened here may not be responded to in a timely manner. [Conveyal](https://www.conveyal.com) is unable to provide technical support for custom deployments of this software unless your company has a support contract with us. Please remove this note when creating the issue._
+
+## Observed behavior
+
+Please explain what is being observed within the application here.
+
+## Expected behavior
+
+Please explain what should happen instead.
+
+## Steps to reproduce the problem
+
+Please be as specific as possible.
+
+## Notes on configuration and files required to reproduce the issue
+
+Please describe any applicable config files that were used. If this issue is reporting behavior that is related to the data format of a particular GTFS file, please upload that GTFS file and note the exact files and lines that are causing the issue. If this issue is reproducable in [datatools-server](https://github.com/conveyal/datatools-server) or [datatools-ui](https://github.com/conveyal/datatools-ui) please include the exact commits of those libraries and any configuration of those libraries that differs from the default configuration.

.github/pull_request_template.md

@@ -0,0 +1,13 @@
+### Checklist
+
+- [ ] Appropriate branch selected _(all PRs must first be merged to `dev` before they can be merged to `master`)_
+- [ ] Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
+- [ ] The description lists all applicable issues this PR seeks to resolve
+- [ ] The description lists any configuration setting(s) that differ from the default settings
+- [ ] All tests and CI builds passing
+- [ ] The description lists all relevant PRs included in this release _(remove this if not merging to master)_
+- [ ] Code coverage does not significantly worsen (ideally it improves) _(remove this if not merging to master)_
+
+### Description
+
+Please explain the changes you made here and, if not immediately obvious from the code, how they resolve any referenced issues. Be sure to include all issues being resolved and any special configuration settings that are need for the software to run properly with these changes. If merging to master, please also list the PRs that are to be included.

src/main/java/com/conveyal/gtfs/error/NewGTFSError.java

@@ -17,7 +17,7 @@
 public class NewGTFSError {
 
     /** The class of the table in which the error was encountered. */
-    public final Class<? extends Entity> entityType;
+    public Class<? extends Entity> entityType;
 
     /** The kind of error encountered. */
     public final NewGTFSErrorType errorType;

src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java

@@ -10,6 +10,7 @@ public enum NewGTFSErrorType {
     TIME_FORMAT(Priority.MEDIUM, "Time format should be HH:MM:SS."),
     URL_FORMAT(Priority.MEDIUM, "URL format should be <scheme>://<authority><path>?<query>#<fragment>"),
     LANGUAGE_FORMAT(Priority.LOW, "Language should be specified with a valid BCP47 tag."),
+    ILLEGAL_FIELD_VALUE(Priority.MEDIUM, "Fields may not contain tabs, carriage returns or new lines."),
     INTEGER_FORMAT(Priority.MEDIUM, "Incorrect integer format."),
     FLOATING_FORMAT(Priority.MEDIUM, "Incorrect floating point number format."),
     COLUMN_NAME_UNSAFE(Priority.HIGH, "Column header contains characters not safe in SQL, it was renamed."),
@@ -59,7 +60,7 @@ public enum NewGTFSErrorType {
     TRAVEL_TIME_ZERO(Priority.HIGH, "The vehicle arrives at this stop at the same time it departs from the previous stop."),
     MISSING_ARRIVAL_OR_DEPARTURE(Priority.MEDIUM, "First and last stop times are required to have both an arrival and departure time."),
     TRIP_TOO_FEW_STOP_TIMES(Priority.MEDIUM, "A trip must have at least two stop times to represent travel."),
-    TRIP_OVERLAP_IN_BLOCK(Priority.MEDIUM, "Blocks"),
+    TRIP_OVERLAP_IN_BLOCK(Priority.MEDIUM, "A trip overlaps another trip and shares the same block_id."),
     TRAVEL_TOO_SLOW(Priority.MEDIUM, "The vehicle is traveling very slowly to reach this stop from the previous one."),
     TRAVEL_TOO_FAST(Priority.MEDIUM, "The vehicle travels extremely fast to reach this stop from the previous one."),
     VALIDATOR_FAILED(Priority.HIGH, "The specified validation stage failed due to an error encountered during loading. This is likely due to an error encountered during loading (e.g., a date or number field is formatted incorrectly.)."),

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

@@ -333,6 +333,18 @@ public class GraphQLGtfsSchema {
             .field(MapFetcher.field("parent_station"))
             .field(MapFetcher.field("location_type", GraphQLInt))
             .field(MapFetcher.field("wheelchair_boarding", GraphQLInt))
+            // Returns all stops that reference parent stop's stop_id
+            .field(newFieldDefinition()
+                    .name("child_stops")
+                    .type(new GraphQLList(new GraphQLTypeReference("stop")))
+                    .dataFetcher(new JDBCFetcher(
+                        "stops",
+                        "stop_id",
+                        null,
+                        false,
+                        "parent_station"
+                    ))
+                    .build())
             .field(RowCountFetcher.field("stop_time_count", "stop_times", "stop_id"))
             .field(newFieldDefinition()
                     .name("patterns")

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

@@ -69,6 +69,7 @@ public class JDBCFetcher implements DataFetcher<List<Map<String, Object>>> {
     final String parentJoinField;
     private final String sortField;
     private final boolean autoLimit;
+    private final String childJoinField;
 
     /**
      * Constructor for tables that need neither restriction by a where clause nor sorting based on the enclosing entity.
@@ -96,10 +97,22 @@ public JDBCFetcher (String tableName, String parentJoinField) {
      *                  tables that it is unnatural to expect a limit (e.g., shape points or pattern stops).
      */
     public JDBCFetcher (String tableName, String parentJoinField, String sortField, boolean autoLimit) {
+        this(tableName, parentJoinField, sortField, autoLimit, null);
+    }
+
+    /**
+     *
+     * @param childJoinField The child table field that should be joined to the parent join field. This enables joining
+     *                       where references from the child table to the parent table do not share the same field name,
+     *                       e.g., stops#stop_id -> transfers#from_stop_id. This value defaults to parentJoinField if
+     *                       argument is null.
+     */
+    public JDBCFetcher (String tableName, String parentJoinField, String sortField, boolean autoLimit, String childJoinField) {
         this.tableName = tableName;
         this.parentJoinField = parentJoinField;
         this.sortField = sortField;
         this.autoLimit = autoLimit;
+        this.childJoinField = childJoinField != null ? childJoinField : parentJoinField;
     }
 
     // We can't automatically generate JDBCFetcher based field definitions for inclusion in a GraphQL schema (as we
@@ -198,8 +211,8 @@ List<Map<String, Object>> getResults (
         // If we are fetching an item nested within a GTFS entity in the GraphQL query, we want to add an SQL "where"
         // clause. This could conceivably be done automatically, but it's clearer to just express the intent.
         // Note, this is assuming the type of the field in the parent is a string.
-        if (parentJoinField != null && parentJoinValues != null && !parentJoinValues.isEmpty()) {
-            whereConditions.add(makeInClause(parentJoinField, parentJoinValues, preparedStatementParameters));
+        if (childJoinField != null && parentJoinValues != null && !parentJoinValues.isEmpty()) {
+            whereConditions.add(makeInClause(childJoinField, parentJoinValues, preparedStatementParameters));
         }
         if (sortField != null) {
             // Sort field is not provided by user input, so it's ok to add here (i.e., it's not prone to SQL injection).
@@ -341,7 +354,7 @@ List<Map<String, Object>> getResults (
             }
             // This logging produces a lot of noise during testing due to large numbers of joined sub-queries
 //            LOG.info("table name={}", tableName);
-            LOG.debug("SQL: {}", preparedStatement.toString());
+            LOG.info("SQL: {}", preparedStatement.toString());
             if (preparedStatement.execute()) {
                 ResultSet resultSet = preparedStatement.getResultSet();
                 ResultSetMetaData meta = resultSet.getMetaData();

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

@@ -34,15 +34,19 @@ public void addBatch() throws SQLException {
         }
     }
 
-    public void executeRemaining() throws SQLException {
+    /**
+     * Execute any remaining statements and return the total records processed.
+     */
+    public int executeRemaining() throws SQLException {
         if (currentBatchSize > 0) {
             totalRecordsProcessed += currentBatchSize;
             preparedStatement.executeBatch();
             currentBatchSize = 0;
         }
         // Avoid reuse, signal that this was cleanly closed.
         preparedStatement = null;
-        LOG.info(String.format("Inserted %d %s records", totalRecordsProcessed, recordType));
+        LOG.info(String.format("Processed %d %s records", totalRecordsProcessed, recordType));
+        return totalRecordsProcessed;
     }
 
     public void finalize () {

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

@@ -1,11 +1,13 @@
 package com.conveyal.gtfs.loader;
 
+import com.conveyal.gtfs.error.NewGTFSError;
 import com.conveyal.gtfs.error.NewGTFSErrorType;
 import com.conveyal.gtfs.storage.StorageException;
 
 import java.sql.JDBCType;
 import java.sql.PreparedStatement;
 import java.sql.SQLType;
+import java.util.Set;
 
 /**
  * A GTFS boolean field, coded as a single character string 0 or 1.
@@ -17,17 +19,21 @@ public BooleanField (String name, Requirement requirement) {
         super(name, requirement);
     }
 
-    private boolean validate (String string) {
+    private ValidateFieldResult<Boolean> validate (String string) {
+        ValidateFieldResult<Boolean> result = new ValidateFieldResult<>();
         if ( ! ("0".equals(string) || "1".equals(string))) {
-            throw new StorageException(NewGTFSErrorType.BOOLEAN_FORMAT, string);
+            result.errors.add(NewGTFSError.forFeed(NewGTFSErrorType.BOOLEAN_FORMAT, string));
         }
-        return "1".equals(string);
+        result.clean = "1".equals(string);
+        return result;
     }
 
     @Override
-    public void setParameter (PreparedStatement preparedStatement, int oneBasedIndex, String string) {
+    public Set<NewGTFSError> setParameter (PreparedStatement preparedStatement, int oneBasedIndex, String string) {
         try {
-            preparedStatement.setBoolean(oneBasedIndex, validate(string));
+            ValidateFieldResult<Boolean> result = validate(string);
+            preparedStatement.setBoolean(oneBasedIndex, result.clean);
+            return result.errors;
         } catch (Exception ex) {
             throw new StorageException(ex);
         }
@@ -37,8 +43,8 @@ public void setParameter (PreparedStatement preparedStatement, int oneBasedIndex
      * The 0 or 1 will be converted to the string "true" or "false" for SQL COPY.
      */
     @Override
-    public String validateAndConvert (String string) {
-        return Boolean.toString(validate(string));
+    public ValidateFieldResult<String> validateAndConvert (String string) {
+        return ValidateFieldResult.from(validate(string));
     }
 
     @Override

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

@@ -1,11 +1,13 @@
 package com.conveyal.gtfs.loader;
 
+import com.conveyal.gtfs.error.NewGTFSError;
 import com.conveyal.gtfs.error.NewGTFSErrorType;
 import com.conveyal.gtfs.storage.StorageException;
 
 import java.sql.JDBCType;
 import java.sql.PreparedStatement;
 import java.sql.SQLType;
+import java.util.Set;
 
 import static com.conveyal.gtfs.loader.Field.cleanString;
 
@@ -21,21 +23,24 @@ public ColorField(String name, Requirement requirement) {
     }
 
     /** Check that a string can be properly parsed and is in range. */
-    public String validateAndConvert (String string) {
+    public ValidateFieldResult<String> validateAndConvert (String string) {
+        ValidateFieldResult<String> result = new ValidateFieldResult<>(string);
         try {
             if (string.length() != 6) {
-                throw new StorageException(NewGTFSErrorType.COLOR_FORMAT, string);
+                result.errors.add(NewGTFSError.forFeed(NewGTFSErrorType.COLOR_FORMAT, string));
             }
             int integer = Integer.parseInt(string, 16);
-            return string; // Could also store the integer.
+            return result; // Could also store the integer.
         } catch (Exception ex) {
             throw new StorageException(NewGTFSErrorType.COLOR_FORMAT, string);
         }
     }
 
-    public void setParameter(PreparedStatement preparedStatement, int oneBasedIndex, String string) {
+    public Set<NewGTFSError> setParameter(PreparedStatement preparedStatement, int oneBasedIndex, String string) {
         try {
-            preparedStatement.setString(oneBasedIndex, validateAndConvert(string));
+            ValidateFieldResult<String> result = validateAndConvert(string);
+            preparedStatement.setString(oneBasedIndex, result.clean);
+            return result.errors;
         } catch (Exception ex) {
             throw new StorageException(ex);
         }

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

@@ -1,5 +1,6 @@
 package com.conveyal.gtfs.loader;
 
+import com.conveyal.gtfs.error.NewGTFSError;
 import com.conveyal.gtfs.error.NewGTFSErrorType;
 import com.conveyal.gtfs.storage.StorageException;
 import com.google.common.collect.Sets;
@@ -198,21 +199,24 @@ public CurrencyField (String name, Requirement requirement) {
         super(name, requirement);
     }
 
-    private String validate (String string) {
+    private ValidateFieldResult<String> validate (String string) {
+        ValidateFieldResult<String> result = new ValidateFieldResult<>(string);
         if (!CURRENCY_CODES.contains(string)) {
-            throw new StorageException(NewGTFSErrorType.CURRENCY_UNKNOWN, string);
+            result.errors.add(NewGTFSError.forFeed(NewGTFSErrorType.CURRENCY_UNKNOWN, string));
         }
-        return string;
+        return result;
     }
 
     /** Check that a string can be properly parsed and is in range. */
-    public String validateAndConvert (String string) {
+    public ValidateFieldResult<String> validateAndConvert (String string) {
         return validate(string);
     }
 
-    public void setParameter(PreparedStatement preparedStatement, int oneBasedIndex, String string) {
+    public Set<NewGTFSError> setParameter(PreparedStatement preparedStatement, int oneBasedIndex, String string) {
         try {
-            preparedStatement.setString(oneBasedIndex, validateAndConvert(string));
+            ValidateFieldResult<String> result = validateAndConvert(string);
+            preparedStatement.setString(oneBasedIndex, result.clean);
+            return result.errors;
         } catch (Exception ex) {
             throw new StorageException(ex);
         }

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

@@ -1,5 +1,6 @@
 package com.conveyal.gtfs.loader;
 
+import com.conveyal.gtfs.error.NewGTFSError;
 import com.conveyal.gtfs.error.NewGTFSErrorType;
 import com.conveyal.gtfs.storage.StorageException;
 
@@ -9,6 +10,8 @@
 import java.time.LocalDate;
 import java.time.format.DateTimeFormatter;
 import java.time.format.DateTimeParseException;
+import java.util.Collections;
+import java.util.Set;
 
 /**
  * A GTFS date in the format YYYYMMDD.
@@ -26,26 +29,33 @@ public DateField (String name, Requirement requirement) {
         super(name, requirement);
     }
 
-    public static String validate (String string) {
+    public static ValidateFieldResult<String> validate (String string) {
+        // Initialize default value as null (i.e., don't use the input value).
+        ValidateFieldResult<String> result = new ValidateFieldResult<>();
         // Parse the date out of the supplied string.
         LocalDate date;
         try {
             date = LocalDate.parse(string, GTFS_DATE_FORMATTER);
+            // Only set the clean result after the date parse is successful.
+            result.clean = string;
         } catch (DateTimeParseException ex) {
-            throw new StorageException(NewGTFSErrorType.DATE_FORMAT, string);
+            result.errors.add(NewGTFSError.forFeed(NewGTFSErrorType.DATE_FORMAT, string));
+            return result;
         }
         // Range check on year. Parsing operation above should already have checked month and day ranges.
         int year = date.getYear();
         if (year < 2000 || year > 2100) {
-            throw new StorageException(NewGTFSErrorType.DATE_RANGE, string);
+            result.errors.add(NewGTFSError.forFeed(NewGTFSErrorType.DATE_RANGE, string));
         }
-        return string;
+        return result;
     }
 
     @Override
-    public void setParameter (PreparedStatement preparedStatement, int oneBasedIndex, String string) {
+    public Set<NewGTFSError> setParameter (PreparedStatement preparedStatement, int oneBasedIndex, String string) {
         try {
-            preparedStatement.setString(oneBasedIndex, validate(string));
+            ValidateFieldResult<String> result = validate(string);
+            preparedStatement.setString(oneBasedIndex, result.clean);
+            return result.errors;
         } catch (Exception ex) {
             throw new StorageException(ex);
         }
@@ -54,17 +64,18 @@ public void setParameter (PreparedStatement preparedStatement, int oneBasedIndex
     /**
      * DateField specific method to set a statement parameter from a {@link LocalDate}.
      */
-    public void setParameter (PreparedStatement preparedStatement, int oneBasedIndex, LocalDate localDate) {
+    public Set<NewGTFSError> setParameter (PreparedStatement preparedStatement, int oneBasedIndex, LocalDate localDate) {
         try {
             if (localDate == null) setNull(preparedStatement, oneBasedIndex);
             else preparedStatement.setString(oneBasedIndex, localDate.format(GTFS_DATE_FORMATTER));
+            return Collections.EMPTY_SET;
         } catch (Exception e) {
             throw new StorageException(e);
         }
     }
 
     @Override
-    public String validateAndConvert (String string) {
+    public ValidateFieldResult<String> validateAndConvert (String string) {
         return validate(string);
     }