Do not fail on duplicated content field filters #85382
Conversation
arteam
added
:Search Foundations/Mapping
Set.of() throws a runtime exception if it encounters duplicated elements.
arteam
force-pushed
the
do-not-fail-on-duplicated-include-exclude-filters
branch
from
61f3bc8
to
852bcfc
Compare
arteam
changed the title
|
Hi @arteam, I've created a changelog YAML for you. |
arteam
force-pushed
the
do-not-fail-on-duplicated-include-exclude-filters
branch
from
ba41f42
to
a6bb728
Compare
|
Pinging @elastic/es-search (Team:Search) |
|
hey @arteam do you have any background on what bug this PR is fixing? Is it something that you encountered, or is there an issue filed for it? Would it be possible to add a description around what it is addressing from a user perspective? Thanks! |
|
This seems to have been introduced with #83152 , @nik9000 @romseygeek would you mind having a look as you were involved in getting that one change in? |
|
Would it make sense to add a test to SourceFieldMapperTests that is affected by this change? The other caller that is affected is the fetch phase, but I haven't found a great place to add a contained test. Maybe a yaml test would do too. |
I probably would have added a yaml test myself but I'm not sure we super need one for this. OTOH, I'm paranoid and paranoia is good. |
|
I've added a couple of test to |
|
@elasticmachine update branch |
`Set.of()` throws a runtime exception if it encounters a duplicated element, `Set.copyOf`, on the contrary, doesn't do that. We shouldn't fail if the user configure multiple include or exclude filters for _source fields. See https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-source-field.html#include-exclude
💚 Backport successful
|
|
Thanks Alan, Luca, and Nik! |
`Set.of()` throws a runtime exception if it encounters a duplicated element, `Set.copyOf`, on the contrary, doesn't do that. We shouldn't fail if the user configure multiple include or exclude filters for _source fields. See https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-source-field.html#include-exclude
| Set.of(includes), | ||
| Set.of(excludes), | ||
| Set.copyOf(Arrays.asList(includes)), | ||
| Set.copyOf(Arrays.asList(excludes)), |
There was a problem hiding this comment.
I had a chance to review it only after the pr was merged, may be we could consider this for a followup or a future change:
May be we should have a dedicated method for this in CollectionUtils so that intention is clear and nobody revert/simplify code to the shorter Set.of version. May be something like CollectionUtils.setOfCollectionWithPossibleDuplicates(Collection<T> c)
There was a problem hiding this comment.
not advocating against your suggestion, but we do have tests for this now so reverting the change would cause test failures
There was a problem hiding this comment.
This PR introduced a test for duplicate filters. I think that's what you want, right?
There was a problem hiding this comment.
Correct, the test would catch the revert. I was suggesting to have a dedicated set factory that would display the intention (create set from collection that may contain duplicates)
Source includes and excludes, both configured in the mappings as well as provided as part of a search and get request, may include duplicate entries. This caused no problems until #83152 which makes Elasticsearch throw an error when we are processing the request. While users can update their request for get and search, this prevents users from upgrading to 8.1 if they have duplicate source includes/excludes configured in their existing mapping.
Set.of()throws a runtime exception if it encounters a duplicated element,Set.copyOf, on the contrary, doesn't do that.We shouldn't fail if the user configure multiple include or exclude filters for
_sourcefields.