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.
6 participants
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.