Enhancements to improve validation on BoundFilterBuilding around numb…

CHANGELOG.md

@@ -10,6 +10,11 @@ Current
 
 ### Added:
 
+- [Add more BoundFilterBuilding validation and hooks](https://github.com/yahoo/fili/issues/850)
+  * Added minimum and maximum arguments to FilterOperation
+  * Added validation on number of arguments to the bound filter builder
+  * Added hook for normalizing BoundFilterBuilder arguments
+
 - [Force update of cardinality to SearchIndexes](https://github.com/yahoo/fili/issues/846)
   * `SearchProvider` now has method `int getDimensionCardinality(boolean refresh)`, where refresh indicates the cardinality count should be refreshed before being returned.
     - default implementation just defers to existing method `int getDimensionCardinality()`

fili-core/src/main/java/com/yahoo/bard/webservice/data/filterbuilders/DruidBoundFilterBuilder.java

@@ -16,6 +16,8 @@
 import com.yahoo.bard.webservice.exception.TooManyDruidFiltersException;
 import com.yahoo.bard.webservice.web.ApiFilter;
 import com.yahoo.bard.webservice.web.DefaultFilterOperation;
+import com.yahoo.bard.webservice.web.ErrorMessageFormat;
+import com.yahoo.bard.webservice.web.FilterOperation;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -24,6 +26,7 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 /**
  * A DruidBoundFilterBuilder builds a Druid Bound Filter for supported dimensions, which can be sent directly to Druid.
@@ -69,22 +72,34 @@ public class DruidBoundFilterBuilder implements DruidFilterBuilder {
     public Filter buildFilters(final Map<Dimension, Set<ApiFilter>> filterMap) throws FilterBuilderException {
         LOG.trace("Building Druid Bound Filters using filter map: {}", filterMap);
 
+        if (filterMap.isEmpty()) {
+            return null;
+        }
+
         List<Filter> filters;
         try {
             filters =
                     filterMap.values()
                             .stream()
                             .flatMap(Set::stream)
-                            .map(filter -> buildDruidBoundFilters(filter))
+                            // Normalize allows us to expand one filter to many, or prune filters from processing
+                            // without a failure
+                            .flatMap(this::normalize)
+                            .peek(this::validateFilter)
+                            .map(this::buildDruidBoundFilters)
                             .collect(Collectors.toList());
-        } catch (RuntimeException e) {
+        } catch (IllegalArgumentException e) {
             if (e.getCause() instanceof FilterBuilderException) {
                 throw (FilterBuilderException) e.getCause();
             } else {
                 throw e;
             }
         }
 
+        if (filters.isEmpty()) {
+            return null;
+        }
+
         if (filters.size() == 1) {
             return filters.get(0);
         }
@@ -121,14 +136,63 @@ public Filter buildDruidBoundFilters(ApiFilter filter) {
             case lt:
                 return BoundFilter.buildUpperBoundFilter(dimension, values.get(0), false);
             case between:
-                return BoundFilter
-                        .buildUpperBoundFilter(dimension, values.get(1), false)
-                        .withLowerBound(values.get(0));
+                String lowerBound = values.get(0);
+                String upperBound = values.get(1);
+                return new BoundFilter(dimension, lowerBound, upperBound, false, true, null);
             default:
                 LOG.error(FILTER_OPERATOR_INVALID.logFormat(filterOperation.getName()));
-                throw new RuntimeException(
+                throw new IllegalArgumentException(
                         new FilterBuilderException(FILTER_OPERATOR_INVALID.format(filterOperation.getName()))
                 );
         }
     }
+
+    /**
+     * Validate the expected values for this Filter.
+     *
+     * This is useful for clean error messaging on filters as well as to clean up Filters before conversion.
+     *
+     * @param filter  the api filter being validated
+     *
+     */
+    public void validateFilter(ApiFilter filter) {
+        FilterOperation op = filter.getOperation();
+        List<String> values = filter.getValuesList();
+
+        // Verify that this is a valid operation for this builder
+        if (! (op instanceof DefaultFilterOperation)) {
+            LOG.error(FILTER_OPERATOR_INVALID.logFormat(op));
+            throw new IllegalArgumentException(
+                    new FilterBuilderException(FILTER_OPERATOR_INVALID.format(op.getName()))
+            );
+        }
+
+        // Verify that this filter uses a correct number of filters
+        if (
+                (op.getMinimumArguments().isPresent() && op.getMinimumArguments().get() < values.size()) ||
+                (op.getMaximumArguments().isPresent() && op.getMaximumArguments().get() > values.size())
+        ) {
+            String error = ErrorMessageFormat.FILTER_WRONG_NUMBER_OF_VALUES.format(
+                    op,
+                    op.expectedRangeDescription(),
+                    filter.getValues().size(),
+                    filter.getValuesList()
+            );
+            LOG.error(error);
+            throw new IllegalArgumentException(new FilterBuilderException(error));
+        }
+
+
+    }
+
+    /**
+     * An extension hook to permit subclasses to transform, remove, or make multiple versions of an apiFilter.
+     *
+     * @param filter  The filter to transform
+     *
+     * @return the transformed filter
+     */
+    public Stream<ApiFilter> normalize(ApiFilter filter) {
+        return Stream.of(filter);
+    }
 }

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/BoundFilter.java

@@ -179,4 +179,15 @@ public boolean equals(Object obj) {
                 Objects.equals(ordering, other.ordering)
         ;
     }
+
+    @Override
+    public String toString() {
+        return String.format("Lower: [%s] strict %s, Upper: [%s] strict %s, ordering: %s",
+                lower == null ? "" : lower,
+                lowerStrict == null ? "?" : lowerStrict.toString(),
+                upper == null ? "" : upper,
+                upperStrict == null ? "?" : upperStrict.toString(),
+                ordering == null ? "" : ordering.toString()
+        );
+    }
 }

