fix(loader): refactor field loader to clean value and return errors

fixes #191
conveyal · Feb 19, 2019 · 2f0cb34 · 2f0cb34
1 parent eca69fb
commit 2f0cb34
Show file tree

Hide file tree

Showing 20 changed files with 287 additions and 129 deletions.
diff --git a/src/main/java/com/conveyal/gtfs/error/NewGTFSError.java b/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;

diff --git a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java b/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."),

diff --git a/src/main/java/com/conveyal/gtfs/loader/BooleanField.java b/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

diff --git a/src/main/java/com/conveyal/gtfs/loader/ColorField.java b/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);
         }

diff --git a/src/main/java/com/conveyal/gtfs/loader/CurrencyField.java b/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);
         }

diff --git a/src/main/java/com/conveyal/gtfs/loader/DateField.java b/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);
     }
 

diff --git a/src/main/java/com/conveyal/gtfs/loader/DateListField.java b/src/main/java/com/conveyal/gtfs/loader/DateListField.java
@@ -1,15 +1,15 @@
 package com.conveyal.gtfs.loader;
 
+import com.conveyal.gtfs.error.NewGTFSError;
 import com.conveyal.gtfs.storage.StorageException;
 
 import java.sql.Array;
 import java.sql.JDBCType;
 import java.sql.PreparedStatement;
 import java.sql.SQLType;
 import java.util.Arrays;
-import java.util.stream.Collectors;
-
-import static com.conveyal.gtfs.loader.DateField.validate;
+import java.util.Collections;
+import java.util.Set;
 
 public class DateListField extends Field {
 
@@ -18,19 +18,22 @@ public DateListField(String name, Requirement requirement) {
     }
 
     @Override
-    public String validateAndConvert(String original) {
-        // FIXME
+    public ValidateFieldResult<String> validateAndConvert(String original) {
+        // FIXME: This function is currently only used during feed loading. Because the DateListField only exists for the
+        //  schedule exceptions table (which is not a GTFS spec table), we do not expect to ever call this function on
+        //  this table/field.
         return null;
     }
 
     @Override
-    public void setParameter(PreparedStatement preparedStatement, int oneBasedIndex, String string) {
+    public Set<NewGTFSError> setParameter(PreparedStatement preparedStatement, int oneBasedIndex, String string) {
         try {
             String[] dateStrings = Arrays.stream(string.split(","))
                     .map(DateField::validate)
                     .toArray(String[]::new);
             Array array = preparedStatement.getConnection().createArrayOf("text", dateStrings);
             preparedStatement.setArray(oneBasedIndex, array);
+            return Collections.EMPTY_SET;
         } catch (Exception ex) {
             throw new StorageException(ex);
         }

diff --git a/src/main/java/com/conveyal/gtfs/loader/DoubleField.java b/src/main/java/com/conveyal/gtfs/loader/DoubleField.java
@@ -1,10 +1,12 @@
 package com.conveyal.gtfs.loader;
 
+import com.conveyal.gtfs.error.NewGTFSError;
 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.error.NewGTFSErrorType.NUMBER_PARSING;
 import static com.conveyal.gtfs.error.NewGTFSErrorType.NUMBER_TOO_LARGE;
@@ -29,31 +31,32 @@ public DoubleField (String name, Requirement requirement, double minValue, doubl
         this.outputPrecision = outputPrecision;
     }
 
-    private double validate(String string) {
-        double d;
+    private ValidateFieldResult<Double> validate(String string) {
+        ValidateFieldResult<Double> result = new ValidateFieldResult<>();
         try {
-            d = Double.parseDouble(string);
+            result.clean = Double.parseDouble(string);
         } catch (NumberFormatException e) {
             throw new StorageException(NUMBER_PARSING, string);
         }
-        if (d < minValue) throw new StorageException(NUMBER_TOO_SMALL, Double.toString(d));
-        if (d > maxValue) throw new StorageException(NUMBER_TOO_LARGE, Double.toString(d));
-        return d;
+        if (result.clean < minValue) NewGTFSError.forFeed(NUMBER_TOO_SMALL, string);
+        if (result.clean > maxValue) NewGTFSError.forFeed(NUMBER_TOO_LARGE, 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.setDouble(oneBasedIndex, validate(string));
+            ValidateFieldResult<Double> result = validate(string);
+            preparedStatement.setDouble(oneBasedIndex, result.clean);
+            return result.errors;
         } catch (Exception ex) {
             throw new StorageException(ex);
         }
     }
 
     @Override
-    public String validateAndConvert(String string) {
-        validate(string);
-        return string;
+    public ValidateFieldResult<String> validateAndConvert(String string) {
+        return ValidateFieldResult.from(validate(string));
     }
 
     @Override