Upgrade to Elastic.Clients.Elasticsearch (ES 9.x)#216
Open
Upgrade to Elastic.Clients.Elasticsearch (ES 9.x)#216
Conversation
- Resolved 9 merge conflicts keeping new Elastic.Clients.Elasticsearch API - Adapted DefaultSortQueryBuilder from NEST to new client (SortOptions pattern) - Fixed Xunit.Abstractions -> Xunit namespace (xUnit v3 migration) - Fixed Task -> ValueTask for InitializeAsync/DisposeAsync - Removed ct: parameter from RefreshAsync (not in new client API) - Adapted NestedFieldTests aggregation API to new client fluent pattern - Took newer package versions from main (RandomData 2.0.1, Jint 4.6.0)
Ensures package versions start at 8.0 or higher. This aligns versioning with significant breaking changes, such as the upgrade to a new major Elasticsearch client, justifying a major version increment for the repository packages.
Updates Elasticsearch client usage across various index management and repository operations to align with "Opus" client API changes and address compatibility issues. * Changes wildcard index resolution from `ResolveIndexAsync` to `GetAsync` to avoid an issue where the ES 9.x client sends a body that Elasticsearch rejects. * Refines `Indices` parameter passing for delete operations using `Indices.Parse` or `Indices.Index`. * Ensures `Reopen()` is invoked when updating index settings that necessitate it. * Improves bulk operation reliability by checking `hit.IsValid` rather than specific status codes. * Corrects reindex tests to account for `Version` (SeqNo/PrimaryTerm) not being preserved across reindex operations. * Includes minor project file cleanup and type aliasing for improved readability.
…needs-review Elastic 9 subbranch work.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
src/Foundatio.Repositories.Elasticsearch/Queries/Builders/FieldConditionsQueryBuilder.cs
Fixed
Show fixed
Hide fixed
src/Foundatio.Repositories.Elasticsearch/Repositories/ElasticReindexer.cs
Fixed
Show fixed
Hide fixed
tests/Foundatio.Repositories.Elasticsearch.Tests/AggregationQueryTests.cs
Fixed
Show fixed
Hide fixed
Contributor
There was a problem hiding this comment.
Pull request overview
Updates Foundatio.Repositories’ Elasticsearch integration and supporting utilities/tests to use Elastic.Clients.Elasticsearch (System.Text.Json-based) instead of NEST/Elasticsearch.Net/Newtonsoft, aligning serialization, query building, and index/mapping APIs with the new client.
Changes:
- Migrate repository/query/index configuration code from NEST types (
IElasticClient,SearchDescriptor,QueryContainer, etc.) toElastic.Clients.Elasticsearchequivalents. - Convert JsonPatch implementation and related tests from Newtonsoft (
JToken) to System.Text.Json (JsonNode/Utf8JsonWriter). - Update test infrastructure/helpers and docker-compose to run against newer Elasticsearch/Kibana images; adjust versioning/build settings.
Reviewed changes
Copilot reviewed 104 out of 104 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Foundatio.Repositories.Tests/JsonPatch/JsonPatchTests.cs | Switch JsonPatch tests to System.Text.Json JsonNode and update serialization assertions. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/VersionedTests.cs | Update client usage and request/response validation for new Elasticsearch client. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/RepositoryTests.cs | Refresh calls updated for new client API. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Queries/EmailAddressQuery.cs | Replace NEST query DSL with Elastic.Clients.Elasticsearch.QueryDsl queries. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Queries/CompanyQuery.cs | Replace NEST term/terms queries with new client equivalents. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Queries/AgeQuery.cs | Replace NEST term/terms queries with new client equivalents. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/ParentRepository.cs | Update parent-child discriminator assignment for new JoinField type. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Models/Parent.cs | Expose discriminator as public property and set JSON name for join field. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Models/Child.cs | Expose discriminator as public property and set JSON name for join field. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/EmployeeWithCustomFieldsRepository.cs | Update missing-field filters to new query DSL. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/EmployeeRepository.cs | Update missing-field filters to new query DSL. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Configuration/MyAppElasticConfiguration.cs | Replace connection pool/client setup with ElasticsearchClientSettings/NodePool. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Configuration/Indexes/ParentChildIndex.cs | Port index creation/mappings to new client descriptors and join mapping API. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Configuration/Indexes/MonthlyLogEventIndex.cs | Port create-index configuration to new descriptors. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Configuration/Indexes/MonthlyFileAccessHistoryIndex.cs | Port create-index configuration to new descriptors. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Configuration/Indexes/IdentityIndex.cs | Port mapping/configuration to new mapping types. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Configuration/Indexes/EmployeeWithCustomFieldsIndex.cs | Port complex mappings (aliases, nested/object props) to new mapping APIs. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Configuration/Indexes/EmployeeIndex.cs | Port employee mappings and versioned index hooks to new mapping APIs. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Configuration/Indexes/DailyLogEventIndex.cs | Port daily index mapping/config to new mapping APIs. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Configuration/Indexes/DailyFileAccessHistoryIndex.cs | Port create-index configuration to new descriptors. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Configuration/ElasticsearchJsonNetSerializer.cs | Disable old JsonNet serializer shim (kept as reference). |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/ChildRepository.cs | Update parent-child discriminator assignment for new JoinField type. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/ReadOnlyRepositoryTests.cs | Adjust snapshot paging assertions and scroll cleanup for new client. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/QueryTests.cs | Update logging/options usage for queries. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/QueryBuilderTests.cs | Adjust runtime fields test for new request/runtime mapping behavior. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/ParentChildTests.cs | Ensure immediate consistency and adjust query logging for parent/child tests. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/NestedFieldTests.cs | Port nested aggregations API usage to new client aggregations builder. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/MigrationTests.cs | Refresh calls updated for new client API. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Foundatio.Repositories.Elasticsearch.Tests.csproj | Remove NEST JsonNet serializer package reference. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/Extensions/ElasticsearchExtensions.cs | Update alias helpers for new client response shapes and add convenience methods. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/ElasticRepositoryTestBase.cs | Add wildcard index deletion helper compatible with ES destructive settings. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/CustomFieldTests.cs | Adjust custom field value assertions to handle JsonElement-backed values. |
| tests/Foundatio.Repositories.Elasticsearch.Tests/AggregationQueryTests.cs | Add/adjust aggregation tests for new client aggregation response conversion/roundtrips. |
| src/Foundatio.Repositories/Models/Aggregations/ObjectValueAggregate.cs | Support JsonNode/JsonElement values in aggregate value conversion. |
| src/Foundatio.Repositories/JsonPatch/TestOperation.cs | Port JsonPatch operation to System.Text.Json read/write. |
| src/Foundatio.Repositories/JsonPatch/ReplaceOperation.cs | Port JsonPatch operation to System.Text.Json read/write. |
| src/Foundatio.Repositories/JsonPatch/RemoveOperation.cs | Port JsonPatch operation to System.Text.Json read/write. |
| src/Foundatio.Repositories/JsonPatch/PatchDocumentConverter.cs | Implement System.Text.Json converter for PatchDocument. |
| src/Foundatio.Repositories/JsonPatch/PatchDocument.cs | Port PatchDocument model/serialization to System.Text.Json. |
| src/Foundatio.Repositories/JsonPatch/Operation.cs | Port operation base class to System.Text.Json writer/reader APIs. |
| src/Foundatio.Repositories/JsonPatch/MoveOperation.cs | Port JsonPatch operation to System.Text.Json read/write. |
| src/Foundatio.Repositories/JsonPatch/JsonPatcher.cs | Port patching engine to JsonNode, plus pointer navigation helpers. |
| src/Foundatio.Repositories/JsonPatch/JsonDiffer.cs | Port diff generation to JsonNode and System.Text.Json output. |
| src/Foundatio.Repositories/JsonPatch/CopyOperation.cs | Port JsonPatch operation to System.Text.Json read/write. |
| src/Foundatio.Repositories/JsonPatch/AddOperation.cs | Port JsonPatch operation to System.Text.Json read/write. |
| src/Foundatio.Repositories/Extensions/StringExtensions.cs | Remove ToJTokenPath helper (Newtonsoft-specific). |
| src/Foundatio.Repositories/Extensions/AggregationsExtensions.cs | Adjust keyed bucket extraction to handle multiple generic bucket key types. |
| src/Foundatio.Repositories.Elasticsearch/Repositories/MigrationStateRepository.cs | Port migration index mapping/config to new mapping/index descriptors. |
| src/Foundatio.Repositories.Elasticsearch/Repositories/IParentChildDocument.cs | Switch JoinField import to new client package. |
| src/Foundatio.Repositories.Elasticsearch/Repositories/ElasticReindexer.cs | Port reindex orchestration (tasks, aliases, failures) to new client APIs. |
| src/Foundatio.Repositories.Elasticsearch/Repositories/ElasticReadOnlyRepositoryBase.cs | Port search/mget/exists/scroll/async search flows to new client APIs. |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/SortQueryBuilder.cs | Change sort handling to store sorts in context data for later application. |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/SoftDeletesQueryBuilder.cs | Update has_parent query usage and parent type naming for new client. |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/SearchAfterQueryBuilder.cs | Rework applying/reversing sorts + ID tiebreaker for search_after paging. |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/RuntimeFieldsQueryBuilder.cs | Port runtime field mapping to RuntimeMappings dictionary model. |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/ParentQueryBuilder.cs | Port has_parent query construction to new DSL. |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/PageableQueryBuilder.cs | Port paging options to From/SearchAfter with FieldValue conversion. |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/IdentityQueryBuilder.cs | Port ids query/exclude ids behavior to new DSL types. |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/IElasticQueryBuilder.cs | Core query-builder context now uses Query + SearchRequestDescriptor<T>. |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/FieldIncludesQueryBuilder.cs | Port source includes/excludes to new SourceConfig/SourceFilter. |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/FieldConditionsQueryBuilder.cs | Port term/terms/must_not/exists queries to new DSL with FieldValue. |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/ExpressionQueryBuilder.cs | Store sorts in context data instead of applying directly to search descriptor. |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/ElasticFilterQueryBuilder.cs | Change elastic filter option type from QueryContainer to Query. |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/DefaultSortQueryBuilder.cs | Ensure default ID sort is always present (via context data). |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/DateRangeQueryBuilder.cs | Port date range query properties to new names (Gte/Lte). |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/ChildQueryBuilder.cs | Port has_child query construction to new DSL. |
| src/Foundatio.Repositories.Elasticsearch/Options/ElasticCommandOptions.cs | Switch Elasticsearch imports to new client package. |
| src/Foundatio.Repositories.Elasticsearch/Jobs/SnapshotJob.cs | Port snapshot creation/status polling to new snapshot APIs. |
| src/Foundatio.Repositories.Elasticsearch/Jobs/ReindexWorkItemHandler.cs | Port handler to accept ElasticsearchClient. |
| src/Foundatio.Repositories.Elasticsearch/Jobs/CleanupSnapshotJob.cs | Port snapshot listing/deletion to new snapshot APIs. |
| src/Foundatio.Repositories.Elasticsearch/Jobs/CleanupIndexesJob.cs | Port index enumeration/deletion to new indices APIs. |
| src/Foundatio.Repositories.Elasticsearch/Foundatio.Repositories.Elasticsearch.csproj | Normalize project file header/formatting (and supports other changes). |
| src/Foundatio.Repositories.Elasticsearch/Extensions/ResolverExtensions.cs | Port sort resolution from IFieldSort to SortOptions. |
| src/Foundatio.Repositories.Elasticsearch/Extensions/LoggerExtensions.cs | Port logging helpers to ElasticsearchResponse and new ApiCall details. |
| src/Foundatio.Repositories.Elasticsearch/Extensions/IBodyWithApiCallDetailsExtensions.cs | Update raw deserialization helper and add OriginalException() accessor. |
| src/Foundatio.Repositories.Elasticsearch/Extensions/FindHitExtensions.cs | Handle new sort value representation (FieldValue) and update reverse-sort logic. |
| src/Foundatio.Repositories.Elasticsearch/Extensions/ElasticLazyDocument.cs | Replace NEST LazyDocument reflection with new hit/source-based lazy deserialization. |
| src/Foundatio.Repositories.Elasticsearch/ElasticUtility.cs | Port snapshot/index listing and snapshot creation to new APIs. |
| src/Foundatio.Repositories.Elasticsearch/CustomFields/StandardFieldTypes/StringFieldType.cs | Update custom field mapping signature to PropertyFactory<T> function. |
| src/Foundatio.Repositories.Elasticsearch/CustomFields/StandardFieldTypes/LongFieldType.cs | Update custom field mapping signature to PropertyFactory<T> function. |
| src/Foundatio.Repositories.Elasticsearch/CustomFields/StandardFieldTypes/KeywordFieldType.cs | Update custom field mapping signature to PropertyFactory<T> function. |
| src/Foundatio.Repositories.Elasticsearch/CustomFields/StandardFieldTypes/IntegerFieldType.cs | Update custom field mapping signature to PropertyFactory<T> function. |
| src/Foundatio.Repositories.Elasticsearch/CustomFields/StandardFieldTypes/FloatFieldType.cs | Update custom field mapping signature to PropertyFactory<T> function. |
| src/Foundatio.Repositories.Elasticsearch/CustomFields/StandardFieldTypes/DoubleFieldType.cs | Update custom field mapping signature to PropertyFactory<T> function. |
| src/Foundatio.Repositories.Elasticsearch/CustomFields/StandardFieldTypes/DateFieldType.cs | Update custom field mapping signature to PropertyFactory<T> function. |
| src/Foundatio.Repositories.Elasticsearch/CustomFields/StandardFieldTypes/BooleanFieldType.cs | Update custom field mapping signature to PropertyFactory<T> function. |
| src/Foundatio.Repositories.Elasticsearch/CustomFields/ICustomFieldType.cs | Update mapping contract to return Func<PropertyFactory<T>, IProperty>. |
| src/Foundatio.Repositories.Elasticsearch/CustomFields/CustomFieldDefinitionRepository.cs | Port mapping/index config and remove NEST-specific constructs. |
| src/Foundatio.Repositories.Elasticsearch/Configuration/VersionedIndex.cs | Port alias/index/mapping update flows to new indices/mapping APIs. |
| src/Foundatio.Repositories.Elasticsearch/Configuration/MonthlyIndex.cs | Port monthly index config/mapping to new APIs. |
| src/Foundatio.Repositories.Elasticsearch/Configuration/Index.cs | Port index lifecycle (create/update/delete/settings) to new APIs and wildcard delete handling. |
| src/Foundatio.Repositories.Elasticsearch/Configuration/IIndex.cs | Update settings/mapping method signatures for new client descriptors. |
| src/Foundatio.Repositories.Elasticsearch/Configuration/IElasticConfiguration.cs | Switch configuration client type to ElasticsearchClient. |
| src/Foundatio.Repositories.Elasticsearch/Configuration/ElasticConfiguration.cs | Port client creation/settings/pool types to new client. |
| src/Foundatio.Repositories.Elasticsearch/Configuration/DynamicIndex.cs | Port dynamic mapping configuration to new mapping API. |
| src/Foundatio.Repositories.Elasticsearch/Configuration/DailyIndex.cs | Port daily index creation/aliasing/mapping update flows to new APIs. |
| samples/Foundatio.SampleApp/Server/Repositories/Configuration/SampleAppElasticConfiguration.cs | Port sample app configuration to new client settings/pool types. |
| samples/Foundatio.SampleApp/Server/Repositories/Configuration/Indexes/GameReviewIndex.cs | Port sample index settings/mapping configuration to new APIs. |
| docker-compose.yml | Update Elasticsearch/Kibana images to 9.3.1. |
| build/common.props | Set MinVer minimum major/minor to 8.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/Foundatio.Repositories.Elasticsearch.Tests/QueryBuilderTests.cs
Outdated
Show resolved
Hide resolved
tests/Foundatio.Repositories.Elasticsearch.Tests/AggregationQueryTests.cs
Show resolved
Hide resolved
- Restore JSONPath support in JsonPatcher with built-in filter evaluator (no external dep) and re-add 3 previously deleted JSONPath tests - Re-add 3 pagination tests deleted during upgrade (no-duplicate paging) - Enable TopHits aggregation round-trip for caching: serialize raw hit JSON in new Hits property; update both JSON converters to handle tophits type - Implement ElasticUtility stubs: WaitForTaskAsync, WaitForSafeToSnapshotAsync, DeleteSnapshotsAsync, DeleteIndicesAsync, and complete CreateSnapshotAsync - Fix Foundatio.Parsers.ElasticQueries package ref to published pre-release so CI builds without source reference override - Remove CA2264 no-op ThrowIfNull on non-nullable param in IElasticQueryBuilder - Delete PipelineTests.cs and ElasticsearchJsonNetSerializer.cs (dead code) - Update all docs to ES9 API: ConnectionPool, ConnectionSettings, ConfigureIndex and ConfigureIndexMapping void return, property mapping syntax, IsValidResponse - Add upgrading-to-es9.md migration guide with full breaking changes checklist Made-with: Cursor
Upgrades the `Foundatio.Parsers.ElasticQueries` package to its latest 8.0.0-preview version. This update supports the ongoing migration to a new major version of the Elasticsearch client.
Removes Rider-specific project files and updates .gitignore. Adjusts build configuration to conditionally reference Foundatio.Repositories source. Refines nullability checks in Elasticsearch logger extensions and improves JSON patch logic with better pattern matching, simplified path creation, and robust error handling for unknown operations. Includes a fix for a test case ID assignment and adds a new test for runtime fields query builder.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 120 out of 124 changed files in this pull request and generated 4 comments.
Files not reviewed (3)
- .idea/.idea.Foundatio.Repositories/.idea/indexLayout.xml: Language not supported
- .idea/.idea.Foundatio.Repositories/.idea/projectSettingsUpdater.xml: Language not supported
- .idea/.idea.Foundatio.Repositories/.idea/vcs.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Foundatio.Repositories.Elasticsearch/Repositories/ElasticReadOnlyRepositoryBase.cs
Outdated
Show resolved
Hide resolved
tests/Foundatio.Repositories.Elasticsearch.Tests/Extensions/ElasticsearchExtensions.cs
Outdated
Show resolved
Hide resolved
niemyjski
changed the title
- Fix STJ vs Newtonsoft serialization test by using semantic JSON comparison (JsonNode.DeepEquals) instead of raw string equality for TopHits encoding - Replace TestContext.Current.CancellationToken with TestCancellationToken - Replace all string concatenation (+) with string interpolation ($"") - Add null guards, improve error handling for alias retrieval - Fix constant condition warnings in LoggerExtensions and JsonDiffer - Add convenience overloads for PatchDocument Add/Replace primitives - Implement DoubleSystemTextJsonConverter.Read (was NotImplementedException) - Add NotSupportedException for unsupported JSONPath in patch operations - Validate patch array elements in PatchDocument.Load - Remove dead code, unused imports, and commented-out routing call - Suppress CS8002 for test-only GitHubActionsTestLogger dependency - Simplify RuntimeFieldsQueryBuilder test to context-level assertions Made-with: Cursor
Made-with: Cursor
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 123 out of 127 changed files in this pull request and generated 3 comments.
Files not reviewed (3)
- .idea/.idea.Foundatio.Repositories/.idea/indexLayout.xml: Language not supported
- .idea/.idea.Foundatio.Repositories/.idea/projectSettingsUpdater.xml: Language not supported
- .idea/.idea.Foundatio.Repositories/.idea/vcs.xml: Language not supported
Comments suppressed due to low confidence (1)
docs/guide/troubleshooting.md:63
- The troubleshooting guide shows
ElasticsearchClientSettingsbut still usessettings.BasicAuthentication(...), which is a NEST-era API. With Elastic.Transport, authentication is configured viasettings.Authentication(new BasicAuthentication(...))(or ApiKey/BearerToken equivalents). Update this snippet so readers can copy/paste working code.
**Solutions:**
```csharp
protected override void ConfigureSettings(ElasticsearchClientSettings settings)
{
// Basic authentication
settings.BasicAuthentication("username", "password");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Foundatio.Repositories.Elasticsearch/Jobs/CleanupIndexesJob.cs
Outdated
Show resolved
Hide resolved
…ON aggregate values - Move ToFieldValue to Utility/FieldValueHelper.cs (was duplicated in two query builders) - Require serializer for JsonNode/JsonElement in ObjectValueAggregate.ValueAs (default STJ is silently wrong) Made-with: Cursor
Made-with: Cursor
Made-with: Cursor # Conflicts: # src/Foundatio.Repositories.Elasticsearch/Configuration/VersionedIndex.cs # src/Foundatio.Repositories.Elasticsearch/Repositories/ElasticReadOnlyRepositoryBase.cs
src/Foundatio.Repositories/Models/Aggregations/ObjectValueAggregate.cs
Dismissed
Show dismissed
Hide dismissed
src/Foundatio.Repositories/Models/Aggregations/ObjectValueAggregate.cs
Dismissed
Show dismissed
Hide dismissed
src/Foundatio.Repositories/Models/Aggregations/ObjectValueAggregate.cs
Dismissed
Show dismissed
Hide dismissed
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 131 out of 135 changed files in this pull request and generated 4 comments.
Files not reviewed (3)
- .idea/.idea.Foundatio.Repositories/.idea/indexLayout.xml: Language not supported
- .idea/.idea.Foundatio.Repositories/.idea/projectSettingsUpdater.xml: Language not supported
- .idea/.idea.Foundatio.Repositories/.idea/vcs.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/Foundatio.Repositories.Elasticsearch.Tests/ReadOnlyRepositoryTests.cs
Show resolved
Hide resolved
tests/Foundatio.Repositories.Elasticsearch.Tests/ReadOnlyRepositoryTests.cs
Show resolved
Hide resolved
…test quality - Replace standalone JsonSerializerOptions with ITextSerializer in IBodyWithApiCallDetailsExtensions, ElasticReindexer, ElasticRepositoryBase, and ElasticIndexExtensions to ensure consistent serialization behavior - Thread ITextSerializer through ElasticReindexer constructor and all call sites (Index, DailyIndex, VersionedIndex, ReindexWorkItemHandler) - Fix bare catch in JsonPatcher that swallowed all exceptions including critical ones; now catches only InvalidOperationException and FormatException - Add using to MemoryStream in LoggerExtensions and AggregationQueryTests - Add using to StreamReader/MemoryStream in JsonPatchTests - Add meaningful assertions to EnsuredDates test (was zero-assertion) - Validate alias failure exception message in UpdateAliasesAsync test - Use Assert.Equal for HTTP 404 check in ReindexTests for clearer failures Made-with: Cursor
…adow options - Move all JSON converters from Utility/ to Serialization/ namespace - Replace simple ObjectToClrTypesConverter with robust ObjectToInferredTypesConverter that handles nested objects, arrays, raw byte checking for decimal preservation, and ISO 8601 date parsing - Rename generic ObjectToInferredTypesConverter factory to InferredTypesConverterFactory to avoid naming collision - Remove ConditionalWeakTable/derived options from AggregationsSystemTextJsonConverter since DoubleSystemTextJsonConverter is already in ConfigureFoundatioRepositoryDefaults - Thread ITextSerializer through sort token encode/decode in FindHitExtensions, eliminating static JsonSerializerOptions and ObjectConverter - Move JsonSerializerOptionsExtensions to Serialization namespace Made-with: Cursor
- Add ObjectToInferredTypesConverterTests: 20 tests covering integers, floats, strings, bools, nulls, dates, objects, arrays, nested structures, and round-trips - Add DoubleSystemTextJsonConverterTests: 8 tests covering decimal preservation, round-trips, and object property serialization - Add JsonSerializerOptionsExtensionsTests: 8 tests covering converter registration, enum serialization, CLR type inference, and double preservation - Move SerializerTestHelper to Serialization folder, configure with repository defaults - All tests follow 3-part naming (Method_State_Expected) and AAA pattern Made-with: Cursor
tests/Foundatio.Repositories.Elasticsearch.Tests/IndexTests.cs
Dismissed
Show dismissed
Hide dismissed
tests/Foundatio.Repositories.Tests/JsonPatch/JsonPatchTests.cs
Dismissed
Show dismissed
Hide dismissed
src/Foundatio.Repositories.Elasticsearch/Configuration/ElasticConfiguration.cs
Fixed
Show fixed
Hide fixed
src/Foundatio.Repositories.Elasticsearch/Configuration/ElasticConfiguration.cs
Fixed
Show fixed
Hide fixed
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 142 out of 146 changed files in this pull request and generated 2 comments.
Files not reviewed (3)
- .idea/.idea.Foundatio.Repositories/.idea/indexLayout.xml: Language not supported
- .idea/.idea.Foundatio.Repositories/.idea/projectSettingsUpdater.xml: Language not supported
- .idea/.idea.Foundatio.Repositories/.idea/vcs.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Foundatio.Repositories/Serialization/JsonSerializerOptionsExtensions.cs
Show resolved
Hide resolved
src/Foundatio.Repositories.Elasticsearch/Queries/Builders/ExpressionQueryBuilder.cs
Show resolved
Hide resolved
…n robustness - Update index mapping documentation to use simplified property selection syntax - Prevent duplicate converter registration in JsonSerializerOptionsExtensions by checking if they already exist - Throw NotSupportedException instead of silently skipping unsupported bucket types in AggregationsExtensions Made-with: Cursor
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 142 out of 146 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- .idea/.idea.Foundatio.Repositories/.idea/indexLayout.xml: Language not supported
- .idea/.idea.Foundatio.Repositories/.idea/projectSettingsUpdater.xml: Language not supported
- .idea/.idea.Foundatio.Repositories/.idea/vcs.xml: Language not supported
Comments suppressed due to low confidence (1)
src/Foundatio.Repositories/Serialization/DoubleSystemTextJsonConverter.cs:18
- This converter’s
Writeimplementation formats doubles via a fixed0.0pattern. That will round/truncate non-whole values (e.g. 3.14 -> 3.1) and can also produce invalid JSON in cultures that use,as the decimal separator. Consider writingwriter.WriteNumberValue(value)for non-whole numbers, and only special-case whole numbers to force a trailing.0using invariant formatting.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/Foundatio.Repositories.Elasticsearch.Tests/Extensions/ElasticsearchExtensions.cs
Show resolved
Hide resolved
- DoubleSystemTextJsonConverter: fix culture-sensitive formatting (invalid JSON in non-English locales), handle NaN/Infinity gracefully, preserve full precision for fractional doubles using WriteNumberValue instead of truncating format string - AggregationsExtensions: copy Data property in GetKeyedBuckets to prevent silent bucket metadata loss during key type conversion - ElasticReadOnlyRepositoryBase: use captured scrollId local instead of redundant GetScrollId() call - PatchDocument.Parse: throw JsonException for non-array input per RFC 6902 instead of silently returning empty document - Add comprehensive edge case tests for DoubleSystemTextJsonConverter and PatchDocument.Parse validation Made-with: Cursor
Member
Author
Post-Audit Fixes (5f146bf)Comprehensive audit of the full branch diff against main identified and fixed the following issues: Bugs Fixed
Code Quality
Test Coverage Added
Verification
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 143 out of 147 changed files in this pull request and generated 3 comments.
Files not reviewed (3)
- .idea/.idea.Foundatio.Repositories/.idea/indexLayout.xml: Language not supported
- .idea/.idea.Foundatio.Repositories/.idea/projectSettingsUpdater.xml: Language not supported
- .idea/.idea.Foundatio.Repositories/.idea/vcs.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Foundatio.Repositories.Elasticsearch/Queries/Builders/SearchAfterQueryBuilder.cs
Show resolved
Hide resolved
src/Foundatio.Repositories/Serialization/ObjectToInferredTypesConverter.cs
Show resolved
Hide resolved
…regation robustness - Update `DoubleSystemTextJsonConverter` to handle NaN/Infinity as "0.0" and ensure consistent decimal formatting for whole numbers while preserving full precision - Enforce RFC 6902 compliance in `PatchDocument.Parse` by validating that input is a JSON array - Map the `Data` property when converting aggregation buckets across all supported types - Optimize scroll clearing in `ElasticReadOnlyRepositoryBase` by caching the scroll ID Made-with: Cursor
src/Foundatio.Repositories.Elasticsearch/Repositories/ElasticReindexer.cs
Dismissed
Show dismissed
Hide dismissed
src/Foundatio.Repositories.Elasticsearch/Repositories/ElasticReindexer.cs
Dismissed
Show dismissed
Hide dismissed
… serializer requirement
- ObjectToInferredTypesConverter: restrict ISO 8601 date parsing to strings with
a time component ('T'). Date-only strings like "2025-01-01" satisfy
TryGetDateTimeOffset but should remain as strings since they are typically
identifiers or display values, not timestamps. Fixes the silent implicit
conversion Copilot flagged on the ReadString method.
- TopHitsAggregate.Documents<T>: add XML doc clarifying that serializer is
required when Hits (cache round-trip path) is populated, explaining the
misleading optional default.
- Add targeted tests: Read_WithDateOnlyOrNonIsoString_ReturnsString (3 cases)
and Read_WithIso8601DatetimeWithTimeComponent_ReturnsDateTimeOffset (3 cases)
covering the new boundary exactly.
Made-with: Cursor
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 143 out of 147 changed files in this pull request and generated no new comments.
Files not reviewed (3)
- .idea/.idea.Foundatio.Repositories/.idea/indexLayout.xml: Language not supported
- .idea/.idea.Foundatio.Repositories/.idea/projectSettingsUpdater.xml: Language not supported
- .idea/.idea.Foundatio.Repositories/.idea/vcs.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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
Major upgrade migrating from NEST (Elasticsearch.Net 7.x) to Elastic.Clients.Elasticsearch (official ES 8.x/9.x client). This is a breaking change release (v8.0) that replaces the entire Elasticsearch client layer and serialization stack.
Depends on FoundatioFx/Foundatio.Parsers#84
Core Changes
IElasticClient(NEST) ->ElasticsearchClient(Elastic.Clients.Elasticsearch)JToken/JObject) -> System.Text.Json (JsonNode/JsonElement) throughoutQueryContainer/SearchDescriptor<T>toQuery/SearchRequestDescriptor<T>Breaking API Changes
All breaking changes are direct 1:1 type replacements required by the client migration:
IElasticClientElasticsearchClientConnectionSettingsElasticsearchClientSettingsIConnectionPoolNodePoolQueryContainerQuerySearchDescriptor<T>SearchRequestDescriptor<T>CreateIndexDescriptorCreateIndexRequestDescriptorJToken/JObjectJsonNode/JsonObjectKnown Limitation: PartialPatch Cannot Set Fields to Null
This is a behavioral change from NEST. The
DefaultSourceSerializerinElastic.Clients.ElasticsearchsetsJsonIgnoreCondition.WhenWritingNullby default, which means any property explicitly set tonullin aPartialPatchdocument is silently stripped from the serialized JSON before it reaches Elasticsearch. The field retains its original value instead of being set to null.Root cause: Decompiled source of
DefaultSourceSerializerOptionsProvider.MutateOptionsinElastic.Clients.Elasticsearch9.3.1 confirms this is intentional:Why we cannot work around it globally: Setting
JsonIgnoreCondition.Neveron the SourceSerializer (the obvious fix) is not viable because:object-typed fields get routed through the SourceSerializer and fail withNever(as reported in the upstream issue)Upstream tracking: elastic/elasticsearch-net#8763 — still open. We have commented on the issue with our specific use case.
Workarounds for consumers who need to set a field to null:
ScriptPatch:new ScriptPatch("ctx._source.companyName = null;")JsonPatch(RFC 6902):new JsonPatch(new PatchDocument(new ReplaceOperation { Path = "companyName", Value = null }))A regression test (
PartialPatchAsync_WithNullField_RetainsOriginalValue) documents this behavior and will flip automatically once the upstream issue is resolved.Quality & Robustness Improvements
30s) instead of .NET TimeSpan formatDoubleSystemTextJsonConverter.Read()(was throwingNotImplementedException)NotSupportedExceptionfor JSONPath ($-prefixed) paths in patch operations with clear error messageOperation.Build()for malformed patch JSONPatchDocument.Load()-- now throws on non-object elementsJsonDiffer,LoggerExtensions,ElasticReindexerAssert.NotNull(x?.Y)into proper two-step assertions)GetAliasIndexCountto only treat HTTP 404 as "no aliases" and throw on real failuresPatchDocument.Add/Replaceconvenience overloads forstring,int,long,double,boolRuntimeFieldsQueryBuildertest to verify builder output, not just input stateTestContext.Current.CancellationTokento all test async calls for proper cancellation supportdotnet format-- no unused usings or style issuesConfigureFoundatioRepositoryDefaults()idempotent to prevent duplicate converter registrationTopHitsAggregate._hitsfield to initialize with empty list in parameterless constructorElasticReindexer.CreateRangeQueryto usestartTime.ValueafterHasValueguard for explicit intentReview Feedback Addressed
All github-code-quality and Copilot review comments have been resolved:
partsToCreateunused container removed (was already fixed in prior commit)JsonDiffer.GetHashCodefixed with explicit null guardLoggerExtensions.LogRequestfixedstartTime.Valuechanged inElasticReindexerfor explicit intent after null guardascasts replaced with direct casts in test assertions.ToString()to ES duration formatOperation.Buildnull guard addedPatchDocument.Loadstrict validation addedGetAliasIndexCounterror handling improvedGetKeyedBucketsreplaced with explicit error