Skip to content

Commit

Permalink
Remove test only MapperMergeContext#root method
Browse files Browse the repository at this point in the history 
MapperMergeContext exposes a package private static root method that is only used in tests.
That makes the production code slightly harder to follow because there are two possible code-paths. This commit
simplifies that by moving the logic around providing a mapping reason to the caller tests, and removes
the additional root method that is not needed in prod code.
  • Loading branch information
@javanna
javanna committed Jun 20, 2024
1 parent b9e7965 commit b851a25
Show file tree
Hide file tree
Showing 6 changed files with 58 additions and 50 deletions.
4 changes: 0 additions & 4 deletions server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,6 @@ private MapperMergeContext(MapperBuilderContext mapperBuilderContext, NewFieldsB
this.newFieldsBudget = newFieldsBudget;
}

static MapperMergeContext root(boolean isSourceSynthetic, boolean isDataStream, long newFieldsBudget) {
return root(isSourceSynthetic, isDataStream, MergeReason.MAPPING_UPDATE, newFieldsBudget);
}

/**
* The root context, to be used when merging a tree of mappers
*/
Expand Down
10 changes: 6 additions & 4 deletions server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,27 +10,29 @@

import org.elasticsearch.test.ESTestCase;

import static org.elasticsearch.index.mapper.MapperService.MergeReason.*;

public class MapperMergeContextTests extends ESTestCase {

public void testAddFieldIfPossibleUnderLimit() {
MapperMergeContext context = MapperMergeContext.root(false, false, 1);
MapperMergeContext context = MapperMergeContext.root(false, false, MAPPING_UPDATE, 1);
assertTrue(context.decrementFieldBudgetIfPossible(1));
assertFalse(context.decrementFieldBudgetIfPossible(1));
}

public void testAddFieldIfPossibleAtLimit() {
MapperMergeContext context = MapperMergeContext.root(false, false, 0);
MapperMergeContext context = MapperMergeContext.root(false, false, MAPPING_UPDATE, 0);
assertFalse(context.decrementFieldBudgetIfPossible(1));
}

public void testAddFieldIfPossibleUnlimited() {
MapperMergeContext context = MapperMergeContext.root(false, false, Long.MAX_VALUE);
MapperMergeContext context = MapperMergeContext.root(false, false, MAPPING_UPDATE, Long.MAX_VALUE);
assertTrue(context.decrementFieldBudgetIfPossible(Integer.MAX_VALUE));
assertTrue(context.decrementFieldBudgetIfPossible(Integer.MAX_VALUE));
}

public void testMergeReasons() {
MapperService.MergeReason mergeReason = randomFrom(MapperService.MergeReason.values());
MapperService.MergeReason mergeReason = randomFrom(values());
MapperMergeContext context = MapperMergeContext.root(false, false, mergeReason, Integer.MAX_VALUE);
assertEquals(mergeReason, context.getMapperBuilderContext().getMergeReason());
}
Expand Down
8 changes: 7 additions & 1 deletion server/src/test/java/org/elasticsearch/index/mapper/MultiFieldsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

import java.util.Map;

import static org.elasticsearch.index.mapper.MapperService.MergeReason.MAPPING_UPDATE;

public class MultiFieldsTests extends ESTestCase {

public void testMultiFieldsBuilderHasSyntheticSourceCompatibleKeywordField() {
Expand Down Expand Up @@ -45,7 +47,11 @@ public void testMultiFieldsBuilderHasSyntheticSourceCompatibleKeywordFieldDuring
keywordFieldMapperBuilder
).build(MapperBuilderContext.root(false, false));

builder.merge(newField, new FieldMapper.Conflicts("TextFieldMapper"), MapperMergeContext.root(false, false, Long.MAX_VALUE));
builder.merge(
newField,
new FieldMapper.Conflicts("TextFieldMapper"),
MapperMergeContext.root(false, false, MAPPING_UPDATE, Long.MAX_VALUE)
);

var expected = hasNormalizer == false;
assertEquals(expected, builder.multiFieldsBuilder.hasSyntheticSourceCompatibleKeywordField());
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1511,7 +1511,7 @@ public void testMergeNested() {

MapperException e = expectThrows(
MapperException.class,
() -> firstMapper.merge(secondMapper, MapperMergeContext.root(false, false, Long.MAX_VALUE))
() -> firstMapper.merge(secondMapper, MapperMergeContext.root(false, false, MergeReason.MAPPING_UPDATE, Long.MAX_VALUE))
);
assertThat(e.getMessage(), containsString("[include_in_parent] parameter can't be updated on a nested object mapping"));

Expand Down
62 changes: 31 additions & 31 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 @@ -13,6 +13,9 @@

import java.util.Collections;

import static org.elasticsearch.index.mapper.MapperService.MergeReason.MAPPING_UPDATE;
import static org.elasticsearch.index.mapper.MapperService.MergeReason.INDEX_TEMPLATE;

public final class ObjectMapperMergeTests extends ESTestCase {

private final RootObjectMapper rootObjectMapper = createMapping(false, true, true, false);
Expand Down Expand Up @@ -41,7 +44,10 @@ public void testMerge() {
ObjectMapper mergeWith = createMapping(false, true, true, true);

// WHEN merging mappings
final ObjectMapper merged = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE));
final ObjectMapper merged = rootObjectMapper.merge(
mergeWith,
MapperMergeContext.root(false, false, MAPPING_UPDATE, Long.MAX_VALUE)
);

// THEN "baz" new field is added to merged mapping
final ObjectMapper mergedFoo = (ObjectMapper) merged.getMapper("foo");
Expand All @@ -63,7 +69,7 @@ public void testMergeWhenDisablingField() {
// THEN a MapperException is thrown with an excepted message
MapperException e = expectThrows(
MapperException.class,
() -> rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE))
() -> rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, Long.MAX_VALUE))
);
assertEquals("the [enabled] parameter can't be updated for the object mapping [foo]", e.getMessage());
}
Expand All @@ -75,7 +81,7 @@ public void testMergeDisabledField() {
new ObjectMapper.Builder("disabled", Explicit.IMPLICIT_TRUE)
).build(MapperBuilderContext.root(false, false));

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

