Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RuntimeField.Builder should not extend FieldMapper.Builder #73840

Merged
merged 6 commits into from
Jun 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,10 @@ public Parameter<T> acceptsNull() {
return this;
}

public boolean canAcceptNull() {
return acceptsNull;
}

/**
* Adds a deprecated parameter name.
*
Expand Down Expand Up @@ -757,7 +761,13 @@ private void init(FieldMapper toInit) {
setValue(initializer.apply(toInit));
}

private void parse(String field, ParserContext context, Object in) {
/**
* Parse the field value from an Object
* @param field the field name
* @param context the parser context
* @param in the object
*/
public void parse(String field, ParserContext context, Object in) {
setValue(parser.apply(field, context, in));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,13 @@

package org.elasticsearch.index.mapper;

import org.elasticsearch.Version;
import org.elasticsearch.common.logging.DeprecationCategory;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.index.mapper.FieldMapper.Parameter;

import java.io.IOException;
import java.util.Collection;
Expand Down Expand Up @@ -58,51 +63,69 @@ default XContentBuilder toXContent(XContentBuilder builder, Params params) throw
*/
Collection<MappedFieldType> asMappedFieldTypes();

/**
* For runtime fields the {@link RuntimeField.Parser} returns directly the {@link MappedFieldType}.
* Internally we still create a {@link RuntimeField.Builder} so we reuse the {@link FieldMapper.Parameter} infrastructure,
* but {@link RuntimeField.Builder#init(FieldMapper)} and {@link RuntimeField.Builder#build(ContentPath)} are never called as
* {@link RuntimeField.Parser#parse(String, Map, Mapper.TypeParser.ParserContext)} calls
* {@link RuntimeField.Builder#parse(String, Mapper.TypeParser.ParserContext, Map)} and returns the corresponding
* {@link MappedFieldType}.
*/
abstract class Builder extends FieldMapper.Builder {
final FieldMapper.Parameter<Map<String, String>> meta = FieldMapper.Parameter.metaParam();
abstract class Builder implements ToXContent {
final String name;
final Parameter<Map<String, String>> meta = Parameter.metaParam();

private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RuntimeField.class);

protected Builder(String name) {
super(name);
this.name = name;
}

public Map<String, String> meta() {
return meta.getValue();
}

@Override
protected List<FieldMapper.Parameter<?>> getParameters() {
protected List<Parameter<?>> getParameters() {
return Collections.singletonList(meta);
}

@Override
public FieldMapper.Builder init(FieldMapper initializer) {
throw new UnsupportedOperationException();
}
protected abstract RuntimeField createRuntimeField(Mapper.TypeParser.ParserContext parserContext);

@Override
public final FieldMapper build(ContentPath context) {
throw new UnsupportedOperationException();
public final XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been wondering why the builder is only responsible to print out its parameters. Could we include RuntimeField#toXContent in this so that we don't need to wrap it? If we do so maybe the toXContent within AbstractScriptFieldType could take a RUntimeField.Builder argument instead of ToXCOntent?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's see if we can do this as a follow-up, not important for merging

boolean includeDefaults = params.paramAsBoolean("include_defaults", false);
for (Parameter<?> parameter : getParameters()) {
parameter.toXContent(builder, includeDefaults);
}
return builder;
}

protected abstract RuntimeField createRuntimeField(Mapper.TypeParser.ParserContext parserContext);

private void validate() {
ContentPath contentPath = parentPath(name());
FieldMapper.MultiFields multiFields = multiFieldsBuilder.build(this, contentPath);
if (multiFields.iterator().hasNext()) {
throw new IllegalArgumentException("runtime field [" + name + "] does not support [fields]");
public final void parse(String name, Mapper.TypeParser.ParserContext parserContext, Map<String, Object> fieldNode) {
Map<String, Parameter<?>> paramsMap = new HashMap<>();
for (Parameter<?> param : getParameters()) {
paramsMap.put(param.name, param);
}
FieldMapper.CopyTo copyTo = this.copyTo.build();
if (copyTo.copyToFields().isEmpty() == false) {
throw new IllegalArgumentException("runtime field [" + name + "] does not support [copy_to]");
String type = (String) fieldNode.remove("type");
for (Iterator<Map.Entry<String, Object>> iterator = fieldNode.entrySet().iterator(); iterator.hasNext();) {
Map.Entry<String, Object> entry = iterator.next();
final String propName = entry.getKey();
final Object propNode = entry.getValue();
Parameter<?> parameter = paramsMap.get(propName);
if (parameter == null) {
if (parserContext.isFromDynamicTemplate() && parserContext.indexVersionCreated().before(Version.V_8_0_0)) {
// The parameter is unknown, but this mapping is from a dynamic template.
// Until 7.x it was possible to use unknown parameters there, so for bwc we need to ignore it
deprecationLogger.deprecate(DeprecationCategory.API, propName,
"Parameter [{}] is used in a dynamic template mapping and has no effect on type [{}]. "
+ "Usage will result in an error in future major versions and should be removed.",
propName,
type
);
iterator.remove();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this deserve a new test that targets runtime fields specifically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been writing tests for this and I think I've found a bug with the existing impl - we're supposed to issue a deprecation warning if dynamic templates contain mappings with bad parameters, but we actually throw an error because the template validation doesn't propagate its 'within a dynamic template' context. Will open a separate PR for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good

continue;
}
throw new MapperParsingException(
"unknown parameter [" + propName + "] on runtime field [" + name + "] of type [" + type + "]"
);
}
if (propNode == null && parameter.canAcceptNull() == false) {
throw new MapperParsingException("[" + propName + "] on runtime field [" + name
+ "] of type [" + type + "] must not have a [null] value");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's worth factoring out this code that is common between the two implementations instead of duplicating it, though it would mean that the base class has to allow to be extended to introduce parsing of common fields etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the two methods are sufficiently different that it's not worth trying to share stuff here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are they? I did not realize that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have not deprecated anything on the runtime section so far, but I do wonder: should we handle deprecations like we do for field mappers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this makes sense only if we share the code between the two, supporting parameters deprecation while there isn't one deprecated parameter is overkill

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ we can copy the machinery from FieldMapper easily enough if we need to

parameter.parse(name, parserContext, propNode);
iterator.remove();
}
}
}
Expand All @@ -123,7 +146,6 @@ RuntimeField parse(String name, Map<String, Object> node, Mapper.TypeParser.Pars

RuntimeField.Builder builder = builderFunction.apply(name);
builder.parse(name, parserContext, node);
builder.validate();
return builder.createRuntimeField(parserContext);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.util.Map;
import java.util.function.BiConsumer;

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.mockito.Matchers.anyString;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -106,7 +107,7 @@ public void testCopyToIsNotSupported() throws IOException {
b.field("copy_to", "target");
});
MapperParsingException exception = expectThrows(MapperParsingException.class, () -> createMapperService(mapping));
assertEquals("Failed to parse mapping: runtime field [field] does not support [copy_to]", exception.getMessage());
assertThat(exception.getMessage(), containsString("unknown parameter [copy_to] on runtime field"));
}

public void testMultiFieldsIsNotSupported() throws IOException {
Expand All @@ -115,7 +116,7 @@ public void testMultiFieldsIsNotSupported() throws IOException {
b.startObject("fields").startObject("test").field("type", "keyword").endObject().endObject();
});
MapperParsingException exception = expectThrows(MapperParsingException.class, () -> createMapperService(mapping));
assertEquals("Failed to parse mapping: runtime field [field] does not support [fields]", exception.getMessage());
assertThat(exception.getMessage(), containsString("unknown parameter [fields] on runtime field"));
}

public void testStoredScriptsAreNotSupported() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ public void testIllegalDynamicTemplateUnknownAttributeRuntime() throws Exception
assertEquals("Failed to parse mapping: dynamic template [my_template] has invalid content [" +
"{\"match_mapping_type\":\"string\",\"runtime\":{\"foo\":\"bar\",\"type\":\"keyword\"}}], " +
"attempted to validate it with the following match_mapping_type: [string]", e.getMessage());
assertEquals("unknown parameter [foo] on mapper [__dynamic__my_template] of type [keyword]", e.getRootCause().getMessage());
assertEquals("unknown parameter [foo] on runtime field [__dynamic__my_template] of type [keyword]", e.getRootCause().getMessage());
}

public void testIllegalDynamicTemplateInvalidAttribute() throws Exception {
Expand Down Expand Up @@ -497,7 +497,7 @@ public void testIllegalDynamicTemplateNoMappingTypeRuntime() throws Exception {
assertThat(e.getMessage(), containsString("Failed to parse mapping: dynamic template [my_template] has invalid content ["));
assertThat(e.getMessage(), containsString("attempted to validate it with the following match_mapping_type: " +
"[string, long, double, boolean, date]"));
assertEquals("unknown parameter [foo] on mapper [__dynamic__my_template] of type [date]", e.getRootCause().getMessage());
assertEquals("unknown parameter [foo] on runtime field [__dynamic__my_template] of type [date]", e.getRootCause().getMessage());
}

public void testIllegalDynamicTemplate7DotXIndex() throws Exception {
Expand Down Expand Up @@ -680,7 +680,7 @@ public void testRuntimeSectionWrongFormat() throws IOException {
public void testRuntimeSectionRemainingField() throws IOException {
XContentBuilder mapping = runtimeFieldMapping(builder -> builder.field("type", "keyword").field("unsupported", "value"));
MapperParsingException e = expectThrows(MapperParsingException.class, () -> createMapperService(mapping));
assertEquals("Failed to parse mapping: unknown parameter [unsupported] on mapper [field] of type [keyword]", e.getMessage());
assertEquals("Failed to parse mapping: unknown parameter [unsupported] on runtime field [field] of type [keyword]", e.getMessage());
}

public void testTemplateWithoutMatchPredicates() throws Exception {
Expand Down