Conversation
epugh
left a comment
There was a problem hiding this comment.
Looks like you are on the right path..
|
@Bharathi-Kanna could you set up signing of yoru commits? WHen you sign with |
|
Looks like good progress. We still need some tests for handling it.. Let me know how I can help! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #342 +/- ##
===========================
===========================
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Added 4 test methods to SearchRequestBuilderTests.java - testMustacheWithNullScriptService: Validates error handling - testLegacyWildcardStillWorks: Confirms backward compatibility - testDetectionLogicUsesLegacyForNonMustache: Verifies detection logic - testHybridSearchMustacheDetection: Tests hybrid query support - Updated CHANGELOG.md with feature entry - All tests passing (13/13 SearchRequestBuilderTests) Addresses maintainer feedback on PR opensearch-project#342 Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com>
|
Hi @epugh, Thanks for the feedback! I've added:
This PR enables Mustache templating with {{query_string}} support. Would you prefer this incremental approach, or include data model changes in this PR? |
Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com>
- Added 4 test methods to SearchRequestBuilderTests.java - testMustacheWithNullScriptService: Validates error handling - testLegacyWildcardStillWorks: Confirms backward compatibility - testDetectionLogicUsesLegacyForNonMustache: Verifies detection logic - testHybridSearchMustacheDetection: Tests hybrid query support - Updated CHANGELOG.md with feature entry - All tests passing (13/13 SearchRequestBuilderTests) Addresses maintainer feedback on PR opensearch-project#342 Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com>
9edb7eb to
7039e8b
Compare
Signed-off-by: Bharathi Kanna <99189546+Bharathi-Kanna@users.noreply.github.com>
|
This is good but I think we could extend this further and support multiple custom variables instead of single fixed query text. Example 1Example 2 |
Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com>
…ields Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com>
Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com>
|
Hi @heemin32 ! Thank you for the feedback, I completely agree. I've just pushed a major update to this PR that fully implements what you described! And those values will automatically hydrate the exact Mustache template configurations you provided in your examples (e.g., {{status}}, {{category}}). To ensure a seamless implementation, I also: |
|
@Bharathi-Kanna Thanks for the change! By the way, wouldn't it be sufficient to simply use string replacement instead of relying on the script service? Or at least can we disable partial from mustache? |
|
Thanks @heemin32 ! On string replacement: I'd prefer to keep On partials: You're right, I checked, and OpenSearch's |
Simple validation won't handle several other edge cases. One example is |
|
We could use mustache now as OpenSearch core disabled partial template resolution. |
|
@Bharathi-Kanna could you open the corresponding document update pr for this change? |
…mplate-support Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com> # Conflicts: # CHANGELOG.md
buildCacheKey had been changed to a newline-delimited format
(queryText#\nkey:value), which silently invalidated existing judgment
cache entries on upgrade — the key is used for both writes and reads, so
entries stored in the released JSON format (queryText#{json}) would no
longer be found, forcing needless LLM recomputation. The change also
contradicted the method's own Javadoc and QuerySetEntry.parseLegacyQueryText,
which both expect the JSON format, and was unrelated to the Mustache
feature (SearchRequestBuilder never touches buildCacheKey).
Restore the upstream JSON serialization via OBJECT_MAPPER (already a
dependency in this file). No test changes.
Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com>
Add MustacheTemplateRenderTests, which exercises the real SearchRequestBuilder
render path against a live ScriptService backed by the core MustacheScriptEngine
(the existing SearchRequestBuilderTests only cover the legacy %SearchText% and
null-ScriptService error paths). It deterministically asserts single-variable and
multi-variable substitution, JSON escaping of special characters, empty rendering
of missing variables, and rejection of Mustache partials ({{>...}}) — the latter
verifying OpenSearch core's partial-resolution guard is present.
Pull in lang-mustache-client as a test dependency so the engine is available to
unit tests (the node provides it at runtime).
Fix the multi-variable search-configuration fixture: it referenced an unprovided
{{category_filter}} variable and applied a term filter on the analyzed 'category'
field. Reference the supplied {{category}} custom field and target the
'category_filter' keyword field, using an optional should clause so results are
driven by the title match (the ESCI sample data is not category-filtered).
Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com>
A search configuration whose query is an invalid Mustache template — most
notably one containing an unsupported partial ({{>...}}), which OpenSearch
core now rejects at compile time — caused SearchRequestBuilder to throw
synchronously inside ExperimentTaskManager.executeVariantAsync, before
client.search was invoked. That path never called completeVariantFailure(),
so ExperimentTaskContext.remainingVariants never reached zero and the
experiment hung in PROCESSING indefinitely.
Wrap the request build in a try/catch that routes the failure through the
same handleSearchFailure path used for search errors, so the variant is
counted, the failure is recorded, and the experiment reaches COMPLETED.
Add a regression IT (partial-template search config -> experiment COMPLETED)
plus the supporting fixture and helper.
Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com>
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 0a9290b.
The table above displays the top 10 most important findings. Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
|
Hi @heemin32, |
Thanks. While I reviewing the document, I found this is a little confusing. Mustache Template (New): Legacy Placeholder (Still Works): Shouldn't we just keep it consistent from query sets field name? |
The query text was exposed to Mustache templates as {{query_string}}, which did
not match the query set field name (queryText). Custom fields are already
referenced by their field names (for example, {{category}}), so the query text
now follows the same convention and is available as {{queryText}}. Also updates
the processMustacheTemplate Javadoc to document customFields.
Addresses review feedback on opensearch-project#342.
Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com>
Match the query set field name (queryText) instead of {{query_string}}, per
review feedback on opensearch-project/search-relevance#342.
Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com>
|
Applied and updated all ,Thanks @heemin32 ! |
) * Document Mustache template support in Search Relevance Workbench Search configurations can now use Mustache template variables ({{query_string}} and query set custom fields) in the query, in addition to the existing %SearchText% placeholder. Document the templating behavior, variable sources, automatic JSON escaping, and that Mustache partials are not supported, with an example. Also document custom fields on query set entries and how they map to template variables. Corresponds to opensearch-project/search-relevance#342. Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com> * Use {{queryText}} for the Mustache query variable Match the query set field name (queryText) instead of {{query_string}}, per review feedback on opensearch-project/search-relevance#342. Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com> * Doc review Signed-off-by: Fanit Kolchina <kolchfa@amazon.com> * Apply suggestion from @kolchfa-aws Signed-off-by: kolchfa-aws <105444904+kolchfa-aws@users.noreply.github.com> --------- Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com> Signed-off-by: Fanit Kolchina <kolchfa@amazon.com> Signed-off-by: kolchfa-aws <105444904+kolchfa-aws@users.noreply.github.com> Co-authored-by: Fanit Kolchina <kolchfa@amazon.com> Co-authored-by: kolchfa-aws <105444904+kolchfa-aws@users.noreply.github.com>
…nsearch-project#12810) * Document Mustache template support in Search Relevance Workbench Search configurations can now use Mustache template variables ({{query_string}} and query set custom fields) in the query, in addition to the existing %SearchText% placeholder. Document the templating behavior, variable sources, automatic JSON escaping, and that Mustache partials are not supported, with an example. Also document custom fields on query set entries and how they map to template variables. Corresponds to opensearch-project/search-relevance#342. Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com> * Use {{queryText}} for the Mustache query variable Match the query set field name (queryText) instead of {{query_string}}, per review feedback on opensearch-project/search-relevance#342. Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com> * Doc review Signed-off-by: Fanit Kolchina <kolchfa@amazon.com> * Apply suggestion from @kolchfa-aws Signed-off-by: kolchfa-aws <105444904+kolchfa-aws@users.noreply.github.com> --------- Signed-off-by: Bharathi-Kanna <99189546+Bharathi-Kanna@users.noreply.github.com> Signed-off-by: Fanit Kolchina <kolchfa@amazon.com> Signed-off-by: kolchfa-aws <105444904+kolchfa-aws@users.noreply.github.com> Co-authored-by: Fanit Kolchina <kolchfa@amazon.com> Co-authored-by: kolchfa-aws <105444904+kolchfa-aws@users.noreply.github.com>

Description
This PR implements backend support for Mustache templates in search queries using OpenSearch's native
ScriptService
Key Changes:
Integrated Mustache Templating
Maintained Backward Compatibility
Example Usage
Mustache Template (New):
Legacy Placeholder (Still Works):
Both syntaxes work side-by-side. The system auto-detects which one to use.
Current Limitation: Single-parameter queries only (matches existing QuerySetEntry as List)
Multi-parameter support (e.g., {{brand}}, {{category}}) requires data model changes and will be addressed in a future PR.
Issues Resolved
Relates to #41
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.