Merge pull request #872 from conveyal/issue-844

Always checkGridSize when creating grids

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/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/r5/analyst/Grid.java

@@ -107,6 +107,11 @@ public Grid (int west, int north, int width, int height, int zoom) {
         this(new WebMercatorExtents(west, north, width, height, zoom));
     }
 
+    /**
+     * Other constructors and factory methods all call this one, and Grid has a WebMercatorExtents field, which means
+     * Grid construction always follows construction of a WebMercatorExtents, whose constructor always performs a
+     * check on the size of the grid.
+     */
     public Grid (WebMercatorExtents extents) {
         this.extents = extents;
         this.grid = new double[extents.width][extents.height];

src/main/java/com/conveyal/r5/analyst/WebMercatorExtents.java

@@ -1,5 +1,6 @@
 package com.conveyal.r5.analyst;
 
+import com.conveyal.analysis.AnalysisServerException;
 import com.conveyal.r5.analyst.cluster.AnalysisWorkerTask;
 import org.geotools.geometry.jts.ReferencedEnvelope;
 import org.geotools.referencing.CRS;
@@ -17,15 +18,19 @@
 import static com.google.common.base.Preconditions.checkState;
 
 /**
- * Really we should be embedding one of these in the tasks, grids, etc. to factor out all the common fields.
- * Equals and hashcode are semantic, for use as or within hashtable keys.
+ * Really we should have a field pointing to an instance of this in tasks, grids, etc. to factor out all the common
+ * fields. Equals and hashcode are semantic, for use as or within hashtable keys.
  *
  * TODO may want to distinguish between WebMercatorExtents, WebMercatorGrid (adds lat/lon conversion functions),
  *      and OpportunityGrid (AKA Grid) which adds opportunity counts. These can compose, not necessarily subclass.
  *      Of course they could all be one class, with the opportunity grid nulled out when there is no density.
  */
 public class WebMercatorExtents {
 
+    private static final int MIN_ZOOM = 9;
+    private static final int MAX_ZOOM = 12;
+    private static final int MAX_GRID_CELLS = 5_000_000;
+
     /** The pixel number of the westernmost pixel (smallest x value). */
     public final int west;
 
@@ -44,12 +49,17 @@ public class WebMercatorExtents {
     /** Web mercator zoom level. */
     public final int zoom;
 
+    /**
+     * All factory methods or constructors for WebMercatorExtents should eventually call this constructor,
+     * as it will enforce size constraints that prevent straining or stalling the system.
+     */
     public WebMercatorExtents (int west, int north, int width, int height, int zoom) {
         this.west = west;
         this.north = north;
         this.width = width;
         this.height = height;
         this.zoom = zoom;
+        checkGridSize();
     }
 
     /**
@@ -84,7 +94,11 @@ public static WebMercatorExtents forPointsets (PointSet[] pointSets) {
         }
     }
 
-    /** Create the minimum-size immutable WebMercatorExtents containing both this one and the other one. */
+    /**
+     * Create the minimum-size immutable WebMercatorExtents containing both this one and the other one.
+     * Note that WebMercatorExtents fields are immutable, and this method does not modify the instance in place.
+     * It creates a new instance. This behavior differs from GeoTools / JTS Envelopes.
+     */
     public WebMercatorExtents expandToInclude (WebMercatorExtents other) {
         checkState(this.zoom == other.zoom, "All grids supplied must be at the same zoom level.");
         final int thisEast = this.west + this.width;
@@ -207,4 +221,34 @@ public static Coordinate mercatorPixelToMeters (double xPixel, double yPixel, in
         return new Coordinate(xMeters, yMeters);
     }
 
+    /**
+     * Users may create very large grids in various ways. For example, by setting large custom analysis bounds or by
+     * uploading spatial data sets with very large extents. This method checks some limits on the zoom level and total
+     * number of cells to avoid straining or stalling the system.
+     *
+     * The fields of WebMercatorExtents are immutable and are not typically deserialized from incoming HTTP API
+     * requests. As all instances are created through a constructor, so we can perform this check every time a grid is
+     * created. If we do eventually deserialize these from HTTP API requests, we'll have to call the check explicitly.
+     * The Grid class internally uses a WebMercatorExtents field, so dimensions are also certain to be checked while
+     * constructing a Grid.
+     *
+     * An analysis destination grid might become problematic at a smaller size than an opportunity data grid. But until
+     * we have a reason to distinguish different situations, MAX_GRID_CELLS is defined as a constant in this class.
+     * If needed, we can make the max size a method parameter and pass in different limits in different contexts.
+     */
+    public void checkGridSize () {
+        if (this.zoom < MIN_ZOOM || this.zoom > MAX_ZOOM) {
+            throw AnalysisServerException.badRequest(String.format(
+                    "Requested zoom (%s) is outside valid range (%s - %s)", this.zoom, MIN_ZOOM, MAX_ZOOM
+            ));
+        }
+        if (this.height * this.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.",
+                    this.height * this.width, MAX_GRID_CELLS
+            ));
+        }
+    }
+
 }

src/main/java/com/conveyal/r5/util/EncodedPolylineSerializer.java

@@ -11,9 +11,14 @@
 import java.io.IOException;
 
 /**
- * Serialize to Google encoded polyline.
- * Hopefully we can get rid of this - it's the only thing still using JTS objects under the vividsolutions package name
- * so is pulling in extra dependencies and requiring conversions (toLegacyLineString).
+ * Serialize JTS LineString to Google encoded polyline.
+ *
+ * This class is the only use of dependency com.axiomalaska:polyline-encoder, and it is only used in
+ * com.conveyal.r5.transitive.TransitivePattern, which is in turn only used in
+ * com.conveyal.r5.transitive.TransitiveNetwork, which is in turn only used in
+ * com.conveyal.r5.analyst.cluster.AnalysisWorker#saveTauiMetadata.
+ * That dependency has required maintainance on a few occasions and was hosted at a repo outside Maven Central which has
+ * become unavailable on a few occations. We have copied the artifact to our S3-backed Conveyal Maven repo.
  */
 public class EncodedPolylineSerializer extends JsonSerializer<LineString> {
 

src/test/java/com/conveyal/r5/analyst/GridTest.java

@@ -26,31 +26,33 @@ public class GridTest {
     @Test
     public void testGetMercatorEnvelopeMeters() throws Exception {
         // Southeastern Australia
-        // Correct meter coordinates from http://www.maptiler.org/google-maps-coordinates-tile-bounds-projection/
-        int zoom = 4;
-        int xTile = 14;
-        int yTile = 9;
+        // Reference coordinates in meters from http://www.maptiler.org/google-maps-coordinates-tile-bounds-projection/
+        int zoom = 9;
+        int xTile = 465;
+        int yTile = 312;
         Grid grid = new Grid(256 * xTile, 256 * yTile, 256, 256, zoom);
         ReferencedEnvelope envelope = grid.getWebMercatorExtents().getMercatorEnvelopeMeters();
-        assertEquals(15028131.257091936, envelope.getMinX(), 0.1);
-        assertEquals(-5009377.085697312, envelope.getMinY(), 0.1);
-        assertEquals(17532819.79994059, envelope.getMaxX(), 0.1);
-        assertEquals(-2504688.542848654, envelope.getMaxY(), 0.1);
-
-        // Cutting through Paris
-        zoom = 5;
-        xTile = 16;
-        yTile = 11;
+        assertEquals(16358747.0, envelope.getMinX(), 1);
+        assertEquals(-4461476.0, envelope.getMinY(), 1);
+        assertEquals(16437019.0, envelope.getMaxX(), 1);
+        assertEquals(-4383205.0, envelope.getMaxY(), 1);
+
+        // The tile east of Greenwich
+        // Reference coordinates in meters from http://www.maptiler.org/google-maps-coordinates-tile-bounds-projection/
+        // Expect the zero edge to be more exact, others to within one meter.
+        zoom = 12;
+        xTile = 2048;
+        yTile = 1362;
         grid = new Grid(256 * xTile, 256 * yTile, 256, 256, zoom);
         envelope = grid.getWebMercatorExtents().getMercatorEnvelopeMeters();
-        assertEquals(0, envelope.getMinX(), 0.1);
-        assertEquals(5009377.085697312, envelope.getMinY(), 0.1);
-        assertEquals(1252344.271424327, envelope.getMaxX(), 0.1);
-        assertEquals(6261721.357121639, envelope.getMaxY(), 0.1);
+        assertEquals(0.0, envelope.getMinX(), 0.01);
+        assertEquals(6701999.0, envelope.getMinY(), 1);
+        assertEquals(9784.0, envelope.getMaxX(), 1);
+        assertEquals(6711783.0, envelope.getMaxY(), 1);
 
 //        /**
-//         * Make sure the Mercator projection works properly. Open the resulting file in GIS and
-//         * compare with http://www.maptiler.org/google-maps-coordinates-tile-bounds-projection/
+//         * Uncomment to manually verify that the Mercator projection works properly. Open the resulting file in GIS
+//         * and compare with http://www.maptiler.org/google-maps-coordinates-tile-bounds-projection/
 //         */
 //        OutputStream outputStream = new FileOutputStream("test.tiff");
 //        grid.writeGeotiff(outputStream);

src/test/java/com/conveyal/r5/analyst/network/GridSinglePointTaskBuilder.java

@@ -108,6 +108,10 @@ public GridSinglePointTaskBuilder maxRides(int rides) {
         return this;
     }
 
+    /**
+     * Even if you're not actually using the opportunity count, you should call this to set the grid extents on the
+     * resulting task. Otherwise it will fail checks on the grid dimensions and zoom level.
+     */
     public GridSinglePointTaskBuilder uniformOpportunityDensity (double density) {
         Grid grid = gridLayout.makeUniformOpportunityDataset(density);
         task.destinationPointSets = new PointSet[] { grid };

src/test/java/com/conveyal/r5/analyst/network/RandomFrequencyPhasingTests.java

@@ -52,6 +52,7 @@ public void testFilteredTripRandomization () throws Exception {
                 .weekendMorningPeak()
                 .setOrigin(20, 20)
                 .monteCarloDraws(1000)
+                .uniformOpportunityDensity(10)
                 .build();
 
         TravelTimeComputer computer = new TravelTimeComputer(task, network);