Skip to content

Commit c81dc2b

Browse files
romseygeekromseygeek
committed
Convert KeywordFieldMapper to parametrized form (#60645)
This makes KeywordFieldMapper extend ParametrizedFieldMapper, with explicitly defined parameters. In addition, we add a new option to Parameter, restrictedStringParam, which accepts a restricted set of string options.
1 parent 3a046e1 commit c81dc2b

File tree

11 files changed

+204
-259
lines changed

11 files changed

+204
-259
lines changed

‎modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,8 +153,6 @@ private static boolean getMapUnmappedFieldAsText(Settings indexSettings) {
153153
static KeywordFieldMapper createExtractQueryFieldBuilder(String name, BuilderContext context) {
154154
KeywordFieldMapper.Builder queryMetadataFieldBuilder = new KeywordFieldMapper.Builder(name);
155155
queryMetadataFieldBuilder.docValues(false);
156-
queryMetadataFieldBuilder.store(false);
157-
queryMetadataFieldBuilder.indexOptions(IndexOptions.DOCS);
158156
return queryMetadataFieldBuilder.build(context);
159157
}
160158

‎server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java

Lines changed: 111 additions & 175 deletions
Large diffs are not rendered by default.

‎server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import java.util.HashMap;
3838
import java.util.HashSet;
3939
import java.util.Iterator;
40+
import java.util.LinkedHashSet;
4041
import java.util.List;
4142
import java.util.Map;
4243
import java.util.Objects;
@@ -318,6 +319,28 @@ public static Parameter<List<String>> stringArrayParam(String name, boolean upda
318319
}, initializer);
319320
}
320321

