Open
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
📝 WalkthroughWalkthroughThis change introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@server/src/main/java/org/elasticsearch/common/xcontent/XContentFieldFilter.java`:
- Around line 65-72: The code in XContentFieldFilter uses
xContentTypeMayCompressed(...) but then feeds the original (possibly compressed)
bytes into the parser and leaves parser/builder unclosed; replace the manual
parser creation with XContentHelper.createParser(parserConfig, originalSource)
which handles compressed inputs, and wrap both the XContentParser and
XContentBuilder in a try-with-resources to ensure they are closed; call
builder.copyCurrentStructure(parser) inside the try block and return the bytes
from the builder/stream after closing (e.g., use BytesReference.bytes(builder)
or bytes from the stream output) so resources are released properly.
In `@server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java`:
- Around line 521-545: The method xContentTypeMayCompressed leaks the
decompression InputStream created by
Compressor.threadLocalInputStream(bytes.streamInput()); wrap the creation of
compressedStreamInput (and the BufferedInputStream wrapper) in a
try-with-resources block so the stream is closed after calling
XContentFactory.xContentType(compressedStreamInput), preserving the existing
IOException handling and semantics; update xContentTypeMayCompressed to obtain
the XContentType inside the try-with-resources and return it (or rethrow as
UncheckedIOException as now) so compressor buffers are released reliably.
🧹 Nitpick comments (1)
server/src/main/java/org/elasticsearch/common/xcontent/XContentFieldFilter.java (1)
60-63: ReconsiderSet.of()for user-supplied arrays that may contain duplicates.
Set.of(includes/excludes)will throwIllegalArgumentExceptionif the arrays contain duplicate field names. WhileFetchSourceContextprevents null arrays, duplicates are possible if users specify the same field multiple times. Consider usingSet.copyOf()(Java 10+) to deduplicate, or aHashSetfor explicit flexibility—depending on whether duplicate field filters should error or be silently deduplicated.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/src/main/java/org/elasticsearch/common/xcontent/XContentFieldFilter.javaserver/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.javaserver/src/main/java/org/elasticsearch/index/get/ShardGetService.javaserver/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java
🔇 Additional comments (2)
server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java (1)
36-43: LGTM: filtering is now centralized viaXContentFieldFilter.This simplifies the apply path and keeps filtering byte-based without local map reconstruction.
Also applies to: 139-143, 173-178
server/src/main/java/org/elasticsearch/index/get/ShardGetService.java (1)
255-256: LGTM: request-level filtering now routes throughXContentFieldFilter.Keeps the fetch path consistent with the new abstraction while preserving error handling.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
+65
to
+72
| if (contentType == null) { | ||
| contentType = XContentHelper.xContentTypeMayCompressed(originalSource); | ||
| } | ||
| BytesStreamOutput streamOutput = new BytesStreamOutput(Math.min(1024, originalSource.length())); | ||
| XContentBuilder builder = new XContentBuilder(contentType.xContent(), streamOutput); | ||
| XContentParser parser = contentType.xContent().createParser(parserConfig, originalSource.streamInput()); | ||
| builder.copyCurrentStructure(parser); | ||
| return BytesReference.bytes(builder); |
There was a problem hiding this comment.
Parse compressed sources and close parser/builder.
xContentTypeMayCompressed(...) only infers type; the parser still reads the compressed bytes, which will fail when source is compressed. Also, the parser/builder aren’t closed. Use XContentHelper.createParser(...) and try-with-resources.
🔧 Proposed fix
- return (originalSource, contentType) -> {
- if (contentType == null) {
- contentType = XContentHelper.xContentTypeMayCompressed(originalSource);
- }
- BytesStreamOutput streamOutput = new BytesStreamOutput(Math.min(1024, originalSource.length()));
- XContentBuilder builder = new XContentBuilder(contentType.xContent(), streamOutput);
- XContentParser parser = contentType.xContent().createParser(parserConfig, originalSource.streamInput());
- builder.copyCurrentStructure(parser);
- return BytesReference.bytes(builder);
- };
+ return (originalSource, contentType) -> {
+ if (contentType == null) {
+ contentType = XContentHelper.xContentTypeMayCompressed(originalSource);
+ }
+ BytesStreamOutput streamOutput = new BytesStreamOutput(Math.min(1024, originalSource.length()));
+ try (
+ XContentParser parser = XContentHelper.createParser(parserConfig, originalSource, contentType);
+ XContentBuilder builder = new XContentBuilder(contentType.xContent(), streamOutput)
+ ) {
+ builder.copyCurrentStructure(parser);
+ return BytesReference.bytes(builder);
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (contentType == null) { | |
| contentType = XContentHelper.xContentTypeMayCompressed(originalSource); | |
| } | |
| BytesStreamOutput streamOutput = new BytesStreamOutput(Math.min(1024, originalSource.length())); | |
| XContentBuilder builder = new XContentBuilder(contentType.xContent(), streamOutput); | |
| XContentParser parser = contentType.xContent().createParser(parserConfig, originalSource.streamInput()); | |
| builder.copyCurrentStructure(parser); | |
| return BytesReference.bytes(builder); | |
| if (contentType == null) { | |
| contentType = XContentHelper.xContentTypeMayCompressed(originalSource); | |
| } | |
| BytesStreamOutput streamOutput = new BytesStreamOutput(Math.min(1024, originalSource.length())); | |
| try ( | |
| XContentParser parser = XContentHelper.createParser(parserConfig, originalSource, contentType); | |
| XContentBuilder builder = new XContentBuilder(contentType.xContent(), streamOutput) | |
| ) { | |
| builder.copyCurrentStructure(parser); | |
| return BytesReference.bytes(builder); | |
| } |
🤖 Prompt for AI Agents
In
`@server/src/main/java/org/elasticsearch/common/xcontent/XContentFieldFilter.java`
around lines 65 - 72, The code in XContentFieldFilter uses
xContentTypeMayCompressed(...) but then feeds the original (possibly compressed)
bytes into the parser and leaves parser/builder unclosed; replace the manual
parser creation with XContentHelper.createParser(parserConfig, originalSource)
which handles compressed inputs, and wrap both the XContentParser and
XContentBuilder in a try-with-resources to ensure they are closed; call
builder.copyCurrentStructure(parser) inside the try block and return the bytes
from the builder/stream after closing (e.g., use BytesReference.bytes(builder)
or bytes from the stream output) so resources are released properly.
Comment on lines
+521
to
+545
| /** | ||
| * Guesses the content type based on the provided bytes which may be compressed. | ||
| * | ||
| * @deprecated the content type should not be guessed except for few cases where we effectively don't know the content type. | ||
| * The REST layer should move to reading the Content-Type header instead. There are other places where auto-detection may be needed. | ||
| * This method is deprecated to prevent usages of it from spreading further without specific reasons. | ||
| */ | ||
| @Deprecated | ||
| public static XContentType xContentTypeMayCompressed(BytesReference bytes) { | ||
| Compressor compressor = CompressorFactory.compressor(bytes); | ||
| if (compressor != null) { | ||
| try { | ||
| InputStream compressedStreamInput = compressor.threadLocalInputStream(bytes.streamInput()); | ||
| if (compressedStreamInput.markSupported() == false) { | ||
| compressedStreamInput = new BufferedInputStream(compressedStreamInput); | ||
| } | ||
| return XContentFactory.xContentType(compressedStreamInput); | ||
| } catch (IOException e) { | ||
| assert false : "Should not happen, we're just reading bytes from memory"; | ||
| throw new UncheckedIOException(e); | ||
| } | ||
| } else { | ||
| return XContentHelper.xContentType(bytes); | ||
| } | ||
| } |
There was a problem hiding this comment.
Close the decompression stream to avoid leaking buffers.
compressor.threadLocalInputStream(...) is never closed here, unlike other call sites in this class. Wrap it in try-with-resources so compressor buffers are released reliably.
🔧 Proposed fix
- if (compressor != null) {
- try {
- InputStream compressedStreamInput = compressor.threadLocalInputStream(bytes.streamInput());
- if (compressedStreamInput.markSupported() == false) {
- compressedStreamInput = new BufferedInputStream(compressedStreamInput);
- }
- return XContentFactory.xContentType(compressedStreamInput);
+ if (compressor != null) {
+ try (
+ InputStream raw = compressor.threadLocalInputStream(bytes.streamInput());
+ InputStream in = raw.markSupported() ? raw : new BufferedInputStream(raw)
+ ) {
+ return XContentFactory.xContentType(in);
} catch (IOException e) {
assert false : "Should not happen, we're just reading bytes from memory";
throw new UncheckedIOException(e);
}
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Guesses the content type based on the provided bytes which may be compressed. | |
| * | |
| * @deprecated the content type should not be guessed except for few cases where we effectively don't know the content type. | |
| * The REST layer should move to reading the Content-Type header instead. There are other places where auto-detection may be needed. | |
| * This method is deprecated to prevent usages of it from spreading further without specific reasons. | |
| */ | |
| @Deprecated | |
| public static XContentType xContentTypeMayCompressed(BytesReference bytes) { | |
| Compressor compressor = CompressorFactory.compressor(bytes); | |
| if (compressor != null) { | |
| try { | |
| InputStream compressedStreamInput = compressor.threadLocalInputStream(bytes.streamInput()); | |
| if (compressedStreamInput.markSupported() == false) { | |
| compressedStreamInput = new BufferedInputStream(compressedStreamInput); | |
| } | |
| return XContentFactory.xContentType(compressedStreamInput); | |
| } catch (IOException e) { | |
| assert false : "Should not happen, we're just reading bytes from memory"; | |
| throw new UncheckedIOException(e); | |
| } | |
| } else { | |
| return XContentHelper.xContentType(bytes); | |
| } | |
| } | |
| /** | |
| * Guesses the content type based on the provided bytes which may be compressed. | |
| * | |
| * `@deprecated` the content type should not be guessed except for few cases where we effectively don't know the content type. | |
| * The REST layer should move to reading the Content-Type header instead. There are other places where auto-detection may be needed. | |
| * This method is deprecated to prevent usages of it from spreading further without specific reasons. | |
| */ | |
| `@Deprecated` | |
| public static XContentType xContentTypeMayCompressed(BytesReference bytes) { | |
| Compressor compressor = CompressorFactory.compressor(bytes); | |
| if (compressor != null) { | |
| try ( | |
| InputStream raw = compressor.threadLocalInputStream(bytes.streamInput()); | |
| InputStream in = raw.markSupported() ? raw : new BufferedInputStream(raw) | |
| ) { | |
| return XContentFactory.xContentType(in); | |
| } catch (IOException e) { | |
| assert false : "Should not happen, we're just reading bytes from memory"; | |
| throw new UncheckedIOException(e); | |
| } | |
| } else { | |
| return XContentHelper.xContentType(bytes); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java`
around lines 521 - 545, The method xContentTypeMayCompressed leaks the
decompression InputStream created by
Compressor.threadLocalInputStream(bytes.streamInput()); wrap the creation of
compressedStreamInput (and the BufferedInputStream wrapper) in a
try-with-resources block so the stream is closed after calling
XContentFactory.xContentType(compressedStreamInput), preserving the existing
IOException handling and semantics; update xContentTypeMayCompressed to obtain
the XContentType inside the try-with-resources and return it (or rethrow as
UncheckedIOException as now) so compressor buffers are released reliably.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR_013
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.