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 application/json content type in favour of x-ndjson where content streaming is supported #25718

Closed
javanna opened this issue Jul 14, 2017 · 13 comments
Labels
>bug :Core/Infra/REST API REST infrastructure and utilities >deprecation Team:Core/Infra Meta label for core/infra team

Comments

@javanna
Copy link
Member

javanna commented Jul 14, 2017

Endpoints that support content streaming (bulk, msearch) currently supports both application/json and application/x-ndjson as content type. We'd like to enforce application/x-ndjson in the future and remove support for application/json, but we first need to deprecate it.

This will require some coordination with the clients team and we may need to update the REST spec in order to indicate which APIs support content streaming.

@dakrone
Copy link
Member

dakrone commented Jul 14, 2017

I don't think we should deprecate application/json until ndjson is an registered spec.

For instance, in RFC 6648 the x- prefix is already decided as "deprecated".

I don't think we should allow only application/x-ndjson until ndjson/ndjson-spec#19 is resolved, the spec is registered with the IANA, and application/ndjson can be used.

@javanna
Copy link
Member Author

javanna commented Jul 14, 2017

good point then we should document that we still support application/json ? I will mark for discussion just to make sure.

@dadoonet
Copy link
Member

Should we start to support application/ndjson then? Even though it's not official yet we can may be bet on it so we are ready for the future?

@jasontedor
Copy link
Member

Should we start to support application/ndjson then? Even though it's not official yet we can may be bet on it so we are ready for the future?

No. It's easy to add but hard to take away if it never becomes a thing.

@jpountz
Copy link
Contributor

jpountz commented Aug 18, 2017

Discussed in FixitFriday. We agree there is a bug that needs fixing but the discussion so far suggests that we need to do more research in order to find out the proper way to handle the _msearch and _bulk endpoints and their newline-separated json docs.

@epixa
Copy link
Contributor

epixa commented Aug 18, 2017

If/when you do deprecate application/json for the bulk/msearch requests, can you give the Kibana team a heads up? We're sending application/json for all console requests, and it'd be good to get that implementation updated before or as it gets deprecated so using console doesn't result in a bunch of deprecation logs in elasticsearch.

wdauchy added a commit to wdauchy/rsyslog that referenced this issue Aug 30, 2017
my original patch attented to fix an deprecation warnings while sending
json related to content type.
I however wanted to improve my commit and support the mentioned x-ndjson
content type in case of bulk mode.
see
https://www.elastic.co/guide/en/elasticsearch/reference/master/docs-bulk.html

However, firstly my patch was wrong, since, in bulk mode, the x-ndjson was
set even for connection checks. Secondly I discovered that ES community
kind of disagree of what is the correct behavior:
 in RFC 6648 the x- prefix is already decided as "deprecated"
see elastic/elasticsearch#25718

So I decided to put only application/json as content type; it is
supported for both simple request or bulk requests containing multiple
json. We can improve it when there is a final decision about it.

Re-Fixes: rsyslog#1642
Fixes: commit 462be8b (omelasticsearch: avoid ES5 warnings while sending json)
Signed-off-by: William Dauchy <w.dauchy@criteo.com>
@dakrone
Copy link
Member

dakrone commented Mar 20, 2018

@javanna is this still something we want to pursue? My original trepidation still stands

@javanna
Copy link
Member Author

javanna commented Mar 20, 2018

I would think we should do something about it @dakrone but I would discuss this with the core/infra team so that we reach a final decision.

@jasontedor jasontedor added :Core/Infra/REST API REST infrastructure and utilities and removed :Core/Infra/REST API REST infrastructure and utilities labels Mar 21, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@dakrone
Copy link
Member

dakrone commented Mar 21, 2018

We discussed this today a bit in the core/infra team meeting, but no conclusion we reached, a quick summary of the points/questions:

  • Do we want to standardize on a spec that isn't a registered media type?
  • Is it worth making this breaking change for the minor (if any) benefits it would provide?
  • Should we come up with our own application/elasticsearch-awesome-format media type so we can control the destiny of the spec?
  • Would it be better to invest the time in something like having a spec for CBOR for the _bulk API?

There will be more discussion about this, so far no definite conclusion.

@henningandersen
Copy link
Contributor

Could also affect ML post data.

@droberts195
Copy link
Contributor

This came back to life because of the discussion in #26280.

  • Is it worth making this breaking change for the minor (if any) benefits it would provide?

In my opinion it is not. It will cause extra work and complexity for Kibana, extra work and complexity for the docs snippet framework and extra work and complexity for the language clients. All this time adds up and surely as a company we have something more worthwhile to spend our time on.

So while it doesn't hurt to permit application/x-ndjson as a content type and document that as the preferred type for certain endpoints I think that removing the option to specify application/json instead will just result in fewer features being developed that users could actually benefit from.

@rjernst rjernst added the Team:Core/Infra Meta label for core/infra team label May 4, 2020
@jaymode
Copy link
Member

jaymode commented Dec 14, 2020

Given the lack of traction on this change especially due to whether it is worth making this change, I am going to close this issue. application/x-ndjson still does not have an official spec and we really wouldn't gain much from making the change or implementing a vendor specific mime type for this. If things change, we can always re-evaluate this decision.

@jaymode jaymode closed this as completed Dec 14, 2020
@jaymode jaymode removed the needs:triage Requires assignment of a team area label label Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/REST API REST infrastructure and utilities >deprecation Team:Core/Infra Meta label for core/infra team
Projects
None yet
Development

No branches or pull requests