Fix SemanticHighlighterExtBuilder.toXContent

[nomoa]

Description

SearchExtBuilder expects toXContent to produce their encloding field name.
Add test to ensure we can do a round trip with XContent.

Related Issues

Resolves #1906

Check List

• New functionality includes testing.
• [ ] New functionality has been documented.
• [ ] API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• [ ] Public documentation issue/PR created.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 7cd347a)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid double-wrapping ext builder name SearchExtBuilder.toXContent is typically invoked within an already-started object where the framework writes the ext builder's name as the wrapping field. Emitting builder.field(NAME, enabled) here will produce a nested structure like {"semantic_highlighter":{"semantic_highlighter":true}} when serialized as part of a search request. Consider writing only the value (or an object body without re-emitting NAME) to match the standard SearchExtBuilder contract. src/main/java/org/opensearch/neuralsearch/query/ext/SemanticHighlighterExtBuilder.java [64] @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - return builder.field(NAME, enabled); + return builder.value(enabled); } Suggestion importance[1-10]: 6 __ Why: The suggestion raises a legitimate concern about SearchExtBuilder.toXContent typically being invoked within a named context, which could cause double-wrapping. However, the PR's test (roundTripXContent) explicitly asserts the field name is emitted, and the parser flow in the test expects a FIELD_NAME token, suggesting this is the intended behavior. The correctness depends on how the framework invokes it, making this a valid but uncertain concern.	Low

[github-actions]

Persistent review updated to latest commit add0deb

[heemin32]

@nomoa could you add integ test for this case?

[nomoa]

@nomoa could you add integ test for this case?

I'm not sure I can trigger toXContent from an integration test but please let me know if you have suggestions.

What I could do perhaps is test this behavior building a SearchSourceBuilder and trying to mimic what's done in the query-insight plugin when it tries to serialize the query (which is where the issue surfaced in the first place)?

[heemin32]

@nomoa could you add integ test for this case?

I'm not sure I can trigger toXContent from an integration test but please let me know if you have suggestions.

What I could do perhaps is test this behavior building a SearchSourceBuilder and trying to mimic what's done in the query-insight plugin when it tries to serialize the query (which is where the issue surfaced in the first place)?

That sounds good to me. I don't know any alternative.

[heemin32]

Also, please fix the CI failures.

[github-actions]

Persistent review updated to latest commit 53b4ca6

[github-actions]

Persistent review updated to latest commit 6255458

[nomoa]

PR Code Suggestions ✨

Avoid double-wrapping ext builder name

This PR suggestion is actually wrong, wondering if it could have misled other PRs. A bit worried that this issue might have spread to other ext since toXContent is rarely tested/called in production setups (except via the query-insights plugin)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.42%. Comparing base (f9c1bc0) to head (7cd347a).

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #1907   +/-   ##
=========================================
  Coverage     83.42%   83.42%           
  Complexity     3896     3896           
=========================================
  Files           291      291           
  Lines         13844    13844           
  Branches       2304     2304           
=========================================
  Hits          11550    11550           
+ Misses         1458     1457    -1     
- Partials        836      837    +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[heemin32]

rolling upgrade keeps failing.

[github-actions]

Persistent review updated to latest commit 7cd347a

[nomoa]

just rebased the PR (CHANGELOG conflict). Unsure about the rolling upgrade problem, quickly skimmed through the logs I don't see anything directly related to this change.

[heemin32]

The failure is not from this PR. The ml-common created a new index proactively for a new node and its replica could not be assigned to old node. Let me merge this PR.