Merge pull request #143 from conveyal/fix-bad-date-npe

Fix bad date NPE

src/main/java/com/conveyal/gtfs/GTFS.java

@@ -5,6 +5,7 @@
 import com.conveyal.gtfs.loader.JdbcGtfsExporter;
 import com.conveyal.gtfs.loader.JdbcGtfsLoader;
 import com.conveyal.gtfs.loader.JdbcGtfsSnapshotter;
+import com.conveyal.gtfs.loader.SnapshotResult;
 import com.conveyal.gtfs.util.InvalidNamespaceException;
 import com.conveyal.gtfs.validator.ValidationResult;
 import com.fasterxml.jackson.databind.ObjectMapper;
@@ -83,9 +84,9 @@ public static FeedLoadResult load (String filePath, DataSource dataSource) {
      * @param dataSource    JDBC connection to existing database
      * @return              FIXME should this be a separate SnapshotResult object?
      */
-    public static FeedLoadResult makeSnapshot (String feedId, DataSource dataSource) {
+    public static SnapshotResult makeSnapshot (String feedId, DataSource dataSource) {
         JdbcGtfsSnapshotter snapshotter = new JdbcGtfsSnapshotter(feedId, dataSource);
-        FeedLoadResult result = snapshotter.copyTables();
+        SnapshotResult result = snapshotter.copyTables();
         return result;
     }
 

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

@@ -131,6 +131,7 @@ private void commit() {
      */
     public void commitAndClose() {
         try {
+            LOG.info("Committing errors and closing SQL connection.");
             this.commit();
             // Close the connection permanently (should be called only after errorStorage instance no longer needed).
             connection.close();

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

@@ -81,9 +81,7 @@ public ValidationResult validate () {
         SQLErrorStorage errorStorage = null;
         try {
             errorStorage = new SQLErrorStorage(dataSource.getConnection(), tablePrefix, false);
-        } catch (SQLException ex) {
-            throw new StorageException(ex);
-        } catch (InvalidNamespaceException ex) {
+        } catch (SQLException | InvalidNamespaceException ex) {
             throw new StorageException(ex);
         }
         int errorCountBeforeValidation = errorStorage.getErrorCount();

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

@@ -309,7 +309,7 @@ public FeedLoadResult exportTables() {
             // TableLoadResult.
             LOG.error("Exception while creating snapshot: {}", ex.toString());
             ex.printStackTrace();
-            result.fatalException = ex.getMessage();
+            result.fatalException = ex.toString();
         }
         return result;
     }
@@ -354,7 +354,7 @@ private TableLoadResult export (Table table, Connection connection) {
             } catch (SQLException e) {
                 LOG.error("failed to generate select statement for existing fields");
                 TableLoadResult tableLoadResult = new TableLoadResult();
-                tableLoadResult.fatalException = e.getMessage();
+                tableLoadResult.fatalException = e.toString();
                 e.printStackTrace();
                 return tableLoadResult;
             }
@@ -402,7 +402,7 @@ private TableLoadResult export (Table table, String filterSql) {
             } catch (SQLException ex) {
                 ex.printStackTrace();
             }
-            tableLoadResult.fatalException = e.getMessage();
+            tableLoadResult.fatalException = e.toString();
             LOG.error("Exception while exporting tables", e);
         }
         return tableLoadResult;

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

@@ -169,7 +169,7 @@ public FeedLoadResult loadTables () {
             // TODO catch exceptions separately while loading each table so load can continue, store in TableLoadResult
             LOG.error("Exception while loading GTFS file: {}", ex.toString());
             ex.printStackTrace();
-            result.fatalException = ex.getMessage();
+            result.fatalException = ex.toString();
         }
         return result;
     }
@@ -251,7 +251,7 @@ private void registerFeed (File gtfsFile) {
             connection.commit();
             LOG.info("Created new feed namespace: {}", insertStatement);
         } catch (Exception ex) {
-            LOG.error("Exception while registering new feed namespace in feeds table: {}", ex.getMessage());
+            LOG.error("Exception while registering new feed namespace in feeds table", ex);
             DbUtils.closeQuietly(connection);
         }
     }

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

@@ -56,9 +56,9 @@ public JdbcGtfsSnapshotter(String feedId, DataSource dataSource) {
     /**
      * Copy primary entity tables as well as Pattern and PatternStops tables.
      */
-    public FeedLoadResult copyTables() {
+    public SnapshotResult copyTables() {
         // This result object will be returned to the caller to summarize the feed and report any critical errors.
-        FeedLoadResult result = new FeedLoadResult();
+        SnapshotResult result = new SnapshotResult();
 
         try {
             long startTime = System.currentTimeMillis();
@@ -89,7 +89,7 @@ public FeedLoadResult copyTables() {
             copy(Table.PATTERNS, true);
             copy(Table.PATTERN_STOP, true);
             // see method comments fo why different logic is needed for this table
-            createScheduleExceptionsTable();
+            result.scheduleExceptions = createScheduleExceptionsTable();
             result.shapes = copy(Table.SHAPES, true);
             result.stops = copy(Table.STOPS, true);
             // TODO: Should we defer index creation on stop times?
@@ -106,7 +106,7 @@ public FeedLoadResult copyTables() {
             // TableLoadResult.
             LOG.error("Exception while creating snapshot: {}", ex.toString());
             ex.printStackTrace();
-            result.fatalException = ex.getMessage();
+            result.fatalException = ex.toString();
         }
         return result;
     }
@@ -145,7 +145,7 @@ private TableLoadResult copy (Table table, boolean createIndexes) {
             connection.commit();
             LOG.info("Done.");
         } catch (Exception ex) {
-            tableLoadResult.fatalException = ex.getMessage();
+            tableLoadResult.fatalException = ex.toString();
             LOG.error("Error: ", ex);
             try {
                 connection.rollback();
@@ -217,6 +217,11 @@ private TableLoadResult createScheduleExceptionsTable() {
                 Multimap<String, String> removedServiceForDate = HashMultimap.create();
                 Multimap<String, String> addedServiceForDate = HashMultimap.create();
                 for (CalendarDate calendarDate : calendarDates) {
+                    // Skip any null dates
+                    if (calendarDate.date == null) {
+                        LOG.warn("Encountered calendar date record with null value for date field. Skipping.");
+                        continue;
+                    }
                     String date = calendarDate.date.format(DateTimeFormatter.BASIC_ISO_DATE);
                     if (calendarDate.exception_type == 1) {
                         addedServiceForDate.put(date, calendarDate.service_id);
@@ -293,8 +298,8 @@ private TableLoadResult createScheduleExceptionsTable() {
                 }
 
                 connection.commit();
-            } catch (SQLException e) {
-                tableLoadResult.fatalException = e.getMessage();
+            } catch (Exception e) {
+                tableLoadResult.fatalException = e.toString();
                 LOG.error("Error creating schedule Exceptions: ", e);
                 e.printStackTrace();
                 try {
@@ -427,7 +432,7 @@ private void registerSnapshot () {
             connection.commit();
             LOG.info("Created new snapshot namespace: {}", insertStatement);
         } catch (Exception ex) {
-            LOG.error("Exception while registering snapshot namespace in feeds table: {}", ex.getMessage());
+            LOG.error("Exception while registering snapshot namespace in feeds table", ex);
             DbUtils.closeQuietly(connection);
         }
     }

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

@@ -0,0 +1,11 @@
+package com.conveyal.gtfs.loader;
+
+/**
+ * This contains the result of a feed snapshot operation. It is nearly identical to {@link FeedLoadResult} except that
+ * it has some additional tables that only exist for snapshots/editor feeds.
+ */
+public class SnapshotResult extends FeedLoadResult {
+    private static final long serialVersionUID = 1L;
+
+    public TableLoadResult scheduleExceptions;
+}

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

@@ -117,8 +117,7 @@ public void complete(ValidationResult validationResult) {
                     }
                 }
             } catch (Exception ex) {
-                LOG.error(ex.getMessage());
-                ex.printStackTrace();
+                LOG.error("Error validating service entries (merging calendars and calendar_dates)", ex);
                 // Continue on to next calendar entry.
             }
         }
@@ -181,6 +180,11 @@ select durations.service_id, duration_seconds, days_active from (
             LocalDate firstDate = LocalDate.MAX;
             LocalDate lastDate = LocalDate.MIN;
             for (LocalDate date : dateInfoForDate.keySet()) {
+                // If the date is invalid, skip.
+                if (date == null) {
+                    LOG.error("Encountered null date. Did something go wrong with computeIfAbsent?");
+                    continue;
+                }
                 if (date.isBefore(firstDate)) firstDate = date;
                 if (date.isAfter(lastDate)) lastDate = date;
             }
@@ -191,7 +195,8 @@ select durations.service_id, duration_seconds, days_active from (
             validationResult.firstCalendarDate = firstDate;
             validationResult.lastCalendarDate = lastDate;
             // Is this any different? firstDate.until(lastDate, ChronoUnit.DAYS);
-            int nDays = (int) ChronoUnit.DAYS.between(firstDate, lastDate) + 1;
+            // If no days were found in the dateInfoForDate, nDays is a very large negative number, so we default to 0.
+            int nDays = Math.max(0, (int) ChronoUnit.DAYS.between(firstDate, lastDate) + 1);
             validationResult.dailyBusSeconds = new int[nDays];
             validationResult.dailyTramSeconds = new int[nDays];
             validationResult.dailyMetroSeconds = new int[nDays];