Skip to content

Commit

Permalink
Remove the merge method that received both the reason and merge context
Browse files Browse the repository at this point in the history 
This removes the (additional) `merge`  method with signature
```
public Mapper merge(Mapper mergeWith, MapperService.MergeReason reason, MapperMergeContext mapperMergeContext);
```
in favour of the existing
```
public abstract Mapper merge(Mapper mergeWith, MapperMergeContext mapperMergeContext);
```
as the `MapperMergeContext` arealdy contains the merge reason.

This will help the API clients avoid the pitfalls that already exist in tests where the provided
merge `reason` is not the same as the `MapperMergeContext#reason`
  • Loading branch information
@andreidan
andreidan committed Apr 8, 2024
1 parent 9c5541a commit cdb4711
Show file tree
Hide file tree
Showing 7 changed files with 27 additions and 61 deletions.
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/index/mapper/Mapping.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ public SourceLoader.SyntheticFieldLoader syntheticFieldLoader() {
*/
Mapping merge(Mapping mergeWith, MergeReason reason, long newFieldsBudget) {
MapperMergeContext mergeContext = MapperMergeContext.root(isSourceSynthetic(), false, reason, newFieldsBudget);
RootObjectMapper mergedRoot = root.merge(mergeWith.root, reason, mergeContext);
RootObjectMapper mergedRoot = root.merge(mergeWith.root, mergeContext);

// When merging metadata fields as part of applying an index template, new field definitions
// completely overwrite existing ones instead of being merged. This behavior matches how we
Expand Down Expand Up @@ -180,7 +180,7 @@ public Mapping withFieldsBudget(long fieldsBudget) {
// get a copy of the root mapper, without any fields
RootObjectMapper shallowRoot = root.withoutMappers();
// calling merge on the shallow root to ensure we're only adding as many fields as allowed by the fields budget
return new Mapping(shallowRoot.merge(root, MergeReason.MAPPING_RECOVERY, mergeContext), metadataMappers, meta);
return new Mapping(shallowRoot.merge(root, mergeContext), metadataMappers, meta);
}

@Override
Expand Down
8 changes: 3 additions & 5 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 @@ -227,16 +227,14 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
}

@Override
public ObjectMapper merge(Mapper mergeWith, MapperService.MergeReason reason, MapperMergeContext parentMergeContext) {
public ObjectMapper merge(Mapper mergeWith, MapperMergeContext parentMergeContext) {
if ((mergeWith instanceof NestedObjectMapper) == false) {
MapperErrors.throwNestedMappingConflictError(mergeWith.name());
}
NestedObjectMapper mergeWithObject = (NestedObjectMapper) mergeWith;
return merge(mergeWithObject, reason, parentMergeContext);
}

ObjectMapper merge(NestedObjectMapper mergeWithObject, MapperService.MergeReason reason, MapperMergeContext parentMergeContext) {
var mergeResult = MergeResult.build(this, mergeWithObject, reason, parentMergeContext);
final MapperService.MergeReason reason = parentMergeContext.getMapperBuilderContext().getMergeReason();
var mergeResult = MergeResult.build(this, mergeWithObject, parentMergeContext);
Explicit<Boolean> incInParent = this.includeInParent;
Explicit<Boolean> incInRoot = this.includeInRoot;
if (reason == MapperService.MergeReason.INDEX_TEMPLATE) {
Expand Down
41 changes: 11 additions & 30 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 @@ -454,11 +454,6 @@ public final boolean subobjects() {
return subobjects.value();
}

@Override
public ObjectMapper merge(Mapper mergeWith, MapperMergeContext mapperMergeContext) {
return merge(mergeWith, mapperMergeContext.getMapperBuilderContext().getMergeReason(), mapperMergeContext);
}

@Override
public void validate(MappingLookup mappers) {
for (Mapper mapper : this.mappers.values()) {
Expand All @@ -470,19 +465,16 @@ protected MapperMergeContext createChildContext(MapperMergeContext mapperMergeCo
return mapperMergeContext.createChildContext(name, dynamic);
}

public ObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperMergeContext parentMergeContext) {
@Override
public ObjectMapper merge(Mapper mergeWith, MapperMergeContext parentMergeContext) {
if (mergeWith instanceof ObjectMapper == false) {
MapperErrors.throwObjectMappingConflictError(mergeWith.name());
}
if (this instanceof NestedObjectMapper == false && mergeWith instanceof NestedObjectMapper) {
// TODO stop NestedObjectMapper extending ObjectMapper?
MapperErrors.throwNestedMappingConflictError(mergeWith.name());
}
return merge((ObjectMapper) mergeWith, reason, parentMergeContext);
}

ObjectMapper merge(ObjectMapper mergeWith, MergeReason reason, MapperMergeContext parentMergeContext) {
var mergeResult = MergeResult.build(this, mergeWith, reason, parentMergeContext);
var mergeResult = MergeResult.build(this, (ObjectMapper) mergeWith, parentMergeContext);
return new ObjectMapper(
simpleName(),
fullPath,
Expand All @@ -499,13 +491,9 @@ protected record MergeResult(
ObjectMapper.Dynamic dynamic,
Map<String, Mapper> mappers
) {
static MergeResult build(
ObjectMapper existing,
ObjectMapper mergeWithObject,
MergeReason reason,
MapperMergeContext parentMergeContext
) {
static MergeResult build(ObjectMapper existing, ObjectMapper mergeWithObject, MapperMergeContext parentMergeContext) {
final Explicit<Boolean> enabled;
final MergeReason reason = parentMergeContext.getMapperBuilderContext().getMergeReason();
if (mergeWithObject.enabled.explicit()) {
if (reason == MergeReason.INDEX_TEMPLATE) {
enabled = mergeWithObject.enabled;
Expand All @@ -532,13 +520,7 @@ static MergeResult build(
subObjects = existing.subobjects;
}
MapperMergeContext objectMergeContext = existing.createChildContext(parentMergeContext, existing.simpleName());
Map<String, Mapper> mergedMappers = buildMergedMappers(
existing,
mergeWithObject,
reason,
objectMergeContext,
subObjects.value()
);
Map<String, Mapper> mergedMappers = buildMergedMappers(existing, mergeWithObject, objectMergeContext, subObjects.value());
return new MergeResult(
enabled,
subObjects,
Expand All @@ -550,7 +532,6 @@ static MergeResult build(
private static Map<String, Mapper> buildMergedMappers(
ObjectMapper existing,
ObjectMapper mergeWithObject,
MergeReason reason,
MapperMergeContext objectMergeContext,
boolean subobjects
) {
Expand All @@ -576,11 +557,11 @@ private static Map<String, Mapper> buildMergedMappers(
} else if (objectMergeContext.decrementFieldBudgetIfPossible(mergeWithMapper.getTotalFieldsCount())) {
putMergedMapper(mergedMappers, mergeWithMapper);
} else if (mergeWithMapper instanceof ObjectMapper om) {
putMergedMapper(mergedMappers, truncateObjectMapper(reason, objectMergeContext, om));
putMergedMapper(mergedMappers, truncateObjectMapper(objectMergeContext, om));
}
} else if (mergeIntoMapper instanceof ObjectMapper objectMapper) {
assert subobjects : "existing object mappers are supposed to be flattened if subobjects is false";
putMergedMapper(mergedMappers, objectMapper.merge(mergeWithMapper, reason, objectMergeContext));
putMergedMapper(mergedMappers, objectMapper.merge(mergeWithMapper, objectMergeContext));
} else {
assert mergeIntoMapper instanceof FieldMapper || mergeIntoMapper instanceof FieldAliasMapper;
if (mergeWithMapper instanceof NestedObjectMapper) {
Expand All @@ -591,7 +572,7 @@ private static Map<String, Mapper> buildMergedMappers(

// If we're merging template mappings when creating an index, then a field definition always
// replaces an existing one.
if (reason == MergeReason.INDEX_TEMPLATE) {
if (objectMergeContext.getMapperBuilderContext().getMergeReason() == MergeReason.INDEX_TEMPLATE) {
putMergedMapper(mergedMappers, mergeWithMapper);
} else {
putMergedMapper(mergedMappers, mergeIntoMapper.merge(mergeWithMapper, objectMergeContext));
Expand All @@ -607,13 +588,13 @@ private static void putMergedMapper(Map<String, Mapper> mergedMappers, @Nullable
}
}

private static ObjectMapper truncateObjectMapper(MergeReason reason, MapperMergeContext context, ObjectMapper objectMapper) {
private static ObjectMapper truncateObjectMapper(MapperMergeContext context, ObjectMapper objectMapper) {
// there's not enough capacity for the whole object mapper,
// so we're just trying to add the shallow object, without it's sub-fields
ObjectMapper shallowObjectMapper = objectMapper.withoutMappers();
if (context.decrementFieldBudgetIfPossible(shallowObjectMapper.getTotalFieldsCount())) {
// now trying to add the sub-fields one by one via a merge, until we hit the limit
return shallowObjectMapper.merge(objectMapper, reason, context);
return shallowObjectMapper.merge(objectMapper, context);
}
return null;
}
Expand Down
5 changes: 2 additions & 3 deletions server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

import org.elasticsearch.common.Explicit;
import org.elasticsearch.index.IndexVersion;
import org.elasticsearch.index.mapper.MapperService.MergeReason;
import org.elasticsearch.xcontent.XContentBuilder;

import java.io.IOException;
Expand Down Expand Up @@ -100,8 +99,8 @@ public PassThroughObjectMapper.Builder newBuilder(IndexVersion indexVersionCreat
return builder;
}

public PassThroughObjectMapper merge(ObjectMapper mergeWith, MergeReason reason, MapperMergeContext parentBuilderContext) {
final var mergeResult = MergeResult.build(this, mergeWith, reason, parentBuilderContext);
public PassThroughObjectMapper merge(ObjectMapper mergeWith, MapperMergeContext parentBuilderContext) {
final var mergeResult = MergeResult.build(this, mergeWith, parentBuilderContext);
PassThroughObjectMapper mergeWithObject = (PassThroughObjectMapper) mergeWith;

final Explicit<Boolean> containsDimensions = (mergeWithObject.timeSeriesDimensionSubFields.explicit())
Expand Down
11 changes: 4 additions & 7 deletions server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -297,16 +297,13 @@ protected MapperMergeContext createChildContext(MapperMergeContext mapperMergeCo
return mapperMergeContext.createChildContext(null, dynamic);
}

@Override
public RootObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperMergeContext parentMergeContext) {
public RootObjectMapper merge(Mapper mergeWith, MapperMergeContext parentMergeContext) {
if (mergeWith instanceof RootObjectMapper == false) {
MapperErrors.throwObjectMappingConflictError(mergeWith.name());
}
return merge((RootObjectMapper) mergeWith, reason, parentMergeContext);
}

RootObjectMapper merge(RootObjectMapper mergeWithObject, MergeReason reason, MapperMergeContext parentMergeContext) {
final var mergeResult = MergeResult.build(this, mergeWithObject, reason, parentMergeContext);
RootObjectMapper mergeWithObject = (RootObjectMapper) mergeWith;
final var mergeResult = MergeResult.build(this, mergeWithObject, parentMergeContext);
final Explicit<Boolean> numericDetection;
if (mergeWithObject.numericDetection.explicit()) {
numericDetection = mergeWithObject.numericDetection;
Expand All @@ -330,7 +327,7 @@ RootObjectMapper merge(RootObjectMapper mergeWithObject, MergeReason reason, Map

final Explicit<DynamicTemplate[]> dynamicTemplates;
if (mergeWithObject.dynamicTemplates.explicit()) {
if (reason == MergeReason.INDEX_TEMPLATE) {
if (parentMergeContext.getMapperBuilderContext().getMergeReason() == MergeReason.INDEX_TEMPLATE) {
Map<String, DynamicTemplate> templatesByKey = new LinkedHashMap<>();
for (DynamicTemplate template : this.dynamicTemplates.value()) {
templatesByKey.put(template.name(), template);
Expand Down
3 changes: 1 addition & 2 deletions server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1515,8 +1515,7 @@ public void testMergeNested() {

NestedObjectMapper result = (NestedObjectMapper) firstMapper.merge(
secondMapper,
MapperService.MergeReason.INDEX_TEMPLATE,
MapperMergeContext.root(false, false, Long.MAX_VALUE)
MapperMergeContext.root(false, false, MapperService.MergeReason.INDEX_TEMPLATE, Long.MAX_VALUE)
);
assertFalse(result.isIncludeInParent());
assertTrue(result.isIncludeInRoot());
Expand Down
16 changes: 4 additions & 12 deletions server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,7 @@ public void testMergeDisabledField() {
new ObjectMapper.Builder("disabled", Explicit.IMPLICIT_TRUE)
).build(MapperBuilderContext.root(false, false));

RootObjectMapper merged = (RootObjectMapper) rootObjectMapper.merge(
mergeWith,
MapperMergeContext.root(false, false, Long.MAX_VALUE)
);
RootObjectMapper merged = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE));
assertFalse(((ObjectMapper) merged.getMapper("disabled")).isEnabled());
}

Expand All @@ -93,8 +90,7 @@ public void testMergeEnabled() {

ObjectMapper result = rootObjectMapper.merge(
mergeWith,
MapperService.MergeReason.INDEX_TEMPLATE,
MapperMergeContext.root(false, false, Long.MAX_VALUE)
MapperMergeContext.root(false, false, MapperService.MergeReason.INDEX_TEMPLATE, Long.MAX_VALUE)
);
assertTrue(result.isEnabled());
}
Expand All @@ -115,8 +111,7 @@ public void testMergeEnabledForRootMapper() {

ObjectMapper result = firstMapper.merge(
secondMapper,
MapperService.MergeReason.INDEX_TEMPLATE,
MapperMergeContext.root(false, false, Long.MAX_VALUE)
MapperMergeContext.root(false, false, MapperService.MergeReason.INDEX_TEMPLATE, Long.MAX_VALUE)
);
assertFalse(result.isEnabled());
}
Expand All @@ -131,10 +126,7 @@ public void testMergeDisabledRootMapper() {
Collections.singletonMap("test", new TestRuntimeField("test", "long"))
).build(MapperBuilderContext.root(false, false));

RootObjectMapper merged = (RootObjectMapper) rootObjectMapper.merge(
mergeWith,
MapperMergeContext.root(false, false, Long.MAX_VALUE)
);
RootObjectMapper merged = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE));
assertFalse(merged.isEnabled());
assertEquals(1, merged.runtimeFields().size());
assertEquals("test", merged.runtimeFields().iterator().next().name());
Expand Down

0 comments on commit cdb4711

Please sign in to comment.