Skip to content

Conversation

@dakrone
Copy link
Member

@dakrone dakrone commented Feb 1, 2018

This commit switches all the modules and server test code to use the
non-deprecated ParseField.match method, passing in the logging deprecation
handler.

Relates to #28449

This commit switches all the modules and server test code to use the
non-deprecated `ParseField.match` method, passing in the logging deprecation
handler.

Relates to elastic#28449
@dakrone
Copy link
Member Author

dakrone commented Feb 1, 2018

This moved everything except for the server/src/main stuff over to non-deprecated. I figured it's easier to review in small chunks

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

I think you should use the deprecation handler in the parser. That way we can decide how to handle deprecations at parser construction time. Which is the time when you know who sent you the request and you can reason about how to tell them about the deprecation. The way you have it now you will always use the LoggingDeprecationHandler in these locations regardless of how the parser was built which seems wrong.

protected boolean token(String aggregationName, String currentFieldName, XContentParser.Token token, XContentParser parser,
Map<ParseField, Object> otherOptions) throws IOException {
if (MULTIVALUE_MODE_FIELD.match(currentFieldName)) {
if (MULTIVALUE_MODE_FIELD.match(currentFieldName, LoggingDeprecationHandler.INSTANCE)) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should be

 +        if (MULTIVALUE_MODE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { 

@dakrone
Copy link
Member Author

dakrone commented Feb 1, 2018

I think you should use the deprecation handler in the parser. That way we can decide how to handle deprecations at parser construction time.

Makes sense, I will re-open this with those since the history doesn't really make sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants