Skip to content

Conversation

@pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Nov 6, 2020

Browsers are sending media ranges with quality factors on Accept header.
We should ignore the value and respond with applicaiton/json

closes #64689
relates #51816

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

Browsers are sending media ranges with quality factors on Accept header.
We should ignore the value and responde with applicaiton/json

closes elastic#64689
relates elastic#51816
@pgomulka pgomulka self-assigned this Nov 6, 2020
@pgomulka pgomulka added the :Core/Infra/REST API REST infrastructure and utilities label Nov 6, 2020
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Nov 6, 2020
@pgomulka pgomulka added the v8.0.0 label Nov 6, 2020
Copy link
Contributor

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

LGTM as a workaround thanks!

if (headerValue != null) {
if (isMediaRange(headerValue) || "*/*".equals(headerValue)) {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be moved down to and checked against elements[0] instead of the whole headerValue. A parameter could have a comma. Probably not..but slightly more correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we check for element[0] we could not check against something like text/HTML;q=0, application/json
Media range is a coma separated list where each media type can have a list of parameters after semicolon .
Comma (delimiters at all) are not allowed within media type name or parameters

Copy link
Contributor

Choose a reason for hiding this comment

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

OK ..that makes sense. sorry for the noise.

however...minor clarification your values for the parameters may be quoted which can contain any ASCII char accept-extension = ";" token [ "=" ( token | quoted-string ) ] however, this is very unlikely to encounter a comma there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah.. you are totally right. I did not look into quoted string part. Indeed this can contain a comma.
And CVS could be a media type that would like to have it.
we don't support that at the moment, but at some point I think it would make sense that we do.

Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

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

One suggestion to move the check down a line. This is a good workaround, however long term we should probably consider supporting some simple form of content negotiation.

@pgomulka pgomulka merged commit dd5bcd4 into elastic:master Nov 9, 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 Team:Core/Infra Meta label for core/infra team v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't Open ES GET APIs from Browser

4 participants