Fix freeform guardrails

Correct logic for checking oneToOne analyses Check limit on number of destinations for path analyses in broker
conveyal · Dec 19, 2023 · 6b6c767 · 6b6c767
1 parent ef9638b
commit 6b6c767
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 18 deletions.
diff --git a/src/main/java/com/conveyal/analysis/results/MultiOriginAssembler.java b/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 new AnalysisServerException(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 new AnalysisServerException(String.format(
+                        "Path results limited to %d destinations and %d origin-destination pairs.",
+                        PathResult.MAX_PATH_DESTINATIONS, MAX_FREEFORM_OD_PAIRS
+                    ));
                 }
             }
 

diff --git a/src/main/java/com/conveyal/r5/analyst/cluster/PathResult.java b/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 int MAX_PATH_DESTINATIONS = 5_000;
 
     private final int nDestinations;
     /**
@@ -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];