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.

Sign up for GitHub

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

Jump to bottom

Using incorrect Accept/Content-Type headers in compatibility mode has bad client-side UX #78214

Closed
sethmlarson opened this issue Sep 22, 2021 · 4 comments · Fixed by #80253
Closed

Using incorrect Accept/Content-Type headers in compatibility mode has bad client-side UX #78214

sethmlarson opened this issue Sep 22, 2021 · 4 comments · Fixed by #80253
Assignees
@pgomulka
Labels
>bug :Core/Infra/REST API REST infrastructure and utilities Team:Core/Infra Meta label for core/infra team

Comments

@sethmlarson
Copy link
Contributor

sethmlarson commented Sep 22, 2021 •
edited
Loading

Discovered this behavior when testing the compatibility mimetype (application/vnd.elasticsearch+json?compatible-with=X) being used with Accept and Content-Type. When using the compatibility mimetype in either Accept or Content-Type the other HTTP header if given must be set to the compatibility mimetype as well otherwise an error occurs.

When running the following command:

$ curl -k -XPOST -H "Accept: application/vnd.elasticsearch+json;compatible-with=7" \
  -H "Content-Type: application/json" -d '{"query":{"match_all":{}}}' \
  http://localhost:9200/_search

The error message server-side in the logs is a good one, can tell exactly what's wrong:

A compatible version is required on both Content-Type and Accept headers if either one has requested a compatible version and the compatible versions must match. Accept=application/vnd.elasticsearch+json;compatible-with=7, Content-Type=application/json

but client-side this is what's returned:

curl: (52) Empty reply from server

Elasticsearch doesn't send an HTTP response and instead hangs up the socket. This isn't a great user-experience, perhaps this can be changed to Elasticsearch returning a 4XX HTTP response (maybe 400, 406, 415?) with the error message above?
@sethmlarson sethmlarson added >bug :Core/Infra/REST API REST infrastructure and utilities needs:triage Requires assignment of a team area label labels Sep 22, 2021
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Sep 22, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@jakelandis
Copy link
Contributor

@sethpayne - The behavior of needing to setting both mime types the same value when both are sent is intentional. We don't want to allow a non-compatible request with a compatible response (or vice versa).

The lack of a proper response is a bug. Thanks for finding that, and please let me know if ^^ does not make sense or causes difficulty in adoption.

cc @pgomulka

@sethmlarson
Copy link
Contributor Author

@jakelandis The behavior of requiring both to be set when in compatibility mode is fine for clients.

Also I think you pinged Seth Payne on accident, the curse of multiple Seths on the same small team!

@pgomulka pgomulka self-assigned this Sep 29, 2021
@romseygeek romseygeek removed the needs:triage Requires assignment of a team area label label Oct 1, 2021
@pgomulka
Copy link
Contributor

I totally forgot about this one. We have a separate issue for this one #79060
will be fixed in #80253

@pgomulka pgomulka mentioned this issue Nov 10, 2021
Merged
pgomulka added a commit that referenced this issue Nov 10, 2021
@pgomulka
Handle exceptions thrown from RestCompatibleVersionHelper (#80253)
5e98ea4 
RestCompatibleVersionHelper is used to validate versions on Accept and Content-Type headers
When a validation fails, an exception is being thrown indicating which headers are incorrect.
That exception was not handled in RestRequest where the helper was used.
This commit gracefully handles the exception from validation failure. A bad request response is returned to a user.

closes #79060
closes #78214
pgomulka added a commit to pgomulka/elasticsearch that referenced this issue Nov 10, 2021
@pgomulka
Handle exceptions thrown from RestCompatibleVersionHelper (elastic#80253
7703562 
)

RestCompatibleVersionHelper is used to validate versions on Accept and Content-Type headers
When a validation fails, an exception is being thrown indicating which headers are incorrect.
That exception was not handled in RestRequest where the helper was used.
This commit gracefully handles the exception from validation failure. A bad request response is returned to a user.

closes elastic#79060
closes elastic#78214
@pgomulka pgomulka mentioned this issue Nov 10, 2021
Merged
elasticsearchmachine pushed a commit that referenced this issue Nov 10, 2021
@pgomulka
Handle exceptions thrown from RestCompatibleVersionHelper (#80253) (#…
6497a9e 
…80605)

RestCompatibleVersionHelper is used to validate versions on Accept and Content-Type headers
When a validation fails, an exception is being thrown indicating which headers are incorrect.
That exception was not handled in RestRequest where the helper was used.
This commit gracefully handles the exception from validation failure. A bad request response is returned to a user.

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

@pgomulka pgomulka

Labels
>bug :Core/Infra/REST API REST infrastructure and utilities Team:Core/Infra Meta label for core/infra team
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Handle exceptions thrown from RestCompatibleVersionHelper pgomulka/elasticsearch
5 participants
@jakelandis @romseygeek @pgomulka @elasticmachine @sethmlarson