Expand All @@ -84,14 +90,11 @@ public void testMergeEnabled() {

MapperException e = expectThrows(
MapperException.class,
() -> rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE))
() -> rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, Long.MAX_VALUE))
);
assertEquals("the [enabled] parameter can't be updated for the object mapping [disabled]", e.getMessage());

ObjectMapper result = rootObjectMapper.merge(
mergeWith,
MapperMergeContext.root(false, false, MapperService.MergeReason.INDEX_TEMPLATE, Long.MAX_VALUE)
);
ObjectMapper result = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, INDEX_TEMPLATE, Long.MAX_VALUE));
assertTrue(result.isEnabled());
}

Expand All @@ -105,14 +108,11 @@ public void testMergeEnabledForRootMapper() {

MapperException e = expectThrows(
MapperException.class,
() -> firstMapper.merge(secondMapper, MapperMergeContext.root(false, false, Long.MAX_VALUE))
() -> firstMapper.merge(secondMapper, MapperMergeContext.root(false, false, MAPPING_UPDATE, Long.MAX_VALUE))
);
assertEquals("the [enabled] parameter can't be updated for the object mapping [" + type + "]", e.getMessage());

ObjectMapper result = firstMapper.merge(
secondMapper,
MapperMergeContext.root(false, false, MapperService.MergeReason.INDEX_TEMPLATE, Long.MAX_VALUE)
);
ObjectMapper result = firstMapper.merge(secondMapper, MapperMergeContext.root(false, false, INDEX_TEMPLATE, Long.MAX_VALUE));
assertFalse(result.isEnabled());
}

Expand All @@ -126,7 +126,7 @@ public void testMergeDisabledRootMapper() {
Collections.singletonMap("test", new TestRuntimeField("test", "long"))
).build(MapperBuilderContext.root(false, false));

RootObjectMapper merged = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE));
RootObjectMapper merged = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, Long.MAX_VALUE));
assertFalse(merged.isEnabled());
assertEquals(1, merged.runtimeFields().size());
assertEquals("test", merged.runtimeFields().iterator().next().name());
Expand All @@ -136,7 +136,7 @@ public void testMergedFieldNamesFieldWithDotsSubobjectsFalseAtRoot() {
RootObjectMapper mergeInto = createRootSubobjectFalseLeafWithDots();
RootObjectMapper mergeWith = createRootSubobjectFalseLeafWithDots();

final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE));
final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, Long.MAX_VALUE));

final KeywordFieldMapper keywordFieldMapper = (KeywordFieldMapper) merged.getMapper("host.name");
assertEquals("host.name", keywordFieldMapper.name());
Expand All @@ -151,7 +151,7 @@ public void testMergedFieldNamesFieldWithDotsSubobjectsFalse() {
createObjectSubobjectsFalseLeafWithDots()
).build(MapperBuilderContext.root(false, false));

final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE));
final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, Long.MAX_VALUE));

ObjectMapper foo = (ObjectMapper) merged.getMapper("foo");
ObjectMapper metrics = (ObjectMapper) foo.getMapper("metrics");
Expand All @@ -166,7 +166,7 @@ public void testMergedFieldNamesMultiFields() {
RootObjectMapper mergeWith = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add(createTextKeywordMultiField("text"))
.build(MapperBuilderContext.root(false, false));

final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE));
final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, Long.MAX_VALUE));

