Continue to accept unused 'universal' params in <8.0 indexes #59381
Conversation
romseygeek
added
>breaking
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
| @@ -330,6 +335,12 @@ public final void parse(String name, TypeParser.ParserContext parserContext, Map | |||
| } | |||
| Parameter<?> parameter = paramsMap.get(propName); | |||
| if (parameter == null) { | |||
| if (checkForDeprecatedParameter(propName, parserContext.indexVersionCreated())) { | |||
| deprecationLogger.deprecate(propName, | |||
| "Parameter [{}] is unused on type [{}] and will be removed in future", propName, type); | |||
There was a problem hiding this comment.
maybe saying "has no effect" would be clearer to users?
| private static final Set<String> DEPRECATED_PARAMS | ||
| = Set.of("store", "meta", "index", "doc_values", "boost", "index_options", "similarity"); | ||
|
|
||
| private boolean checkForDeprecatedParameter(String propName, Version indexCreatedVersion) { |
There was a problem hiding this comment.
also should we rename to isDeprecatedParameter? I find check more ambiguous in what it does.
Yes, the idea is that we shouldn't fail to open an index that had its mappings configured without any problems in an earlier version of ES. |
|
@romseygeek is this a breaking change alone? Possibly the introduction of parameterized field mapper was instead? |
|
Yeah, #58663 is the actual breaking change - I'll remove the tag from this issue and put it on that one instead. Thanks! |
|
@elasticmachine update branch |
We have a number of parameters which are universally parsed by almost all mappers, whether or not they make sense. Migrating the binary and boolean mappers to the new style of declaring their parameters explicitly has meant that these universal parameters stopped being accepted, which would break existing mappings. This commit adds some extra logic to ParametrizedFieldMapper that checks for the existence of these universal parameters, and issues a warning on 7x indexes if it finds them. Indexes created in 8.0 and beyond will throw an error. Fixes #59359
|
@romseygeek I'm trying to understand how this work affects the mappings editor UI in Kibana. This editor provides a form for configuring the parameters of each mapping type. We need a list of the acceptable parameters for each mapping type in order to make sure the forms accurately represent each type's parameters. Could you link to the documentation that makes these parameters explicit for the mappings types you've touched, or clarify your changes in the PR description? Thanks! |
|
@cjcenizal in each case I'm checking the set of parameters for a field mapper against its existing documentation page. The set of parameters mentioned in this PR are ones that were automatically parsed, but either had no effect or only had a single allowed value, so we wouldn't want them exposed in the mappings editor in any case. |
|
@romseygeek Thanks for explaining! To be clear, I'm just looking for an explicit list of which parameters now issue warnings, and for which field types. Could you write that down for me or direct me to somewhere it is written down (e.g. docs)? |
|
I chatted with Alan about this over Zoom and he cleared things up for me. Some fields used to accept undocumented parameters, which they would silently accept. The code has been changed so that these fields still accept the undocumented parameters (which remain undocumented), except now a deprecation warning is issued so that consumers will be notified that they'll be rejected in the future. The net result is that the contract created by the docs hasn't changed, so code built upon this contract (as we did with mappings editor) will still work as expected. |
We have a number of parameters which are universally parsed by almost all
mappers, whether or not they make sense. Migrating the binary and boolean
mappers to the new style of declaring their parameters explicitly has meant
that these universal parameters stopped being accepted, which would break
existing mappings.
This commit adds some extra logic to ParametrizedFieldMapper that checks
for the existence of these universal parameters, and issues a warning on
7x indexes if it finds them. Indexes created in 8.0 and beyond will throw an
error.
Fixes #59359