Make Fuzziness reject illegal values earlier (elastic#33511)

The current java implementation of Fuzziness leaves a lot of room for
initializing it with illegal values that either cause errors later when the
queries reach the shards where they are executed or values that are silently
ignored in favour of defaults. We should instead tighten the java implementation
of the class so that we only accept supported values. Currently those are
numeric values representing the edit distances 0, 1 and 2, optionally also as
float or string, and the "AUTO" fuzziness, which can come in a cusomizable
variant that allows specifying two value that define the positions in a term
where the AUTO option increases the allowed edit distance.

This change removes several redundant ways of object construction and adds input
validation to the remaining ones. Java users should either use one of the
predefined constants or use the static factory methods `fromEdits(int)` or
`fromString(String)` to create instances of the class, while other ctors are
hidden. This allows for instance control, e.g. returning one of the constants
when creating instances from an integer value.

Previously the class would accept any positive integer value and any float
value, while in effect the maximum allowed edit distance was capped at 2 in
practice. These values while throw an error now, as will any other String value
other than "AUTO" that where previously accepted but led to numeric exceptions
when the query was executed.

Author: Christoph Büscher

docs/reference/migration/migrate_8_0.asciidoc

@@ -16,6 +16,7 @@ coming[8.0.0]
 * <<breaking_80_mappings_changes>>
 * <<breaking_80_snapshots_changes>>
 * <<breaking_80_security_changes>>
+* <<breaking_80_java_changes>>
 
 //NOTE: The notable-breaking-changes tagged regions are re-used in the
 //Installation and Upgrade Guide
@@ -43,3 +44,4 @@ include::migrate_8_0/discovery.asciidoc[]
 include::migrate_8_0/mappings.asciidoc[]
 include::migrate_8_0/snapshots.asciidoc[]
 include::migrate_8_0/security.asciidoc[]
+include::migrate_8_0/java.asciidoc[]

docs/reference/migration/migrate_8_0/java.asciidoc

@@ -0,0 +1,20 @@
+[float]
+[[breaking_80_java_changes]]
+=== Java API changes
+
+[float]
+==== Changes to Fuzziness
+
+To create `Fuzziness` instances, use the `fromString` and `fromEdits` method
+instead of the `build` method that used to accept both Strings and numeric
+values. Several fuzziness setters on query builders (e.g.
+MatchQueryBuilder#fuzziness) now accept only a `Fuzziness`instance instead of
+an Object. You should preferably use the available constants (e.g.
+Fuzziness.ONE, Fuzziness.AUTO) or build your own instance using the above
+mentioned factory methods.
+
+Fuzziness used to be lenient when it comes to parsing arbitrary numeric values
+while silently truncating them to one of the three allowed edit distances 0, 1
+or 2. This leniency is now removed and the class will throw errors when trying
+to construct an instance with another value (e.g. floats like 1.3 used to get
+accepted but truncated to 1). You should use one of the allowed values.

server/src/main/java/org/elasticsearch/common/unit/Fuzziness.java

@@ -20,6 +20,7 @@
 
 import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.ParseField;
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.io.stream.Writeable;
@@ -38,41 +39,73 @@
  */
 public final class Fuzziness implements ToXContentFragment, Writeable {
 
-    public static final String X_FIELD_NAME = "fuzziness";
-    public static final Fuzziness ZERO = new Fuzziness(0);
-    public static final Fuzziness ONE = new Fuzziness(1);
-    public static final Fuzziness TWO = new Fuzziness(2);
-    public static final Fuzziness AUTO = new Fuzziness("AUTO");
+    static final String X_FIELD_NAME = "fuzziness";
     public static final ParseField FIELD = new ParseField(X_FIELD_NAME);
-    private static final int DEFAULT_LOW_DISTANCE = 3;
-    private static final int DEFAULT_HIGH_DISTANCE = 6;
+
+    public static final Fuzziness ZERO = new Fuzziness("0");
+    public static final Fuzziness ONE = new Fuzziness("1");
+    public static final Fuzziness TWO = new Fuzziness("2");
+    public static final Fuzziness AUTO = new Fuzziness("AUTO");
+
+    static final int DEFAULT_LOW_DISTANCE = 3;
+    static final int DEFAULT_HIGH_DISTANCE = 6;
 
     private final String fuzziness;
     private int lowDistance = DEFAULT_LOW_DISTANCE;
     private int highDistance = DEFAULT_HIGH_DISTANCE;
 
-    private Fuzziness(int fuzziness) {
-        if (fuzziness != 0 && fuzziness != 1 && fuzziness != 2) {
-            throw new IllegalArgumentException("Valid edit distances are [0, 1, 2] but was [" + fuzziness + "]");
-        }
-        this.fuzziness = Integer.toString(fuzziness);
+    private Fuzziness(String fuzziness) {
+        this.fuzziness = fuzziness;
     }
 
-    private Fuzziness(String fuzziness) {
-        if (fuzziness == null || fuzziness.isEmpty()) {
-            throw new IllegalArgumentException("fuzziness can't be null!");
+    /**
+     * Creates a {@link Fuzziness} instance from an edit distance. The value must be one of {@code [0, 1, 2]}
+     * Note: Using this method only makes sense if the field you are applying Fuzziness to is some sort of string.
+     * @throws IllegalArgumentException if the edit distance is not in [0, 1, 2]
+     */
+    public static Fuzziness fromEdits(int edits) {
+        switch (edits) {
+        case 0: return Fuzziness.ZERO;
+        case 1: return Fuzziness.ONE;
+        case 2: return Fuzziness.TWO;
+        default:
+            throw new IllegalArgumentException("Valid edit distances are [0, 1, 2] but was [" + edits + "]");
         }
-        this.fuzziness = fuzziness.toUpperCase(Locale.ROOT);
     }
 
-    private Fuzziness(String fuzziness, int lowDistance, int highDistance) {
-        this(fuzziness);
-        if (lowDistance < 0 || highDistance < 0 || lowDistance > highDistance) {
-            throw new IllegalArgumentException("fuzziness wrongly configured, must be: lowDistance > 0, highDistance" +
-                " > 0 and lowDistance <= highDistance ");
+    /**
+     * Creates a {@link Fuzziness} instance from a String representation. This can
+     * either be an edit distance where the value must be one of
+     * {@code ["0", "1", "2"]} or "AUTO" for a fuzziness that depends on the term
+     * length. Using the "AUTO" fuzziness, you can optionally supply low and high
+     * distance arguments in the format {@code "AUTO:[low],[high]"}. See the query
+     * DSL documentation for more information about how these values affect the
+     * fuzziness value.
+     * Note: Using this method only makes sense if the field you
+     * are applying Fuzziness to is some sort of string.
+     */
+    public static Fuzziness fromString(String fuzzinessString) {
+        if (Strings.isEmpty(fuzzinessString)) {
+            throw new IllegalArgumentException("fuzziness cannot be null or empty.");
+        }
+        String upperCase = fuzzinessString.toUpperCase(Locale.ROOT);
+        // check if it is one of the "AUTO" variants
+        if (upperCase.equals("AUTO")) {
+            return Fuzziness.AUTO;
+        } else if (upperCase.startsWith("AUTO:")) {
+            return parseCustomAuto(upperCase);
+        } else {
+            // should be a float or int representing a valid edit distance, otherwise throw error
+            try {
+                float parsedFloat = Float.parseFloat(upperCase);
+                if (parsedFloat % 1 > 0) {
+                    throw new IllegalArgumentException("fuzziness needs to be one of 0.0, 1.0 or 2.0 but was " + parsedFloat);
+                }
+                return fromEdits((int) parsedFloat);
+            } catch (NumberFormatException e) {
+                throw new IllegalArgumentException("fuzziness cannot be [" + fuzzinessString + "].", e);
+            }
         }
-        this.lowDistance = lowDistance;
-        this.highDistance = highDistance;
     }
 
     /**
@@ -101,39 +134,23 @@ public void writeTo(StreamOutput out) throws IOException {
         }
     }
 
-    /**
-     * Creates a {@link Fuzziness} instance from an edit distance. The value must be one of {@code [0, 1, 2]}
-     *
-     * Note: Using this method only makes sense if the field you are applying Fuzziness to is some sort of string.
-     */
-    public static Fuzziness fromEdits(int edits) {
-        return new Fuzziness(edits);
-    }
-
-    public static Fuzziness build(Object fuzziness) {
-        if (fuzziness instanceof Fuzziness) {
-            return (Fuzziness) fuzziness;
-        }
-        String string = fuzziness.toString();
-        if (AUTO.asString().equalsIgnoreCase(string)) {
-            return AUTO;
-        } else if (string.toUpperCase(Locale.ROOT).startsWith(AUTO.asString() + ":")) {
-            return parseCustomAuto(string);
-        }
-        return new Fuzziness(string);
-    }
-
-    private static Fuzziness parseCustomAuto( final String string) {
-        assert string.toUpperCase(Locale.ROOT).startsWith(AUTO.asString() + ":");
-        String[] fuzzinessLimit = string.substring(AUTO.asString().length() + 1).split(",");
+    private static Fuzziness parseCustomAuto(final String fuzzinessString) {
+        assert fuzzinessString.toUpperCase(Locale.ROOT).startsWith(AUTO.asString() + ":");
+        String[] fuzzinessLimit = fuzzinessString.substring(AUTO.asString().length() + 1).split(",");
         if (fuzzinessLimit.length == 2) {
             try {
                 int lowerLimit = Integer.parseInt(fuzzinessLimit[0]);
                 int highLimit = Integer.parseInt(fuzzinessLimit[1]);
-                return new Fuzziness("AUTO", lowerLimit, highLimit);
+                if (lowerLimit < 0 || highLimit < 0 || lowerLimit > highLimit) {
+                    throw new ElasticsearchParseException("fuzziness wrongly configured [{}]. Must be 0 < lower value <= higher value.",
+                            fuzzinessString);
+                }
+                Fuzziness fuzziness = new Fuzziness("AUTO");
+                fuzziness.lowDistance = lowerLimit;
+                fuzziness.highDistance = highLimit;
+                return fuzziness;
             } catch (NumberFormatException e) {
-                throw new ElasticsearchParseException("failed to parse [{}] as a \"auto:int,int\"", e,
-                    string);
+                throw new ElasticsearchParseException("failed to parse [{}] as a \"auto:int,int\"", e, fuzzinessString);
             }
         } else {
             throw new ElasticsearchParseException("failed to find low and high distance values");
@@ -144,29 +161,9 @@ public static Fuzziness parse(XContentParser parser) throws IOException {
         XContentParser.Token token = parser.currentToken();
         switch (token) {
             case VALUE_STRING:
+                return fromString(parser.text());
             case VALUE_NUMBER:
-                final String fuzziness = parser.text();
-                if (AUTO.asString().equalsIgnoreCase(fuzziness)) {
-                    return AUTO;
-                } else if (fuzziness.toUpperCase(Locale.ROOT).startsWith(AUTO.asString() + ":")) {
-                    return parseCustomAuto(fuzziness);
-                }
-                try {
-                    final int minimumSimilarity = Integer.parseInt(fuzziness);
-                    switch (minimumSimilarity) {
-                        case 0:
-                            return ZERO;
-                        case 1:
-                            return ONE;
-                        case 2:
-                            return TWO;
-                        default:
-                            return build(fuzziness);
-                    }
-                } catch (NumberFormatException ex) {
-                    return build(fuzziness);
-                }
-
+                return fromEdits(parser.intValue());
             default:
                 throw new IllegalArgumentException("Can't parse fuzziness on token: [" + token + "]");
         }
@@ -200,7 +197,7 @@ public float asFloat() {
         if (this.equals(AUTO) || isAutoWithCustomValues()) {
             return 1f;
         }
-        return Float.parseFloat(fuzziness.toString());
+        return Float.parseFloat(fuzziness);
     }
 
     private int termLen(String text) {
@@ -209,13 +206,13 @@ private int termLen(String text) {
 
     public String asString() {
         if (isAutoWithCustomValues()) {
-            return fuzziness.toString() + ":" + lowDistance + "," + highDistance;
+            return fuzziness + ":" + lowDistance + "," + highDistance;
         }
-        return fuzziness.toString();
+        return fuzziness;
     }
 
     private boolean isAutoWithCustomValues() {
-        return fuzziness.startsWith("AUTO") && (lowDistance != DEFAULT_LOW_DISTANCE ||
+        return fuzziness.equals("AUTO") && (lowDistance != DEFAULT_LOW_DISTANCE ||
             highDistance != DEFAULT_HIGH_DISTANCE);
     }
 

server/src/main/java/org/elasticsearch/index/query/MatchBoolPrefixQueryBuilder.java

@@ -161,8 +161,8 @@ public String minimumShouldMatch() {
     }
 
     /** Sets the fuzziness used when evaluated to a fuzzy query type. Defaults to "AUTO". */
-    public MatchBoolPrefixQueryBuilder fuzziness(Object fuzziness) {
-        this.fuzziness = Fuzziness.build(fuzziness);
+    public MatchBoolPrefixQueryBuilder fuzziness(Fuzziness fuzziness) {
+        this.fuzziness = fuzziness;
         return this;
     }
 

server/src/main/java/org/elasticsearch/index/query/MatchQueryBuilder.java

@@ -183,8 +183,8 @@ public String analyzer() {
     }
 
     /** Sets the fuzziness used when evaluated to a fuzzy query type. Defaults to "AUTO". */
-    public MatchQueryBuilder fuzziness(Object fuzziness) {
-        this.fuzziness = Fuzziness.build(fuzziness);
+    public MatchQueryBuilder fuzziness(Fuzziness fuzziness) {
+        this.fuzziness = Objects.requireNonNull(fuzziness);
         return this;
     }
 

server/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java

@@ -376,10 +376,8 @@ public int slop() {
     /**
      * Sets the fuzziness used when evaluated to a fuzzy query type. Defaults to "AUTO".
      */
-    public MultiMatchQueryBuilder fuzziness(Object fuzziness) {
-        if (fuzziness != null) {
-            this.fuzziness = Fuzziness.build(fuzziness);
-        }
+    public MultiMatchQueryBuilder fuzziness(Fuzziness fuzziness) {
+        this.fuzziness = Objects.requireNonNull(fuzziness);
         return this;
     }
 
@@ -707,7 +705,6 @@ public static MultiMatchQueryBuilder fromXContent(XContentParser parser) throws
                 .type(type)
                 .analyzer(analyzer)
                 .cutoffFrequency(cutoffFrequency)
-                .fuzziness(fuzziness)
                 .fuzzyRewrite(fuzzyRewrite)
                 .maxExpansions(maxExpansions)
                 .minimumShouldMatch(minimumShouldMatch)
@@ -723,6 +720,9 @@ public static MultiMatchQueryBuilder fromXContent(XContentParser parser) throws
         if (lenient != null) {
             builder.lenient(lenient);
         }
+        if (fuzziness != null) {
+            builder.fuzziness(fuzziness);
+        }
         return builder;
     }
 

server/src/main/java/org/elasticsearch/index/search/QueryStringQueryParser.java

@@ -423,7 +423,7 @@ protected Query handleBareFuzzy(String field, Token fuzzySlop, String termImage)
         if (fuzzySlop.image.length() == 1) {
             return getFuzzyQuery(field, termImage, fuzziness.asDistance(termImage));
         }
-        return getFuzzyQuery(field, termImage, Fuzziness.build(fuzzySlop.image.substring(1)).asFloat());
+        return getFuzzyQuery(field, termImage, Fuzziness.fromString(fuzzySlop.image.substring(1)).asFloat());
     }
 
     @Override
@@ -434,7 +434,7 @@ protected Query getFuzzyQuery(String field, String termStr, float minSimilarity)
         }
         List<Query> queries = new ArrayList<>();
         for (Map.Entry<String, Float> entry : fields.entrySet()) {
-            Query q = getFuzzyQuerySingle(entry.getKey(), termStr, minSimilarity);
+            Query q = getFuzzyQuerySingle(entry.getKey(), termStr, (int) minSimilarity);
             assert q != null;
             queries.add(applyBoost(q, entry.getValue()));
         }
@@ -447,15 +447,15 @@ protected Query getFuzzyQuery(String field, String termStr, float minSimilarity)
         }
     }
 
-    private Query getFuzzyQuerySingle(String field, String termStr, float minSimilarity) throws ParseException {
+    private Query getFuzzyQuerySingle(String field, String termStr, int minSimilarity) throws ParseException {
         currentFieldType = context.fieldMapper(field);
         if (currentFieldType == null) {
             return newUnmappedFieldQuery(field);
         }
         try {
             Analyzer normalizer = forceAnalyzer == null ? queryBuilder.context.getSearchAnalyzer(currentFieldType) : forceAnalyzer;
             BytesRef term = termStr == null ? null : normalizer.normalize(field, termStr);
-            return currentFieldType.fuzzyQuery(term, Fuzziness.fromEdits((int) minSimilarity),
+            return currentFieldType.fuzzyQuery(term, Fuzziness.fromEdits(minSimilarity),
                 getFuzzyPrefixLength(), fuzzyMaxExpansions, fuzzyTranspositions);
         } catch (RuntimeException e) {
             if (lenient) {
@@ -467,7 +467,7 @@ private Query getFuzzyQuerySingle(String field, String termStr, float minSimilar
 
     @Override
     protected Query newFuzzyQuery(Term term, float minimumSimilarity, int prefixLength) {
-        int numEdits = Fuzziness.build(minimumSimilarity).asDistance(term.text());
+        int numEdits = Fuzziness.fromEdits((int) minimumSimilarity).asDistance(term.text());
         FuzzyQuery query = new FuzzyQuery(term, numEdits, prefixLength,
             fuzzyMaxExpansions, fuzzyTranspositions);
         QueryParsers.setRewriteMethod(query, fuzzyRewriteMethod);