Use Explicit<> rather than Optional<> for Subobjects in ObjectMapper (#135014)

Using `Optional<>` here gets confusing when trying to work out what the
default value is, and storing `Optional<>` as fields or using them as
method parameters always feels wrong.

Author: romseygeek

server/src/main/java/org/elasticsearch/common/Explicit.java

@@ -35,6 +35,20 @@ public static Explicit<Boolean> explicitBoolean(boolean value) {
         return value ? EXPLICIT_TRUE : EXPLICIT_FALSE;
     }
 
+    /**
+     * Create an explicitly set value
+     */
+    public static <T> Explicit<T> of(T value) {
+        return new Explicit<>(value, true);
+    }
+
+    /**
+     * Create an implicitly set value
+     */
+    public static <T> Explicit<T> implicit(T value) {
+        return new Explicit<>(value, false);
+    }
+
     /**
      * Create a value with an indication if this was an explicit choice
      * @param value a setting value

server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java

@@ -1048,7 +1048,7 @@ protected SyntheticSourceSupport syntheticSourceSupport() {
 
     private static class NoOpObjectMapper extends ObjectMapper {
         NoOpObjectMapper(String name, String fullPath) {
-            super(name, fullPath, Explicit.IMPLICIT_TRUE, Optional.empty(), Optional.empty(), Dynamic.RUNTIME, Collections.emptyMap());
+            super(name, fullPath, Explicit.IMPLICIT_TRUE, Defaults.SUBOBJECTS, Optional.empty(), Dynamic.RUNTIME, Collections.emptyMap());
         }
 
         @Override

server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java

@@ -60,7 +60,7 @@ public Builder(
             Function<Query, BitSetProducer> bitSetProducer,
             IndexSettings indexSettings
         ) {
-            super(name, Optional.empty());
+            super(name, Defaults.SUBOBJECTS);
             this.indexCreatedVersion = indexCreatedVersion;
             this.bitSetProducer = bitSetProducer;
             this.indexSettings = indexSettings;
@@ -145,7 +145,7 @@ public static class TypeParser extends ObjectMapper.TypeParser {
         @Override
         public Mapper.Builder parse(String name, Map<String, Object> node, MappingParserContext parserContext)
             throws MapperParsingException {
-            if (parseSubobjects(node).isPresent()) {
+            if (parseSubobjects(node).explicit()) {
                 throw new MapperParsingException("Nested type [" + name + "] does not support [subobjects] parameter");
             }
             NestedObjectMapper.Builder builder = new NestedObjectMapper.Builder(
@@ -236,7 +236,7 @@ public MapperBuilderContext createChildContext(String name, Dynamic dynamic) {
         Function<Query, BitSetProducer> bitsetProducer,
         IndexSettings indexSettings
     ) {
-        super(name, fullPath, enabled, Optional.empty(), sourceKeepMode, dynamic, mappers);
+        super(name, fullPath, enabled, Defaults.SUBOBJECTS, sourceKeepMode, dynamic, mappers);
         this.parentTypeFilter = parentTypeFilter;
         this.nestedTypePath = nestedTypePath;
         this.nestedTypeFilter = nestedTypeFilter;

server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java

@@ -96,7 +96,7 @@ public String toString() {
 
     public static class Defaults {
         public static final boolean ENABLED = true;
-        public static final Optional<Subobjects> SUBOBJECTS = Optional.empty();
+        public static final Explicit<Subobjects> SUBOBJECTS = Explicit.implicit(Subobjects.ENABLED);
         public static final Explicit<Boolean> STORE_ARRAY_SOURCE = Explicit.IMPLICIT_FALSE;
         public static final Dynamic DYNAMIC = Dynamic.TRUE;
     }
@@ -133,13 +133,17 @@ static Dynamic getRootDynamic(MappingLookup mappingLookup) {
     }
 
     public static class Builder extends Mapper.Builder {
-        protected Optional<Subobjects> subobjects;
+        protected Explicit<Subobjects> subobjects;
         protected Explicit<Boolean> enabled = Explicit.IMPLICIT_TRUE;
         protected Optional<SourceKeepMode> sourceKeepMode = Optional.empty();
         protected Dynamic dynamic;
         protected final List<Mapper.Builder> mappersBuilders = new ArrayList<>();
 
-        public Builder(String name, Optional<Subobjects> subobjects) {
+        public Builder(String name) {
+            this(name, Defaults.SUBOBJECTS);
+        }
+
+        public Builder(String name, Explicit<Subobjects> subobjects) {
             super(name);
             this.subobjects = subobjects;
         }
@@ -184,7 +188,7 @@ public Mapper build(MapperBuilderContext context) {
         public final void addDynamic(String name, String prefix, Mapper mapper, DocumentParserContext context) {
             // If the mapper to add has no dots, or the current object mapper has subobjects set to false,
             // we just add it as it is for sure a leaf mapper
-            if (name.contains(".") == false || (subobjects.isPresent() && (subobjects.get() == Subobjects.DISABLED))) {
+            if (name.contains(".") == false || subobjects.value() == Subobjects.DISABLED) {
                 add(name, mapper);
             } else {
                 // We strip off the first object path of the mapper name, load or create
@@ -198,7 +202,7 @@ public final void addDynamic(String name, String prefix, Mapper mapper, Document
                 if (parentBuilder != null) {
                     parentBuilder.addDynamic(name.substring(firstDotIndex + 1), immediateChildFullName, mapper, context);
                     add(parentBuilder);
-                } else if (subobjects.isPresent() && subobjects.get() == Subobjects.AUTO) {
+                } else if (subobjects.value() == Subobjects.AUTO) {
                     // No matching parent object was found, the mapper is added as a leaf - similar to subobjects false.
                     add(name, mapper);
                 } else {
@@ -236,7 +240,7 @@ protected final Map<String, Mapper> buildMappers(MapperBuilderContext mapperBuil
                     // mix of object notation and dot notation.
                     mapper = existing.merge(mapper, MapperMergeContext.from(mapperBuilderContext, Long.MAX_VALUE));
                 }
-                if (subobjects.isPresent() && subobjects.get() == Subobjects.DISABLED && mapper instanceof ObjectMapper objectMapper) {
+                if (subobjects.value() == Subobjects.DISABLED && mapper instanceof ObjectMapper objectMapper) {
                     // We're parsing a mapping that has set `subobjects: false` but has defined sub-objects
                     objectMapper.asFlattenedFieldMappers(mapperBuilderContext).forEach(m -> mappers.put(m.leafName(), m));
                 } else {
@@ -279,7 +283,7 @@ public boolean supportsVersion(IndexVersion indexCreatedVersion) {
         public Mapper.Builder parse(String name, Map<String, Object> node, MappingParserContext parserContext)
             throws MapperParsingException {
             parserContext.incrementMappingObjectDepth(); // throws MapperParsingException if depth limit is exceeded
-            Optional<Subobjects> subobjects = parseSubobjects(node);
+            Explicit<Subobjects> subobjects = parseSubobjects(node);
             Builder builder = new Builder(name, subobjects);
             parseObjectFields(node, parserContext, builder);
             parserContext.decrementMappingObjectDepth();
@@ -344,10 +348,10 @@ protected static boolean parseObjectOrDocumentTypeProperties(
             return false;
         }
 
-        protected static Optional<Subobjects> parseSubobjects(Map<String, Object> node) {
+        protected static Explicit<Subobjects> parseSubobjects(Map<String, Object> node) {
             Object subobjectsNode = node.remove("subobjects");
             if (subobjectsNode != null) {
-                return Optional.of(Subobjects.from(subobjectsNode));
+                return Explicit.of(Subobjects.from(subobjectsNode));
             }
             return Defaults.SUBOBJECTS;
         }
@@ -384,9 +388,7 @@ protected static void parseProperties(Builder objBuilder, Map<String, Object> pr
                         }
                     }
 
-                    if (objBuilder.subobjects.isPresent()
-                        && objBuilder.subobjects.get() == Subobjects.DISABLED
-                        && type.equals(NestedObjectMapper.CONTENT_TYPE)) {
+                    if (objBuilder.subobjects.value() == Subobjects.DISABLED && type.equals(NestedObjectMapper.CONTENT_TYPE)) {
                         throw new MapperParsingException(
                             "Tried to add nested object ["
                                 + fieldName
@@ -408,7 +410,7 @@ protected static void parseProperties(Builder objBuilder, Map<String, Object> pr
                         );
                     }
                     Mapper.Builder fieldBuilder;
-                    if (objBuilder.subobjects.isPresent() && objBuilder.subobjects.get() != Subobjects.ENABLED) {
+                    if (objBuilder.subobjects.value() != Subobjects.ENABLED) {
                         fieldBuilder = typeParser.parse(fieldName, propNode, parserContext);
                     } else {
                         String[] fieldNameParts = fieldName.split("\\.");
@@ -456,7 +458,7 @@ private static void validateFieldName(String fieldName, IndexVersion indexCreate
     private final String fullPath;
 
     protected final Explicit<Boolean> enabled;
-    protected final Optional<Subobjects> subobjects;
+    protected final Explicit<Subobjects> subobjects;
     protected final Optional<SourceKeepMode> sourceKeepMode;
     protected final Dynamic dynamic;
 
@@ -466,7 +468,7 @@ private static void validateFieldName(String fieldName, IndexVersion indexCreate
         String name,
         String fullPath,
         Explicit<Boolean> enabled,
-        Optional<Subobjects> subobjects,
+        Explicit<Subobjects> subobjects,
         Optional<SourceKeepMode> sourceKeepMode,
         Dynamic dynamic,
         Map<String, Mapper> mappers
@@ -484,9 +486,7 @@ private static void validateFieldName(String fieldName, IndexVersion indexCreate
         } else {
             this.mappers = Map.copyOf(mappers);
         }
-        assert subobjects.isEmpty()
-            || subobjects.get() != Subobjects.DISABLED
-            || this.mappers.values().stream().noneMatch(m -> m instanceof ObjectMapper)
+        assert subobjects.value() != Subobjects.DISABLED || this.mappers.values().stream().noneMatch(m -> m instanceof ObjectMapper)
             : "When subobjects is false, mappers must not contain an ObjectMapper";
     }
 
@@ -540,7 +540,7 @@ public final Dynamic dynamic() {
     }
 
     public final Subobjects subobjects() {
-        return subobjects.orElse(Subobjects.ENABLED);
+        return subobjects.value();
     }
 
     public final Optional<SourceKeepMode> sourceKeepMode() {
@@ -581,7 +581,7 @@ public ObjectMapper merge(Mapper mergeWith, MapperMergeContext parentMergeContex
 
     protected record MergeResult(
         Explicit<Boolean> enabled,
-        Optional<Subobjects> subObjects,
+        Explicit<Subobjects> subObjects,
         Optional<SourceKeepMode> sourceKeepMode,
         Dynamic dynamic,
         Map<String, Mapper> mappers
@@ -602,8 +602,8 @@ static MergeResult build(ObjectMapper existing, ObjectMapper mergeWithObject, Ma
             } else {
                 enabled = existing.enabled;
             }
-            final Optional<Subobjects> subObjects;
-            if (mergeWithObject.subobjects.isPresent()) {
+            final Explicit<Subobjects> subObjects;
+            if (mergeWithObject.subobjects.explicit()) {
                 if (reason == MergeReason.INDEX_TEMPLATE) {
                     subObjects = mergeWithObject.subobjects;
                 } else if (existing.subobjects() != mergeWithObject.subobjects()) {
@@ -650,13 +650,11 @@ private static Map<String, Mapper> buildMergedMappers(
             ObjectMapper existing,
             ObjectMapper mergeWithObject,
             MapperMergeContext objectMergeContext,
-            Optional<Subobjects> subobjects
+            Explicit<Subobjects> subobjects
         ) {
             Map<String, Mapper> mergedMappers = new HashMap<>();
             for (Mapper childOfExistingMapper : existing.mappers.values()) {
-                if (subobjects.isPresent()
-                    && subobjects.get() == Subobjects.DISABLED
-                    && childOfExistingMapper instanceof ObjectMapper objectMapper) {
+                if (subobjects.value() == Subobjects.DISABLED && childOfExistingMapper instanceof ObjectMapper objectMapper) {
                     // An existing mapping with sub-objects is merged with a mapping that has set `subobjects: false`
                     objectMapper.asFlattenedFieldMappers(objectMergeContext.getMapperBuilderContext())
                         .forEach(m -> mergedMappers.put(m.leafName(), m));
@@ -667,9 +665,7 @@ private static Map<String, Mapper> buildMergedMappers(
             for (Mapper mergeWithMapper : mergeWithObject) {
                 Mapper mergeIntoMapper = mergedMappers.get(mergeWithMapper.leafName());
                 if (mergeIntoMapper == null) {
-                    if (subobjects.isPresent()
-                        && subobjects.get() == Subobjects.DISABLED
-                        && mergeWithMapper instanceof ObjectMapper objectMapper) {
+                    if (subobjects.value() == Subobjects.DISABLED && mergeWithMapper instanceof ObjectMapper objectMapper) {
                         // An existing mapping that has set `subobjects: false` is merged with a mapping with sub-objects.
                         List<FieldMapper> flattenedMappers = objectMapper.asFlattenedFieldMappers(
                             objectMergeContext.getMapperBuilderContext()
@@ -691,7 +687,7 @@ private static Map<String, Mapper> buildMergedMappers(
                         putMergedMapper(mergedMappers, truncateObjectMapper(objectMergeContext, om));
                     }
                 } else if (mergeIntoMapper instanceof ObjectMapper objectMapper) {
-                    assert subobjects.isEmpty() || subobjects.get() != Subobjects.DISABLED
+                    assert subobjects.explicit() == false || subobjects.value() != Subobjects.DISABLED
                         : "existing object mappers are supposed to be flattened if subobjects is false";
                     putMergedMapper(mergedMappers, objectMapper.merge(mergeWithMapper, objectMergeContext));
                 } else {
@@ -803,7 +799,7 @@ private void ensureFlattenable(MapperBuilderContext context, ContentPath path) {
         if (isEnabled() == false) {
             throwAutoFlatteningException(path, "the value of [enabled] is [false]");
         }
-        if (subobjects.isPresent() && subobjects.get() == Subobjects.ENABLED) {
+        if (subobjects.explicit() && subobjects.value() == Subobjects.ENABLED) {
             throwAutoFlatteningException(path, "the value of [subobjects] is [true]");
         }
     }
@@ -838,8 +834,8 @@ void toXContent(XContentBuilder builder, Params params, ToXContent custom) throw
         if (isEnabled() != Defaults.ENABLED) {
             builder.field("enabled", enabled.value());
         }
-        if (subobjects.isPresent()) {
-            builder.field("subobjects", subobjects.get().printedValue);
+        if (subobjects.explicit()) {
+            builder.field("subobjects", subobjects.value().printedValue);
         }
         if (sourceKeepMode.isPresent()) {
             builder.field("synthetic_source_keep", sourceKeepMode.get());

server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java

@@ -48,7 +48,7 @@ public static class Builder extends ObjectMapper.Builder {
 
         public Builder(String name) {
             // Subobjects are not currently supported.
-            super(name, Optional.of(Subobjects.DISABLED));
+            super(name, Explicit.implicit(Subobjects.DISABLED));
         }
 
         @Override
@@ -101,7 +101,7 @@ public PassThroughObjectMapper build(MapperBuilderContext context) {
         int priority
     ) {
         // Subobjects are not currently supported.
-        super(name, fullPath, enabled, Optional.of(Subobjects.DISABLED), sourceKeepMode, dynamic, mappers);
+        super(name, fullPath, enabled, Explicit.implicit(Subobjects.DISABLED), sourceKeepMode, dynamic, mappers);
         this.timeSeriesDimensionSubFields = timeSeriesDimensionSubFields;
         this.priority = priority;
         if (priority < 0) {

server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java

@@ -77,7 +77,11 @@ public static class Builder extends ObjectMapper.Builder {
         protected Explicit<Boolean> dateDetection = Defaults.DATE_DETECTION;
         protected Explicit<Boolean> numericDetection = Defaults.NUMERIC_DETECTION;
 
-        public Builder(String name, Optional<Subobjects> subobjects) {
+        public Builder(String name) {
+            this(name, ObjectMapper.Defaults.SUBOBJECTS);
+        }
+
+        public Builder(String name, Explicit<Subobjects> subobjects) {
             super(name, subobjects);
         }
 
@@ -134,7 +138,7 @@ public RootObjectMapper build(MapperBuilderContext context) {
     RootObjectMapper(
         String name,
         Explicit<Boolean> enabled,
-        Optional<Subobjects> subobjects,
+        Explicit<Subobjects> subobjects,
         Optional<SourceKeepMode> sourceKeepMode,
         Dynamic dynamic,
         Map<String, Mapper> mappers,
@@ -449,7 +453,7 @@ protected boolean isRoot() {
 
     public static RootObjectMapper.Builder parse(String name, Map<String, Object> node, MappingParserContext parserContext)
         throws MapperParsingException {
-        Optional<Subobjects> subobjects = parseSubobjects(node);
+        Explicit<Subobjects> subobjects = parseSubobjects(node);
         RootObjectMapper.Builder builder = new Builder(name, subobjects);
         Iterator<Map.Entry<String, Object>> iterator = node.entrySet().iterator();
         while (iterator.hasNext()) {

server/src/test/java/org/elasticsearch/index/mapper/DynamicFieldsBuilderTests.java

@@ -19,7 +19,6 @@
 import java.io.IOException;
 import java.util.List;
 import java.util.Map;
-import java.util.Optional;
 
 import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
 
@@ -70,7 +69,7 @@ public void testCreateDynamicStringFieldAsKeywordForDimension() throws IOExcepti
         SourceToParse sourceToParse = new SourceToParse("test", new BytesArray(source), XContentType.JSON);
 
         SourceFieldMapper sourceMapper = new SourceFieldMapper.Builder(null, Settings.EMPTY, false, false, false).setSynthetic().build();
-        RootObjectMapper root = new RootObjectMapper.Builder("_doc", Optional.empty()).add(
+        RootObjectMapper root = new RootObjectMapper.Builder("_doc").add(
             new PassThroughObjectMapper.Builder("labels").setPriority(0).setContainsDimensions().dynamic(ObjectMapper.Dynamic.TRUE)
         ).build(MapperBuilderContext.root(false, false));
         Mapping mapping = new Mapping(root, new MetadataFieldMapper[] { sourceMapper }, Map.of());

server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java

@@ -178,7 +178,7 @@ private static ObjectMapper createObjectMapper(String name) {
             name,
             name,
             Explicit.IMPLICIT_TRUE,
-            Optional.empty(),
+            ObjectMapper.Defaults.SUBOBJECTS,
             Optional.empty(),
             ObjectMapper.Dynamic.FALSE,
             emptyMap()

server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java

@@ -83,7 +83,7 @@ public void testSubfieldOverride() {
             "object",
             "object",
             Explicit.EXPLICIT_TRUE,
-            Optional.empty(),
+            ObjectMapper.Defaults.SUBOBJECTS,
             Optional.empty(),
             ObjectMapper.Dynamic.TRUE,
             Collections.singletonMap("object.subfield", fieldMapper)