-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Remove unnecessary RestHandler#supportsBulkContent
#135900
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
Remove unnecessary RestHandler#supportsBulkContent
#135900
Conversation
This method only exists to add some additional `Content-type` validation, but we can do all the validation needed using `RestHandler#mediaTypesValid` instead.
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
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.
LGTM.
But I would slap this check on XContentType enum.
public static boolean isAnyOf(XContentType t, List<XContentType> ts) {
if (t == null) return false;
return ts.contains(t);
}
public static boolean isJsonOrSmile(XContentType t) {
return isAnyOf(t, List.of(JSON, SMILE));
}
Also remove indirection of hasValidMediaTypeForBulkRequest, saying isJsonOrSmile. RestBulkAction imports look odd here.
public boolean mediaTypesValid(RestRequest request) {
return XContentType.isJsonOrSmile(request.getXContentType());
}
Yeah fair point this isn't just about |
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.
Neat
RestResponse.createSimpleErrorResponse( | ||
channel, | ||
RestStatus.NOT_ACCEPTABLE, | ||
"Content-Type [" + xContentType + "] does not support stream parsing. Use JSON or SMILE instead" |
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.
A new version will return a generic unsupported type error rather than "stream parsing" error, right?
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.
Yeah it will say Content-Type header [...] is not supported
instead, with a (technically incorrect) 406 Not Acceptable
either way.
This method only exists to add some additional
Content-type
validation, but we can do all the validation needed using
RestHandler#mediaTypesValid
instead.