Centralize doc values checking (#77089) (#77124)

This adds two utility methods for to validate the parameters to the `docValueFormat` method and replaces a pile of copy and pasted code with calls to them. They just emit a standard error message if the any unsupported parameters are provided.
elastic · Sep 1, 2021 · 4339a49 · 4339a49
1 parent aab02be
commit 4339a49
Show file tree

Hide file tree

Showing 12 changed files with 39 additions and 67 deletions.
diff --git a/...es/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java b/...es/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java
@@ -247,15 +247,11 @@ public Object valueForDisplay(Object value) {
 
         @Override
         public DocValueFormat docValueFormat(String format, ZoneId timeZone) {
-            if (timeZone != null) {
-                throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName()
-                    + "] does not support custom time zones");
-            }
+            checkNoTimeZone(timeZone);
             if (format == null) {
                 return DocValueFormat.RAW;
-            } else {
-                return new DocValueFormat.Decimal(format);
             }
+            return new DocValueFormat.Decimal(format);
         }
 
         /**

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java
@@ -215,13 +215,8 @@ public IndexFieldData.Builder fielddataBuilder(String fullyQualifiedIndexName, S
 
         @Override
         public DocValueFormat docValueFormat(@Nullable String format, ZoneId timeZone) {
-            if (format != null) {
-                throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] does not support custom formats");
-            }
-            if (timeZone != null) {
-                throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName()
-                    + "] does not support custom time zones");
-            }
+            checkNoFormat(format);
+            checkNoTimeZone(timeZone);
             return DocValueFormat.BOOLEAN;
         }
 

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BooleanScriptFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/BooleanScriptFieldType.java
@@ -95,12 +95,8 @@ public Object valueForDisplay(Object value) {
 
     @Override
     public DocValueFormat docValueFormat(String format, ZoneId timeZone) {
-        if (format != null) {
-            throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] does not support custom formats");
-        }
-        if (timeZone != null) {
-            throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] does not support custom time zones");
-        }
+        checkNoFormat(format);
+        checkNoTimeZone(timeZone);
         return DocValueFormat.BOOLEAN;
     }
 

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DoubleScriptFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/DoubleScriptFieldType.java
@@ -86,9 +86,7 @@ public Object valueForDisplay(Object value) {
 
     @Override
     public DocValueFormat docValueFormat(String format, ZoneId timeZone) {
-        if (timeZone != null) {
-            throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] does not support custom time zones");
-        }
+        checkNoTimeZone(timeZone);
         if (format == null) {
             return DocValueFormat.RAW;
         }

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java
@@ -357,13 +357,8 @@ public Object valueForDisplay(Object value) {
 
         @Override
         public DocValueFormat docValueFormat(@Nullable String format, ZoneId timeZone) {
-            if (format != null) {
-                throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] does not support custom formats");
-            }
-            if (timeZone != null) {
-                throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName()
-                    + "] does not support custom time zones");
-            }
+            checkNoFormat(format);
+            checkNoTimeZone(timeZone);
             return DocValueFormat.IP;
         }
     }

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IpScriptFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/IpScriptFieldType.java
@@ -21,8 +21,8 @@
 import org.elasticsearch.core.Tuple;
 import org.elasticsearch.index.fielddata.IpScriptFieldData;
 import org.elasticsearch.index.query.SearchExecutionContext;
-import org.elasticsearch.script.IpFieldScript;
 import org.elasticsearch.script.CompositeFieldScript;
+import org.elasticsearch.script.IpFieldScript;
 import org.elasticsearch.script.Script;
 import org.elasticsearch.search.DocValueFormat;
 import org.elasticsearch.search.lookup.SearchLookup;
@@ -36,7 +36,6 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
-import java.util.Locale;
 import java.util.Map;
 import java.util.function.Function;
 import java.util.function.Supplier;
@@ -88,14 +87,8 @@ public Object valueForDisplay(Object value) {
 
     @Override
     public DocValueFormat docValueFormat(String format, ZoneId timeZone) {
-        if (format != null) {
-            String message = "Runtime field [%s] of type [%s] does not support custom formats";
-            throw new IllegalArgumentException(String.format(Locale.ROOT, message, name(), typeName()));
-        }
-        if (timeZone != null) {
-            String message = "Runtime field [%s] of type [%s] does not support custom time zones";
-            throw new IllegalArgumentException(String.format(Locale.ROOT, message, name(), typeName()));
-        }
+        checkNoFormat(format);
+        checkNoTimeZone(timeZone);
         return DocValueFormat.IP;
     }
 

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/LongScriptFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/LongScriptFieldType.java
@@ -83,9 +83,7 @@ public Object valueForDisplay(Object value) {
 
     @Override
     public DocValueFormat docValueFormat(String format, ZoneId timeZone) {
-        if (timeZone != null) {
-            throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] does not support custom time zones");
-        }
+        checkNoTimeZone(timeZone);
         if (format == null) {
             return DocValueFormat.RAW;
         }

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java
@@ -377,17 +377,32 @@ public boolean eagerGlobalOrdinals() {
         return false;
     }
 
-    /** Return a {@link DocValueFormat} that can be used to display and parse
-     *  values as returned by the fielddata API.
-     *  The default implementation returns a {@link DocValueFormat#RAW}. */
+    /**
+     * Pick a {@link DocValueFormat} that can be used to display and parse
+     * values of fields of this type.
+     */
     public DocValueFormat docValueFormat(@Nullable String format, ZoneId timeZone) {
+        checkNoFormat(format);
+        checkNoTimeZone(timeZone);
+        return DocValueFormat.RAW;
+    }
+
+    /**
+     * Validate the provided {@code format} is null.
+     */
+    protected void checkNoFormat(@Nullable String format) {
         if (format != null) {
             throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] does not support custom formats");
         }
+    }
+
+    /**
+     * Validate the provided {@code timeZone} is null.
+     */
+    protected void checkNoTimeZone(@Nullable ZoneId timeZone) {
         if (timeZone != null) {
             throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] does not support custom time zones");
         }
