We should enforce these required inputs by returning a useful error and 400 response. right now it is 500 #780
Conversation
…nd 400 response. right now it is 500
| private static int getHowMany(JsonNode body) { | ||
| return body.get("size").asInt(); | ||
| } | ||
|
|
||
| // This is required. consider returning a more useful error to the user than 500 |
There was a problem hiding this comment.
probably something like this:
{
"error": {
"code": 400,
"message": "Bad Request",
"details": "The 'query.gte' field is required but missing in the request."
}
}
|
@autata we are looking to next quarter or the following to move to using the Opensearch library for the entire request parsing, which should allow us better compatibility with these fields and options. Currently we do this for a subsection of the request (aggregations), and would like to extend this to the entire request body. This should allow us to support more complex request types that are not currently issued via Grafana today. |
|
@bryanlb this is helpful context. Do you think it makes sense to open a PR with slightly better return codes (as suggested in these comments) or potentially change this to an issue since following Opensearch API is in the roadmap for the next quarter or two and this would be fixed if moving to Opensearch full functionality. |
|
@autata I think that moving this to an issue makes the most sense here. If you still want to PR a change to these response codes that would be fine, but if all goes according to plan this would only be in place for a few months before we can entirely rework the request parsing. |
Summary
I think we should set a reasonable default for when size is missing (10? - i think that is what ES uses when missing). Opening a draft PR as a discussion before going further in this direction. There may be a better way to more closely follow the ES API spec. (_msearch, range query docs)
For
lteandgteI think if they are missing, we should return a 400 (Bad Request) and a helpful error string.Right now, if any of these are missing, we get a 500 (Internal Server Error) response.
Requirements