Merge branch 'dev' into update-mongodb-driver

.github/workflows/gradle.yml

@@ -33,14 +33,11 @@ jobs:
         fetch-depth: 0
     # Java setup step completes very fast, no need to run in a preconfigured docker container.
     - name: Set up JDK 11
-      uses: actions/setup-java@v1
+      uses: actions/setup-java@v3
       with:
         java-version: 11
-    - uses: actions/cache@v1
-      id: cache
-      with:
-        path: ~/.gradle/caches
-        key: gradle-caches
+        distribution: temurin
+        cache: 'gradle'
     - name: Show version string
       run: gradle -q printVersion | head -n1
     - name: Build and Test

build.gradle

@@ -1,8 +1,8 @@
 plugins {
     id 'java'
-    id 'com.github.johnrengelman.shadow' version '6.0.0'
+    id 'com.github.johnrengelman.shadow' version '8.1.0'
     id 'maven-publish'
-    id 'com.palantir.git-version' version '0.12.3'
+    id 'com.palantir.git-version' version '2.0.0'
 }
 
 group = 'com.conveyal'
@@ -72,17 +72,19 @@ tasks.withType(JavaCompile) {
     options.encoding = 'UTF-8'
 }
 
-// A task to copy all dependencies of the project into a single directory
+// A task to copy all dependency JARs needed at runtime into a single directory
 task copyDependencies(type: Copy) {
-    from configurations.default
+    from(sourceSets.main.runtimeClasspath) {
+        include '*.jar'
+    }
     into 'dependencies'
 }
 
 // Run R5 as a local analysis backend with all dependencies on the classpath, without building a shadowJar.
 task runBackend (type: JavaExec) {
    dependsOn(build)
    classpath(sourceSets.main.runtimeClasspath)
-   main("com.conveyal.analysis.BackendMain")
+   mainClass = 'com.conveyal.analysis.BackendMain'
 }
 
 // Start up an analysis local backend from a shaded JAR and ask it to shut down immediately.
@@ -91,7 +93,7 @@ task runBackend (type: JavaExec) {
 task testShadowJarRunnable(type: JavaExec) {
     dependsOn(shadowJar)
     classpath(shadowJar.archiveFile.get())
-    main("com.conveyal.analysis.BackendMain")
+    mainClass = 'com.conveyal.analysis.BackendMain'
     jvmArgs("-Dconveyal.immediate.shutdown=true")
 }
 
@@ -123,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/components/broker/Broker.java

@@ -530,8 +530,9 @@ private void requestExtraWorkersIfAppropriate(Job job) {
 
             // Do not exceed the limit on workers per category TODO add similar limit per accessGroup or user
             targetWorkerTotal = Math.min(targetWorkerTotal, MAX_WORKERS_PER_CATEGORY);
-            // Guardrail until freeform pointsets are tested more thoroughly
-            if (job.templateTask.originPointSet != null) targetWorkerTotal = Math.min(targetWorkerTotal, 5);
+            // Guardrails until freeform pointsets are tested more thoroughly
+            if (job.templateTask.originPointSet != null) targetWorkerTotal = Math.min(targetWorkerTotal, 80);
+            if (job.templateTask.includePathResults) targetWorkerTotal = Math.min(targetWorkerTotal, 20);
             int nSpot =  targetWorkerTotal - categoryWorkersAlreadyRunning;
             createWorkersInCategory(job.workerCategory, job.workerTags, 0, nSpot);
         }

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

@@ -67,14 +67,11 @@ public class BundleController implements HttpController {
 
     private final FileStorage fileStorage;
     private final GTFSCache gtfsCache;
-    // FIXME The backend appears to use an osmcache purely to get a file key at which to store incoming OSM. Refactor.
-    private final OSMCache osmCache;
     private final TaskScheduler taskScheduler;
 
     public BundleController (BackendComponents components) {
         this.fileStorage = components.fileStorage;
         this.gtfsCache = components.gtfsCache;
-        this.osmCache = components.osmCache;
         this.taskScheduler = components.taskScheduler;
     }
 
@@ -176,7 +173,7 @@ private Bundle create (Request req, Response res) {
                     osm.close();
                     checkWgsEnvelopeSize(osmBounds, "OSM data");
                     // Store the source OSM file. Note that we're not storing the derived MapDB file here.
-                    fileStorage.moveIntoStorage(osmCache.getKey(bundle.osmId), fi.getStoreLocation());
+                    fileStorage.moveIntoStorage(OSMCache.getKey(bundle.osmId), fi.getStoreLocation());
                 }
 
                 if (bundle.feedGroupId == null) {

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