Merge pull request #918 from conveyal/freeform-guardrails

Fix freeform guardrails

Author: ansoncfit

src/main/java/com/conveyal/analysis/components/HttpApi.java

@@ -179,7 +179,7 @@ private void respondToException(Exception e, Request request, Response response,
         // Include a stack trace except when the error is known to be about unauthenticated or unauthorized access,
         // in which case we don't want to leak information about the server to people scanning it for weaknesses.
         if (type != UNAUTHORIZED && type != FORBIDDEN) {
-            body.put("stackTrace", errorEvent.stackTrace);
+            body.put("stackTrace", errorEvent.filteredStackTrace);
         }
         response.status(code);
         response.type("application/json");

src/main/java/com/conveyal/analysis/components/eventbus/ErrorEvent.java

@@ -11,11 +11,11 @@
  */
 public class ErrorEvent extends Event {
 
-    // We may serialize this object, so we convert the Throwable to two strings to control its representation.
+    // All Events are intended to be eligible for serialization into a log or database, so we convert the Throwable to
+    // some Strings to determine its representation in a simple way.
     // For flexibility in event handlers, it is tempting to hold on to the original Throwable instead of derived
     // Strings. Exceptions are famously slow, but it's the initial creation and filling in the stack trace that are
-    // slow. Once the instace exists, repeatedly examining its stack trace should not be prohibitively costly. Still,
-    // we do probably gain some efficiency by converting the stack trace to a String once and reusing that.
+    // slow. Once the instance exists, repeatedly examining its stack trace should not be prohibitively costly.
 
     public final String summary;
 
@@ -25,11 +25,16 @@ public class ErrorEvent extends Event {
      */
     public final String httpPath;
 
+    /** The full stack trace of the exception that occurred. */
     public final String stackTrace;
 
+    /** A minimal stack trace showing the immediate cause within Conveyal code. */
+    public final String filteredStackTrace;
+
     public ErrorEvent (Throwable throwable, String httpPath) {
         this.summary = ExceptionUtils.shortCauseString(throwable);
         this.stackTrace = ExceptionUtils.stackTraceString(throwable);
+        this.filteredStackTrace = ExceptionUtils.filterStackTrace(throwable);
         this.httpPath = httpPath;
     }
 
@@ -56,7 +61,7 @@ public String traceWithContext (boolean verbose) {
         if (verbose) {
             builder.append(stackTrace);
         } else {
-            builder.append(filterStackTrace(stackTrace));
+            builder.append(filteredStackTrace);
         }
         return builder.toString();
     }

src/main/java/com/conveyal/analysis/results/MultiOriginAssembler.java

@@ -7,6 +7,8 @@
 import com.conveyal.file.FileStorage;
 import com.conveyal.file.FileStorageFormat;
 import com.conveyal.r5.analyst.PointSet;
+import com.conveyal.r5.analyst.cluster.PathResult;
+import com.conveyal.r5.analyst.cluster.RegionalTask;
 import com.conveyal.r5.analyst.cluster.RegionalWorkResult;
 import com.conveyal.r5.util.ExceptionUtils;
 import org.slf4j.Logger;
@@ -89,21 +91,27 @@ public MultiOriginAssembler (RegionalAnalysis regionalAnalysis, Job job, FileSto
             this.job = job;
             this.nOriginsTotal = job.nTasksTotal;
             this.originsReceived = new BitSet(job.nTasksTotal);
-            // Check that origin and destination sets are not too big for generating CSV files.
-            if (!job.templateTask.makeTauiSite &&
-                 job.templateTask.destinationPointSetKeys[0].endsWith(FileStorageFormat.FREEFORM.extension)
-            ) {
-               // This requires us to have already loaded this destination pointset instance into the transient field.
-                PointSet destinationPointSet = job.templateTask.destinationPointSets[0];
-                if ((job.templateTask.recordTimes || job.templateTask.includePathResults) && !job.templateTask.oneToOne) {
-                    if (nOriginsTotal * destinationPointSet.featureCount() > MAX_FREEFORM_OD_PAIRS ||
-                        destinationPointSet.featureCount() > MAX_FREEFORM_DESTINATIONS
-                    ) {
-                        throw new AnalysisServerException(String.format(
-                            "Freeform requests limited to %d destinations and %d origin-destination pairs.",
-                            MAX_FREEFORM_DESTINATIONS, MAX_FREEFORM_OD_PAIRS
-                        ));
-                    }
+            // If results have been requested for freeform origins, check that the origin and
+            // destination pointsets are not too big for generating CSV files.
+            RegionalTask task = job.templateTask;
+            if (!task.makeTauiSite && task.destinationPointSetKeys[0].endsWith(FileStorageFormat.FREEFORM.extension)) {
+                // This requires us to have already loaded this destination pointset instance into the transient field.
+                PointSet destinationPointSet = task.destinationPointSets[0];
+                int nDestinations = destinationPointSet.featureCount();
+                int nODPairs = task.oneToOne ? nOriginsTotal : nOriginsTotal * nDestinations;
+                if (task.recordTimes &&
+                    (nDestinations > MAX_FREEFORM_DESTINATIONS || nODPairs > MAX_FREEFORM_OD_PAIRS)) {
+                    throw AnalysisServerException.badRequest(String.format(
+                       "Travel time results limited to %d destinations and %d origin-destination pairs.",
+                       MAX_FREEFORM_DESTINATIONS, MAX_FREEFORM_OD_PAIRS
+                    ));
+                }
+                if (task.includePathResults &&
+                    (nDestinations > PathResult.MAX_PATH_DESTINATIONS || nODPairs > MAX_FREEFORM_OD_PAIRS)) {
+                    throw AnalysisServerException.badRequest(String.format(
+                        "Path results limited to %d destinations and %d origin-destination pairs.",
+                        PathResult.MAX_PATH_DESTINATIONS, MAX_FREEFORM_OD_PAIRS
+                    ));
                 }
             }
 

src/main/java/com/conveyal/r5/analyst/cluster/PathResult.java

@@ -38,7 +38,7 @@ public class PathResult {
      * These results are returned to the backend over an HTTP API so we don't want to risk making them too huge.
      * This could be set to a higher number in cases where you know the result return channel can handle the size.
      */
-    public static int maxDestinations = 5000;
+    public static final int MAX_PATH_DESTINATIONS = 5_000;
 
     private final int nDestinations;
     /**
@@ -49,7 +49,7 @@ public class PathResult {
     public final Multimap<RouteSequence, Iteration>[] iterationsForPathTemplates;
     private final TransitLayer transitLayer;
 
-    public static String[] DATA_COLUMNS = new String[]{
+    public static final String[] DATA_COLUMNS = new String[]{
             "routes",
             "boardStops",
             "alightStops",
@@ -70,8 +70,8 @@ public PathResult(AnalysisWorkerTask task, TransitLayer transitLayer) {
             // In regional analyses, return paths to all destinations
             nDestinations = task.nTargetsPerOrigin();
             // This limitation reflects the initial design, for use with freeform pointset destinations
-            if (nDestinations > maxDestinations) {
-                throw new UnsupportedOperationException("Number of detailed path destinations exceeds limit of " + maxDestinations);
+            if (nDestinations > MAX_PATH_DESTINATIONS) {
+                throw new UnsupportedOperationException("Number of detailed path destinations exceeds limit of " + MAX_PATH_DESTINATIONS);
             }
         }
         iterationsForPathTemplates = new Multimap[nDestinations];

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

@@ -55,8 +55,8 @@ public static String shortAndLongString (Throwable throwable) {
      * and all additional frames that come from Conveyal packages. This yields a much shorter stack trace that still
      * shows where the exception was thrown and where the problem originates in our own code.
      */
-    public static String filterStackTrace (String stackTrace) {
-        if (stackTrace == null) return null;
+    public static String filterStackTrace (Throwable throwable) {
+        String stackTrace = stackTraceString(throwable);
         final String unknownFrame = "Unknown stack frame, probably optimized out by JVM.";
         String error = stackTrace.lines().findFirst().get();
         String frame = stackTrace.lines()
@@ -71,8 +71,4 @@ public static String filterStackTrace (String stackTrace) {
         return String.join("\n", error, frame, conveyalFrame);
     }
 
-    public static String filterStackTrace (Throwable throwable) {
-        return filterStackTrace(stackTraceString(throwable));
-    }
-
 }