Merge branch 'dev' into issue-844

src/main/java/com/conveyal/analysis/datasource/ShapefileDataSourceIngester.java

@@ -44,24 +44,25 @@ public void ingest (File file, ProgressListener progressListener) {
         try {
             ShapefileReader reader = new ShapefileReader(file);
             // Iterate over all features to ensure file is readable, geometries are valid, and can be reprojected.
-            Envelope envelope = reader.wgs84Bounds();
-            checkWgsEnvelopeSize(envelope, "Shapefile");
+            // Note that the method reader.wgs84Bounds() transforms the envelope in projected coordinates to WGS84,
+            // which does not necessarily include all the points transformed individually.
+            Envelope envelope = new Envelope();
             reader.wgs84Stream().forEach(f -> {
-                checkState(envelope.contains(((Geometry)f.getDefaultGeometry()).getEnvelopeInternal()));
+                envelope.expandToInclude(((Geometry)f.getDefaultGeometry()).getEnvelopeInternal());
             });
+            checkWgsEnvelopeSize(envelope, "Shapefile");
             reader.close();
             progressListener.increment();
             dataSource.wgsBounds = Bounds.fromWgsEnvelope(envelope);
             dataSource.attributes = reader.attributes();
             dataSource.geometryType = reader.geometryType();
             dataSource.featureCount = reader.featureCount();
-            dataSource.coordinateSystem =
-                    reader.crs.getName().getCode();
-
+            dataSource.coordinateSystem = reader.crs.getName().getCode();
             progressListener.increment();
         } catch (FactoryException | TransformException e) {
-            throw new DataSourceException("Shapefile transform error. " +
-                    "Try uploading an unprojected WGS84 (EPSG:4326) file.", e);
+            throw new DataSourceException(
+                "Shapefile transform error. Try uploading an unprojected WGS84 (EPSG:4326) file.", e
+            );
         } catch (IOException e) {
             // ShapefileReader throws a checked IOException.
             throw new DataSourceException("Error parsing shapefile. Ensure the files you uploaded are valid.", e);

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

@@ -1,6 +1,7 @@
 package com.conveyal.gtfs;
 
 import com.conveyal.gtfs.error.GTFSError;
+import com.conveyal.gtfs.error.ReferentialIntegrityError;
 import com.conveyal.gtfs.model.Agency;
 import com.conveyal.gtfs.model.Calendar;
 import com.conveyal.gtfs.model.CalendarDate;
@@ -19,6 +20,7 @@
 import com.conveyal.gtfs.model.StopTime;
 import com.conveyal.gtfs.model.Transfer;
 import com.conveyal.gtfs.model.Trip;
+import com.conveyal.gtfs.validator.model.Priority;
 import com.conveyal.gtfs.validator.service.GeoUtils;
 import com.conveyal.r5.analyst.progress.ProgressListener;
 import com.google.common.collect.HashMultimap;
@@ -253,7 +255,15 @@ public void loadFromFile(ZipFile zip, String fid) throws Exception {
         // There are conceivably cases where the extra step of identifying and naming patterns is not necessary.
         // In current usage we do always need them, and performing this step during load allows enforcing subsequent
         // read-only access.
-        findPatterns();
+        // Find patterns only if there are no referential integrity errors or other severe problems. Those problems
+        // can cause pattern finding to fail hard with null pointer exceptions, causing detailed error messages to be
+        // lost and hiding underlying problems from the user. If high-priority problems are present, the feed should be
+        // presented to the user as unuseable anyway.
+        if (errors.stream().anyMatch(e -> e.getPriority() == Priority.HIGH)) {
+            LOG.warn("Feed contains high priority errors, not finding patterns. It will be useless for routing.");
+        } else {
+            findPatterns();
+        }
 
         // Prevent loading additional feeds into this MapDB.
         loaded = true;
@@ -576,25 +586,23 @@ private void namePatterns(Collection<Pattern> patterns) {
 
     public LineString getStraightLineForStops(String trip_id) {
         CoordinateList coordinates = new CoordinateList();
-        LineString ls = null;
+        LineString lineString = null;
         Trip trip = trips.get(trip_id);
 
         Iterable<StopTime> stopTimes;
         stopTimes = getOrderedStopTimesForTrip(trip.trip_id);
+        // lineString must remain null if there are less than two stopTimes to avoid
+        // an exception when creating linestring.
         if (Iterables.size(stopTimes) > 1) {
             for (StopTime stopTime : stopTimes) {
                 Stop stop = stops.get(stopTime.stop_id);
                 Double lat = stop.stop_lat;
                 Double lon = stop.stop_lon;
                 coordinates.add(new Coordinate(lon, lat));
             }
-            ls = geometryFactory.createLineString(coordinates.toCoordinateArray());
+            lineString = geometryFactory.createLineString(coordinates.toCoordinateArray());
         }
-        // set ls equal to null if there is only one stopTime to avoid an exception when creating linestring
-        else{
-            ls = null;
-        }
-        return ls;
+        return lineString;
     }
 
     /**

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

@@ -8,8 +8,8 @@
 public class DuplicateKeyError extends GTFSError implements Serializable {
     public static final long serialVersionUID = 1L;
 
-    public DuplicateKeyError(String file, long line, String field) {
-        super(file, line, field);
+    public DuplicateKeyError(String file, long line, String field, String id) {
+        super(file, line, field, id);
     }
 
     @Override public String getMessage() {

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

@@ -0,0 +1,22 @@
+package com.conveyal.gtfs.error;
+
+import com.conveyal.gtfs.validator.model.Priority;
+
+import java.io.Serializable;
+
+/** Indicates that a GTFS entity was not added to a table because it had no primary key. */
+public class MissingKeyError extends GTFSError implements Serializable {
+    public static final long serialVersionUID = 1L;
+
+    public MissingKeyError(String file, long line, String field) {
+        super(file, line, field);
+    }
+
+    @Override public String getMessage() {
+        return "Missing primary key.";
+    }
+
+    @Override public Priority getPriority() {
+        return Priority.MEDIUM;
+    }
+}

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

@@ -20,6 +20,11 @@ public class Agency extends Entity {
     public URL    agency_branding_url;
     public String feed_id;
 
+    @Override
+    public String getId() {
+        return agency_id;
+    }
+
     public static class Loader extends Entity.Loader<Agency> {
 
         public Loader(GTFSFeed feed) {
@@ -45,11 +50,11 @@ public void loadOneRow() throws IOException {
             a.agency_fare_url = getUrlField("agency_fare_url", false);
             a.agency_branding_url = getUrlField("agency_branding_url", false);
             a.feed_id = feed.feedId;
-
-            // TODO clooge due to not being able to have null keys in mapdb
-            if (a.agency_id == null) a.agency_id = "NONE";
-
-            feed.agency.put(a.agency_id, a);
+            // Kludge because mapdb does not support null keys
+            if (a.agency_id == null) {
+                a.agency_id = "NONE";
+            }
+            insertCheckingDuplicateKey(feed.agency, a, "agency_id");
         }
 
     }

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

@@ -52,7 +52,7 @@ public void loadOneRow() throws IOException {
             String service_id = getStringField("service_id", true); // TODO service_id can reference either calendar or calendar_dates.
             Service service = services.computeIfAbsent(service_id, Service::new);
             if (service.calendar != null) {
-                feed.errors.add(new DuplicateKeyError(tableName, row, "service_id"));
+                feed.errors.add(new DuplicateKeyError(tableName, row, "service_id", service_id));
             } else {
                 Calendar c = new Calendar();
                 c.sourceFileLine = row + 1; // offset line number by 1 to account for 0-based row index

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

@@ -52,7 +52,8 @@ public void loadOneRow() throws IOException {
             Service service = services.computeIfAbsent(service_id, Service::new);
             LocalDate date = getDateField("date", true);
             if (service.calendar_dates.containsKey(date)) {
-                feed.errors.add(new DuplicateKeyError(tableName, row, "(service_id, date)"));
+                String keyString = String.format("(%s,%s)", service_id, date.toString());
+                feed.errors.add(new DuplicateKeyError(tableName, row, "(service_id, date)", keyString));
             } else {
                 CalendarDate cd = new CalendarDate();
                 cd.sourceFileLine = row + 1; // offset line number by 1 to account for 0-based row index

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

@@ -1,6 +1,8 @@
 package com.conveyal.gtfs.model;
 
 import com.beust.jcommander.internal.Sets;
+import com.conveyal.gtfs.error.DuplicateKeyError;
+import com.conveyal.gtfs.error.MissingKeyError;
 import com.conveyal.r5.analyst.progress.ProgressInputStream;
 import com.conveyal.gtfs.GTFSFeed;
 import com.conveyal.gtfs.error.DateParseError;
@@ -57,8 +59,10 @@ public abstract class Entity implements Serializable {
      *          with a sequence number. For example stop_times and shapes have no single field that uniquely
      *          identifies a row, and the stop_sequence or shape_pt_sequence must also be considered.
      */
-    public String getId () {
-        return null;
+    public String getId() {
+        // Several entities have compound keys which are handled as tuple objects, not strings.
+        // Fail fast if anything tries to fetch a string ID for those Entity types.
+        throw new UnsupportedOperationException();
     }
 
     /**
@@ -301,6 +305,24 @@ public void loadTable (ZipFile zip) throws IOException {
             }
         }
 
+        /**
+         * Insert the given value into the map, checking whether a value already exists with its key.
+         * The entity type must override getId() for this to work. We have to pass in the name of the key field for
+         * error reporting purposes because although there is a method to get the ID of an Entity there is not a method
+         * to get the name of the field(s) it is taken from.
+         */
+        protected void insertCheckingDuplicateKey (Map<String, E> map, E value, String keyField) {
+            String key = value.getId();
+            if (key == null) {
+                feed.errors.add(new MissingKeyError(tableName, row, keyField));
+                return;
+            }
+            // Map returns previous value if one was already present
+            E previousValue = map.put(key, value);
+            if (previousValue != null) {
+                feed.errors.add(new DuplicateKeyError(tableName, row, keyField, key));
+            }
+        }
     }
 
     /**

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

@@ -39,7 +39,7 @@ public void loadOneRow() throws IOException {
             String fareId = getStringField("fare_id", true);
             Fare fare = fares.computeIfAbsent(fareId, Fare::new);
             if (fare.fare_attribute != null) {
-                feed.errors.add(new DuplicateKeyError(tableName, row, "fare_id"));
+                feed.errors.add(new DuplicateKeyError(tableName, row, "fare_id", fareId));
             } else {
                 FareAttribute fa = new FareAttribute();
                 fa.sourceFileLine = row + 1; // offset line number by 1 to account for 0-based row index

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

@@ -32,6 +32,11 @@ public class Route extends Entity { // implements Entity.Factory<Route>
     public URL route_branding_url;
     public String feed_id;
 
+    @Override
+    public String getId() {
+        return route_id;
+    }
+
     public static class Loader extends Entity.Loader<Route> {
 
         public Loader(GTFSFeed feed) {
@@ -72,7 +77,7 @@ public void loadOneRow() throws IOException {
             r.route_text_color = getStringField("route_text_color", false);
             r.route_branding_url = getUrlField("route_branding_url", false);
             r.feed_id = feed.feedId;
-            feed.routes.put(r.route_id, r);
+            insertCheckingDuplicateKey(feed.routes, r, "route_id");
         }
 
     }

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

@@ -1,10 +1,12 @@
 package com.conveyal.gtfs.model;
 
 import com.conveyal.gtfs.GTFSFeed;
+import com.conveyal.gtfs.error.DuplicateKeyError;
 
 import java.io.IOException;
 import java.net.URL;
 import java.util.Iterator;
+import java.util.Map;
 
 public class Stop extends Entity {
 
@@ -61,12 +63,13 @@ public void loadOneRow() throws IOException {
             s.stop_timezone  = getStringField("stop_timezone", false);
             s.wheelchair_boarding = getStringField("wheelchair_boarding", false);
             s.feed_id = feed.feedId;
-            /* TODO check ref integrity later, this table self-references via parent_station */
-            feed.stops.put(s.stop_id, s);
+            // Referential integrity is checked later after fully loaded. Stops self-reference via parent_station. 
+            insertCheckingDuplicateKey(feed.stops, s, "stop_id");
         }
 
     }
 
+
     public static class Writer extends Entity.Writer<Stop> {
         public Writer (GTFSFeed feed) {
             super(feed, "stops");

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

@@ -28,7 +28,7 @@ public class StopTime extends Entity implements Cloneable, Serializable {
 
     @Override
     public String getId() {
-        return trip_id; // Needs sequence number to be unique
+        return trip_id; // Concatenate with sequence number to make unique
     }
 
     @Override

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

@@ -20,6 +20,11 @@ public class Trip extends Entity {
     public int    wheelchair_accessible;
     public String feed_id;
 
+    @Override
+    public String getId() {
+        return trip_id;
+    }
+
     public static class Loader extends Entity.Loader<Trip> {
 
         public Loader(GTFSFeed feed) {
@@ -47,7 +52,7 @@ public void loadOneRow() throws IOException {
             t.bikes_allowed   = getIntField("bikes_allowed", false, 0, 2);
             t.wheelchair_accessible = getIntField("wheelchair_accessible", false, 0, 2);
             t.feed_id = feed.feedId;
-            feed.trips.put(t.trip_id, t);
+            insertCheckingDuplicateKey(feed.trips, t, "trip_id");
 
             /*
               Check referential integrity without storing references. Trip cannot directly reference Services or