refactor(search): consume Elastic.Internal.Search.* from NuGet#3364
Open
Mpdreamz wants to merge 5 commits into
Open
refactor(search): consume Elastic.Internal.Search.* from NuGet#3364Mpdreamz wants to merge 5 commits into
Mpdreamz wants to merge 5 commits into
Conversation
…te-search-data
Move all shared search types (SearchDocumentBase polymorphic root, document
records, mapping contexts, request/response DTOs, ISearchService<TDocument>)
to the new Elastic.Internal.Search.Contract package owned by website-search-data.
Reference it via on-disk ProjectReference for now; swap to a real NuGet feed
once published.
- Delete the docs-builder Elastic.Documentation.Search.Contract project; all
three duplicate types (DocumentationDocument, IndexedProduct, AppliesToEntry)
now come from the shared package.
- Bump Elastic.Mapping to 0.44.1 to match the package's transitive requirement.
- DocumentationMappingContext (with the docs-{type}.semantic-{env} index template)
moves into the shared package too; docs-builder consumes it directly.
- Migrate code from settable Type = "doc"/"api" to ContentType assignments; the
new Contract makes Type a JsonIgnore virtual discriminator. Existing indexed
data keeps working via the content_type field already written by docs-builder.
dotnet build docs-builder.slnx: 0 errors.
dotnet test tests/Elastic.Markdown.Tests --filter '~Search': 16/16 pass.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Swap on-disk ProjectReferences (sibling website-ai-search checkout) for PackageReferences to Elastic.Internal.Search.Contract and Elastic.Internal.Search.Elasticsearch (0.0.0-alpha.0.139). Remove the local NuGet feed from NuGet.config now that packages are on nuget.org. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
dotnet format fixes import ordering and IDE0002 simplifications across files that gained new using directives from the Elastic.Internal.Search.* package swap. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Mpdreamz
had a problem deploying
to
integration-tests
GitHub Actions
Failure
Contributor
📝 WalkthroughWalkthroughThis PR externalizes the docs-builder's search implementation by migrating from a local 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 |
First published version on nuget.org; version scheme changed from 0.0.0-alpha.* to 0.1.0-canary.*. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Mpdreamz
had a problem deploying
to
integration-tests
GitHub Actions
Failure
![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: 3
🧹 Nitpick comments (1)
src/services/search/Elastic.Documentation.Search/ChangesService.cs (1)
112-120: ⚡ Quick winAlign
_sourceincludes withDocumentationDocument.ContentType
ChangesService’s_sourcefilter still requestse => e.Type, butDocumentationDocumentSerializationTestsshowTypeis the polymorphic discriminator (while the actual content type isContentType, used in other search services). Keep the payload/contract consistent by includingContentType.Proposed fix
- e => e.Type, + e => e.ContentType,🤖 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/ChangesService.cs` around lines 112 - 120, The _source filter in ChangesService is including e => e.Type (the polymorphic discriminator) instead of the actual content type field; update the .Source(...).Filter(...).Includes(...) call in ChangesService to remove e => e.Type and include e => e.ContentType so the payload matches DocumentationDocument's contract (as used by other search services and tested by DocumentationDocumentSerializationTests).
🤖 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 `@NuGet.config`:
- Around line 3-5: The NuGet.config's <packageSources> currently adds only
nuget.org but will inherit other feeds from higher-level configs; update the
<packageSources> block to include a <clear /> element before the <add
key="nuget.org" ... /> entry so that nuget.org becomes the sole package source
(i.e., insert <clear /> above the existing <add> line inside the packageSources
element).
In `@src/services/search/Elastic.Documentation.Search/FullSearchService.cs`:
- Around line 86-90: The RelatedProducts projection in FullSearchService (the
block that maps item.Document.RelatedProducts using MapProduct) can return null
when there are no related products; change it to normalize to an empty array
like Parents does — filter and map as before but ensure the final value is
non-null by returning Array.Empty<YourProductType>() (or use the equivalent
empty array construct) when the source is null or yields no items, keeping
MapProduct and the null-check on p.Id intact.
In `@src/services/search/Elastic.Documentation.Search/ServicesExtension.cs`:
- Around line 31-62: ElasticsearchClientAccessor is registered as a singleton
but NavigationSearchService and FullSearchService (the scoped adapters)
currently call clientAccessor.Dispose() in their Dispose methods, which will
tear down the shared singleton; remove that disposal call (or add an explicit
ownership flag so only the component that created the accessor disposes it) from
the Dispose/DisposeAsync implementations in NavigationSearchService and
FullSearchService (and any other scoped types that call Dispose on
ElasticsearchClientAccessor), ensuring only the owner of
ElasticsearchClientAccessor disposes it.
---
Nitpick comments:
In `@src/services/search/Elastic.Documentation.Search/ChangesService.cs`:
- Around line 112-120: The _source filter in ChangesService is including e =>
e.Type (the polymorphic discriminator) instead of the actual content type field;
update the .Source(...).Filter(...).Includes(...) call in ChangesService to
remove e => e.Type and include e => e.ContentType so the payload matches
DocumentationDocument's contract (as used by other search services and tested by
DocumentationDocumentSerializationTests).
🪄 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: f2ae2ec0-bd5e-49aa-b22a-741a5d0f4d4d
📒 Files selected for processing (35)
Directory.Packages.propsNuGet.configdocs-builder.slnxsrc/Elastic.ApiExplorer/Elasticsearch/OpenApiDocumentExporter.cssrc/Elastic.Documentation.Configuration/Products/Product.cssrc/Elastic.Documentation/AppliesTo/ApplicableTo.cssrc/Elastic.Documentation/Elastic.Documentation.csprojsrc/Elastic.Documentation/Serialization/SourceGenerationContext.cssrc/Elastic.Markdown/Exporters/Elasticsearch/ElasticsearchMarkdownExporter.Export.cssrc/Elastic.Markdown/Exporters/Elasticsearch/ElasticsearchMarkdownExporter.cssrc/api/Elastic.Documentation.Mcp.Remote/Gateways/DocumentGateway.cssrc/services/Elastic.Documentation.Assembler/Building/AssemblerSitemapService.cssrc/services/search/Elastic.Documentation.Search.Contract/AppliesToEntry.cssrc/services/search/Elastic.Documentation.Search.Contract/DocumentationDocument.cssrc/services/search/Elastic.Documentation.Search.Contract/DocumentationMappingConfig.cssrc/services/search/Elastic.Documentation.Search.Contract/Elastic.Documentation.Search.Contract.csprojsrc/services/search/Elastic.Documentation.Search.Contract/IndexedProduct.cssrc/services/search/Elastic.Documentation.Search/ChangesService.cssrc/services/search/Elastic.Documentation.Search/Common/ElasticsearchClientAccessor.cssrc/services/search/Elastic.Documentation.Search/Common/SearchQueryBuilder.cssrc/services/search/Elastic.Documentation.Search/Common/SearchResultProcessor.cssrc/services/search/Elastic.Documentation.Search/Elastic.Documentation.Search.csprojsrc/services/search/Elastic.Documentation.Search/EsJsonContext.cssrc/services/search/Elastic.Documentation.Search/FullSearchService.cssrc/services/search/Elastic.Documentation.Search/NavigationSearchService.cssrc/services/search/Elastic.Documentation.Search/ServicesExtension.cssrc/services/search/Elastic.Documentation.Search/StringHighlightExtensions.cstests-integration/Elastic.ContentDateEnrichment.IntegrationTests/ContentDateEnrichmentTests.cstests-integration/Elastic.Documentation.IntegrationTests/Search/SearchBootstrapFixture.cstests-integration/Mcp.Remote.IntegrationTests/McpToolsIntegrationTestsBase.cstests-integration/Search.IntegrationTests/SearchRelevanceTests.cstests/Elastic.ApiExplorer.Tests/OpenApiDocumentExporterTests.cstests/Elastic.Changelog.Tests/Changelogs/ChangelogConfigurationTests.cstests/Elastic.Documentation.Api.Infrastructure.Tests/Adapters/Search/StringHighlightExtensionsTests.cstests/Elastic.Markdown.Tests/Search/DocumentationDocumentSerializationTests.cs
💤 Files with no reviewable changes (10)
- src/services/search/Elastic.Documentation.Search.Contract/IndexedProduct.cs
- src/services/search/Elastic.Documentation.Search.Contract/DocumentationDocument.cs
- src/services/search/Elastic.Documentation.Search.Contract/AppliesToEntry.cs
- src/services/search/Elastic.Documentation.Search.Contract/Elastic.Documentation.Search.Contract.csproj
- src/services/search/Elastic.Documentation.Search/Common/SearchQueryBuilder.cs
- src/services/search/Elastic.Documentation.Search/StringHighlightExtensions.cs
- src/services/search/Elastic.Documentation.Search/Common/SearchResultProcessor.cs
- src/services/search/Elastic.Documentation.Search.Contract/DocumentationMappingConfig.cs
- docs-builder.slnx
- tests/Elastic.Changelog.Tests/Changelogs/ChangelogConfigurationTests.cs
Comment on lines
+3
to
+5
| <packageSources> | ||
| <add key="nuget.org" value="https://api.nuget.org/v3/index.json" protocolVersion="3" /> | ||
| </packageSources> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In NuGet.config, if <packageSources> does not include <clear />, do sources from user-level or machine-level config remain active due to hierarchical merging?
💡 Result:
Yes, if the element is not included in the section of a NuGet.config file, the package sources defined in higher-level configuration files (such as user-level or machine-level configs) remain active due to the hierarchical nature of NuGet configuration [1][2][3]. NuGet resolves its configuration by merging files from multiple locations—typically starting from the project or solution directory and traversing up the directory tree, finally including user-level and machine-level configuration files [2][3]. By default, this process aggregates sources from all encountered configuration files [1]. Including the element within the section is the explicit mechanism to instruct NuGet to ignore (clear) all package sources defined in any previously loaded (higher-level) configuration files [4][1][5].
Citations:
- 1: https://learn.microsoft.com/en-us/nuget/consume-packages/configuring-nuget-behavior
- 2: https://stackoverflow.com/questions/33831689/how-to-make-nuget-package-source-settings-per-solution
- 3: https://docs.myget.org/docs/how-to/nuget-configuration-inheritance
- 4: https://learn.microsoft.com/en-us/nuget/reference/nuget-config-file
- 5: https://learn.microsoft.com/en-us/nuget/consume-packages/package-source-mapping
Add <clear /> to make nuget.org the only NuGet package source
NuGet merges <packageSources> from higher-level configs unless <clear /> is present, so other feeds can remain active and conflict with the “sole source” intent. (NuGet.config, lines 3–5)
Suggested fix
<configuration>
<packageSources>
+ <clear />
<add key="nuget.org" value="https://api.nuget.org/v3/index.json" protocolVersion="3" />
</packageSources>
</configuration>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <packageSources> | |
| <add key="nuget.org" value="https://api.nuget.org/v3/index.json" protocolVersion="3" /> | |
| </packageSources> | |
| <configuration> | |
| <packageSources> | |
| <clear /> | |
| <add key="nuget.org" value="https://api.nuget.org/v3/index.json" protocolVersion="3" /> | |
| </packageSources> | |
| </configuration> |
🤖 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 `@NuGet.config` around lines 3 - 5, The NuGet.config's <packageSources>
currently adds only nuget.org but will inherit other feeds from higher-level
configs; update the <packageSources> block to include a <clear /> element before
the <add key="nuget.org" ... /> entry so that nuget.org becomes the sole package
source (i.e., insert <clear /> above the existing <add> line inside the
packageSources element).
Comment on lines
+86
to
+90
| RelatedProducts = item.Document.RelatedProducts? | ||
| .Where(p => p.Id is not null) | ||
| .Select(p => MapProduct(p)!) | ||
| .ToArray() | ||
| }; |
Contributor
There was a problem hiding this comment.
Normalize RelatedProducts to an empty collection.
This path returns null when the document has no related products, which pushes a nullable collection into the public search response. Default it to [] the same way Parents does.
Proposed fix
RelatedProducts = item.Document.RelatedProducts?
.Where(p => p.Id is not null)
.Select(p => MapProduct(p)!)
- .ToArray()
+ .ToArray() ?? []
};As per coding guidelines "Collections should never return null; return an empty array/collection instead."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| RelatedProducts = item.Document.RelatedProducts? | |
| .Where(p => p.Id is not null) | |
| .Select(p => MapProduct(p)!) | |
| .ToArray() | |
| }; | |
| RelatedProducts = item.Document.RelatedProducts? | |
| .Where(p => p.Id is not null) | |
| .Select(p => MapProduct(p)!) | |
| .ToArray() ?? [] | |
| }; |
🤖 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/FullSearchService.cs` around
lines 86 - 90, The RelatedProducts projection in FullSearchService (the block
that maps item.Document.RelatedProducts using MapProduct) can return null when
there are no related products; change it to normalize to an empty array like
Parents does — filter and map as before but ensure the final value is non-null
by returning Array.Empty<YourProductType>() (or use the equivalent empty array
construct) when the source is null or yields no items, keeping MapProduct and
the null-check on p.Id intact.
Comment on lines
31
to
62
| _ = services.AddSingleton<ElasticsearchClientAccessor>(); | ||
|
|
||
| // Navigation Search (autocomplete/navigation search) | ||
| _ = services.AddScoped<INavigationSearchService, NavigationSearchService>(); | ||
| // ProductsConfiguration already implements IProductNameLookup; surface it via the | ||
| // shared interface so both the inner DefaultSearchService (for aggregation buckets) and | ||
| // the FullSearchService adapter (for per-hit display names) can resolve it from DI. | ||
| _ = services.AddScoped<IProductNameLookup>(sp => sp.GetRequiredService<ProductsConfiguration>()); | ||
|
|
||
| // Inner search service: docs-builder pairs the typed contract with the docs index alias. | ||
| // docs-builder's SearchConfiguration (in Elastic.Documentation.Configuration.Search) is | ||
| // translated to the contract's shape — both carry synonyms+diminish-terms+ruleset, but | ||
| // the docs-builder type has richer QueryRule POCOs the shared service doesn't need. | ||
| _ = services.AddScoped<ISearchService<DocumentationDocument>>(sp => | ||
| { | ||
| var acc = sp.GetRequiredService<ElasticsearchClientAccessor>(); | ||
| var lookup = sp.GetRequiredService<IProductNameLookup>(); | ||
| var innerLogger = sp.GetRequiredService<ILogger<DefaultSearchService<DocumentationDocument>>>(); | ||
|
|
||
| var sharedConfig = new Internal.Search.Configuration.SearchConfiguration | ||
| { | ||
| SynonymBiDirectional = acc.SynonymBiDirectional, | ||
| DiminishTerms = acc.DiminishTerms.ToArray(), | ||
| RulesetName = acc.RulesetName, | ||
| SemanticEnabled = true | ||
| }; | ||
|
|
||
| // FullSearch (full-page search with hybrid RRF) | ||
| return new DefaultSearchService<DocumentationDocument>( | ||
| acc.Client, acc.SearchIndex, sharedConfig, innerLogger, lookup); | ||
| }); | ||
|
|
||
| // Docs-specific adapters preserve the existing API/MCP wire format. | ||
| _ = services.AddScoped<INavigationSearchService, NavigationSearchService>(); | ||
| _ = services.AddScoped<IFullSearchService, FullSearchService>(); |
Contributor
There was a problem hiding this comment.
Don't let scoped adapters tear down the singleton accessor.
ElasticsearchClientAccessor is registered as a singleton here, but both search adapters still call clientAccessor.Dispose() in their own Dispose() methods. The first scope that disposes either adapter will also dispose the shared accessor, and subsequent search requests will start failing.
🤖 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/ServicesExtension.cs` around
lines 31 - 62, ElasticsearchClientAccessor is registered as a singleton but
NavigationSearchService and FullSearchService (the scoped adapters) currently
call clientAccessor.Dispose() in their Dispose methods, which will tear down the
shared singleton; remove that disposal call (or add an explicit ownership flag
so only the component that created the accessor disposes it) from the
Dispose/DisposeAsync implementations in NavigationSearchService and
FullSearchService (and any other scoped types that call Dispose on
ElasticsearchClientAccessor), ensuring only the owner of
ElasticsearchClientAccessor disposes it.
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.
Why
The search contract and Elasticsearch implementation were developed in a sibling repo (
website-ai-search) and temporarily wired in via on-diskProjectReferenceentries. Now that both packages ship to nuget.org asElastic.Internal.Search.ContractandElastic.Internal.Search.Elasticsearch, the temporary workaround can be removed.What
ProjectReferenceentries inElastic.Documentation.Search.csprojandElastic.Documentation.csprojwith properPackageReferenceentries forElastic.Internal.Search.ContractandElastic.Internal.Search.Elasticsearch.website-search-data-locallocal feed fromNuGet.config; nuget.org is now the sole source.Directory.Packages.propsto0.0.0-alpha.0.139(.138was no longer available on the feed).