Skip to content

Commit

Permalink
Convert TextFieldMapper to parametrized form (#63269)
Browse files Browse the repository at this point in the history
As a result of this, we can remove a chunk of code from TypeParsers as well. Tests
for search/index mode analyzers have moved into their own file. This commit also
rationalises the serialization checks for parameters into a single SerializerCheck
interface that takes the values includeDefaults, isConfigured and the value
itself.

Relates to #62988
  • Loading branch information
romseygeek authored Oct 7, 2020
1 parent 9d5f59e commit f4c85e4
Show file tree
Hide file tree
Showing 44 changed files with 1,042 additions and 1,300 deletions.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@

package org.elasticsearch.plugin.mapper;

import java.util.Collections;
import java.util.Map;

import org.elasticsearch.index.mapper.Mapper;
import org.elasticsearch.index.mapper.annotatedtext.AnnotatedTextFieldMapper;
import org.elasticsearch.plugins.MapperPlugin;
Expand All @@ -30,15 +27,18 @@
import org.elasticsearch.search.fetch.subphase.highlight.AnnotatedTextHighlighter;
import org.elasticsearch.search.fetch.subphase.highlight.Highlighter;

import java.util.Collections;
import java.util.Map;

public class AnnotatedTextPlugin extends Plugin implements MapperPlugin, SearchPlugin {

@Override
public Map<String, Mapper.TypeParser> getMappers() {
return Collections.singletonMap(AnnotatedTextFieldMapper.CONTENT_TYPE, new AnnotatedTextFieldMapper.TypeParser());
return Collections.singletonMap(AnnotatedTextFieldMapper.CONTENT_TYPE, AnnotatedTextFieldMapper.PARSER);
}

@Override
public Map<String, Highlighter> getHighlighters() {
return Collections.singletonMap(AnnotatedTextHighlighter.NAME, new AnnotatedTextHighlighter());
return Collections.singletonMap(AnnotatedTextHighlighter.NAME, new AnnotatedTextHighlighter());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,7 @@ public void testFetchSourceValue() throws IOException {
Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build();
Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath());

MappedFieldType fieldType = new AnnotatedTextFieldMapper.Builder("field")
.indexAnalyzer(Lucene.STANDARD_ANALYZER)
.searchAnalyzer(Lucene.STANDARD_ANALYZER)
.searchQuoteAnalyzer(Lucene.STANDARD_ANALYZER)
MappedFieldType fieldType = new AnnotatedTextFieldMapper.Builder("field", () -> Lucene.STANDARD_ANALYZER)
.build(context)
.fieldType();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,6 @@ setup:
doc_values: false
text:
type: text
doc_values: false

- do:
headers:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.index.engine.VersionConflictEngineException;
import org.elasticsearch.index.mapper.FieldMapper;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.test.MockKeywordPlugin;

Expand Down Expand Up @@ -218,6 +217,28 @@ public void testSimpleTermVectors() throws IOException {
}
}

public static String termVectorOptionsToString(FieldType fieldType) {
if (!fieldType.storeTermVectors()) {
return "no";
} else if (!fieldType.storeTermVectorOffsets() && !fieldType.storeTermVectorPositions()) {
return "yes";
} else if (fieldType.storeTermVectorOffsets() && !fieldType.storeTermVectorPositions()) {
return "with_offsets";
} else {
StringBuilder builder = new StringBuilder("with");
if (fieldType.storeTermVectorPositions()) {
builder.append("_positions");
}
if (fieldType.storeTermVectorOffsets()) {
builder.append("_offsets");
}
if (fieldType.storeTermVectorPayloads()) {
builder.append("_payloads");
}
return builder.toString();
}
}

public void testRandomSingleTermVectors() throws IOException {
FieldType ft = new FieldType();
int config = randomInt(4);
Expand Down Expand Up @@ -256,7 +277,7 @@ public void testRandomSingleTermVectors() throws IOException {
ft.setStoreTermVectorOffsets(storeOffsets);
ft.setStoreTermVectorPositions(storePositions);

String optionString = FieldMapper.termVectorOptionsToString(ft);
String optionString = termVectorOptionsToString(ft);
XContentBuilder mapping = jsonBuilder().startObject().startObject("_doc")
.startObject("properties")
.startObject("field")
Expand Down Expand Up @@ -988,7 +1009,7 @@ public void testFilterDocFreq() throws ExecutionException, InterruptedException,
}
}

public void testArtificialDocWithPreference() throws ExecutionException, InterruptedException, IOException {
public void testArtificialDocWithPreference() throws InterruptedException, IOException {
// setup indices
Settings.Builder settings = Settings.builder()
.put(indexSettings())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ public void testRecoverMissingAnalyzer() throws Exception {
assertEquals(ex.getMessage(), "Failed to verify index " + metadata.getIndex());
assertNotNull(ex.getCause());
assertEquals(MapperParsingException.class, ex.getCause().getClass());
assertThat(ex.getCause().getMessage(), containsString("analyzer [test] not found for field [field1]"));
assertThat(ex.getCause().getMessage(), containsString("analyzer [test] has not been configured in mappings"));
}

public void testArchiveBrokenClusterSettings() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ public void testUpdateMappingWithNormsConflicts() {
.actionGet();
fail("Expected MergeMappingException");
} catch (IllegalArgumentException e) {
assertThat(e.getMessage(), containsString("mapper [body] has different [norms]"));
assertThat(e.getMessage(), containsString("Cannot update parameter [norms] from [false] to [true]"));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,7 @@ public void testCombineTemplates() throws Exception{
.startObject("field2").field("type", "text").field("analyzer", "custom_1").endObject()
.endObject().endObject().endObject())
.get());
assertThat(e.getMessage(), containsString("analyzer [custom_1] not found for field [field2]"));
assertThat(e.getMessage(), containsString("analyzer [custom_1] has not been configured in mappings"));

response = client().admin().indices().prepareGetTemplates().get();
assertThat(response.getIndexTemplates(), hasSize(1));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,8 @@ public void checkAllowedInMode(AnalysisMode mode) {
TokenFilterFactory[] tokenFilters = ((AnalyzerComponentsProvider) analyzer).getComponents().getTokenFilters();
List<String> offendingFilters = new ArrayList<>();
for (TokenFilterFactory tokenFilter : tokenFilters) {
if (tokenFilter.getAnalysisMode() != mode) {
AnalysisMode filterMode = tokenFilter.getAnalysisMode();
if (filterMode != AnalysisMode.ALL && filterMode != mode) {
offendingFilters.add(tokenFilter.name());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,8 @@ private static Mapper.Builder<?> createBuilderFromDynamicValue(final ParseContex

Mapper.Builder builder = context.root().findTemplateBuilder(context, currentFieldName, XContentFieldType.STRING);
if (builder == null) {
builder = new TextFieldMapper.Builder(currentFieldName)
builder = new TextFieldMapper.Builder(currentFieldName,
() -> context.mapperService().getIndexAnalyzers().getDefaultIndexAnalyzer())
.addMultiField(new KeywordFieldMapper.Builder("keyword").ignoreAbove(256));
}
return builder;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -501,28 +501,6 @@ protected static String indexOptionToString(IndexOptions indexOption) {
}
}

public static String termVectorOptionsToString(FieldType fieldType) {
if (!fieldType.storeTermVectors()) {
return "no";
} else if (!fieldType.storeTermVectorOffsets() && !fieldType.storeTermVectorPositions()) {
return "yes";
} else if (fieldType.storeTermVectorOffsets() && !fieldType.storeTermVectorPositions()) {
return "with_offsets";
} else {
StringBuilder builder = new StringBuilder("with");
if (fieldType.storeTermVectorPositions()) {
builder.append("_positions");
}
if (fieldType.storeTermVectorOffsets()) {
builder.append("_offsets");
}
if (fieldType.storeTermVectorPayloads()) {
builder.append("_payloads");
}
return builder.toString();
}
}

protected abstract String contentType();

public static class MultiFields implements Iterable<Mapper> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
import java.util.Set;
import java.util.function.BiFunction;
import java.util.function.BiPredicate;
import java.util.function.BooleanSupplier;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Supplier;
Expand Down Expand Up @@ -118,7 +117,7 @@ protected final void mergeOptions(FieldMapper other, List<String> conflicts) {
}

@Override
protected final void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
builder.field("type", contentType());
getMergeBuilder().toXContent(builder, includeDefaults);
multiFields.toXContent(builder, params);
Expand All @@ -132,11 +131,25 @@ protected interface Serializer<T> {
void serialize(XContentBuilder builder, String name, T value) throws IOException;
}

/**
* Check on whether or not a parameter should be serialized
*/
protected interface SerializerCheck<T> {
/**
* Check on whether or not a parameter should be serialized
* @param includeDefaults if defaults have been requested
* @param isConfigured if the parameter has a different value to the default
* @param value the parameter value
* @return {@code true} if the value should be serialized
*/
boolean check(boolean includeDefaults, boolean isConfigured, T value);
}

/**
* A configurable parameter for a field mapper
* @param <T> the type of the value the parameter holds
*/
public static final class Parameter<T> {
public static final class Parameter<T> implements Supplier<T> {

public final String name;
private final List<String> deprecatedNames = new ArrayList<>();
Expand All @@ -146,8 +159,7 @@ public static final class Parameter<T> {
private boolean acceptsNull = false;
private Consumer<T> validator = null;
private Serializer<T> serializer = XContentBuilder::field;
private BooleanSupplier serializerPredicate = () -> true;
private boolean alwaysSerialize = false;
private SerializerCheck<T> serializerCheck = (includeDefaults, isConfigured, value) -> includeDefaults || isConfigured;
private Function<T, String> conflictSerializer = Objects::toString;
private BiPredicate<T, T> mergeValidator;
private T value;
Expand Down Expand Up @@ -178,6 +190,11 @@ public T getValue() {
return isSet ? value : defaultValue.get();
}

@Override
public T get() {
return getValue();
}

/**
* Returns the default value of the parameter
*/
Expand Down Expand Up @@ -234,19 +251,26 @@ public Parameter<T> setSerializer(Serializer<T> serializer, Function<T, String>
}

/**
* Sets an additional check on whether or not this parameter should be serialized,
* after the existing 'set' and 'include_defaults' checks.
* Configure a custom serialization check for this parameter
*/
public Parameter<T> setShouldSerialize(BooleanSupplier shouldSerialize) {
this.serializerPredicate = shouldSerialize;
public Parameter<T> setSerializerCheck(SerializerCheck<T> check) {
this.serializerCheck = check;
return this;
}

/**
* Ensures that this parameter is always serialized, no matter its value
* Always serialize this parameter, no matter its value
*/
public Parameter<T> alwaysSerialize() {
this.alwaysSerialize = true;
this.serializerCheck = (id, ic, v) -> true;
return this;
}

/**
* Never serialize this parameter, no matter its value
*/
public Parameter<T> neverSerialize() {
this.serializerCheck = (id, ic, v) -> false;
return this;
}

Expand Down Expand Up @@ -283,8 +307,8 @@ private void merge(FieldMapper toMerge, Conflicts conflicts) {
}
}

private void toXContent(XContentBuilder builder, boolean includeDefaults) throws IOException {
if (alwaysSerialize || ((includeDefaults || isConfigured()) && serializerPredicate.getAsBoolean())) {
protected void toXContent(XContentBuilder builder, boolean includeDefaults) throws IOException {
if (serializerCheck.check(includeDefaults, isConfigured(), get())) {
serializer.serialize(builder, name, getValue());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ public Builder(String name, RangeType type, boolean coerceByDefault) {
this.type = type;
this.coerce = Parameter.explicitBoolParam("coerce", true, m -> toType(m).coerce, coerceByDefault);
if (this.type != RangeType.DATE) {
format.setShouldSerialize(() -> false);
locale.setShouldSerialize(() -> false);
format.neverSerialize();
locale.neverSerialize();
}
}

Expand Down
Loading

0 comments on commit f4c85e4

Please sign in to comment.