-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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 lenient parsing of bulk actions #78876
Conversation
2f2dc18
to
81b22c3
Compare
a21de9e
to
f9abffa
Compare
Make sure there are no arbitrary fields after an action declaration and it gets closed by a curly bracket. Add a deprecation warning for strict bulk action parsing
Pinging @elastic/es-distributed (Team:Distributed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested some other ways that we accept invalid input. Could you add tests that assert that the new warnings are emitted too please?
server/src/main/java/org/elasticsearch/action/bulk/BulkRequestParser.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/bulk/BulkRequestParser.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/bulk/BulkRequestParser.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/bulk/BulkRequestParser.java
Outdated
Show resolved
Hide resolved
4fff9f6
to
6bbb5bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, I left only minor comments/questions.
server/src/main/java/org/elasticsearch/action/bulk/BulkRequestParser.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/bulk/BulkRequestParser.java
Outdated
Show resolved
Hide resolved
…Parser.java Co-authored-by: David Turner <david.turner@elastic.co>
…Parser.java Co-authored-by: David Turner <david.turner@elastic.co>
@elasticmachine update branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes all LGTM, but since this adds deprecations I think we need to add docs about the change too. #80410 adds the deprecation docs files for 8.1.0 so you can either wait for that to be merged or else duplicate those files here (and I'll sort out any merge conflicts on my PR later).
@elasticmachine update branch |
Thanks David! |
Follow-up to elastic#78876 to rework the deprecation docs slightly.
I would have preferred not to have merged this until the docs changes were reviewed and approved - it's hard to keep track of outstanding things to review otherwise. I opened #80580 to suggest alternative wording and revert the unnecessary docs formatting changes that this PR introduces. |
Adds a mention of `Version.V_7_17_0` so that we don't forget to remove this deprecated behaviour in the next major version. Relates elastic#78876
Make sure there are no arbitrary fields after an action declaration and that it gets properly closed by a curly bracket.
Resolves #43774