feat(search): bring query implementation back in-repo#3550
Conversation
…mentation.Search Reverses commit b38a552 which extracted the search query implementation into the Elastic.Internal.Search.Elasticsearch NuGet package. The query code now lives in-repo as part of Elastic.Documentation.Search, which is the right long-term home — it's tightly coupled to docs-builder's index structure and rarely consumed outside this repo. Changes: - Add SearchQueryConfiguration record (replaces the lean DTO from the package) - Add DefaultSearchService<TDocument> (hybrid lex+semantic, query builder, autocomplete) - Add SearchQueryBuilder (lexical/semantic/diminish/rule-query construction) - Add QueryFieldNames (centralised ES field-name constants) - Add Highlighting: SearchResultProcessor, StringHighlightExtensions, HighlightOptions - Add Diagnostics: SearchExplainExtensions (_explain API for relevance tests) - Add SourceGenerationContext (AOT-safe JSON source-gen for RuleQueryMatchCriteria) - Remove Elastic.Internal.Search.Elasticsearch package reference and pin NOTE: three features present in the website-search-data source are stripped because they depend on types not yet published in Contract 0.9.2: - ElasticsearchTookMs / IsValidResponse on SearchResponse/AutocompleteResponse - probe-mode SearchAsync (requires SearchQueryComponents) Restore when Contract publishes a version with these types. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Is this a true revert? Or are we keeping like attribute names? |
|
Warning Review limit reached
More reviews will be available in 38 minutes and 40 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR removes the Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/services/search/Elastic.Documentation.Search/Highlighting/SearchResultProcessor.cs (1)
38-42: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueHardcoded field names should use
QueryFieldNamesconstants.The highlight field names
"stripped_body"and"title"are hardcoded here, butQueryFieldNames(from the query contracts layer) centralizes these constants. Using the constants would ensure consistency if field names change.Suggested fix
- if (highlights.TryGetValue("stripped_body", out var bodyHighlights) && bodyHighlights.Count > 0) + if (highlights.TryGetValue(QueryFieldNames.StrippedBody, out var bodyHighlights) && bodyHighlights.Count > 0) highlightedBody = string.Join(". ", bodyHighlights.Select(h => h.Trim(['|', ' ', '.', '-']))); - if (highlights.TryGetValue("title", out var titleHighlights) && titleHighlights.Count > 0) + if (highlights.TryGetValue(QueryFieldNames.Title, out var titleHighlights) && titleHighlights.Count > 0) highlightedTitle = string.Join(". ", titleHighlights.Select(h => h.Trim(['|', ' ', '.', '-'])));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/search/Elastic.Documentation.Search/Highlighting/SearchResultProcessor.cs` around lines 38 - 42, In the SearchResultProcessor class, replace the hardcoded field names "stripped_body" and "title" in the highlights.TryGetValue calls with the corresponding constants from QueryFieldNames. This ensures field name consistency across the codebase and makes future maintenance easier if field names need to be updated in one centralized location rather than scattered throughout the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/search/Elastic.Documentation.Search/DefaultSearchService.cs`:
- Around line 184-188: The nested aggregation named "applies_to_type" with inner
terms "types" is being extracted at line 227-228 but is never defined in the
.Aggregations() builder at lines 184-187. Add the missing aggregation definition
to the .Aggregations() builder using .Add("applies_to_type", ...) with the
appropriate nested Terms aggregation for the types, or alternatively remove the
extraction code at line 227-228 if the deployment type facet is not currently
needed.
In
`@src/services/search/Elastic.Documentation.Search/Diagnostics/SearchExplainExtensions.cs`:
- Around line 38-41: All await expressions in this library file must include
.ConfigureAwait(false) to prevent ambient context capture and deadlock risks.
Locate the six await sites mentioned (lines 38, 56, 89, 101, 104-105) and add
.ConfigureAwait(false) to each await call, including the
service.Client.SearchAsync await shown in the diff and any other async
operations throughout the file. This applies to all async method calls to ensure
proper library code behavior.
---
Nitpick comments:
In
`@src/services/search/Elastic.Documentation.Search/Highlighting/SearchResultProcessor.cs`:
- Around line 38-42: In the SearchResultProcessor class, replace the hardcoded
field names "stripped_body" and "title" in the highlights.TryGetValue calls with
the corresponding constants from QueryFieldNames. This ensures field name
consistency across the codebase and makes future maintenance easier if field
names need to be updated in one centralized location rather than scattered
throughout the code.
🪄 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: Enterprise
Run ID: 408c7acf-537e-41b7-a2ab-0c5125e8cdcf
📒 Files selected for processing (17)
Directory.Packages.propssrc/services/search/Elastic.Documentation.Search/Common/ElasticsearchClientJsonResolver.cssrc/services/search/Elastic.Documentation.Search/Configuration/SearchQueryConfiguration.cssrc/services/search/Elastic.Documentation.Search/DefaultSearchService.cssrc/services/search/Elastic.Documentation.Search/Diagnostics/SearchExplainExtensions.cssrc/services/search/Elastic.Documentation.Search/Elastic.Documentation.Search.csprojsrc/services/search/Elastic.Documentation.Search/Highlighting/HighlightOptions.cssrc/services/search/Elastic.Documentation.Search/Highlighting/SearchResultProcessor.cssrc/services/search/Elastic.Documentation.Search/Highlighting/StringHighlightExtensions.cssrc/services/search/Elastic.Documentation.Search/NavigationSearchService.cssrc/services/search/Elastic.Documentation.Search/Query/QueryFieldNames.cssrc/services/search/Elastic.Documentation.Search/Query/SearchQueryBuilder.cssrc/services/search/Elastic.Documentation.Search/ServicesExtension.cssrc/services/search/Elastic.Documentation.Search/SourceGenerationContext.cstests-integration/Mcp.Remote.IntegrationTests/McpToolsIntegrationTestsBase.cstests-integration/Search.IntegrationTests/SearchRelevanceTests.cstests/Elastic.Documentation.Api.Infrastructure.Tests/Adapters/Search/StringHighlightExtensionsTests.cs
💤 Files with no reviewable changes (2)
- src/services/search/Elastic.Documentation.Search/Elastic.Documentation.Search.csproj
- Directory.Packages.props
- Remove dead DeploymentType extraction in DefaultSearchService — the 'applies_to_type' nested aggregation was never defined in the query builder, so ExtractNestedTermsAggregation always returned []; the default on SearchAggregations covers this - Add .ConfigureAwait(false) to all six await sites in SearchExplainExtensions (library code requirement) - Replace hardcoded "stripped_body" / "title" strings in SearchResultProcessor with QueryFieldNames.StrippedBody / .Title Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@reakaleek this does not touch the attributes. |
Summary
Elastic.Internal.Search.ElasticsearchNuGet package from website-search-dataElastic.Documentation.Search.*— the right long-term home, since it's tightly coupled to docs-builder's index structureSearchQueryConfiguration(lean 4-field record) replacing the lean DTO from the packageElastic.Internal.Search.Elasticsearchpackage reference entirelyWhat moved in
DefaultSearchService.csISearchService<T>Configuration/SearchQueryConfiguration.csQuery/SearchQueryBuilder.csQuery/QueryFieldNames.csHighlighting/SearchResultProcessor.csSearchResultItem<T>, merges ES highlight fragmentsHighlighting/StringHighlightExtensions.cs<mark>over un-highlighted occurrencesHighlighting/HighlightOptions.csDiagnostics/SearchExplainExtensions.cs_explainAPI helpers for relevance testsSourceGenerationContext.csRuleQueryMatchCriteriaFeatures stripped pending a Contract update
Three features from the website-search-data source require types not yet published in
Elastic.Internal.Search.Contract0.9.2. They are stripped with// NOTE:comments to restore:ElasticsearchTookMs/IsValidResponseonSearchResponse<T>/AutocompleteResponse<T>SearchAsyncbranch (requiresSearchQueryComponents)A companion PR in website-search-data will remove
Elastic.Internal.Search.Elasticsearchand reworkesscto validate instead of publishing synonym sets (since docs-builder owns those resources).Test plan
dotnet build— 0 errors, 0 warnings./build.sh unit-test— 1761 + 314 tests pass (14 pre-existing scoped-FS failures unrelated to this change)grep -rn "Elastic.Internal.Search.Elasticsearch" src tests tests-integration→ zero hitsStringHighlightExtensionsTests(91 API infra tests) — all pass🤖 Generated with Claude Code