Skip to content
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

Deprecate XContentType auto detection methods in XContentFactory #22181

Merged

Conversation

javanna
Copy link
Member

@javanna javanna commented Dec 14, 2016

With recent changes to our parsing code we have drastically reduced the places where we auto-detect the content type from the input. The usage of these methods spread in our codebase for no reason, given that in most of the cases we know the content type upfront and we don't need any auto-detection mechanism. Deprecating these methods is a way to try and make sure that these methods are carefully used, and hopefully not introduced in newly written code.

We have yet to fix the REST layer to read the Content-Type header, which is the long term solution, but for now we just want to make sure that the usage of these methods doesn't spread any further.

Relates to #19388

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@jpountz
Copy link
Contributor

jpountz commented Dec 15, 2016

How do we plan to address usage of those methods in non-REST code? For instance I think it is used to detect the format of the _source field in stored fields.

@javanna
Copy link
Member Author

javanna commented Dec 15, 2016

@jpountz I am not sure how we are going to fix all the single places where we use auto-detection. In the REST layer we should start reading the Content-Type header. It is possible that in some other places we will still need it, this deprecation doesn't fix anything but hopefully prevents usages of these methods from spreading further. I see them being used all over the place without any reason.

With recent changes to our parsing code we have drastically reduced the places where we auto-detect the content type from the input. The usage of these methods spread in our codebase for no reason, given that in most of the cases we know the content type upfront and we don't need any auto-detection mechanism. Deprecating these methods is a way to try and make sure that these methods are carefully used, and hopefully not introduced in newly written code.

We have yet to fix the REST layer to read the Content-Type header, which is the long term solution, but for now we just want to make sure that the usage of these methods doesn't spread any further.

Relates to elastic#19388
@javanna javanna force-pushed the deprecation/content_type_auto_detection branch from 5ad5b80 to a7cfd69 Compare December 16, 2016 18:31
@javanna javanna merged commit 2265be6 into elastic:master Dec 16, 2016
@javanna
Copy link
Member Author

javanna commented Dec 16, 2016

Given that there are quite some usages of the deprecated methods some of which will never go away, I went for not backporting this to 5.x. It's just a starting point on master to make people think about whether they need auto-detection or not.

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.

4 participants