diff --git a/build.gradle b/build.gradle index 3ab0dbb0c..618a7bb88 100644 --- a/build.gradle +++ b/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. diff --git a/src/main/java/com/conveyal/analysis/controllers/UserActivityController.java b/src/main/java/com/conveyal/analysis/controllers/UserActivityController.java index c2beffd30..83b6f5389 100644 --- a/src/main/java/com/conveyal/analysis/controllers/UserActivityController.java +++ b/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."); diff --git a/src/main/java/com/conveyal/analysis/datasource/ShapefileDataSourceIngester.java b/src/main/java/com/conveyal/analysis/datasource/ShapefileDataSourceIngester.java index 03e61c6b7..21b1c06d4 100644 --- a/src/main/java/com/conveyal/analysis/datasource/ShapefileDataSourceIngester.java +++ b/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); diff --git a/src/main/java/com/conveyal/analysis/models/AnalysisRequest.java b/src/main/java/com/conveyal/analysis/models/AnalysisRequest.java index daad437e1..10d326656 100644 --- a/src/main/java/com/conveyal/analysis/models/AnalysisRequest.java +++ b/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 getEnumSetFromString (String s) { if (s != null && !"".equals(s)) { return EnumSet.copyOf(Arrays.stream(s.split(",")).map(LegMode::valueOf).collect(Collectors.toList())); diff --git a/src/main/java/com/conveyal/gtfs/GTFSFeed.java b/src/main/java/com/conveyal/gtfs/GTFSFeed.java index f1f380601..83236ad01 100644 --- a/src/main/java/com/conveyal/gtfs/GTFSFeed.java +++ b/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,11 +586,13 @@ private void namePatterns(Collection patterns) { public LineString getStraightLineForStops(String trip_id) { CoordinateList coordinates = new CoordinateList(); - LineString ls = null; + LineString lineString = null; Trip trip = trips.get(trip_id); Iterable 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); @@ -588,13 +600,9 @@ public LineString getStraightLineForStops(String trip_id) { 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; } /** diff --git a/src/main/java/com/conveyal/gtfs/error/DuplicateKeyError.java b/src/main/java/com/conveyal/gtfs/error/DuplicateKeyError.java index 9630449f4..50dca9c59 100644 --- a/src/main/java/com/conveyal/gtfs/error/DuplicateKeyError.java +++ b/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() { diff --git a/src/main/java/com/conveyal/gtfs/error/MissingKeyError.java b/src/main/java/com/conveyal/gtfs/error/MissingKeyError.java new file mode 100644 index 000000000..5a1ccdaeb --- /dev/null +++ b/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; + } +} diff --git a/src/main/java/com/conveyal/gtfs/model/Agency.java b/src/main/java/com/conveyal/gtfs/model/Agency.java index a2e301329..fd1e6031c 100644 --- a/src/main/java/com/conveyal/gtfs/model/Agency.java +++ b/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 { 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"); } } diff --git a/src/main/java/com/conveyal/gtfs/model/Calendar.java b/src/main/java/com/conveyal/gtfs/model/Calendar.java index e0f0be87f..8ac72a000 100644 --- a/src/main/java/com/conveyal/gtfs/model/Calendar.java +++ b/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 diff --git a/src/main/java/com/conveyal/gtfs/model/CalendarDate.java b/src/main/java/com/conveyal/gtfs/model/CalendarDate.java index 081f4c8b3..db4a1e9f3 100644 --- a/src/main/java/com/conveyal/gtfs/model/CalendarDate.java +++ b/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 diff --git a/src/main/java/com/conveyal/gtfs/model/Entity.java b/src/main/java/com/conveyal/gtfs/model/Entity.java index c445f2a41..a2395b473 100644 --- a/src/main/java/com/conveyal/gtfs/model/Entity.java +++ b/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 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)); + } + } } /** diff --git a/src/main/java/com/conveyal/gtfs/model/FareAttribute.java b/src/main/java/com/conveyal/gtfs/model/FareAttribute.java index 35afcbc97..9ea58bede 100644 --- a/src/main/java/com/conveyal/gtfs/model/FareAttribute.java +++ b/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 diff --git a/src/main/java/com/conveyal/gtfs/model/Pattern.java b/src/main/java/com/conveyal/gtfs/model/Pattern.java index 3cd86456c..c2eeae69f 100644 --- a/src/main/java/com/conveyal/gtfs/model/Pattern.java +++ b/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 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(); diff --git a/src/main/java/com/conveyal/gtfs/model/Route.java b/src/main/java/com/conveyal/gtfs/model/Route.java index cb9c208b8..f4965861b 100644 --- a/src/main/java/com/conveyal/gtfs/model/Route.java +++ b/src/main/java/com/conveyal/gtfs/model/Route.java @@ -32,6 +32,11 @@ public class Route extends Entity { // implements Entity.Factory public URL route_branding_url; public String feed_id; + @Override + public String getId() { + return route_id; + } + public static class Loader extends Entity.Loader { 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"); } } diff --git a/src/main/java/com/conveyal/gtfs/model/Stop.java b/src/main/java/com/conveyal/gtfs/model/Stop.java index 580030bef..26a5fabb3 100644 --- a/src/main/java/com/conveyal/gtfs/model/Stop.java +++ b/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 { public Writer (GTFSFeed feed) { super(feed, "stops"); diff --git a/src/main/java/com/conveyal/gtfs/model/StopTime.java b/src/main/java/com/conveyal/gtfs/model/StopTime.java index b15f7b46d..daabfec14 100644 --- a/src/main/java/com/conveyal/gtfs/model/StopTime.java +++ b/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 diff --git a/src/main/java/com/conveyal/gtfs/model/Trip.java b/src/main/java/com/conveyal/gtfs/model/Trip.java index 601d4d60d..d0a16380e 100644 --- a/src/main/java/com/conveyal/gtfs/model/Trip.java +++ b/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 { 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 diff --git a/src/main/java/com/conveyal/r5/analyst/Grid.java b/src/main/java/com/conveyal/r5/analyst/Grid.java index 7a11f595e..60bb526c6 100644 --- a/src/main/java/com/conveyal/r5/analyst/Grid.java +++ b/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]; diff --git a/src/main/java/com/conveyal/r5/analyst/WebMercatorExtents.java b/src/main/java/com/conveyal/r5/analyst/WebMercatorExtents.java index 150043b91..0d75a79ca 100644 --- a/src/main/java/com/conveyal/r5/analyst/WebMercatorExtents.java +++ b/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,8 +18,8 @@ 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. @@ -26,6 +27,10 @@ */ 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 + )); + } + } + } diff --git a/src/main/java/com/conveyal/r5/util/EncodedPolylineSerializer.java b/src/main/java/com/conveyal/r5/util/EncodedPolylineSerializer.java index ee2d4ffdb..ee5e8ba35 100644 --- a/src/main/java/com/conveyal/r5/util/EncodedPolylineSerializer.java +++ b/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 { diff --git a/src/test/java/com/conveyal/r5/analyst/GridTest.java b/src/test/java/com/conveyal/r5/analyst/GridTest.java index 6798f548e..03149d1c2 100644 --- a/src/test/java/com/conveyal/r5/analyst/GridTest.java +++ b/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); diff --git a/src/test/java/com/conveyal/r5/analyst/network/GridSinglePointTaskBuilder.java b/src/test/java/com/conveyal/r5/analyst/network/GridSinglePointTaskBuilder.java index a4b71fd9c..431dfcfbc 100644 --- a/src/test/java/com/conveyal/r5/analyst/network/GridSinglePointTaskBuilder.java +++ b/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 }; diff --git a/src/test/java/com/conveyal/r5/analyst/network/RandomFrequencyPhasingTests.java b/src/test/java/com/conveyal/r5/analyst/network/RandomFrequencyPhasingTests.java index 4f762a631..15370f3e7 100644 --- a/src/test/java/com/conveyal/r5/analyst/network/RandomFrequencyPhasingTests.java +++ b/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);