clean up remaning exceptions

docs/changelog/100368.yaml

@@ -1,5 +1,5 @@
 pr: 100368
 summary: "Status codes for Aggregation errors, part 2"
 area: Aggregations
-type: tech debt
+type: Enhancement
 issues: []

server/src/main/java/org/elasticsearch/search/aggregations/AdaptingAggregator.java

@@ -112,7 +112,6 @@ public final InternalAggregation buildEmptyAggregation() {
             return adapt(delegate.buildEmptyAggregation());
         } catch (IOException e) {
             // We don't expect this to happen, but computers are funny.
-            // 500 class error, because we believe it to be unreachable?
             throw new AggregationExecutionException("io error while building empty agg", e);
         }
     }

server/src/main/java/org/elasticsearch/search/aggregations/AggregationErrors.java

@@ -8,7 +8,6 @@
 
 package org.elasticsearch.search.aggregations;
 
-import org.elasticsearch.search.aggregations.pipeline.AbstractPipelineAggregationBuilder;
 import org.elasticsearch.search.aggregations.support.ValuesSource;
 
 import java.util.Optional;
@@ -126,20 +125,33 @@ public static RuntimeException unsupportedRounding(String typeName) {
      * Indicates that an aggregation path (e.g. from a pipeline agg) references an aggregation of the wrong type, for example
      * attempting to take a cumulative cardinality of something other than a cardinality aggregation.
      *
-     * @param aggPath the path element found to be invalid
-     * @param got What we actually got; this may be null.
+     * @param aggPath    the path element found to be invalid
+     * @param expected
+     * @param got        What we actually got; this may be null.
      * @param currentAgg The name of the aggregation in question
      * @return an appropriate exception
      */
-    public static RuntimeException incompatibleAggregationType(String aggPath, String got, String currentAgg) {
+    public static RuntimeException incompatibleAggregationType(String aggPath, String expected, String got, String currentAgg) {
 
-        return new IllegalArgumentException(
+        return new AggregationExecutionException.InvalidBucketPath(
             aggPath
-                + " must reference a cardinality aggregation, got: ["
+                + " must reference a " + expected + " aggregation, got: ["
                 + (got == null ? "null" : got)
                 + "] at aggregation ["
                 + currentAgg
                 + "]"
         );
     }
+
+    /**
+     * This is a 500 class error indicating a programming error.  Hopefully we never see this outside of tests.
+     * @param bucketOrds - the ords we are processing
+     * @param got - the ordinal we got
+     * @param expected - the ordinal we expected
+     * @return an appropriate exception
+     */
+    public static RuntimeException iterationOrderChangedWithoutMutating(String bucketOrds, long got, long expected) {
+        return new AggregationExecutionException(
+            "Iteration order of [" + bucketOrds + "] changed without mutating. [" + got + "] should have been [" + expected + "]" );
+    }
 }

server/src/main/java/org/elasticsearch/search/aggregations/AggregationExecutionException.java

@@ -9,6 +9,7 @@
 
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.common.io.stream.StreamInput;
+import org.elasticsearch.rest.RestStatus;
 
 import java.io.IOException;
 
@@ -28,4 +29,24 @@ public AggregationExecutionException(String msg, Throwable cause) {
     public AggregationExecutionException(StreamInput in) throws IOException {
         super(in);
     }
+
+    public static class InvalidBucketPath extends AggregationExecutionException {
+
+        public InvalidBucketPath(String msg) {
+            super(msg);
+        }
+
+        public InvalidBucketPath(String msg, Throwable cause) {
+            super(msg, cause);
+        }
+
+        public InvalidBucketPath(StreamInput in) throws IOException {
+            super(in);
+        }
+
+        @Override
+        public RestStatus status() {
+            return RestStatus.BAD_REQUEST;
+        }
+    }
 }

server/src/main/java/org/elasticsearch/search/aggregations/InternalOrder.java

@@ -65,8 +65,7 @@ public <T extends Bucket> Comparator<T> partiallyBuiltBucketComparator(ToLongFun
                 BucketComparator bucketComparator = path.bucketComparator(aggregator, order);
                 return (lhs, rhs) -> bucketComparator.compare(ordinalReader.applyAsLong(lhs), ordinalReader.applyAsLong(rhs));
             } catch (IllegalArgumentException e) {
-                // Seems like this should be 400 (non-retryable), but we clearly intentionally throw a 500 here.  Why?
-                throw new AggregationExecutionException("Invalid aggregation order path [" + path + "]. " + e.getMessage(), e);
+                throw new IllegalArgumentException("Invalid aggregation order path [" + path + "]. " + e.getMessage(), e);
             }
         }
 

server/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java

@@ -11,6 +11,7 @@
 import org.elasticsearch.common.breaker.CircuitBreaker;
 import org.elasticsearch.common.util.LongArray;
 import org.elasticsearch.core.Releasable;
+import org.elasticsearch.search.aggregations.AggregationErrors;
 import org.elasticsearch.search.aggregations.AggregationExecutionException;
 import org.elasticsearch.search.aggregations.Aggregator;
 import org.elasticsearch.search.aggregations.AggregatorBase;
@@ -340,8 +341,9 @@ protected final <B> InternalAggregation[] buildAggregationsForVariableBuckets(
             totalOrdsToCollect += bucketCount;
         }
         if (totalOrdsToCollect > Integer.MAX_VALUE) {
-            // Seems like this should be a 400; also, should this check the bucket limit instead of Integer.MAX_VALUE?
-            throw new AggregationExecutionException(
+            // TODO: We should instrument this error.  While it is correct for it to be a 400 class IllegalArgumentException, there is not
+            // much the user can do about that. If this occurs with any frequency, we should do something about it.
+            throw new IllegalArgumentException(
                 "Can't collect more than [" + Integer.MAX_VALUE + "] buckets but attempted [" + totalOrdsToCollect + "]"
             );
         }
@@ -362,16 +364,8 @@ protected final <B> InternalAggregation[] buildAggregationsForVariableBuckets(
             LongKeyedBucketOrds.BucketOrdsEnum ordsEnum = bucketOrds.ordsEnum(owningBucketOrds[ordIdx]);
             while (ordsEnum.next()) {
                 if (bucketOrdsToCollect[b] != ordsEnum.ord()) {
-                    // Not sure what can cause this; 500 seems appropriate, maybe?
-                    throw new AggregationExecutionException(
-                        "Iteration order of ["
-                            + bucketOrds
-                            + "] changed without mutating. ["
-                            + ordsEnum.ord()
-                            + "] should have been ["
-                            + bucketOrdsToCollect[b]
-                            + "]"
-                    );
+                    // If we hit this, something has gone horribly wrong and we need to investigate
+                    throw AggregationErrors.iterationOrderChangedWithoutMutating(bucketOrds.toString(), ordsEnum.ord(), bucketOrdsToCollect[b]);
                 }
                 buckets.add(bucketBuilder.build(ordsEnum.value(), bucketDocCount(ordsEnum.ord()), subAggregationResults[b++]));
             }

server/src/main/java/org/elasticsearch/search/aggregations/bucket/global/GlobalAggregatorFactory.java

@@ -47,7 +47,7 @@ public Aggregator createInternal(Aggregator parent, CardinalityUpperBound cardin
             );
         }
         if (cardinality != CardinalityUpperBound.ONE) {
-            // Not sure what would cause us to land here
+            // Hitting this exception is a programmer error.  Hopefully never seen in production.
             throw new AggregationExecutionException("Aggregation [" + name() + "] must have cardinality 1 but was [" + cardinality + "]");
         }
         return new GlobalAggregator(name, factories, context, metadata);

server/src/main/java/org/elasticsearch/search/aggregations/bucket/prefix/IpPrefixAggregator.java

@@ -12,6 +12,7 @@
 import org.apache.lucene.util.CollectionUtil;
 import org.elasticsearch.core.Releasables;
 import org.elasticsearch.index.fielddata.SortedBinaryDocValues;
+import org.elasticsearch.search.aggregations.AggregationErrors;
 import org.elasticsearch.search.aggregations.AggregationExecutionContext;
 import org.elasticsearch.search.aggregations.AggregationExecutionException;
 import org.elasticsearch.search.aggregations.Aggregator;
@@ -195,16 +196,7 @@ public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws I
             while (ordsEnum.next()) {
                 long ordinal = ordsEnum.ord();
                 if (bucketOrdsToCollect[b] != ordinal) {
-                    // Not sure what can cause this; 500 seems appropriate, maybe?
-                    throw new AggregationExecutionException(
-                        "Iteration order of ["
-                            + bucketOrds
-                            + "] changed without mutating. ["
-                            + ordinal
-                            + "] should have been ["
-                            + bucketOrdsToCollect[b]
-                            + "]"
-                    );
+                    throw AggregationErrors.iterationOrderChangedWithoutMutating(bucketOrds.toString(), ordinal, bucketOrdsToCollect[b]);
                 }
                 BytesRef ipAddress = new BytesRef();
                 ordsEnum.readValue(ipAddress);

server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java

@@ -98,7 +98,7 @@ public GlobalOrdinalsStringTermsAggregator(
         } else {
             this.collectionStrategy = cardinality.map(estimate -> {
                 if (estimate > 1) {
-                    // Seems like it should be 500; something went wrong if we got here
+                    // This is a 500 class error, because we should never be able to reach it.
                     throw new AggregationExecutionException("Dense ords don't know how to collect from many buckets");
                 }
                 return new DenseGlobalOrds();

server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketMetricsPipelineAggregator.java

@@ -56,7 +56,7 @@ public final InternalAggregation doReduce(Aggregations aggregations, Aggregation
                 Aggregation currentAgg = aggregation;
                 while (currElement < parsedPath.size() - 1) {
                     if (currentAgg == null) {
-                        throw new IllegalArgumentException(
+                        throw new AggregationExecutionException.InvalidBucketPath(
                             "bucket_path ["
                                 + bucketsPaths()[0]
                                 + "] expected aggregation with name ["
@@ -74,7 +74,7 @@ public final InternalAggregation doReduce(Aggregations aggregations, Aggregation
                                 );
                             if (bucket == null) {
                                 // seems not retryable, and therefore should be 400?
-                                throw new AggregationExecutionException(
+                                throw new AggregationExecutionException.InvalidBucketPath(
                                     "missing bucket ["
                                         + parsedPath.get(currElement).key()
                                         + "] for agg ["
@@ -93,7 +93,7 @@ public final InternalAggregation doReduce(Aggregations aggregations, Aggregation
                             currentAgg = bucket.getAggregations().get(parsedPath.get(++currElement).name());
                         } else {
                             // Seems not retryable, should be 400
-                            throw new AggregationExecutionException(
+                            throw new AggregationExecutionException.InvalidBucketPath(
                                 "bucket_path ["
                                     + bucketsPaths()[0]
                                     + "] indicates bucket_key ["
@@ -114,7 +114,7 @@ public final InternalAggregation doReduce(Aggregations aggregations, Aggregation
                     String msg = currentAgg == null
                         ? "did not find multi-bucket aggregation for extraction."
                         : "did not find multi-bucket aggregation for extraction. Found [" + currentAgg.getName() + "]";
-                    throw new AggregationExecutionException(msg);
+                    throw new AggregationExecutionException.InvalidBucketPath(msg);
                 }
                 List<String> sublistedPath = AggregationPath.pathElementsAsStringList(parsedPath.subList(currElement, parsedPath.size()));
                 // First element is the current agg, so we want the rest of the path

x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityPipelineAggregator.java

@@ -8,7 +8,6 @@
 
 import org.elasticsearch.search.DocValueFormat;
 import org.elasticsearch.search.aggregations.AggregationErrors;
-import org.elasticsearch.search.aggregations.AggregationExecutionException;
 import org.elasticsearch.search.aggregations.AggregationReduceContext;
 import org.elasticsearch.search.aggregations.InternalAggregation;
 import org.elasticsearch.search.aggregations.InternalAggregations;
@@ -92,7 +91,7 @@ private AbstractHyperLogLogPlusPlus resolveBucketValue(
         if (propertyValue == null) {
             throw AggregationErrors.incompatibleAggregationType(
                 AbstractPipelineAggregationBuilder.BUCKETS_PATH_FIELD.getPreferredName(),
-                "null",
+                "cardinality", "null",
                 currentAggName
             );
         }
@@ -103,7 +102,7 @@ private AbstractHyperLogLogPlusPlus resolveBucketValue(
 
         throw AggregationErrors.incompatibleAggregationType(
             AbstractPipelineAggregationBuilder.BUCKETS_PATH_FIELD.getPreferredName(),
-            propertyValue.getClass().getSimpleName(),
+            "cardinality", propertyValue.getClass().getSimpleName(),
             currentAggName
         );
     }