Skip to content

Commit b27eaa3

Browse files
authored
Remove 'external values', and replace with swapped out XContentParsers (#72203)
The majority of field mappers read a single value from their positioned XContentParser, and do not need to call nextToken. There is a general assumption that the same holds for any multifields defined on them, and so the XContentParser is passed down to their multifields builder as-is. This assumption does not hold for mappers that accept json objects, and so we have a second mechanism for passing values around called 'external values', where a mapper can set a specific value on its context and child mappers can then check for these external values before reading from xcontent. The disadvantage of this is that every field mapper now needs to check its context for external values. Because the values are defined by their java class, we can also know that in the vast majority of cases this functionality is unused. We have only two mappers that actually make use of this, CompletionFieldMapper and GeoPointFieldMapper. This commit removes external values entirely, and replaces it with the ability to pass a modified XContentParser to multifields. FieldMappers can just check the parser attached to their context for data and don't need to worry about multiple sources. Plugins implementing field mappers will need to take the removal of external values into account. Implementations that are passing structured objects as external values should instead use ParseContext.switchParser and wrap the objects using MapXContentParser.wrapObject(). GeoPointFieldMapper passes on a fake parser that just wraps its input data formatted as a geohash; CompletionFieldMapper has a slightly more complicated parser that in general wraps its metadata, but if textOrNull() is called without the parser being advanced just returns its text input. Relates to #56063
1 parent b0a0d56 commit b27eaa3

File tree

40 files changed

+480
-880
lines changed

40 files changed

+480
-880
lines changed
Lines changed: 246 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,246 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0 and the Server Side Public License, v 1; you may not use this file except
5+
* in compliance with, at your election, the Elastic License 2.0 or the Server
6+
* Side Public License, v 1.
7+
*/
8+
9+
package org.elasticsearch.common.xcontent;
10+
11+
import org.elasticsearch.common.CheckedFunction;
12+
import org.elasticsearch.common.RestApiVersion;
13+
14+
import java.io.IOException;
15+
import java.nio.CharBuffer;
16+
import java.util.List;
17+
import java.util.Map;
18+
import java.util.function.Supplier;
19+
20+
/**
21+
* Filters an existing XContentParser by using a delegate
22+
*/
23+
public abstract class FilterXContentParser implements XContentParser {
24+
25+
protected final XContentParser in;
26+
27+
protected FilterXContentParser(XContentParser in) {
28+
this.in = in;
29+
}
30+
31+
@Override
32+
public XContentType contentType() {
33+
return in.contentType();
34+
}
35+
36+
@Override
37+
public Token nextToken() throws IOException {
38+
return in.nextToken();
39+
}
40+
41+
@Override
42+
public void skipChildren() throws IOException {
43+
in.skipChildren();
44+
}
45+
46+
@Override
47+
public Token currentToken() {
48+
return in.currentToken();
49+
}
50+
51+
@Override
52+
public String currentName() throws IOException {
53+
return in.currentName();
54+
}
55+
56+
@Override
57+
public Map<String, Object> map() throws IOException {
58+
return in.map();
59+
}
60+
61+
@Override
62+
public Map<String, Object> mapOrdered() throws IOException {
63+
return in.mapOrdered();
64+
}
65+
66+
@Override
67+
public Map<String, String> mapStrings() throws IOException {
68+
return in.mapStrings();
69+
}
70+
71+
@Override
72+
public <T> Map<String, T> map(
73+
Supplier<Map<String, T>> mapFactory, CheckedFunction<XContentParser, T, IOException> mapValueParser) throws IOException {
74+
return in.map(mapFactory, mapValueParser);
75+
}
76+
77+
@Override
78+
public List<Object> list() throws IOException {
79+
return in.list();
80+
}
81+
82+
@Override
83+
public List<Object> listOrderedMap() throws IOException {
84+
return in.listOrderedMap();
85+
}
86+
87+
@Override
88+
public String text() throws IOException {
89+
return in.text();
90+
}
91+
92+
@Override
93+
public String textOrNull() throws IOException {
94+
return in.textOrNull();
95+
}
96+
97+
@Override
98+
public CharBuffer charBufferOrNull() throws IOException {
99+
return in.charBufferOrNull();
100+
}
101+
102+
@Override
103+
public CharBuffer charBuffer() throws IOException {
104+
return in.charBuffer();
105+
}
106+
107+
@Override
108+
public Object objectText() throws IOException {
109+
return in.objectText();
110+
}
111+
112+
@Override
113+
public Object objectBytes() throws IOException {
114+
return in.objectBytes();
115+
}
116+
117+
@Override
118+
public boolean hasTextCharacters() {
119+
return in.hasTextCharacters();
120+
}
121+
122+
@Override
123+
public char[] textCharacters() throws IOException {
124+
return in.textCharacters();
125+
}
126+
127+
@Override
128+
public int textLength() throws IOException {
129+
return in.textLength();
130+
}
131+
132+
@Override
133+
public int textOffset() throws IOException {
134+
return in.textOffset();
135+
}
136+
137+
@Override
138+
public Number numberValue() throws IOException {
139+
return in.numberValue();
140+
}
141+
142+
@Override
143+
public NumberType numberType() throws IOException {
144+
return in.numberType();
145+
}
146+
147+
@Override
148+
public short shortValue(boolean coerce) throws IOException {
149+
return in.shortValue(coerce);
150+
}
151+
152+
@Override
153+
public int intValue(boolean coerce) throws IOException {
154+
return in.intValue(coerce);
155+
}
156+
157+
@Override
158+
public long longValue(boolean coerce) throws IOException {
159+
return in.longValue(coerce);
160+
}
161+
162+
@Override
163+
public float floatValue(boolean coerce) throws IOException {
164+
return in.floatValue(coerce);
165+
}
166+
167+
@Override
168+
public double doubleValue(boolean coerce) throws IOException {
169+
return in.doubleValue(coerce);
170+
}
171+
172+
@Override
173+
public short shortValue() throws IOException {
174+
return in.shortValue();
175+
}
176+
177+
@Override
178+
public int intValue() throws IOException {
179+
return in.intValue();
180+
}
181+
182+
@Override
183+
public long longValue() throws IOException {
184+
return in.longValue();
185+
}
186+
187+
@Override
188+
public float floatValue() throws IOException {
189+
return in.floatValue();
190+
}
191+
192+
@Override
193+
public double doubleValue() throws IOException {
194+
return in.doubleValue();
195+
}
196+
197+
@Override
198+
public boolean isBooleanValue() throws IOException {
199+
return in.isBooleanValue();
200+
}
201+
202+
@Override
203+
public boolean booleanValue() throws IOException {
204+
return in.booleanValue();
205+
}
206+
207+
@Override
208+
public byte[] binaryValue() throws IOException {
209+
return in.binaryValue();
210+
}
211+
212+
@Override
213+
public XContentLocation getTokenLocation() {
214+
return in.getTokenLocation();
215+
}
216+
217+
@Override
218+
public <T> T namedObject(Class<T> categoryClass, String name, Object context) throws IOException {
219+
return in.namedObject(categoryClass, name, context);
220+
}
221+
222+
@Override
223+
public NamedXContentRegistry getXContentRegistry() {
224+
return in.getXContentRegistry();
225+
}
226+
227+
@Override
228+
public boolean isClosed() {
229+
return in.isClosed();
230+
}
231+
232+
@Override
233+
public void close() throws IOException {
234+
in.close();
235+
}
236+
237+
@Override
238+
public RestApiVersion getRestApiVersion() {
239+
return in.getRestApiVersion();
240+
}
241+
242+
@Override
243+
public DeprecationHandler getDeprecationHandler() {
244+
return in.getDeprecationHandler();
245+
}
246+
}

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/MatchOnlyTextFieldMapper.java

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -318,12 +318,7 @@ public FieldMapper.Builder getMergeBuilder() {
318318

319319
@Override
320320
protected void parseCreateField(ParseContext context) throws IOException {
321-
final String value;
322-
if (context.externalValueSet()) {
323-
value = context.externalValue().toString();
324-
} else {
325-
value = context.parser().textOrNull();
326-
}
321+
final String value = context.parser().textOrNull();
327322

328323
if (value == null) {
329324
return;

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureFieldMapper.java

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -136,10 +136,7 @@ public RankFeatureFieldType fieldType() {
136136
@Override
137137
protected void parseCreateField(ParseContext context) throws IOException {
138138
float value;
139-
if (context.externalValueSet()) {
140-
Object v = context.externalValue();
141-
value = objectToFloat(v);
142-
} else if (context.parser().currentToken() == Token.VALUE_NULL) {
139+
if (context.parser().currentToken() == Token.VALUE_NULL) {
143140
// skip
144141
return;
145142
} else {

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeaturesFieldMapper.java

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ name, new RankFeaturesFieldType(buildFullName(contentPath), meta.getValue(), pos
5656
}
5757
}
5858

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

6161
public static final class RankFeaturesFieldType extends MappedFieldType {
6262

@@ -117,9 +117,6 @@ public RankFeaturesFieldType fieldType() {
117117

118118
@Override
119119
public void parse(ParseContext context) throws IOException {
120-
if (context.externalValueSet()) {
121-
throw new IllegalArgumentException("[rank_features] fields can't be used in multi-fields");
122-
}
123120

124121
if (context.parser().currentToken() != Token.START_OBJECT) {
125122
throw new IllegalArgumentException("[rank_features] fields must be json objects, expected a START_OBJECT but got: " +

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -316,9 +316,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
316316
XContentParser parser = context.parser();
317317
Object value;
318318
Number numericValue = null;
319-
if (context.externalValueSet()) {
320-
value = context.externalValue();
321-
} else if (parser.currentToken() == Token.VALUE_NULL) {
319+
if (parser.currentToken() == Token.VALUE_NULL) {
322320
value = null;
323321
} else if (coerce.value()
324322
&& parser.currentToken() == Token.VALUE_STRING

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -571,7 +571,7 @@ public SearchAsYouTypeFieldMapper(String simpleName,
571571

572572
@Override
573573
protected void parseCreateField(ParseContext context) throws IOException {
574-
final String value = context.externalValueSet() ? context.externalValue().toString() : context.parser().textOrNull();
574+
final String value = context.parser().textOrNull();
575575
if (value == null) {
576576
return;
577577
}

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/TokenCountFieldMapper.java

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -111,12 +111,7 @@ protected TokenCountFieldMapper(String simpleName, MappedFieldType defaultFieldT
111111

112112
@Override
113113
protected void parseCreateField(ParseContext context) throws IOException {
114-
final String value;
115-
if (context.externalValueSet()) {
116-
value = context.externalValue().toString();
117-
} else {
118-
value = context.parser().textOrNull();
119-
}
114+
final String value = context.parser().textOrNull();
120115

121116
if (value == null && nullValue == null) {
122117
return;

modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeaturesFieldMapperTests.java

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121
import java.util.List;
2222
import java.util.Map;
2323

24+
import static org.hamcrest.Matchers.containsString;
25+
2426
public class RankFeaturesFieldMapperTests extends MapperTestCase {
2527

2628
@Override
@@ -136,6 +138,18 @@ public void testRejectMultiValuedFields() throws MapperParsingException, IOExcep
136138
"the same document", e.getCause().getMessage());
137139
}
138140

141+
public void testCannotBeUsedInMultifields() {
142+
Exception e = expectThrows(MapperParsingException.class, () -> createMapperService(fieldMapping(b -> {
143+
b.field("type", "keyword");
144+
b.startObject("fields");
145+
b.startObject("feature");
146+
b.field("type", "rank_features");
147+
b.endObject();
148+
b.endObject();
149+
})));
150+
assertThat(e.getMessage(), containsString("Field [feature] of type [rank_features] can't be used in multifields"));
151+
}
152+
139153
@Override
140154
protected Object generateRandomInputValue(MappedFieldType ft) {
141155
assumeFalse("Test implemented in a follow up", true);

plugins/analysis-icu/src/main/java/org/elasticsearch/index/mapper/ICUCollationKeywordFieldMapper.java

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -437,15 +437,11 @@ public FieldMapper.Builder getMergeBuilder() {
437437
@Override
438438
protected void parseCreateField(ParseContext context) throws IOException {
439439
final String value;
440-
if (context.externalValueSet()) {
441-
value = context.externalValue().toString();
440+
XContentParser parser = context.parser();
441+
if (parser.currentToken() == XContentParser.Token.VALUE_NULL) {
442+
value = nullValue;
442443
} else {
443-
XContentParser parser = context.parser();
444-
if (parser.currentToken() == XContentParser.Token.VALUE_NULL) {
445-
value = nullValue;
446-
} else {
447-
value = parser.textOrNull();
448-
}
444+
value = parser.textOrNull();
449445
}
450446

451447
if (value == null || value.length() > ignoreAbove) {

plugins/mapper-annotated-text/src/main/java/org/elasticsearch/index/mapper/annotatedtext/AnnotatedTextFieldMapper.java

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -517,12 +517,7 @@ protected AnnotatedTextFieldMapper(String simpleName, FieldType fieldType, Annot
517517

518518
@Override
519519
protected void parseCreateField(ParseContext context) throws IOException {
520-
final String value;
521-
if (context.externalValueSet()) {
522-
value = context.externalValue().toString();
523-
} else {
524-
value = context.parser().textOrNull();
525-
}
520+
final String value = context.parser().textOrNull();
526521

527522
if (value == null) {
528523
return;

0 commit comments

Comments
 (0)