-        return DocValueFormat.RAW;
     }
 
     /**

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
@@ -1048,15 +1048,11 @@ protected Object parseSourceValue(Object value) {
 
         @Override
         public DocValueFormat docValueFormat(String format, ZoneId timeZone) {
-            if (timeZone != null) {
-                throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName()
-                    + "] does not support custom time zones");
-            }
+            checkNoTimeZone(timeZone);
             if (format == null) {
                 return DocValueFormat.RAW;
-            } else {
-                return new DocValueFormat.Decimal(format);
             }
+            return new DocValueFormat.Decimal(format);
         }
 
         public Number parsePoint(byte[] value) {

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/IpScriptFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/IpScriptFieldTypeTests.java
@@ -54,9 +54,9 @@ public class IpScriptFieldTypeTests extends AbstractScriptFieldTypeTestCase {
     public void testFormat() throws IOException {
         assertThat(simpleMappedFieldType().docValueFormat(null, null), sameInstance(DocValueFormat.IP));
         Exception e = expectThrows(IllegalArgumentException.class, () -> simpleMappedFieldType().docValueFormat("ASDFA", null));
-        assertThat(e.getMessage(), equalTo("Runtime field [test] of type [ip] does not support custom formats"));
+        assertThat(e.getMessage(), equalTo("Field [test] of type [ip] does not support custom formats"));
         e = expectThrows(IllegalArgumentException.class, () -> simpleMappedFieldType().docValueFormat(null, ZoneId.of("America/New_York")));
-        assertThat(e.getMessage(), equalTo("Runtime field [test] of type [ip] does not support custom time zones"));
+        assertThat(e.getMessage(), equalTo("Field [test] of type [ip] does not support custom time zones"));
     }
 
     @Override

diff --git a/...gned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongFieldMapper.java b/...gned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongFieldMapper.java
@@ -270,11 +270,7 @@ public Object valueForDisplay(Object value) {
 
         @Override
         public DocValueFormat docValueFormat(String format, ZoneId timeZone) {
-            if (timeZone != null) {
-                throw new IllegalArgumentException(
-                    "Field [" + name() + "] of type [" + typeName() + "] does not support custom time zones"
-                );
-            }
+            checkNoTimeZone(timeZone);
             return DocValueFormat.UNSIGNED_LONG_SHIFTED;
         }
 

diff --git a/...-version/src/main/java/org/elasticsearch/xpack/versionfield/VersionStringFieldMapper.java b/...-version/src/main/java/org/elasticsearch/xpack/versionfield/VersionStringFieldMapper.java
@@ -292,14 +292,8 @@ public Object valueForDisplay(Object value) {
 
         @Override
         public DocValueFormat docValueFormat(@Nullable String format, ZoneId timeZone) {
-            if (format != null) {
-                throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] does not support custom formats");
-            }
-            if (timeZone != null) {
-                throw new IllegalArgumentException(
-                    "Field [" + name() + "] of type [" + typeName() + "] does not support custom time zones"
-                );
-            }
+            checkNoFormat(format);
+            checkNoTimeZone(timeZone);
             return VERSION_DOCVALUE;
         }