Mark all scripted field queries as expensive #59658
Conversation
This will cause them to fail if you don't have permission to execute expensive queries.
nik9000
added
the
:Search/Search
|
Pinging @elastic/es-search (:Search/Search) |
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
left a couple of nits, LGTM otherwise
| private void checkAllowExpensiveQueries(QueryShardContext context) { | ||
| if (context.allowExpensiveQueries() == false) { | ||
| throw new IllegalArgumentException( | ||
| "queries cannot be executed against [script] fields while [" + ALLOW_EXPENSIVE_QUERIES.getKey() + "] is set to [false]." |
There was a problem hiding this comment.
nit: use the constant from the mapper? content type I think it is called
There was a problem hiding this comment.
should we rather throw ElasticsearchException also, and why doesn't the context expose a check method instead that accepts for instance a message? :)
There was a problem hiding this comment.
should we rather throw ElasticsearchException also, and why doesn't the context expose a check method instead that accepts for instance a message? :)
That would probably be a good choice.
I have no idea why we were throwing ElasticsearchException when we were. It feels like this is what IAE is for.
There was a problem hiding this comment.
I have no idea why we were throwing ElasticsearchException when we were. It feels like this is what IAE is for.
I figured it out - we translate all exceptions thrown when building a query into a QueryShardException which is always a 400.
There was a problem hiding this comment.
why doesn't the context expose a check method instead that accepts for instance a message? :)
It really is a good idea. I took a look at it and, ironically, testing it is kind of a pain. I'll hold off on it.
There was a problem hiding this comment.
shall we open issues for possible follow-up tasks that we don't want to spend time on right now?
There was a problem hiding this comment.
Let me give it another go this morning!
|
|
||
| @Override | ||
| public Query existsQuery(QueryShardContext context) { | ||
| checkAllowExpensiveQueries(context); |
There was a problem hiding this comment.
maybe one day we will a base class for runtime mapper field types that does this in one place.
3 participants
This will cause them to fail if you don't have permission to execute
expensive queries.
Relates to #59332