Fix FiltersAggregation NPE when filters is empty
#41459
Conversation
If `keyedFilters` is null it assumes there are unkeyed filters...which will NPE if the unkeyed filters was actually empty. This refactors to simplify the filter assignment a bit, adds an empty check and tidies up some formatting. It also fixes a few tests that _should_ have exposed this NPE, but due to how they were setup (using try blocks) the exception was hidden. The try blocks are removed so that any future exceptions are not swallowed.
|
Pinging @elastic/es-analytics-geo |
|
Huh... yeah you're right. I was thinking the Lemme make sure I'm fixing what I think I'm fixing. :) Going to doublecheck the tests and the original NPE. |
|
Oh right, I see. The tests don't cover the NPE case... I misread the original query. It's a completely empty The fix works as intended because it prevents keyed/unkeyed from being null, but I'll add a test for the empty filter agg, revert the |
|
sounds good thanks @polyfractal ! |
|
@elasticmachine run elasticsearch-ci/1 |
|
@elasticmachine run elasticsearch-ci/default-distro |
|
CI is green but a bit stale for my liking and I've been bit by that recently, sooo.... @elasticmachine retest this please |
If `keyedFilters` is null it assumes there are unkeyed filters...which will NPE if the unkeyed filters was actually empty. This refactors to simplify the filter assignment a bit, adds an empty check and tidies up some formatting.
If `keyedFilters` is null it assumes there are unkeyed filters...which will NPE if the unkeyed filters was actually empty. This refactors to simplify the filter assignment a bit, adds an empty check and tidies up some formatting.
Elasticsearch 7.2.0 introduced a change to empty filters handling in elastic/elasticsearch#41459
Elasticsearch 7.2.0 introduced a change to empty filters handling in elastic/elasticsearch#41459
Elasticsearch 7.2.0 introduced a change to empty filters handling in elastic/elasticsearch#41459 (cherry picked from commit a8e502f)
Elasticsearch 7.2.0 introduced a change to empty filters handling in elastic/elasticsearch#41459 (cherry picked from commit a8e502f)
Elasticsearch 7.2.0 introduced a change to empty filters handling in elastic/elasticsearch#41459 (cherry picked from commit a8e502f)
If
keyedFiltersis null the code assumes there are unkeyed filters...which will NPE if the unkeyed filters was actually empty.This refactors to simplify the filter assignment a bit, adds an empty check and tidies up some formatting.
It also fixes a few tests that should have exposed this NPE, but due to how they were setup (using try blocks) the exception was hidden. The try blocks are removed so that any future exceptions are not swallowed.
I wanted to refactor the agg to use our static parsers, but the filters agg is super lenient which makes parsing tricky (anonymous array of filter objects or keyed object of filters, default other bucket name or configurable, boolean to control other bucket which interacts with other key). I think we should probably just simplify the agg... will open a different issue about that.
Closes #41408