Derived source. Do not initialise parent object during _source reconstruction when document does not have field with some value

[OVyshnevskyi]

Description

Lazy parent object initialisation in org.opensearch.index.mapper.ObjectMapper, that prevents empty objects during _source reconstruction.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 1623df0.

Path	Line	Severity	Description
libs/core/src/main/java/org/opensearch/core/xcontent/XContentBuilder.java	67	low	Removing the 'final' modifier from a public API class (XContentBuilder) reduces its security posture by allowing arbitrary subclassing. While done here to support the internal LazyXContentBuilder pattern, it opens the door for external code to override builder behavior in unexpected ways. The change appears intentional and contextually justified, but widening the inheritance surface of a widely-used serialization utility is worth explicit review.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The LazyXContentBuilder constructor passes delegate as the bos parameter to the parent XContentBuilder constructor. This creates a circular reference where the child builder's output stream is the parent builder itself, which is not a valid OutputStream. This will likely cause a ClassCastException or other runtime errors when the builder attempts to write data. public LazyXContentBuilder(String objectName, XContentBuilder delegate) throws IOException { super(delegate.xContent, delegate.bos, delegate.includes, delegate.excludes, delegate, delegate.humanReadable); this.objectName = objectName; this.delegate = delegate; Design Issue The LazyXContentBuilder extends XContentBuilder but only overrides the field(String) method. All other write methods (e.g., value(), startArray(), field(String, Object)) bypass the lazy initialization logic and will write directly without first calling delegate.startObject(objectName). This means data can be written to the delegate before the object is opened, producing malformed JSON. public static final class LazyXContentBuilder extends XContentBuilder { private boolean initialised = false; private final String objectName; private final XContentBuilder delegate; public LazyXContentBuilder(String objectName, XContentBuilder delegate) throws IOException { super(delegate.xContent, delegate.bos, delegate.includes, delegate.excludes, delegate, delegate.humanReadable); this.objectName = objectName; this.delegate = delegate; } @Override public XContentBuilder field(String name) throws IOException { if (!initialised) { initialised = true; delegate.startObject(objectName); } return delegate.field(name); } public void endObjectIfInitialised() throws IOException { if (initialised) { delegate.endObject(); } } } Breaking Change Removing the final modifier from XContentBuilder allows subclassing, which may break existing code that relies on the class being final for security, immutability guarantees, or optimization assumptions. This is a breaking API change that could affect downstream consumers. public class XContentBuilder implements Closeable, Flushable {

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Override all builder methods consistently The field() method only delegates to the parent builder but doesn't handle other builder methods (e.g., value(), startArray(), endArray()). If callers use these methods after field(), they will bypass the lazy initialization logic and write directly to the parent builder, causing incorrect output structure. libs/core/src/main/java/org/opensearch/core/xcontent/XContentBuilder.java [1188-1195] @Override public XContentBuilder field(String name) throws IOException { + ensureInitialised(); + return delegate.field(name); +} + +private void ensureInitialised() throws IOException { if (!initialised) { initialised = true; delegate.startObject(objectName); } - return delegate.field(name); } Suggestion importance[1-10]: 9 __ Why: Critical issue. The LazyXContentBuilder only overrides field() method, but callers can invoke other XContentBuilder methods (like value(), startArray(), etc.) that bypass lazy initialization, leading to incorrect output structure. All relevant methods should trigger initialization.	High
General	Ensure cleanup with try-finally block If any mapper throws an exception during deriveSource(), the endObjectIfInitialised() will not be called, potentially leaving the builder in an inconsistent state. Wrap the loop in a try-finally block to ensure cleanup happens even if an exception occurs. server/src/main/java/org/opensearch/index/mapper/ObjectMapper.java [1092-1096] LazyXContentBuilder newBuilder = new LazyXContentBuilder(simpleName(), builder); -for (final Mapper mapper : this.mappers.values()) { - mapper.deriveSource(newBuilder, leafReader, docId); +try { + for (final Mapper mapper : this.mappers.values()) { + mapper.deriveSource(newBuilder, leafReader, docId); + } +} finally { + newBuilder.endObjectIfInitialised(); } -newBuilder.endObjectIfInitialised(); Suggestion importance[1-10]: 7 __ Why: Valid suggestion to add exception safety. Using a try-finally block ensures endObjectIfInitialised() is called even if a mapper throws an exception, preventing the builder from being left in an inconsistent state.	Medium

[github-actions]

❌ Gradle check result for 1623df0: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[OVyshnevskyi]

This is the simplest possible and minimalistic solution i was able to produce, but a bit dirty.
Likely better option here would be to introduce an interface for XContentBuilder and avoid direct inheritance ( too big change to pull it out from start in first PR without discussion :) )