Skip to content

Remove 'external values', and replace with swapped out XContentParsers #72203

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

Merged
merged 6 commits into from
Apr 29, 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
@@ -0,0 +1,246 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

package org.elasticsearch.common.xcontent;

import org.elasticsearch.common.CheckedFunction;
import org.elasticsearch.common.RestApiVersion;

import java.io.IOException;
import java.nio.CharBuffer;
import java.util.List;
import java.util.Map;
import java.util.function.Supplier;

/**
* Filters an existing XContentParser by using a delegate
*/
public abstract class FilterXContentParser implements XContentParser {

protected final XContentParser in;

protected FilterXContentParser(XContentParser in) {
this.in = in;
}

@Override
public XContentType contentType() {
return in.contentType();
}

@Override
public Token nextToken() throws IOException {
return in.nextToken();
}

@Override
public void skipChildren() throws IOException {
in.skipChildren();
}

@Override
public Token currentToken() {
return in.currentToken();
}

@Override
public String currentName() throws IOException {
return in.currentName();
}

@Override
public Map<String, Object> map() throws IOException {
return in.map();
}

@Override
public Map<String, Object> mapOrdered() throws IOException {
return in.mapOrdered();
}

@Override
public Map<String, String> mapStrings() throws IOException {
return in.mapStrings();
}

@Override
public <T> Map<String, T> map(
Supplier<Map<String, T>> mapFactory, CheckedFunction<XContentParser, T, IOException> mapValueParser) throws IOException {
return in.map(mapFactory, mapValueParser);
}

@Override
public List<Object> list() throws IOException {
return in.list();
}

@Override
public List<Object> listOrderedMap() throws IOException {
return in.listOrderedMap();
}

@Override
public String text() throws IOException {
return in.text();
}

@Override
public String textOrNull() throws IOException {
return in.textOrNull();
}

@Override
public CharBuffer charBufferOrNull() throws IOException {
return in.charBufferOrNull();
}

@Override
public CharBuffer charBuffer() throws IOException {
return in.charBuffer();
}

@Override
public Object objectText() throws IOException {
return in.objectText();
}

@Override
public Object objectBytes() throws IOException {
return in.objectBytes();
}

@Override
public boolean hasTextCharacters() {
return in.hasTextCharacters();
}

@Override
public char[] textCharacters() throws IOException {
return in.textCharacters();
}

@Override
public int textLength() throws IOException {
return in.textLength();
}

@Override
public int textOffset() throws IOException {
return in.textOffset();
}

@Override
public Number numberValue() throws IOException {
return in.numberValue();
}

@Override
public NumberType numberType() throws IOException {
return in.numberType();
}

@Override
public short shortValue(boolean coerce) throws IOException {
return in.shortValue(coerce);
}

@Override
public int intValue(boolean coerce) throws IOException {
return in.intValue(coerce);
}

@Override
public long longValue(boolean coerce) throws IOException {
return in.longValue(coerce);
}

@Override
public float floatValue(boolean coerce) throws IOException {
return in.floatValue(coerce);
}

@Override
public double doubleValue(boolean coerce) throws IOException {
return in.doubleValue(coerce);
}

@Override
public short shortValue() throws IOException {
return in.shortValue();
}

@Override
public int intValue() throws IOException {
return in.intValue();
}

@Override
public long longValue() throws IOException {
return in.longValue();
}

@Override
public float floatValue() throws IOException {
return in.floatValue();
}

@Override
public double doubleValue() throws IOException {
return in.doubleValue();
}

@Override
public boolean isBooleanValue() throws IOException {
return in.isBooleanValue();
}

@Override
public boolean booleanValue() throws IOException {
return in.booleanValue();
}

@Override
public byte[] binaryValue() throws IOException {
return in.binaryValue();
}

@Override
public XContentLocation getTokenLocation() {
return in.getTokenLocation();
}

@Override
public <T> T namedObject(Class<T> categoryClass, String name, Object context) throws IOException {
return in.namedObject(categoryClass, name, context);
}

@Override
public NamedXContentRegistry getXContentRegistry() {
return in.getXContentRegistry();
}

@Override
public boolean isClosed() {
return in.isClosed();
}

@Override
public void close() throws IOException {
in.close();
}

@Override
public RestApiVersion getRestApiVersion() {
return in.getRestApiVersion();
}

@Override
public DeprecationHandler getDeprecationHandler() {
return in.getDeprecationHandler();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -318,12 +318,7 @@ public FieldMapper.Builder getMergeBuilder() {

@Override
protected void parseCreateField(ParseContext context) throws IOException {
final String value;
if (context.externalValueSet()) {
value = context.externalValue().toString();
} else {
value = context.parser().textOrNull();
}
final String value = context.parser().textOrNull();

if (value == null) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,7 @@ public RankFeatureFieldType fieldType() {
@Override
protected void parseCreateField(ParseContext context) throws IOException {
float value;
if (context.externalValueSet()) {
Object v = context.externalValue();
value = objectToFloat(v);
} else if (context.parser().currentToken() == Token.VALUE_NULL) {
if (context.parser().currentToken() == Token.VALUE_NULL) {
// skip
return;
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ name, new RankFeaturesFieldType(buildFullName(contentPath), meta.getValue(), pos
}
}

public static final TypeParser PARSER = new TypeParser((n, c) -> new Builder(n));
public static final TypeParser PARSER = new TypeParser((n, c) -> new Builder(n), notInMultiFields(CONTENT_TYPE));

public static final class RankFeaturesFieldType extends MappedFieldType {

Expand Down Expand Up @@ -117,9 +117,6 @@ public RankFeaturesFieldType fieldType() {

@Override
public void parse(ParseContext context) throws IOException {
if (context.externalValueSet()) {
throw new IllegalArgumentException("[rank_features] fields can't be used in multi-fields");
}
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 moved these checks (where they existed) to the TypeParser so they are now caught at mapping time, instead of when you try and index your first document. There are probably more places where we could do this, but for now I've limited it to just those mappers that were already checking. I've added tests for them as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice clean-up!


if (context.parser().currentToken() != Token.START_OBJECT) {
throw new IllegalArgumentException("[rank_features] fields must be json objects, expected a START_OBJECT but got: " +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -316,9 +316,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
XContentParser parser = context.parser();
Object value;
Number numericValue = null;
if (context.externalValueSet()) {
value = context.externalValue();
} else if (parser.currentToken() == Token.VALUE_NULL) {
if (parser.currentToken() == Token.VALUE_NULL) {
value = null;
} else if (coerce.value()
&& parser.currentToken() == Token.VALUE_STRING
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ public SearchAsYouTypeFieldMapper(String simpleName,

@Override
protected void parseCreateField(ParseContext context) throws IOException {
final String value = context.externalValueSet() ? context.externalValue().toString() : context.parser().textOrNull();
final String value = context.parser().textOrNull();
if (value == null) {
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,7 @@ protected TokenCountFieldMapper(String simpleName, MappedFieldType defaultFieldT

@Override
protected void parseCreateField(ParseContext context) throws IOException {
final String value;
if (context.externalValueSet()) {
value = context.externalValue().toString();
} else {
value = context.parser().textOrNull();
}
final String value = context.parser().textOrNull();

if (value == null && nullValue == null) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import java.util.List;
import java.util.Map;

import static org.hamcrest.Matchers.containsString;

public class RankFeaturesFieldMapperTests extends MapperTestCase {

@Override
Expand Down Expand Up @@ -136,6 +138,18 @@ public void testRejectMultiValuedFields() throws MapperParsingException, IOExcep
"the same document", e.getCause().getMessage());
}

public void testCannotBeUsedInMultifields() {
Exception e = expectThrows(MapperParsingException.class, () -> createMapperService(fieldMapping(b -> {
b.field("type", "keyword");
b.startObject("fields");
b.startObject("feature");
b.field("type", "rank_features");
b.endObject();
b.endObject();
})));
assertThat(e.getMessage(), containsString("Field [feature] of type [rank_features] can't be used in multifields"));
}

@Override
protected Object generateRandomInputValue(MappedFieldType ft) {
assumeFalse("Test implemented in a follow up", true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -437,15 +437,11 @@ public FieldMapper.Builder getMergeBuilder() {
@Override
protected void parseCreateField(ParseContext context) throws IOException {
final String value;
if (context.externalValueSet()) {
value = context.externalValue().toString();
XContentParser parser = context.parser();
if (parser.currentToken() == XContentParser.Token.VALUE_NULL) {
value = nullValue;
} else {
XContentParser parser = context.parser();
if (parser.currentToken() == XContentParser.Token.VALUE_NULL) {
value = nullValue;
} else {
value = parser.textOrNull();
}
value = parser.textOrNull();
}

if (value == null || value.length() > ignoreAbove) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -517,12 +517,7 @@ protected AnnotatedTextFieldMapper(String simpleName, FieldType fieldType, Annot

@Override
protected void parseCreateField(ParseContext context) throws IOException {
final String value;
if (context.externalValueSet()) {
value = context.externalValue().toString();
} else {
value = context.parser().textOrNull();
}
final String value = context.parser().textOrNull();

if (value == null) {
return;
Expand Down
Loading