Skip to content

Commit

Permalink
Make ObjectMapper Immutable (elastic#101485)
Browse files Browse the repository at this point in the history 
It's hard to reason about the thread safety of these objects (and their
behavior in general) when they are mutable.
This removes the cloning and mutating of these objects and replaces it
with just copy constructors for merging.

Also removes the unused cloneable on field mappers.
  • Loading branch information
@original-brownbear
original-brownbear authored Oct 30, 2023
1 parent dc75cab commit d5c9c7c
Show file tree
Hide file tree
Showing 5 changed files with 204 additions and 120 deletions.
5 changes: 5 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,11 @@ private static class NoOpObjectMapper extends ObjectMapper {
NoOpObjectMapper(String name, String fullPath) {
super(name, fullPath, Explicit.IMPLICIT_TRUE, Explicit.IMPLICIT_TRUE, Dynamic.RUNTIME, Collections.emptyMap());
}

@Override
public ObjectMapper merge(Mapper mergeWith, MapperBuilderContext mapperBuilderContext) {
return this;
}
}

/**
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@

import static org.elasticsearch.core.Strings.format;

public abstract class FieldMapper extends Mapper implements Cloneable {
public abstract class FieldMapper extends Mapper {
private static final Logger logger = LogManager.getLogger(FieldMapper.class);

public static final Setting<Boolean> IGNORE_MALFORMED_SETTING = Setting.boolSetting(
Expand Down
78 changes: 56 additions & 22 deletions server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,24 @@ public NestedObjectMapper build(MapperBuilderContext context) {
}
}
NestedMapperBuilderContext nestedContext = new NestedMapperBuilderContext(context.buildFullName(name), parentIncludedInRoot);
return new NestedObjectMapper(name, context.buildFullName(name), buildMappers(nestedContext), this);
final String fullPath = context.buildFullName(name);
final String nestedTypePath;
if (indexCreatedVersion.before(IndexVersions.V_8_0_0)) {
nestedTypePath = "__" + fullPath;
} else {
nestedTypePath = fullPath;
}
return new NestedObjectMapper(
name,
fullPath,
buildMappers(nestedContext),
enabled,
dynamic,
includeInParent,
includeInRoot,
nestedTypePath,
NestedPathFieldMapper.filter(indexCreatedVersion, nestedTypePath)
);
}
}

Expand Down Expand Up @@ -111,21 +128,27 @@ public MapperBuilderContext createChildContext(String name) {
}
}

private Explicit<Boolean> includeInRoot;
private Explicit<Boolean> includeInParent;
private final Explicit<Boolean> includeInRoot;
private final Explicit<Boolean> includeInParent;
private final String nestedTypePath;
private final Query nestedTypeFilter;

NestedObjectMapper(String name, String fullPath, Map<String, Mapper> mappers, Builder builder) {
super(name, fullPath, builder.enabled, Explicit.IMPLICIT_TRUE, builder.dynamic, mappers);
if (builder.indexCreatedVersion.before(IndexVersions.V_8_0_0)) {
this.nestedTypePath = "__" + fullPath;
} else {
this.nestedTypePath = fullPath;
}
this.nestedTypeFilter = NestedPathFieldMapper.filter(builder.indexCreatedVersion, nestedTypePath);
this.includeInParent = builder.includeInParent;
this.includeInRoot = builder.includeInRoot;
NestedObjectMapper(
String name,
String fullPath,
Map<String, Mapper> mappers,
Explicit<Boolean> enabled,
ObjectMapper.Dynamic dynamic,
Explicit<Boolean> includeInParent,
Explicit<Boolean> includeInRoot,
String nestedTypePath,
Query nestedTypeFilter
) {
super(name, fullPath, enabled, Explicit.IMPLICIT_TRUE, dynamic, mappers);
this.nestedTypePath = nestedTypePath;
this.nestedTypeFilter = nestedTypeFilter;
this.includeInParent = includeInParent;
this.includeInRoot = includeInRoot;
}

public Query nestedTypeFilter() {
Expand Down Expand Up @@ -189,13 +212,15 @@ public ObjectMapper merge(Mapper mergeWith, MapperService.MergeReason reason, Ma
MapperErrors.throwNestedMappingConflictError(mergeWith.name());
}
NestedObjectMapper mergeWithObject = (NestedObjectMapper) mergeWith;
NestedObjectMapper toMerge = (NestedObjectMapper) clone();
var mergeResult = MergeResult.build(this, mergeWith, reason, parentBuilderContext);
Explicit<Boolean> incInParent = this.includeInParent;
Explicit<Boolean> incInRoot = this.includeInRoot;
if (reason == MapperService.MergeReason.INDEX_TEMPLATE) {
if (mergeWithObject.includeInParent.explicit()) {
toMerge.includeInParent = mergeWithObject.includeInParent;
incInParent = mergeWithObject.includeInParent;
}
if (mergeWithObject.includeInRoot.explicit()) {
toMerge.includeInRoot = mergeWithObject.includeInRoot;
incInRoot = mergeWithObject.includeInRoot;
}
} else {
if (includeInParent.value() != mergeWithObject.includeInParent.value()) {
Expand All @@ -206,16 +231,25 @@ public ObjectMapper merge(Mapper mergeWith, MapperService.MergeReason reason, Ma
}
}
if (parentBuilderContext instanceof NestedMapperBuilderContext nc) {
if (nc.parentIncludedInRoot && toMerge.includeInParent.value()) {
toMerge.includeInRoot = Explicit.IMPLICIT_FALSE;
if (nc.parentIncludedInRoot && incInParent.value()) {
incInRoot = Explicit.IMPLICIT_FALSE;
}
} else {
if (toMerge.includeInParent.value()) {
toMerge.includeInRoot = Explicit.IMPLICIT_FALSE;
if (incInParent.value()) {
incInRoot = Explicit.IMPLICIT_FALSE;
}
}
toMerge.doMerge(mergeWithObject, reason, parentBuilderContext);
return toMerge;
return new NestedObjectMapper(
simpleName(),
fullPath(),
mergeResult.mappers(),
mergeResult.enabled(),
mergeResult.dynamic(),
incInParent,
incInRoot,
nestedTypePath,
nestedTypeFilter
);
}

@Override
Expand Down
173 changes: 102 additions & 71 deletions server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
import java.util.Set;
import java.util.stream.Stream;

public class ObjectMapper extends Mapper implements Cloneable {
public class ObjectMapper extends Mapper {
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(ObjectMapper.class);

public static final String CONTENT_TYPE = "object";
Expand Down Expand Up @@ -370,11 +370,11 @@ private static void validateFieldName(String fieldName, IndexVersion indexCreate

private final String fullPath;

protected Explicit<Boolean> enabled;
protected Explicit<Boolean> subobjects;
protected volatile Dynamic dynamic;
protected final Explicit<Boolean> enabled;
protected final Explicit<Boolean> subobjects;
protected final Dynamic dynamic;

protected Map<String, Mapper> mappers;
protected final Map<String, Mapper> mappers;

ObjectMapper(
String name,
Expand All @@ -398,18 +398,6 @@ private static void validateFieldName(String fieldName, IndexVersion indexCreate
}
}

@Override
protected ObjectMapper clone() {
ObjectMapper clone;
try {
clone = (ObjectMapper) super.clone();
} catch (CloneNotSupportedException e) {
throw new RuntimeException(e);
}
clone.mappers = Map.copyOf(clone.mappers);
return clone;
}

/**
* @return a Builder that will produce an empty ObjectMapper with the same configuration as this one
*/
Expand Down Expand Up @@ -476,73 +464,116 @@ protected MapperBuilderContext createChildContext(MapperBuilderContext mapperBui
}

public ObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperBuilderContext parentBuilderContext) {
if ((mergeWith instanceof ObjectMapper) == false) {
MapperErrors.throwObjectMappingConflictError(mergeWith.name());
}
if (mergeWith instanceof NestedObjectMapper) {
// TODO stop NestedObjectMapper extending ObjectMapper?
MapperErrors.throwNestedMappingConflictError(mergeWith.name());
}
ObjectMapper mergeWithObject = (ObjectMapper) mergeWith;
ObjectMapper merged = clone();
merged.doMerge(mergeWithObject, reason, parentBuilderContext);
return merged;
var mergeResult = MergeResult.build(this, mergeWith, reason, parentBuilderContext);
return new ObjectMapper(
simpleName(),
fullPath,
mergeResult.enabled,
mergeResult.subObjects,
mergeResult.dynamic,
mergeResult.mappers
);
}

protected void doMerge(final ObjectMapper mergeWith, MergeReason reason, MapperBuilderContext parentBuilderContext) {
if (mergeWith.dynamic != null) {
this.dynamic = mergeWith.dynamic;
}
protected record MergeResult(
Explicit<Boolean> enabled,
Explicit<Boolean> subObjects,
ObjectMapper.Dynamic dynamic,
Map<String, Mapper> mappers
) {

if (mergeWith.enabled.explicit()) {
if (reason == MergeReason.INDEX_TEMPLATE) {
this.enabled = mergeWith.enabled;
} else if (isEnabled() != mergeWith.isEnabled()) {
throw new MapperException("the [enabled] parameter can't be updated for the object mapping [" + name() + "]");
public static MergeResult build(
ObjectMapper existing,
Mapper mergeWith,
MergeReason reason,
MapperBuilderContext parentBuilderContext
) {
if ((mergeWith instanceof ObjectMapper) == false) {
MapperErrors.throwObjectMappingConflictError(mergeWith.name());
}
}

if (mergeWith.subobjects.explicit()) {
if (reason == MergeReason.INDEX_TEMPLATE) {
this.subobjects = mergeWith.subobjects;
} else if (subobjects != mergeWith.subobjects) {
throw new MapperException("the [subobjects] parameter can't be updated for the object mapping [" + name() + "]");
if (existing instanceof NestedObjectMapper == false && mergeWith instanceof NestedObjectMapper) {
// TODO stop NestedObjectMapper extending ObjectMapper?
MapperErrors.throwNestedMappingConflictError(mergeWith.name());
}
}

MapperBuilderContext objectBuilderContext = createChildContext(parentBuilderContext, simpleName());
Map<String, Mapper> mergedMappers = null;
for (Mapper mergeWithMapper : mergeWith) {
Mapper mergeIntoMapper = (mergedMappers == null ? mappers : mergedMappers).get(mergeWithMapper.simpleName());

Mapper merged;
if (mergeIntoMapper == null) {
merged = mergeWithMapper;
} else if (mergeIntoMapper instanceof ObjectMapper objectMapper) {
merged = objectMapper.merge(mergeWithMapper, reason, objectBuilderContext);
ObjectMapper mergeWithObject = (ObjectMapper) mergeWith;
final Explicit<Boolean> enabled;
if (mergeWithObject.enabled.explicit()) {
if (reason == MergeReason.INDEX_TEMPLATE) {
enabled = mergeWithObject.enabled;
} else if (existing.isEnabled() != mergeWithObject.isEnabled()) {
throw new MapperException("the [enabled] parameter can't be updated for the object mapping [" + existing.name() + "]");
} else {
enabled = existing.enabled;
}
} else {
assert mergeIntoMapper instanceof FieldMapper || mergeIntoMapper instanceof FieldAliasMapper;
if (mergeWithMapper instanceof NestedObjectMapper) {
MapperErrors.throwNestedMappingConflictError(mergeWithMapper.name());
} else if (mergeWithMapper instanceof ObjectMapper) {
MapperErrors.throwObjectMappingConflictError(mergeWithMapper.name());
enabled = existing.enabled;
}
final Explicit<Boolean> subObjects;
if (mergeWithObject.subobjects.explicit()) {
if (reason == MergeReason.INDEX_TEMPLATE) {
subObjects = mergeWithObject.subobjects;
} else if (existing.subobjects != mergeWithObject.subobjects) {
throw new MapperException(
"the [subobjects] parameter can't be updated for the object mapping [" + existing.name() + "]"
);
} else {
subObjects = existing.subobjects;
}
} else {
subObjects = existing.subobjects;
}
MapperBuilderContext objectBuilderContext = existing.createChildContext(parentBuilderContext, existing.simpleName());
Map<String, Mapper> mergedMappers = buildMergedMappers(existing, mergeWith, reason, objectBuilderContext);
return new MergeResult(
enabled,
subObjects,
mergeWithObject.dynamic != null ? mergeWithObject.dynamic : existing.dynamic,
mergedMappers
);
}

// If we're merging template mappings when creating an index, then a field definition always
// replaces an existing one.
if (reason == MergeReason.INDEX_TEMPLATE) {
private static Map<String, Mapper> buildMergedMappers(
ObjectMapper existing,
Mapper mergeWith,
MergeReason reason,
MapperBuilderContext objectBuilderContext
) {
Map<String, Mapper> mergedMappers = null;
for (Mapper mergeWithMapper : mergeWith) {
Mapper mergeIntoMapper = (mergedMappers == null ? existing.mappers : mergedMappers).get(mergeWithMapper.simpleName());

Mapper merged;
if (mergeIntoMapper == null) {
merged = mergeWithMapper;
} else if (mergeIntoMapper instanceof ObjectMapper objectMapper) {
merged = objectMapper.merge(mergeWithMapper, reason, objectBuilderContext);
} else {
merged = mergeIntoMapper.merge(mergeWithMapper, objectBuilderContext);
assert mergeIntoMapper instanceof FieldMapper || mergeIntoMapper instanceof FieldAliasMapper;
if (mergeWithMapper instanceof NestedObjectMapper) {
MapperErrors.throwNestedMappingConflictError(mergeWithMapper.name());
} else if (mergeWithMapper instanceof ObjectMapper) {
MapperErrors.throwObjectMappingConflictError(mergeWithMapper.name());
}

// If we're merging template mappings when creating an index, then a field definition always
// replaces an existing one.
if (reason == MergeReason.INDEX_TEMPLATE) {
merged = mergeWithMapper;
} else {
merged = mergeIntoMapper.merge(mergeWithMapper, objectBuilderContext);
}
}
if (mergedMappers == null) {
mergedMappers = new HashMap<>(existing.mappers);
}
mergedMappers.put(merged.simpleName(), merged);
}
if (mergedMappers == null) {
mergedMappers = new HashMap<>(mappers);
if (mergedMappers != null) {
mergedMappers = Map.copyOf(mergedMappers);
} else {
mergedMappers = Map.copyOf(existing.mappers);
}
mergedMappers.put(merged.simpleName(), merged);
}
if (mergedMappers != null) {
mappers = Map.copyOf(mergedMappers);
return mergedMappers;
}
}

Expand Down
Loading

0 comments on commit d5c9c7c

Please sign in to comment.