SQL: Add a media type parser for SQL requests #74116
Conversation
This adds a (branch-specific) media type parser as a near-drop-in replacement for the (master-only) per-REST-endpoint media types parser (elastic#64406).
elasticmachine
added
the
Team:QL (Deprecated)
|
Pinging @elastic/es-ql (Team:QL) |
| } | ||
|
|
||
| protected SqlQueryRequest createTestInstance(boolean binaryCommunication, Mode mode, boolean columnar) { | ||
| /* |
| }}).build(); | ||
| } | ||
|
|
||
| protected SqlQueryRequest createTestInstance(boolean binaryCommunication, Mode mode, boolean columnar) { |
There was a problem hiding this comment.
binaryCommunications seems to be always false?
There was a problem hiding this comment.
And so the rest of the parameters were always having the same value. I've removed them.
| * isn't but there is a {@code Accept} header then we use that. If there | ||
| * isn't then we use the {@code Content-Type} header which is required. | ||
| */ | ||
| public static SqlMediaType getResponseMediaType(RestRequest request, SqlQueryRequest sqlRequest) { |
There was a problem hiding this comment.
Do those methods need to be public? As far as I can see they're used only for testing, could they be package private?
There was a problem hiding this comment.
I was also wondering: which methods need to be public for the backport?
There was a problem hiding this comment.
Good point. Accessibility corrected.
| private static SqlMediaType checkNonNullMediaType(SqlMediaType mediaType, RestRequest request) { | ||
| if (mediaType == null) { | ||
| String msg = String.format(Locale.ROOT, "Invalid request content type: Accept=[%s], Content-Type=[%s], format=[%s]", | ||
| request.header("Accept"), request.header("Content-Type"), request.param("format")); |
There was a problem hiding this comment.
| request.header("Accept"), request.header("Content-Type"), request.param("format")); | |
| request.header("Accept"), request.header("Content-Type"), request.param(URL_PARAM_FORMAT)); |
Only a nitpick but I noticed you're using the constant everywhere else.
| * isn't but there is a {@code Accept} header then we use that. If there | ||
| * isn't then we use the {@code Content-Type} header which is required. | ||
| */ | ||
| public static SqlMediaType getResponseMediaType(RestRequest request, SqlQueryRequest sqlRequest) { |
There was a problem hiding this comment.
I was also wondering: which methods need to be public for the backport?
- remove stale comments; - restricted methods access.
|
@elasticmachine update branch |
- replace literal with constant.
| if (mediaType == null) { | ||
| String msg = String.format(Locale.ROOT, "Invalid request content type: Accept=[%s], Content-Type=[%s], format=[%s]", | ||
| request.header("Accept"), request.header("Content-Type"), request.param(URL_PARAM_FORMAT)); | ||
| throw new IllegalArgumentException(msg); |
There was a problem hiding this comment.
Is there a more suitable ElasticsearchStatusException instead of the generic IAE?
There was a problem hiding this comment.
True, that might be more suitable, but master emits the IAE in this case, it might be better to keep it as is for consistency.
This adds a (branch-specific) media type parser for SQL requests as a
near-drop-in replacement for the (master-only) per-REST-endpoint media
types parser (#64406).
Required for porting #73991.