Merge branch 'dev' into regular-polling

build.gradle

@@ -125,10 +125,8 @@ repositories {
     // Put Open Source Geospatial before Maven Central to get JAI core, see https://stackoverflow.com/a/26993223
     maven { url 'https://repo.osgeo.org/repository/release/' }
     mavenCentral()
-    // TODO review whether we really need the repositories below
+    // Polyline encoder 0.2 is now in Maven repo
     maven { url 'https://maven.conveyal.com' }
-    // For the polyline encoder
-    maven { url 'https://nexus.axiomalaska.com/nexus/content/repositories/public-releases' }
 }
 
 // Exclude all JUnit 4 transitive dependencies - IntelliJ bug causes it to think we're using Junit 4 instead of 5.

src/main/java/com/conveyal/analysis/controllers/UserActivityController.java

@@ -58,6 +58,10 @@ private Object removeActivity (Request req, Response res) {
         UserPermissions userPermissions = UserPermissions.from(req);
         String id = req.params("id");
         Task task = taskScheduler.getTaskForUser(userPermissions.email, id);
+        // Check if task still exists before attempting to remove.
+        if (task == null) {
+            throw  AnalysisServerException.notFound("Task does not exist. It may have already been removed by another user.");
+        }
         // Disallow removing active tasks via the API.
         if (task.state.equals(Task.State.ACTIVE)) {
             throw AnalysisServerException.badRequest("Cannot clear an active task.");

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/analysis/models/AnalysisRequest.java

@@ -28,9 +28,6 @@
  * sends/forwards to R5 workers (see {@link AnalysisWorkerTask}), though it has many of the same fields.
  */
 public class AnalysisRequest {
-    private static int MIN_ZOOM = 9;
-    private static int MAX_ZOOM = 12;
-    private static int MAX_GRID_CELLS = 5_000_000;
 
     /**
      * These three IDs are redundant, and just help reduce the number of database lookups necessary.
@@ -207,7 +204,6 @@ public void populateTask (AnalysisWorkerTask task, UserPermissions userPermissio
         // TODO define class with static factory function WebMercatorGridBounds.fromLatLonBounds().
         //      Also include getIndex(x, y), getX(index), getY(index), totalTasks()
         WebMercatorExtents extents = WebMercatorExtents.forWgsEnvelope(bounds.envelope(), zoom);
-        checkGridSize(extents);
         task.height = extents.height;
         task.north = extents.north;
         task.west = extents.west;
@@ -271,21 +267,6 @@ public void populateTask (AnalysisWorkerTask task, UserPermissions userPermissio
         }
     }
 
-    private static void checkGridSize (WebMercatorExtents extents) {
-        if (extents.zoom < MIN_ZOOM || extents.zoom > MAX_ZOOM) {
-            throw AnalysisServerException.badRequest(String.format(
-                    "Requested zoom (%s) is outside valid range (%s - %s)", extents.zoom, MIN_ZOOM, MAX_ZOOM
-            ));
-        }
-        if (extents.height * extents.width > MAX_GRID_CELLS) {
-            throw AnalysisServerException.badRequest(String.format(
-                    "Requested number of destinations (%s) exceeds limit (%s). " +
-                            "Set smaller custom geographic bounds or a lower zoom level.",
-                            extents.height * extents.width, MAX_GRID_CELLS
-            ));
-        }
-    }
-
     private EnumSet<LegMode> getEnumSetFromString (String s) {
         if (s != null && !"".equals(s)) {
             return EnumSet.copyOf(Arrays.stream(s.split(",")).map(LegMode::valueOf).collect(Collectors.toList()));

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/Pattern.java

@@ -24,9 +24,15 @@ public class Pattern implements Serializable {
     // TODO: add set of shapes
 //    public Set<String> associatedShapes;
 
-    // This is the only place in the library we are still using the old JTS package name. These objects are
-    // serialized into MapDB files. We want to read and write MapDB files that old workers can understand.
+    // This is the only place we are still using the old JTS package name.
+    // Hopefully we can get rid of this - it's the only thing still using JTS objects under the obsolete vividsolutions
+    // package name so is pulling in extra dependencies and requiring conversions (toLegacyLineString).
+    // Unfortunately these objects are serialized into MapDB files, and we want to read and write MapDB files that
+    // old workers can understand. This cannot be migrated to newer JTS package names without deprecating all older
+    // workers, then deleting all MapDB representations of GTFS data from S3 and the local cache directory, forcing
+    // them to all be rebuilt the next time they're used.
     public com.vividsolutions.jts.geom.LineString geometry;
+
     public String name;
     public String route_id;
     public static Joiner joiner = Joiner.on("-").skipNulls();

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