Support content type application/x-ndjson in DeprecationRestHandler
#36025
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
org.elasticsearch.rest.RestController#hasContentType checks to see if the RestHandler supports the `application/x-ndjson` Content-Type. DeprecationRestHandler is a wrapper around the real RestHandler, and prior to this change would always return `false` due to the interface's default supportsContentStream(). This prevents API's that use multi-line JSON from properly being deprecated resulting in an HTTP 406 error. This change ensures that the DeprecationRestHandler honors the supportsContentStream() of the wrapped RestHandler. Part of elastic#35958
jakelandis
added
:Core/Infra/REST API
|
Pinging @elastic/es-core-infra |
jasontedor
approved these changes
Nov 29, 2018
There was a problem hiding this comment.
Ouch. Let's not make this mistake again. We should have a wrapper rest handler that wraps a rest handler and delegates all methods to the wrapped rest handler. Then deprecation rest handler is a special wrapper rest handler.
The production code LGTM to fix the immediate issue though.
I would rather two separate tests though, one for the case when the underlying handler returns false for supportsContentStream, and one for the case when the underlying handler returns true.
|
@jasontedor - thanks! I split the tests into two as described and also moved the mock initialization to |
jakelandis
added a commit
that referenced
this pull request
Nov 29, 2018
…#36025) org.elasticsearch.rest.RestController#hasContentType checks to see if the RestHandler supports the `application/x-ndjson` Content-Type. DeprecationRestHandler is a wrapper around the real RestHandler, and prior to this change would always return `false` due to the interface's default supportsContentStream(). This prevents API's that use multi-line JSON from properly being deprecated resulting in an HTTP 406 error. This change ensures that the DeprecationRestHandler honors the supportsContentStream() of the wrapped RestHandler. Relates to #35958
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
org.elasticsearch.rest.RestController#hasContentType checks to see if the
RestHandler supports the
application/x-ndjsonContent-Type. DeprecationRestHandleris a wrapper around the real RestHandler, and prior to this change
would always return
falsedue to the interface's default supportsContentStream().This prevents API's that use multi-line JSON from properly being deprecated
resulting in an HTTP 406 error.
This change ensures that the DeprecationRestHandler honors the
supportsContentStream() of the wrapped RestHandler.
Part of #35958