Search: Include synonym ID in synonym rule values#2999
Open
Search: Include synonym ID in synonym rule values#2999
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughSynonyms representation changed from a dictionary to a list of string arrays across the codebase: Sequence Diagram(s)sequenceDiagram
participant Config as "SearchConfiguration\n(synonyms: IReadOnlyList<string[]])"
participant Exporter as "ElasticsearchMarkdownExporter\n(_synonyms: IReadOnlyList<string[]])"
participant ES as "Elasticsearch\n(Synonym API)"
Config->>Exporter: Provide Synonyms (list of string[] )
Exporter->>Exporter: Filter entries where any element in fixedSynonyms
Exporter->>Exporter: For each matching string[] -> join elements -> build indexTimeSynonyms
Exporter->>Exporter: In PublishSynonymsAsync -> for each string[] s: Id = s[0], Synonyms = join(s)
Exporter->>ES: Publish SynonymRule(Id, Synonyms) via API
ES-->>Exporter: Acknowledge
Suggested labels
🚥 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✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/Elastic.Markdown/Exporters/Elasticsearch/ElasticsearchMarkdownExporter.cs`:
- Around line 241-242: The publish path (PublishSynonymsAsync) includes the
synonym key when building the synonym string but the constructor logic that
builds indexTimeSynonyms omits it, causing inconsistent synonym sets and hash
inputs; update the constructor/initialization that builds indexTimeSynonyms to
construct synonyms the same way as PublishSynonymsAsync (include the key as the
first token when joining the synonyms) so SynonymRule entries (Id and Synonyms)
are generated identically for both published and index-time paths—refer to
SynonymRule, the id variable and synonym.Value usage and make the join logic
consistent with the existing PublishSynonymsAsync pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3fcc140-4fe2-4787-ace3-583446a903c8
📒 Files selected for processing (1)
src/Elastic.Markdown/Exporters/Elasticsearch/ElasticsearchMarkdownExporter.cs
src/Elastic.Markdown/Exporters/Elasticsearch/ElasticsearchMarkdownExporter.cs
Outdated
Show resolved
Hide resolved
cotti
requested changes
Mar 30, 2026
Contributor
cotti
left a comment
cotti
left a comment
There was a problem hiding this comment.
I think CodeRabbit's suggestion makes sense, otherwise LGTM
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Elastic.Documentation.Configuration/Search/SearchConfiguration.cs`:
- Around line 127-130: The current mapping of searchDto.Synonyms into synonyms
loses the prior uniqueness enforcement on the first term, allowing duplicate
rule IDs (used as s[0] in ElasticsearchMarkdownExporter.Export or similar) to be
produced; update the transformation in SearchConfiguration (where
searchDto.Synonyms is processed) to enforce uniqueness on the first term — e.g.,
group by s.First()/s[0] and either deduplicate (take first occurrence) or throw
a validation exception when duplicates exist — so that the resulting synonyms
array contains only one entry per unique first term and prevents duplicate rule
IDs during export.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ca3cf4ff-97bd-4ae9-ad57-4c5ad825e155
📒 Files selected for processing (9)
.superset/config.jsonsrc/Elastic.Documentation.Configuration/Search/SearchConfiguration.cssrc/Elastic.Markdown/Exporters/Elasticsearch/ElasticsearchMarkdownExporter.cstests-integration/Elastic.Assembler.IntegrationTests/TestHelpers.cstests-integration/Mcp.Remote.IntegrationTests/McpToolsIntegrationTestsBase.cstests/Elastic.ApiExplorer.Tests/TestHelpers.cstests/Elastic.Changelog.Tests/Changelogs/ChangelogTestBase.cstests/Elastic.Documentation.Build.Tests/TestHelpers.cstests/Elastic.Markdown.Tests/TestHelpers.cs
✅ Files skipped from review due to trivial changes (6)
- tests/Elastic.Documentation.Build.Tests/TestHelpers.cs
- tests-integration/Elastic.Assembler.IntegrationTests/TestHelpers.cs
- tests-integration/Mcp.Remote.IntegrationTests/McpToolsIntegrationTestsBase.cs
- tests/Elastic.Markdown.Tests/TestHelpers.cs
- tests/Elastic.ApiExplorer.Tests/TestHelpers.cs
- .superset/config.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Elastic.Markdown/Exporters/Elasticsearch/ElasticsearchMarkdownExporter.cs
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
|
💚 CLA has been signed |
The synonym dictionary uses the first term as the key and remaining terms as values. When publishing to Elasticsearch, only the values were included in the synonym string, omitting the key term itself. This meant e.g. "ilm" was the rule ID but not part of the synonym set, so it wouldn't match "index lifecycle management". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The dictionary structure used the first term as the key and Skip(1) for values, which artificially separated one synonym from its group. This caused the key term to be omitted from synonym rules sent to Elasticsearch. Replace Dictionary<string, string[]> with IReadOnlyList<string[]> so all terms in a synonym group are treated equally. The first term is still used as the ES rule ID for readability but is no longer excluded from the synonym string. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.
What
Fix synonym rules to include the ID term in the synonym set values sent to Elasticsearch.
Why
The synonym dictionary uses the first term as the key (and rule ID) with remaining terms as values. When publishing, only the values were joined into the synonym string — omitting the key term. For example, a rule with ID
ilmwould only haveindex lifecycle managementas its synonym, meaning searching for "ilm" wouldn't trigger the synonym expansion.How
Prepend the synonym ID to the values array when building the comma-separated synonym string in
PublishSynonymsAsync.Test plan
"ilm, index lifecycle management"instead of just"index lifecycle management")🤖 Generated with Claude Code