TextFieldMapper text = (TextFieldMapper) merged.getMapper("text");
assertEquals("text", text.name());
Expand All @@ -184,7 +184,7 @@ public void testMergedFieldNamesMultiFieldsWithinSubobjectsFalse() {
createObjectSubobjectsFalseLeafWithMultiField()
).build(MapperBuilderContext.root(false, false));

final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE));
final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, Long.MAX_VALUE));

ObjectMapper foo = (ObjectMapper) merged.getMapper("foo");
ObjectMapper metrics = (ObjectMapper) foo.getMapper("metrics");
Expand All @@ -201,8 +201,8 @@ public void testMergeWithLimit() {
ObjectMapper mergeWith = createMapping(false, true, true, true);

// WHEN merging mappings
final ObjectMapper mergedAdd0 = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, 0));
final ObjectMapper mergedAdd1 = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, 1));
final ObjectMapper mergedAdd0 = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, 0));
final ObjectMapper mergedAdd1 = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, 1));

// THEN "baz" new field is added to merged mapping
assertEquals(3, rootObjectMapper.getTotalFieldsCount());
Expand All @@ -219,10 +219,10 @@ public void testMergeWithLimitTruncatedObjectField() {
).add(new KeywordFieldMapper.Builder("child2", IndexVersion.current()))
).build(MapperBuilderContext.root(false, false));

ObjectMapper mergedAdd0 = root.merge(mergeWith, MapperMergeContext.root(false, false, 0));
ObjectMapper mergedAdd1 = root.merge(mergeWith, MapperMergeContext.root(false, false, 1));
ObjectMapper mergedAdd2 = root.merge(mergeWith, MapperMergeContext.root(false, false, 2));
ObjectMapper mergedAdd3 = root.merge(mergeWith, MapperMergeContext.root(false, false, 3));
ObjectMapper mergedAdd0 = root.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, 0));
ObjectMapper mergedAdd1 = root.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, 1));
ObjectMapper mergedAdd2 = root.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, 2));
ObjectMapper mergedAdd3 = root.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, 3));
assertEquals(0, root.getTotalFieldsCount());
assertEquals(0, mergedAdd0.getTotalFieldsCount());
assertEquals(1, mergedAdd1.getTotalFieldsCount());
Expand Down Expand Up @@ -252,8 +252,8 @@ public void testMergeSameObjectDifferentFields() {
).add(new KeywordFieldMapper.Builder("child2", IndexVersion.current()))
).build(MapperBuilderContext.root(false, false));

ObjectMapper mergedAdd0 = root.merge(mergeWith, MapperMergeContext.root(false, false, 0));
ObjectMapper mergedAdd1 = root.merge(mergeWith, MapperMergeContext.root(false, false, 1));
ObjectMapper mergedAdd0 = root.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, 0));
ObjectMapper mergedAdd1 = root.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, 1));
assertEquals(2, root.getTotalFieldsCount());
assertEquals(2, mergedAdd0.getTotalFieldsCount());
assertEquals(3, mergedAdd1.getTotalFieldsCount());
Expand All @@ -280,8 +280,8 @@ public void testMergeWithLimitMultiField() {
assertEquals(2, mergeInto.getTotalFieldsCount());
assertEquals(2, mergeWith.getTotalFieldsCount());

ObjectMapper mergedAdd0 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, 0));
ObjectMapper mergedAdd1 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, 1));
ObjectMapper mergedAdd0 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, 0));
ObjectMapper mergedAdd1 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, 1));
assertEquals(2, mergedAdd0.getTotalFieldsCount());
assertEquals(3, mergedAdd1.getTotalFieldsCount());
}
Expand All @@ -297,8 +297,8 @@ public void testMergeWithLimitRuntimeField() {
assertEquals(3, mergeInto.getTotalFieldsCount());
assertEquals(2, mergeWith.getTotalFieldsCount());

ObjectMapper mergedAdd0 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, 0));
ObjectMapper mergedAdd1 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, 1));
ObjectMapper mergedAdd0 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, 0));
ObjectMapper mergedAdd1 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, 1));
assertEquals(3, mergedAdd0.getTotalFieldsCount());
assertEquals(4, mergedAdd1.getTotalFieldsCount());
}
Expand All @@ -315,7 +315,7 @@ public void testMergeSubobjectsFalseWithObject() {
)
).build(MapperBuilderContext.root(false, false));

ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE));
ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, MAPPING_UPDATE, Long.MAX_VALUE));
ObjectMapper parentMapper = (ObjectMapper) merged.getMapper("parent");
assertNotNull(parentMapper);
assertNotNull(parentMapper.getMapper("child.grandchild"));
Expand Down
Loading

0 comments on commit b851a25

Please sign in to comment.