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

Make scripting media type parsing more flexible #66857

Closed

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Dec 29, 2020

#65500 introduced a change to content_type field used in scripting. For JSON it required application/json; charset=UTF-8 but now the space was removed and lowercase param value is required.
This PR is making content_type field more flexible. Allowing spaces and is case insensitive for param values.
The parsing logic is reusing Content-Type and Accept header parsing

relates #65500

test failure:
closes #66986

@pgomulka pgomulka added the WIP label Dec 29, 2020
@pgomulka pgomulka self-assigned this Dec 29, 2020
@pgomulka pgomulka changed the title WIP Make scripting media type parsing more flexible Make scripting media type parsing more flexible Jan 5, 2021
@pgomulka pgomulka marked this pull request as ready for review January 5, 2021 10:56
@pgomulka pgomulka added the :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache label Jan 5, 2021
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jan 5, 2021
@elasticmachine
Copy link
Collaborator

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

@pgomulka pgomulka added v8.0.0 and removed WIP labels Jan 5, 2021
@pgomulka
Copy link
Contributor Author

pgomulka commented Jan 5, 2021

#65500 also accidentally broke the CI by a change in CustomMustacheFactory
old:
JSON_MIME_TYPE_WITH_CHARSET = "application/json; charset=UTF-8";
new:
JSON_MIME_TYPE_WITH_CHARSET = "application/json;charset=utf-8";
The template was stored in old cluster with old value, but read in upgraded cluster expecting the new value. Because the comparison was done on string equality it failed.
With this PR and use of ParsedMediaType the content_type parsing is more flexible

I am however still worrying about the scenario where a cluster is in a mixed state.
The new template is created with new content type and that request is sent to a v7 node which do not know about it.
Another scenario would be where a new template with new content type is sent to a v8 node, but cluster state propagates this to an old node which do not know about this value. I imagine the Script#writeTo should be updated for this..

@pgomulka pgomulka requested review from jdconrad and rjernst January 5, 2021 16:15
@rjernst
Copy link
Member

rjernst commented Jan 5, 2021

/cc @martijnvg

@jdconrad
Copy link
Contributor

jdconrad commented Jan 7, 2021

@pgomulka The situation you're describing is backwards-incompatible. Unless I'm misunderstanding we need to deprecate the old mime type and replace it with the new one over two versions. If we don't handle this scenario users may get stuck during upgrade when using stored mustache scripts (templates).

Edit: Spoke with @pgomulka and it was my misunderstanding. We just need to ensure we only write the older format to nodes that do not support the newer ones as he stated in his previous comment.

@pgomulka
Copy link
Contributor Author

actually do you think it would be easier if we simply deprecate application/json;charset=utf-8 in ES7 and not allow it anymore in ES8 for new script templates?

we would only allow charset=utf-8 to be used in template if the template was created in ES 7.
I think the code would be simpler

@pgomulka
Copy link
Contributor Author

I am closing this in favour of more simple approach #67677

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache 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.

Test failing due to wrong content_type - 10_basic/Verify that we can still find things with the template
5 participants