322+
/**
323+
* Defines a parameter that takes one of a restricted set of string values
324+
* @param name the parameter name
325+
* @param updateable whether the parameter can be changed by a mapping update
326+
* @param initializer a function that reads the parameter value from an existing mapper
327+
* @param values the set of values that the parameter can take. The first value in the list
328+
* is the default value, to be used if the parameter is undefined in a mapping
329+
*/
330+
public static Parameter<String> restrictedStringParam(String name, boolean updateable,
331+
Function<FieldMapper, String> initializer, String... values) {
332+
assert values.length > 0;
333+
Set<String> acceptedValues = new LinkedHashSet<>(Arrays.asList(values));
334+
return stringParam(name, updateable, initializer, values[0])
335+
.setValidator(v -> {
336+
if (acceptedValues.contains(v)) {
337+
return;
338+
}
339+
throw new MapperParsingException("Unknown value [" + v + "] for field [" + name +
340+
"] - accepted values are " + acceptedValues.toString());
341+
});
342+
}
343+
321344
/**
322345
* Defines a parameter that takes an analyzer name
323346
* @param name the parameter name

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -388,10 +388,11 @@ private static void validateDynamicTemplate(Mapper.TypeParser.ParserContext pars
388388
continue;
389389
}
390390

391-
Map<String, Object> fieldTypeConfig = dynamicTemplate.mappingForName("__dummy__", defaultDynamicType);
392-
fieldTypeConfig.remove("type");
391+
String templateName = "__dynamic__" + dynamicTemplate.name();
392+
Map<String, Object> fieldTypeConfig = dynamicTemplate.mappingForName(templateName, defaultDynamicType);
393393
try {
394-
Mapper.Builder<?> dummyBuilder = typeParser.parse("__dummy__", fieldTypeConfig, parserContext);
394+
Mapper.Builder<?> dummyBuilder = typeParser.parse(templateName, fieldTypeConfig, parserContext);
395+
fieldTypeConfig.remove("type");
395396
if (fieldTypeConfig.isEmpty()) {
396397
Settings indexSettings = parserContext.mapperService().getIndexSettings().getSettings();
397398
BuilderContext builderContext = new BuilderContext(indexSettings, new ContentPath(1));

‎server/src/main/java/org/elasticsearch/indices/IndicesModule.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ public static Map<String, Mapper.TypeParser> getMappers(List<MapperPlugin> mappe
127127
mappers.put(nanoseconds.type(), DateFieldMapper.NANOS_PARSER);
128128
mappers.put(IpFieldMapper.CONTENT_TYPE, IpFieldMapper.PARSER);
129129
mappers.put(TextFieldMapper.CONTENT_TYPE, new TextFieldMapper.TypeParser());
130-
mappers.put(KeywordFieldMapper.CONTENT_TYPE, new KeywordFieldMapper.TypeParser());
130+
mappers.put(KeywordFieldMapper.CONTENT_TYPE, KeywordFieldMapper.PARSER);
131131
mappers.put(ObjectMapper.CONTENT_TYPE, new ObjectMapper.TypeParser());
132132
mappers.put(ObjectMapper.NESTED_CONTENT_TYPE, new ObjectMapper.TypeParser());
133133
mappers.put(CompletionFieldMapper.CONTENT_TYPE, CompletionFieldMapper.PARSER);

‎server/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ public void testExternalValuesWithMultifield() throws Exception {
115115
Map<String, Mapper.TypeParser> mapperParsers = new HashMap<>();
116116
mapperParsers.put(ExternalMapperPlugin.EXTERNAL, new ExternalMapper.TypeParser(ExternalMapperPlugin.EXTERNAL, "foo"));
117117
mapperParsers.put(TextFieldMapper.CONTENT_TYPE, new TextFieldMapper.TypeParser());
118-
mapperParsers.put(KeywordFieldMapper.CONTENT_TYPE, new KeywordFieldMapper.TypeParser());
118+
mapperParsers.put(KeywordFieldMapper.CONTENT_TYPE, KeywordFieldMapper.PARSER);
119119
MapperRegistry mapperRegistry = new MapperRegistry(mapperParsers, Collections.emptyMap(), MapperPlugin.NOOP_FIELD_FILTER);
120120

121121
Supplier<QueryShardContext> queryShardContext = () -> {

‎server/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java

Lines changed: 11 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,6 @@
2626
import org.apache.lucene.index.IndexOptions;
2727
import org.apache.lucene.index.IndexableField;
2828
import org.apache.lucene.index.IndexableFieldType;
29-
import org.apache.lucene.search.similarities.BM25Similarity;
30-
import org.apache.lucene.search.similarities.BooleanSimilarity;
3129
import org.apache.lucene.util.BytesRef;
3230
import org.elasticsearch.Version;
3331
import org.elasticsearch.cluster.metadata.IndexMetadata;
@@ -41,12 +39,12 @@
4139
import org.elasticsearch.index.analysis.PreConfiguredTokenFilter;
4240
import org.elasticsearch.index.analysis.TokenizerFactory;
4341
import org.elasticsearch.index.mapper.MapperService.MergeReason;
44-
import org.elasticsearch.index.similarity.SimilarityProvider;
4542
import org.elasticsearch.index.termvectors.TermVectorsService;
4643
import org.elasticsearch.indices.analysis.AnalysisModule;
4744
import org.elasticsearch.plugins.AnalysisPlugin;
4845
import org.elasticsearch.plugins.Plugin;
4946
import org.elasticsearch.search.lookup.SourceLookup;
47+
import org.elasticsearch.test.ESSingleNodeTestCase;
5048
import org.elasticsearch.test.InternalSettingsPlugin;
5149
import org.junit.Before;
5250

@@ -56,7 +54,6 @@
5654
import java.util.Collections;
5755
import java.util.List;
5856
import java.util.Map;
59-
import java.util.Set;
6057

6158
import static java.util.Collections.singletonList;
6259
import static java.util.Collections.singletonMap;
@@ -65,17 +62,7 @@
6562
import static org.hamcrest.Matchers.equalTo;
6663
import static org.hamcrest.Matchers.instanceOf;
6764

68-
public class KeywordFieldMapperTests extends FieldMapperTestCase<KeywordFieldMapper.Builder> {
69-
70-
@Override
71-
protected KeywordFieldMapper.Builder newBuilder() {
72-
return new KeywordFieldMapper.Builder("keyword");
73-
}
74-
75-
@Override
76-
protected Set<String> unsupportedProperties() {
77-
return org.elasticsearch.common.collect.Set.of("analyzer");
78-
}
65+
public class KeywordFieldMapperTests extends ESSingleNodeTestCase {
7966

8067
/**
8168
* Creates a copy of the lowercase token filter which we use for testing merge errors.
@@ -99,11 +86,6 @@ protected Collection<Class<? extends Plugin>> getPlugins() {
9986
return pluginList(InternalSettingsPlugin.class, MockAnalysisPlugin.class);
10087
}
10188

102-
@Override
103-
protected Settings getIndexMapperSettings() {
104-
return mapperSettings;
105-
}
106-
10789
private static final Settings mapperSettings = Settings.builder()
10890
.put("index.analysis.normalizer.my_lowercase.type", "custom")
10991
.putList("index.analysis.normalizer.my_lowercase.filter", "lowercase")
@@ -117,18 +99,6 @@ protected Settings getIndexMapperSettings() {
11799
public void setup() {
118100
indexService = createIndex("test", mapperSettings);
119101
parser = indexService.mapperService().documentMapperParser();
120-
addModifier("normalizer", false, (a, b) -> {
121-
a.normalizer(indexService.getIndexAnalyzers(), "my_lowercase");
122-
});
123-
addBooleanModifier("split_queries_on_whitespace", true, KeywordFieldMapper.Builder::splitQueriesOnWhitespace);
124-
addModifier("index_options", false, (a, b) -> {
125-
a.indexOptions(IndexOptions.DOCS);
126-
b.indexOptions(IndexOptions.DOCS_AND_FREQS);
127-
});
128-
addModifier("similarity", false, (a, b) -> {
129-
a.similarity(new SimilarityProvider("BM25", new BM25Similarity()));
130-
b.similarity(new SimilarityProvider("boolean", new BooleanSimilarity()));
131-
});
132102
}
133103

134104
public void testDefaults() throws Exception {
@@ -339,16 +309,17 @@ public void testIndexOptions() throws IOException {
339309
.startObject("properties").startObject("field").field("type", "keyword")
340310
.field("index_options", indexOptions).endObject().endObject()
341311
.endObject().endObject());
342-
IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
312+
MapperParsingException e = expectThrows(MapperParsingException.class,
343313
() -> parser.parse("type", new CompressedXContent(mapping2)));
344-
assertEquals("The [keyword] field does not support positions, got [index_options]=" + indexOptions, e.getMessage());
314+
assertEquals("Unknown value [" + indexOptions + "] for field [index_options] - accepted values are [docs, freqs]",
315+
e.getMessage());
345316
}
346317
}
347318

348319
public void testBoost() throws IOException {
349320
String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type")
350-
.startObject("properties").startObject("field").field("type", "keyword").field("boost", 2f).endObject().endObject()
351-
.endObject().endObject());
321+
.startObject("properties").startObject("field").field("type", "keyword").field("boost", 2f).endObject().endObject()
322+
.endObject().endObject());
352323

353324
DocumentMapper mapper = parser.parse("type", new CompressedXContent(mapping));
354325

@@ -524,8 +495,8 @@ public void testUpdateNormalizer() throws IOException {
524495
() -> indexService.mapperService().merge("type",
525496
new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE));
526497
assertEquals(
527-
"Mapper for [field] conflicts with existing mapping:\n" +
528-
"[mapper [field] has different [analyzer], mapper [field] has different [normalizer]]",
498+
"Mapper for [field] conflicts with existing mapper:\n" +
499+
"\tCannot update parameter [normalizer] from [my_lowercase] to [my_other_lowercase]",
529500
e.getMessage());
530501
}
531502

@@ -650,8 +621,8 @@ public void testParseSourceValue() {
650621
assertEquals("42", ignoreAboveMapper.parseSourceValue(42L, null));
651622
assertEquals("true", ignoreAboveMapper.parseSourceValue(true, null));
652623

653-
KeywordFieldMapper normalizerMapper = new KeywordFieldMapper.Builder("field")
654-
.normalizer(indexService.getIndexAnalyzers(), "lowercase")
624+
KeywordFieldMapper normalizerMapper = new KeywordFieldMapper.Builder("field", indexService.getIndexAnalyzers())
625+
.normalizer("lowercase")
655626
.build(context);
656627
assertEquals("value", normalizerMapper.parseSourceValue("VALUE", null));
657628
assertEquals("42", normalizerMapper.parseSourceValue(42L, null));

‎server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -122,14 +122,16 @@ public static class Builder extends ParametrizedFieldMapper.Builder {
122122
throw new IllegalArgumentException("field [required] must be specified");
123123
}
124124
});
125+
final Parameter<String> restricted
126+
= Parameter.restrictedStringParam("restricted", true, m -> toType(m).restricted, "foo", "bar");
125127

126128
protected Builder(String name) {
127129
super(name);
128130
}
129131

130132
@Override
131133
protected List<Parameter<?>> getParameters() {
132-
return Arrays.asList(fixed, fixed2, variable, index, wrapper, intValue, analyzer, searchAnalyzer, required);
134+
return Arrays.asList(fixed, fixed2, variable, index, wrapper, intValue, analyzer, searchAnalyzer, required, restricted);
133135
}
134136

135137
@Override
@@ -160,6 +162,7 @@ public static class TestMapper extends ParametrizedFieldMapper {
160162
private final NamedAnalyzer searchAnalyzer;
161163
private final boolean index;
162164
private final String required;
165+
private final String restricted;
163166

164167
protected TestMapper(String simpleName, String fullName, MultiFields multiFields, CopyTo copyTo,
165168
ParametrizedMapperTests.Builder builder) {
@@ -173,6 +176,7 @@ protected TestMapper(String simpleName, String fullName, MultiFields multiFields
173176
this.searchAnalyzer = builder.searchAnalyzer.getValue();
174177
this.index = builder.index.getValue();
175178
this.required = builder.required.getValue();
179+
this.restricted = builder.restricted.getValue();
176180
}
177181

178182
@Override
@@ -207,7 +211,7 @@ private static TestMapper fromMapping(String mapping, Version version) {
207211
when(mapperService.getIndexAnalyzers()).thenReturn(indexAnalyzers);
208212
Mapper.TypeParser.ParserContext pc = new Mapper.TypeParser.ParserContext(s -> null, mapperService, s -> {
209213
if (Objects.equals("keyword", s)) {
210-
return new KeywordFieldMapper.TypeParser();
214+
return KeywordFieldMapper.PARSER;
211215
}
212216
if (Objects.equals("binary", s)) {
213217
return BinaryFieldMapper.PARSER;
@@ -241,7 +245,7 @@ public void testDefaults() throws IOException {
241245
assertEquals("{\"field\":{\"type\":\"test_mapper\",\"fixed\":true," +
242246
"\"fixed2\":false,\"variable\":\"default\",\"index\":true," +
243247
"\"wrapper\":\"default\",\"int_value\":5,\"analyzer\":\"_keyword\"," +
244-
"\"search_analyzer\":\"_keyword\",\"required\":\"value\"}}",
248+
"\"search_analyzer\":\"_keyword\",\"required\":\"value\",\"restricted\":\"foo\"}}",
245249
Strings.toString(builder));
246250
}
247251

@@ -439,4 +443,22 @@ public void testRequiredField() {
439443
assertEquals("[required] on mapper [field] of type [test_mapper] must not have a [null] value", exc.getMessage());
440444
}
441445
}
446+
447+
public void testRestrictedField() {
448+
{
449+
String mapping = "{\"type\":\"test_mapper\",\"required\":\"a\",\"restricted\":\"baz\"}";
450+
MapperParsingException e = expectThrows(MapperParsingException.class, () -> fromMapping(mapping));
451+
assertEquals("Unknown value [baz] for field [restricted] - accepted values are [foo, bar]", e.getMessage());
452+
}
453+
{
454+
String mapping = "{\"type\":\"test_mapper\",\"required\":\"a\",\"restricted\":\"bar\"}";
455+
TestMapper mapper = fromMapping(mapping);
456+
assertEquals("bar", mapper.restricted);
457+
}
458+
{
459+
String mapping = "{\"type\":\"test_mapper\",\"required\":\"a\"}";
460+
TestMapper mapper = fromMapping(mapping);
461+
assertEquals("foo", mapper.restricted);
462+
}
463+
}
442464
}

‎server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,8 @@ public void testIllegalDynamicTemplateUnknownAttribute() throws Exception {
339339
DocumentMapper mapper = mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE);
340340
assertThat(mapper.mappingSource().toString(), containsString("\"foo\":\"bar\""));
341341
assertWarnings("dynamic template [my_template] has invalid content [{\"match_mapping_type\":\"string\",\"mapping\":{" +
342-
"\"foo\":\"bar\",\"type\":\"keyword\"}}], caused by [Unused mapping attributes [{foo=bar}]]");
342+
"\"foo\":\"bar\",\"type\":\"keyword\"}}], " +
343+
"caused by [unknown parameter [foo] on mapper [__dynamic__my_template] of type [keyword]]");
343344
}
344345

345346
public void testIllegalDynamicTemplateInvalidAttribute() throws Exception {
@@ -367,7 +368,7 @@ public void testIllegalDynamicTemplateInvalidAttribute() throws Exception {
367368
DocumentMapper mapper = mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE);
368369
assertThat(mapper.mappingSource().toString(), containsString("\"analyzer\":\"foobar\""));
369370
assertWarnings("dynamic template [my_template] has invalid content [{\"match_mapping_type\":\"string\",\"mapping\":{" +
370-
"\"analyzer\":\"foobar\",\"type\":\"text\"}}], caused by [analyzer [foobar] not found for field [__dummy__]]");
371+
"\"analyzer\":\"foobar\",\"type\":\"text\"}}], caused by [analyzer [foobar] not found for field [__dynamic__my_template]]");
371372
}
372373

373374
public void testIllegalDynamicTemplateNoMappingType() throws Exception {
@@ -436,11 +437,11 @@ public void testIllegalDynamicTemplateNoMappingType() throws Exception {
436437
if (useMatchMappingType) {
437438
assertWarnings("dynamic template [my_template] has invalid content [{\"match_mapping_type\":\"*\",\"mapping\":{" +
438439
"\"foo\":\"bar\",\"type\":\"{dynamic_type}\"}}], " +
439-
"caused by [unknown parameter [foo] on mapper [__dummy__] of type [null]]");
440+
"caused by [unknown parameter [foo] on mapper [__dynamic__my_template] of type [binary]]");
440441
} else {
441442
assertWarnings("dynamic template [my_template] has invalid content [{\"match\":\"string_*\",\"mapping\":{" +
442443
"\"foo\":\"bar\",\"type\":\"{dynamic_type}\"}}], " +
443-
"caused by [unknown parameter [foo] on mapper [__dummy__] of type [null]]");
444+
"caused by [unknown parameter [foo] on mapper [__dynamic__my_template] of type [binary]]");
444445
}
445446
}
446447
}

0 commit comments

Comments
 (0)