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

Elastica should set the Content-Type header when sending json via Http #1301

Closed
nomoa opened this Issue May 11, 2017 · 1 comment

Comments

Projects
None yet
2 participants
@nomoa
Contributor

nomoa commented May 11, 2017

Not setting Content-Type header in the request headers will now produce (elasticsearch 5.3) a deprecation warning in elastic logs.
I think that Elastica should set this header when sending json.
The deprecation warning is :
[2017-05-11T13:40:27,556][WARN ][o.e.d.r.RestController ] Content type detection for rest requests is deprecated. Specify the content type using the [Content-Type] header.

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin May 11, 2017

Owner

@nomoa 200% agree. Could you open a PR for that?

Owner

ruflin commented May 11, 2017

@nomoa 200% agree. Could you open a PR for that?

nomoa added a commit to nomoa/Elastica that referenced this issue May 11, 2017

Always set Content-Type with Http Transport
Added a new param to Client::request and Request::_construct to allow
setting the content-type of the data being sent.
This defaults to "application/json" and is only overidden by Bulk and MultiSearch
where it should be application/x-ndjson.

Sadly I'm not sure how to test this properly.
I'm not a big a big fan of adding a new param like that. Ideally we should refactor
the $data param of Request::_construct to accept a array (defaults to application/json)
or a new RequestBody object where the client could set the content-type alongside the
the string.

closes #1301

nomoa added a commit to nomoa/Elastica that referenced this issue May 11, 2017

Always set Content-Type with Http Transport
Added a new param to Client::request and Request::_construct to allow
setting the content-type of the data being sent.
This defaults to "application/json" and is only overidden by Bulk and MultiSearch
where it should be application/x-ndjson.

Sadly I'm not sure how to test this properly.
I'm not a big a big fan of adding a new param like that. Ideally we should refactor
the $data param of Request::_construct to accept a array (defaults to application/json)
or a new RequestBody object where the client could set the content-type alongside the
the string.

closes #1301

@ruflin ruflin closed this in #1302 May 12, 2017

ruflin added a commit that referenced this issue May 12, 2017

Always set Content-Type with Http Transport (#1302)
Added a new param to Client::request and Request::_construct to allow
setting the content-type of the data being sent.
This defaults to "application/json" and is only overidden by Bulk and MultiSearch
where it should be application/x-ndjson.

Sadly I'm not sure how to test this properly.
I'm not a big a big fan of adding a new param like that. Ideally we should refactor
the $data param of Request::_construct to accept a array (defaults to application/json)
or a new RequestBody object where the client could set the content-type alongside the
the string.

closes #1301

mhernik pushed a commit to mhernik/Elastica that referenced this issue Jul 24, 2017

Always set Content-Type with Http Transport (#1302)
Added a new param to Client::request and Request::_construct to allow
setting the content-type of the data being sent.
This defaults to "application/json" and is only overidden by Bulk and MultiSearch
where it should be application/x-ndjson.

Sadly I'm not sure how to test this properly.
I'm not a big a big fan of adding a new param like that. Ideally we should refactor
the $data param of Request::_construct to accept a array (defaults to application/json)
or a new RequestBody object where the client could set the content-type alongside the
the string.

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