Conversation
This commit introduces XContentFieldFilter, which applies field includes/excludes to XContent without having to realise the xcontent itself as a java map. SourceFieldMapper and ShardGetService are cut over to use this class
There was a problem hiding this comment.
Pull request overview
This PR introduces a new XContentFieldFilter interface to centralize and optimize source field filtering logic in Elasticsearch. The change replaces scattered filtering implementations with a unified approach that chooses between map-based filtering (for wildcards) and parser-based filtering (for exact matches).
Changes:
- Introduces
XContentFieldFilterinterface with factory method for creating optimized filters - Refactors
SourceFieldMapperto use the new filter interface instead of inline filtering logic - Updates
ShardGetServiceto leverage the new filtering mechanism - Adds
xContentTypeMayCompressedhelper method toXContentHelperfor content type detection
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| XContentFieldFilter.java | New interface defining source filtering contract with factory method that selects optimal implementation |
| SourceFieldMapper.java | Replaces inline filtering logic with XContentFieldFilter, simplifying the applyFilters method |
| ShardGetService.java | Adopts XContentFieldFilter for source filtering in GET operations |
| XContentHelper.java | Adds deprecated helper method for guessing compressed content types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import java.util.function.Function; | ||
|
|
||
| /** | ||
| * A filter that filter fields away from source |
There was a problem hiding this comment.
Corrected grammar: 'A filter that filters fields' instead of 'A filter that filter fields'.
| * A filter that filter fields away from source | |
| * A filter that filters fields away from source |
| * , otherwise return filter using {@link XContentParser} | ||
| */ | ||
| static XContentFieldFilter newFieldFilter(String[] includes, String[] excludes) { | ||
| if ((CollectionUtils.isEmpty(excludes) == false) && Arrays.stream(excludes).filter(field -> field.contains("*")).count() > 0) { |
There was a problem hiding this comment.
The code uses count() > 0 on a stream to check if any element matches. Use anyMatch(field -> field.contains("*")) instead, which short-circuits and avoids counting all matches.
| if ((CollectionUtils.isEmpty(excludes) == false) && Arrays.stream(excludes).filter(field -> field.contains("*")).count() > 0) { | |
| if ((CollectionUtils.isEmpty(excludes) == false) && Arrays.stream(excludes).anyMatch(field -> field.contains("*"))) { |
| final XContentParserConfiguration parserConfig = XContentParserConfiguration.EMPTY.withFiltering( | ||
| Set.of(includes), | ||
| Set.of(excludes) |
There was a problem hiding this comment.
Creating sets from arrays without null checks could cause NullPointerException. If includes or excludes arrays are null, Set.of() will throw an exception. Consider using CollectionUtils or adding null guards.
| final XContentParserConfiguration parserConfig = XContentParserConfiguration.EMPTY.withFiltering( | |
| Set.of(includes), | |
| Set.of(excludes) | |
| final Set<String> includesSet = includes == null ? Set.of() : Set.of(includes); | |
| final Set<String> excludesSet = excludes == null ? Set.of() : Set.of(excludes); | |
| final XContentParserConfiguration parserConfig = XContentParserConfiguration.EMPTY.withFiltering( | |
| includesSet, | |
| excludesSet |
2 participants
PR_013