fix: remove RuleQueryMatchCriteria hacks and prefer env vars over secrets#3462
Merged
Conversation
…rets
- Bump Elastic.Internal.Search.{Contract,Elasticsearch} to 0.9.2 which
exposes RuleQueryMatchCriteria and its SourceGenerationContext for
AOT-compatible serialization
- Remove the reflection-based RuleQueryMatchCriteriaTypeInfoResolver and
RuleQueryMatchCriteriaJsonConverterFactory that broke under .NET 10's
stricter JsonSerializerOptions compatibility checks
- Chain the new Elasticsearch package SourceGenerationContext into the
resolver so rule queries serialize correctly without reflection
- Reorder config lookups in ElasticsearchEndpointFactory so environment
variables take precedence over dotnet user secrets
- Suppress noisy OpenTelemetry logs (default to Warning) in MCP server
appsettings
Co-authored-by: Cursor <cursoragent@cursor.com>
Mpdreamz
temporarily deployed
to
integration-tests
GitHub Actions
Inactive
Contributor
📝 WalkthroughWalkthroughThis PR upgrades Elasticsearch internal search packages to 0.9.2, refactors the configuration system to favor environment variables over user secrets, and removes custom JSON serialization logic in favor of source generation contexts. It adds OpenTelemetry logging configuration across environments and updates integration tests to remove hardcoded environment parameters, allowing them to use the new configuration priority system. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Elastic.Documentation.ServiceDefaults/ElasticsearchEndpointFactory.cs (1)
83-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap standard host environment names before the allowlist.
DOTNET_ENVIRONMENTis typicallyDevelopment,Staging, orProduction. The new check only acceptsdev|prod|stagingbefore normalization, soProductionandStagingnow collapse to"dev". That makes callers that stopped passingenvironment: "dev"resolve the wrong index/endpoint outside local dev.Suggested fix
- string[] allowedEnvironements = ["dev", "prod", "staging"]; - if (!allowedEnvironements.Contains(envVar)) - envVar = "dev"; - - return !string.IsNullOrEmpty(envVar) ? envVar.ToLowerInvariant() : "dev"; + return envVar?.Trim().ToLowerInvariant() switch + { + "development" or "dev" => "dev", + "staging" => "staging", + "production" or "prod" => "prod", + _ => "dev" + };🤖 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/Elastic.Documentation.ServiceDefaults/ElasticsearchEndpointFactory.cs` around lines 83 - 92, The envVar lookup in ElasticsearchEndpointFactory.cs currently compares raw values against allowedEnvironements and defaults to "dev", which causes canonical values like "Production" or "Development" to be treated as invalid; update the logic so you first normalize envVar (e.g., call ToLowerInvariant and Trim) and map standard .NET environment names ("development" -> "dev", "production" -> "prod", "staging" -> "staging") before checking the allowlist (allowedEnvironments) — then validate against the allowlist and return the normalized mapped value (or "dev" as fallback); ensure you update the variable names if needed (envVar, allowedEnvironments) and preserve the final return semantics.
🤖 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.
Outside diff comments:
In `@src/Elastic.Documentation.ServiceDefaults/ElasticsearchEndpointFactory.cs`:
- Around line 83-92: The envVar lookup in ElasticsearchEndpointFactory.cs
currently compares raw values against allowedEnvironements and defaults to
"dev", which causes canonical values like "Production" or "Development" to be
treated as invalid; update the logic so you first normalize envVar (e.g., call
ToLowerInvariant and Trim) and map standard .NET environment names
("development" -> "dev", "production" -> "prod", "staging" -> "staging") before
checking the allowlist (allowedEnvironments) — then validate against the
allowlist and return the normalized mapped value (or "dev" as fallback); ensure
you update the variable names if needed (envVar, allowedEnvironments) and
preserve the final return semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8951534c-2dcb-47b9-b291-d8e50ce72b63
📒 Files selected for processing (11)
Directory.Packages.propssrc/Elastic.Documentation.ServiceDefaults/ElasticsearchEndpointFactory.cssrc/api/Elastic.Documentation.Mcp.Remote/appsettings.development.jsonsrc/api/Elastic.Documentation.Mcp.Remote/appsettings.edge.jsonsrc/api/Elastic.Documentation.Mcp.Remote/appsettings.jsonsrc/services/search/Elastic.Documentation.Search/Common/ElasticsearchClientAccessor.cssrc/services/search/Elastic.Documentation.Search/Common/ElasticsearchClientJsonResolver.cssrc/services/search/Elastic.Documentation.Search/Common/RuleQueryMatchCriteriaJsonConverter.cssrc/services/search/Elastic.Documentation.Search/Common/RuleQueryMatchCriteriaTypeInfoResolver.cstests-integration/Mcp.Remote.IntegrationTests/McpToolsIntegrationTestsBase.cstests-integration/Search.IntegrationTests/SearchRelevanceTests.cs
💤 Files with no reviewable changes (2)
- src/services/search/Elastic.Documentation.Search/Common/RuleQueryMatchCriteriaTypeInfoResolver.cs
- src/services/search/Elastic.Documentation.Search/Common/RuleQueryMatchCriteriaJsonConverter.cs
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
1 participant
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.
Summary
Elastic.Internal.Search.{Contract,Elasticsearch}to 0.9.2 which properly exposesRuleQueryMatchCriteriawith a source-gen context for AOT serializationRuleQueryMatchCriteriaTypeInfoResolverandRuleQueryMatchCriteriaJsonConverterFactorythat broke under .NET 10's stricterJsonSerializerOptionscompatibility checksElasticsearchEndpointFactoryso environment variables take precedence over dotnet user secrets, making it easier to override endpoints in CI/containersContext
The old workarounds for serializing the internal
RuleQueryMatchCriteriatype usedDynamicDependency,UnsafeAccessor, and cachedJsonTypeInfo— these broke in .NET 10 with:The proper fix (elastic/website-search-data#198) makes the type public and adds a
SourceGenerationContextin theElastic.Internal.Search.Elasticsearchpackage. This PR consumes that fix by chaining the new context into the resolver.Test plan
dotnet buildpasses forElastic.Documentation.Search, MCP server, and integration testsMade with Cursor