-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Handle legacy mappings with placeholder fields #85059
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
Changes from 22 commits
a70bf43
20fd0c1
d66e928
1523ca1
2b0395b
42e3911
5e1ea96
114dd7f
2a33e8e
3e579ec
fbd4c4f
b761fd1
cdf33dd
d49e1bb
89e8bfd
5f01d4a
6fd1366
713dbc2
880fbe6
f75f5d2
ad99173
07234b1
68408c8
cb213b1
2482c26
c677101
b25369e
4c86d3c
f163412
0b1157b
07a2602
5b85b55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
|
||
package org.elasticsearch.index.mapper; | ||
|
||
import org.elasticsearch.Version; | ||
import org.elasticsearch.common.xcontent.support.XContentMapValues; | ||
import org.elasticsearch.xcontent.XContentBuilder; | ||
|
||
|
@@ -124,6 +125,11 @@ public Mapper.Builder parse(String name, Map<String, Object> node, MappingParser | |
} | ||
return builder.path(path); | ||
} | ||
|
||
@Override | ||
public boolean supportsVersion(Version indexCreatedVersion) { | ||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this I find a bit surprising especially given that field aliases were added with 6.4 . I wonder if this is never called, in which case maybe we should throw exception instead, or if it could rely on the default impl instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even though field aliases were only added in ES 6.4, they could be used with older indices as well (i.e. you had a 5.x index for example that you kept using in 6.4 and now added the field alias to the mapping of that 5.x index). As field aliases are more of a runtime property (e.g. just like runtime fields, which you could also use on indices created before the release of runtime fields) and have no connection to the actual data, I see no reason to limit their use. |
||
} | ||
} | ||
|
||
public static class Builder extends Mapper.Builder { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1191,7 +1191,7 @@ public Builder init(FieldMapper initializer) { | |
return this; | ||
} | ||
|
||
private void merge(FieldMapper in, Conflicts conflicts) { | ||
protected void merge(FieldMapper in, Conflicts conflicts) { | ||
for (Parameter<?> param : getParameters()) { | ||
param.merge(in, conflicts); | ||
} | ||
|
@@ -1238,7 +1238,7 @@ protected void addScriptValidation( | |
* Writes the current builder parameter values as XContent | ||
*/ | ||
@Override | ||
public final XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
boolean includeDefaults = params.paramAsBoolean("include_defaults", false); | ||
for (Parameter<?> parameter : getParameters()) { | ||
parameter.toXContent(builder, includeDefaults); | ||
|
@@ -1304,6 +1304,12 @@ public final void parse(String name, MappingParserContext parserContext, Map<Str | |
parameter = paramsMap.get(propName); | ||
} | ||
if (parameter == null) { | ||
if (parserContext.indexVersionCreated().isLegacyIndexVersion()) { | ||
// ignore unknown parameters on legacy indices | ||
handleUnknownParam(propName, propNode); | ||
iterator.remove(); | ||
continue; | ||
} | ||
if (isDeprecatedParameter(propName, parserContext.indexVersionCreated())) { | ||
deprecationLogger.warn( | ||
DeprecationCategory.API, | ||
|
@@ -1352,6 +1358,10 @@ public final void parse(String name, MappingParserContext parserContext, Map<Str | |
validate(); | ||
} | ||
|
||
protected void handleUnknownParam(String propName, Object propNode) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we rename it to something that removes any doubt that this is only called for legacy indices? handleLegacyindexUnknownParam ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've renamed the method to handleUnknownParamOnLegacyIndex in f163412 |
||
// ignore | ||
} | ||
|
||
protected static ContentPath parentPath(String name) { | ||
int endPos = name.lastIndexOf("."); | ||
if (endPos == -1) { | ||
|
@@ -1388,21 +1398,35 @@ public static final class TypeParser implements Mapper.TypeParser { | |
|
||
private final BiFunction<String, MappingParserContext, Builder> builderFunction; | ||
private final BiConsumer<String, MappingParserContext> contextValidator; | ||
private Version minimumCompatibilityVersion; // see Mapper.TypeParser#supportsVersion() | ||
ywelsch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* Creates a new TypeParser | ||
* @param builderFunction a function that produces a Builder from a name and parsercontext | ||
*/ | ||
public TypeParser(BiFunction<String, MappingParserContext, Builder> builderFunction) { | ||
this(builderFunction, (n, c) -> {}); | ||
this(builderFunction, (n, c) -> {}, Version.CURRENT.minimumIndexCompatibilityVersion()); | ||
} | ||
|
||
public TypeParser(BiFunction<String, MappingParserContext, Builder> builderFunction, Version minimumCompatibilityVersion) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would it help to add javadocs here to clarify when to use which constructor? as far as I understand, the constructor that takes the version is the one called by all the mappers that are supported on legacy indices, all the rest remains the same? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed in f163412 |
||
this(builderFunction, (n, c) -> {}, minimumCompatibilityVersion); | ||
} | ||
|
||
public TypeParser( | ||
BiFunction<String, MappingParserContext, Builder> builderFunction, | ||
BiConsumer<String, MappingParserContext> contextValidator | ||
) { | ||
this(builderFunction, contextValidator, Version.CURRENT.minimumIndexCompatibilityVersion()); | ||
} | ||
|
||
public TypeParser( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this one could be made private There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed in f163412 |
||
BiFunction<String, MappingParserContext, Builder> builderFunction, | ||
BiConsumer<String, MappingParserContext> contextValidator, | ||
Version minimumCompatibilityVersion | ||
) { | ||
this.builderFunction = builderFunction; | ||
this.contextValidator = contextValidator; | ||
this.minimumCompatibilityVersion = minimumCompatibilityVersion; | ||
} | ||
|
||
@Override | ||
|
@@ -1412,6 +1436,11 @@ public Builder parse(String name, Map<String, Object> node, MappingParserContext | |
builder.parse(name, parserContext, node); | ||
return builder; | ||
} | ||
|
||
@Override | ||
public boolean supportsVersion(Version indexCreatedVersion) { | ||
return indexCreatedVersion.onOrAfter(minimumCompatibilityVersion); | ||
} | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I find it slightly confusing that this check method is now returning the mappings. I can see how it is convenient to return the mappings obtained from the merge operation. Would it make sense renaming the method to adapt it to the updated behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed in 07a2602