fili-core/src/main/java/com/yahoo/bard/webservice/web/DefaultFilterOperation.java

@@ -2,7 +2,6 @@
 // Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms.
 package com.yahoo.bard.webservice.web;
 
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
@@ -21,16 +20,18 @@ public enum DefaultFilterOperation implements FilterOperation {
     contains,
     eq("equals"),
     lt("less"),
-    lte("notgt", "notGreater", "notGreaterThan"),
-    gt("greater"),
-    gte("notlt", "notLess", "notLessThan"),
+    lte(1, "notgt", "notGreater", "notGreaterThan"),
+    gt(1, "greater"),
+    gte(1, "notlt", "notLess", "notLessThan"),
     // The lower bound is inclusive and the upper bound is exclusive. (so it is gte and lt operators together)
-    between("bet")
+    between(2, "bet")
     ;
 
     private static final Map<String, DefaultFilterOperation> ALIASES = new HashMap<>();
 
     private final List<String> aliases;
+    private final Integer minimumArguments;
+    private final Integer maximumArguments;
 
     static {
         for (DefaultFilterOperation op : DefaultFilterOperation.values()) {
@@ -45,16 +46,41 @@ public enum DefaultFilterOperation implements FilterOperation {
      * Constructor.
      */
     DefaultFilterOperation() {
-        this.aliases = new ArrayList<>();
+        this((Integer) null, (Integer) null);
     }
 
     /**
      * Constructor.
      *
+     * Default to no minimum or maximum number of arguments.
+     *
      * @param aliases  List of aliases for the op.
      */
     DefaultFilterOperation(String... aliases) {
+        this(null, null, aliases);
+    }
+
+    /**
+     * Constructor.
+     *
+     * @param fixedArgumentSize Require exactly this many arguments
+     * @param aliases  List of aliases for the op.
+     */
+    DefaultFilterOperation(Integer fixedArgumentSize, String... aliases) {
+        this(fixedArgumentSize, fixedArgumentSize, aliases);
+    }
+
+    /**
+     * Constructor.
+     *
+     * @param minimumArguments  The minimum number of allowed arguments for this operation
+     * @param maximumArguments  The maxmimum number of allowed arguments for this operation
+     * @param aliases  List of aliases for the op.
+     */
+    DefaultFilterOperation(Integer minimumArguments, Integer maximumArguments, String... aliases) {
         this.aliases = Arrays.asList(aliases);
+        this.minimumArguments = minimumArguments;
+        this.maximumArguments = maximumArguments;
     }
 
     @Override
@@ -74,4 +100,14 @@ static public DefaultFilterOperation fromString(@NotNull String value) throws Il
         return Optional.ofNullable(ALIASES.get(value))
                 .orElseThrow(() -> new IllegalArgumentException("unknown filter operation: " + value));
     }
+
+    @Override
+    public Optional<Integer> getMinimumArguments() {
+        return Optional.ofNullable(minimumArguments);
+    }
+
+    @Override
+    public Optional<Integer> getMaximumArguments() {
+        return Optional.ofNullable(maximumArguments);
+    }
 }

fili-core/src/main/java/com/yahoo/bard/webservice/web/ErrorMessageFormat.java

@@ -83,6 +83,7 @@ public enum ErrorMessageFormat implements MessageFormatter {
     FILTER_DIMENSION_NOT_IN_TABLE("Filter dimension '%s' is not supported by the table '%s'."),
     FILTER_FIELD_NOT_IN_DIMENSIONS("Filter dimension field '%s' is not supported by the dimension '%s'."),
     FILTER_OPERATOR_INVALID("Filter operator '%s' is invalid."),
+    FILTER_WRONG_NUMBER_OF_VALUES("Filter operator '%s' expects %s argument(s).  Found %d in '%s'."),
     FILTER_SUBSTRING_OPERATIONS_DISABLED(
             "Filter operations 'startswith' and 'contains' are disabled for data requests.",
             "Filter operations 'startswith' and 'contains' are disabled for data requests. Enable by setting feature" +

fili-core/src/main/java/com/yahoo/bard/webservice/web/FilterOperation.java

@@ -2,6 +2,8 @@
 // Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms.
 package com.yahoo.bard.webservice.web;
 
+import java.util.Optional;
+
 /**
  * Fili filter operation.
  */
@@ -13,4 +15,50 @@ public interface FilterOperation {
      * @return the name
      */
     String getName();
+
+
+    /**
+     * Gets the minimum number of values for this operation.
+     *
+     * @return the minimum number of arguments, empty if unconfigured
+     */
+    default Optional<Integer> getMinimumArguments() {
+        return Optional.empty();
+    }
+
+    /**
+     * Gets the minimum number of values for this operation.
+     *
+     * @return the maximum number of arguments, empty if unconfigured
+     */
+    default Optional<Integer> getMaximumArguments() {
+        return Optional.empty();
+    }
+
+    /**
+     * A string describing the argument validation rule for this operation.
+     *
+     * Most commonly used for creating error messages about number of arguments.
+     *
+     * @return A string representing the accepted number of arguments for this operation.
+     */
+    default String expectedRangeDescription() {
+
+        if (getMinimumArguments().isPresent() && getMaximumArguments().isPresent()) {
+            if (getMinimumArguments().get() == getMaximumArguments().get()) {
+                return String.format("exactly %d arguments", getMaximumArguments().get());
+            }
+            return String.format(
+                    "between %d and %d arguments",
+                    getMinimumArguments().get(),
+                    getMinimumArguments().get()
+            );
+        } else if (getMinimumArguments().isPresent()) {
+            return String.format("as least %d arguments", getMinimumArguments().get());
+        } else if (getMaximumArguments().isPresent()) {
+            return String.format("no more than %d arguments", getMaximumArguments().get());
+        } else {
+            return "any number of arguments";
+        }
+    }
 }