Replace JSON.NET with System.Text.Json across the codebase#2135
Open
niemyjski wants to merge 58 commits into
Open
Replace JSON.NET with System.Text.Json across the codebase#2135niemyjski wants to merge 58 commits into
niemyjski wants to merge 58 commits into
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Migrates application-layer JSON usage from Newtonsoft.Json to System.Text.Json (STJ), aligning core models/plugins/tests and adding STJ-based infrastructure for Elasticsearch/NEST while keeping Newtonsoft only as a transitive dependency for the NEST wire protocol.
Changes:
- Introduces STJ-based
IElasticsearchSerializerimplementation and STJ type metadata modifiers to match prior Newtonsoft serialization behavior (e.g., omit empty collections). - Updates core pipeline/plugins/controllers/jobs to use
ITextSerializer/JsonSerializerOptionsinstead of Newtonsoft abstractions. - Refactors tests and fixtures to validate STJ semantics (including semantic JSON comparison).
Reviewed changes
Copilot reviewed 87 out of 87 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Exceptionless.Tests/Utility/DataBuilder.cs | Updates test builder to use ITextSerializer for manual stacking info. |
| tests/Exceptionless.Tests/Serializer/SerializerTests.cs | Reworks serializer tests away from Newtonsoft-specific behavior toward STJ round-trips. |
| tests/Exceptionless.Tests/Serializer/Models/PersistentEventSerializerTests.cs | Uses ITextSerializer for typed data retrieval in PersistentEvent tests. |
| tests/Exceptionless.Tests/Serializer/Models/DataDictionaryTests.cs | Updates GetValue<T> tests to use ITextSerializer pathway. |
| tests/Exceptionless.Tests/Repositories/StackRepositoryTests.cs | Compares serialized JSON via ITextSerializer instead of ToJson(). |
| tests/Exceptionless.Tests/Repositories/ProjectRepositoryTests.cs | Updates Slack token access to use serializer-aware accessor. |
| tests/Exceptionless.Tests/Repositories/EventRepositoryTests.cs | Uses serializer-based typed accessors in repository tests. |
| tests/Exceptionless.Tests/Plugins/WebHookDataTests.cs | Switches expected/actual comparisons to semantic JSON equivalence using STJ. |
| tests/Exceptionless.Tests/Plugins/SummaryDataTests.cs | Moves summary comparisons to semantic JSON using STJ. |
| tests/Exceptionless.Tests/Plugins/GeoTests.cs | Updates Geo plugin/tests to pass ITextSerializer. |
| tests/Exceptionless.Tests/Plugins/EventUpgraderTests.cs | Uses JsonNode formatting + semantic compare for upgrader outputs. |
| tests/Exceptionless.Tests/Plugins/EventParserTests.cs | Uses ITextSerializer for round-trip verification; removes Newtonsoft formatting asserts. |
| tests/Exceptionless.Tests/Pipeline/EventPipelineTests.cs | Updates pipeline tests to use serializer-based typed accessors. |
| tests/Exceptionless.Tests/Mail/MailerTests.cs | Updates Mailer construction to accept ITextSerializer. |
| tests/Exceptionless.Tests/IntegrationTestsBase.cs | Replaces FluentRest Newtonsoft serializer with STJ JsonContentSerializer. |
| tests/Exceptionless.Tests/Exceptionless.Tests.csproj | Removes FluentRest.NewtonsoftJson, adds STJ-based FluentRest. |
| tests/Exceptionless.Tests/Controllers/EventControllerTests.cs | Uses ITextSerializer for typed accessors in controller tests. |
| src/Exceptionless.Web/Exceptionless.Web.csproj | Removes NEST.JsonNetSerializer package reference. |
| src/Exceptionless.Web/Controllers/ProjectController.cs | Injects ITextSerializer for Slack token access. |
| src/Exceptionless.Web/Controllers/EventController.cs | Injects ITextSerializer and uses it for event payload byte generation. |
| src/Exceptionless.Core/Utility/TypeHelper.cs | Updates equality handling for STJ JsonElement. |
| src/Exceptionless.Core/Utility/ExtensibleObject.cs | Adds JsonElement handling to generic property retrieval. |
| src/Exceptionless.Core/Utility/ErrorSignature.cs | Switches error signature extraction to serializer-based GetValue<T>. |
| src/Exceptionless.Core/Services/SlackService.cs | Uses serializer-aware Slack token access. |
| src/Exceptionless.Core/Serialization/ObjectToInferredTypesConverter.cs | Refines number inference logic for STJ dynamic object conversion. |
| src/Exceptionless.Core/Serialization/LowerCaseUnderscorePropertyNamesContractResolver.cs | Deletes Newtonsoft contract resolver (obsolete after STJ migration). |
| src/Exceptionless.Core/Serialization/JsonSerializerOptionsExtensions.cs | Defines Exceptionless STJ defaults (snake_case, safe encoder, converters, empty-collection skipping). |
| src/Exceptionless.Core/Serialization/ExceptionlessNamingStrategy.cs | Deletes Newtonsoft naming strategy (replaced by STJ naming policy). |
| src/Exceptionless.Core/Serialization/EmptyCollectionModifier.cs | Adds STJ type info modifier to omit empty collections. |
| src/Exceptionless.Core/Serialization/ElasticSystemTextJsonSerializer.cs | Adds STJ IElasticsearchSerializer for NEST with custom converters. |
| src/Exceptionless.Core/Serialization/ElasticJsonNetSerializer.cs | Deletes Newtonsoft-based NEST serializer. |
| src/Exceptionless.Core/Serialization/ElasticConnectionSettingsAwareContractResolver.cs | Deletes Newtonsoft resolver used by old NEST serializer. |
| src/Exceptionless.Core/Serialization/DynamicTypeContractResolver.cs | Deletes Newtonsoft dynamic contract resolver. |
| src/Exceptionless.Core/Serialization/DataObjectConverter.cs | Deletes Newtonsoft-based model/data converter. |
| src/Exceptionless.Core/Repositories/Configuration/ExceptionlessElasticConfiguration.cs | Switches Elasticsearch client wiring to STJ serializer. |
| src/Exceptionless.Core/Plugins/WebHook/Default/010_VersionOnePlugin.cs | Uses serializer-based typed accessors; adds [JsonPropertyName] for v1 webhook compatibility. |
| src/Exceptionless.Core/Plugins/WebHook/Default/005_SlackPlugin.cs | Uses serializer-based typed accessors for Slack webhook creation. |
| src/Exceptionless.Core/Plugins/Formatting/FormattingPluginBase.cs | Converts formatting plugins to depend on ITextSerializer. |
| src/Exceptionless.Core/Plugins/Formatting/Default/99_DefaultFormattingPlugin.cs | Updates default formatting/slack attachment creation to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/Formatting/Default/60_LogFormattingPlugin.cs | Updates typed accessors + slack attachment creation to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/Formatting/Default/50_SessionFormattingPlugin.cs | Updates typed accessors to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/Formatting/Default/40_UsageFormattingPlugin.cs | Updates typed accessors + slack attachment creation to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/Formatting/Default/30_NotFoundFormattingPlugin.cs | Updates typed accessors + IP retrieval to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/Formatting/Default/20_ErrorFormattingPlugin.cs | Updates typed accessors + slack attachment creation to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/Formatting/Default/10_SimpleErrorFormattingPlugin.cs | Updates typed accessors + slack attachment creation to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/Formatting/Default/05_ManualStackingFormattingPlugin.cs | Updates manual stacking info access to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/EventUpgrader/EventUpgraderContext.cs | Migrates upgrader DOM from JObject/JArray to JsonObject/JsonArray. |
| src/Exceptionless.Core/Plugins/EventUpgrader/Default/V2_EventUpgrade.cs | Rewrites upgrader logic from Newtonsoft DOM to JsonNode DOM. |
| src/Exceptionless.Core/Plugins/EventUpgrader/Default/V1R850_EventUpgrade.cs | Rewrites upgrader logic to JsonObject. |
| src/Exceptionless.Core/Plugins/EventUpgrader/Default/V1R844_EventUpgrade.cs | Rewrites upgrader logic to JsonObject. |
| src/Exceptionless.Core/Plugins/EventUpgrader/Default/V1R500_EventUpgrade.cs | Rewrites upgrader logic to JsonObject and normalizes date string formatting. |
| src/Exceptionless.Core/Plugins/EventUpgrader/Default/GetVersion.cs | Rewrites version detection to JsonNode APIs. |
| src/Exceptionless.Core/Plugins/EventProcessor/Default/90_RemovePrivateInformationPlugin.cs | Updates typed accessors to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/EventProcessor/Default/80_AngularPlugin.cs | Updates typed accessors to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/EventProcessor/Default/70_SessionPlugin.cs | Updates typed accessors + session start creation to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/EventProcessor/Default/50_GeoPlugin.cs | Updates IP extraction to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/EventProcessor/Default/45_EnvironmentInfoPlugin.cs | Updates typed accessors to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/EventProcessor/Default/40_RequestInfoPlugin.cs | Updates typed accessors and request exclusion processing to accept ITextSerializer. |
| src/Exceptionless.Core/Plugins/EventProcessor/Default/30_SimpleErrorPlugin.cs | Updates typed accessors to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/EventProcessor/Default/20_ErrorPlugin.cs | Updates typed accessors + ErrorSignature to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/EventProcessor/Default/10_NotFoundPlugin.cs | Updates typed accessors to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/EventProcessor/Default/0_ThrottleBotsPlugin.cs | Updates request-info access to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/EventProcessor/Default/03_ManualStackingPlugin.cs | Updates manual stacking info access to use ITextSerializer. |
| src/Exceptionless.Core/Plugins/EventParser/Default/LegacyErrorParserPlugin.cs | Migrates legacy parsing to JsonNode + STJ options. |
| src/Exceptionless.Core/Plugins/EventParser/Default/JsonEventParserPlugin.cs | Uses ITextSerializer for parsing event payloads instead of Newtonsoft. |
| src/Exceptionless.Core/Models/SummaryData.cs | Adjusts nullability/required-ness to align with STJ behaviors. |
| src/Exceptionless.Core/Models/StackSummaryModel.cs | Adjusts nullability/required-ness to align with STJ behaviors. |
| src/Exceptionless.Core/Models/Stack.cs | Removes Newtonsoft enum converter attribute; keeps STJ converter. |
| src/Exceptionless.Core/Models/SlackToken.cs | Replaces Newtonsoft [JsonProperty] with STJ [JsonPropertyName]; updates SlackAttachment ctor to use ITextSerializer. |
| src/Exceptionless.Core/Models/Messaging/ReleaseNotification.cs | Removes required modifiers (likely to avoid STJ required-member enforcement). |
| src/Exceptionless.Core/Models/Event.cs | Adds [JsonExtensionData] + IJsonOnDeserialized merge into Data. |
| src/Exceptionless.Core/Mail/Mailer.cs | Switches user-info extraction to serializer-based accessors. |
| src/Exceptionless.Core/Jobs/WebHooksJob.cs | Switches webhook POST payload handling to STJ PostAsJsonAsync + options. |
| src/Exceptionless.Core/Jobs/EventPostsJob.cs | Uses serializer-based event bytes for retries. |
| src/Exceptionless.Core/Jobs/EventNotificationsJob.cs | Updates request-info access to use ITextSerializer. |
| src/Exceptionless.Core/Jobs/CloseInactiveSessionsJob.cs | Updates identity extraction to use ITextSerializer. |
| src/Exceptionless.Core/Extensions/RequestInfoExtensions.cs | Requires serializer for post-data exclusion parsing (STJ). |
| src/Exceptionless.Core/Extensions/ProjectExtensions.cs | Makes Slack token extraction serializer-aware via GetValue<T>. |
| src/Exceptionless.Core/Extensions/PersistentEventExtensions.cs | Updates helpers to accept ITextSerializer for typed accessors. |
| src/Exceptionless.Core/Extensions/JsonNodeExtensions.cs | Adds STJ JsonNode helpers used by upgraders/tests (formatting, mutation helpers). |
| src/Exceptionless.Core/Extensions/JsonExtensions.cs | Removes Newtonsoft helpers, retains JSON-type detection helpers. |
| src/Exceptionless.Core/Extensions/EventExtensions.cs | Updates typed accessor APIs to accept ITextSerializer; switches GetBytes to serializer bytes. |
| src/Exceptionless.Core/Extensions/ErrorExtensions.cs | Updates stacking target helper to use serializer-based error access. |
| src/Exceptionless.Core/Extensions/DataDictionaryExtensions.cs | Reworks GetValue<T> around STJ/ITextSerializer, adds case-insensitive JsonElement handling. |
| src/Exceptionless.Core/Exceptionless.Core.csproj | Removes Newtonsoft/JsonNet packages. |
| src/Exceptionless.Core/Bootstrapper.cs | Removes Newtonsoft setup; registers STJ options + SystemTextJsonSerializer in DI. |
| AGENTS.md | Documents the new STJ-only serialization architecture and security posture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 87 out of 87 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
niemyjski
force-pushed
the
feature/system-text-json-v2
branch
from
e6a3b35 to
c292e13
Compare
niemyjski
force-pushed
the
feature/system-text-json-v2
branch
2 times, most recently
from
4d880ad to
09134ce
Compare
Remove all Newtonsoft.Json serialization from the application layer, replacing it with System.Text.Json (STJ). NEST still brings in Newtonsoft transitively, but all application-level serialization now uses STJ exclusively. Key changes: - Add ElasticSystemTextJsonSerializer as custom IElasticsearchSerializer for NEST - Add EmptyCollectionModifier to omit empty collections during serialization - Add ObjectToInferredTypesConverter to handle JObject/JToken from NEST reads - Add JsonNodeExtensions as STJ equivalents of JObject helpers for event upgraders - Add IJsonOnDeserialized to Event model to merge [JsonExtensionData] into Data dict - Add [JsonPropertyName] attributes to V1 webhook models for PascalCase compat - Migrate all event upgraders from JObject to JsonObject (System.Text.Json.Nodes) - Migrate all plugins from ISerializer/JsonSerializerOptions DI injection - Use case-insensitive deserialization for DataDictionary.GetValue<T>() from JsonElement - Use semantic comparison (JsonNode.DeepEquals) in tests for fixture validation - Remove DataObjectConverter, ElasticJsonNetSerializer, and related Newtonsoft classes - Remove Foundatio.JsonNet, NEST.JsonNetSerializer, FluentRest.NewtonsoftJson packages Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix ConvertJsonElement ternary type coercion: (object)l cast prevents implicit long→double widening in switch expression - Make ObjectToInferredTypesConverter configurable with preferInt64 flag for ES serializer to match JSON.NET DataObjectConverter behavior - Fix ElasticSystemTextJsonSerializer: remove ReadStreamToSpan lifetime bug (span backed by disposed MemoryStream), deserialize from stream directly with MemoryStream fast-path - Fix Serialize<T> indentation: pass JsonWriterOptions to Utf8JsonWriter so SerializationFormatting.Indented actually produces indented output - Handle exponent notation (1e5) as floating-point in ReadNumber - Use double consistently (not decimal) for floating-point to match JSON.NET behavior - Fix RenameAll return value: return whether any renames occurred - Add using var to MemoryStream in EventController and EventPostsJob - Handle empty response bodies in SendRequestAsAsync (STJ throws on empty input, Newtonsoft returned default) - Fix SerializerTests: put unknown properties at root level to test JsonExtensionData→ConvertJsonElement path correctly - Revert AGENTS.md to main Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
niemyjski
force-pushed
the
feature/system-text-json-v2
branch
from
09134ce to
13aa277
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 86 out of 86 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…icsearch Replace IElasticsearchSerializer with Elastic.Transport.Serializer, update ExceptionlessElasticConfiguration to use ElasticsearchClient/ElasticsearchClientSettings/ StaticNodePool, pass ITextSerializer to Foundatio base, and register ElasticsearchClient explicitly in DI.
Update all index files to use CreateIndexRequestDescriptor, void returns for ConfigureIndex/ConfigureIndexMapping, expression-based property mappings, DynamicMapping enum, and renamed analysis methods.
Replace QueryContainer with Query, use TermQuery/TermsQuery/BoolQuery/DateRangeQuery object initializers with Infer.Field expressions.
…earch Update CleanupOrphanedDataJob, DataMigrationJob, all migration files, AdminController, EventController, ProjectController, and test files to use new ES 8 client APIs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 117 out of 117 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SetupDefaults() already maps the Id property. The explicit Keyword(e => e.Id) mapping caused a duplicate key error in the new Elastic.Clients.Elasticsearch client.
- Fix StackRepository to use lowercase enum string for FieldEquals - Quote ISO 8601 dates in Lucene filter expressions (EventRepository) - Fix EmptyCollectionModifier to only skip null, not empty collections - Add SnakeCaseOptions for DataDictionary deserialization compatibility - Update event serialization test fixture for UTC Z suffix - Use order-independent JSON comparison in serialization test 5 PersistentEventQueryValidatorTests still fail due to a Foundatio.Parsers bug with grouped TermRangeNode field inheritance. Requires new Foundatio.Parsers preview package.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 129 out of 129 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+47
to
+56
| private static bool IsEmptyCollection(object? value) | ||
| { | ||
| return value switch | ||
| { | ||
| null => true, | ||
| string => false, // strings are IEnumerable but should not be treated as collections | ||
| ICollection { Count: 0 } => true, | ||
| IEnumerable enumerable => !HasAnyElement(enumerable), | ||
| _ => false | ||
| }; |
There was a problem hiding this comment.
EmptyCollectionModifier.IsEmptyCollection treats null as "empty" (returns true), which makes the generated ShouldSerialize return false for null values. That means null properties will be omitted even if a caller configures JsonSerializerOptions to include nulls (DefaultIgnoreCondition = Never). If the intent is only to skip empty collections, null should not be handled here (or should be conditional on the ignore-null policy) and should be left to JsonSerializerOptions.DefaultIgnoreCondition.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 129 out of 129 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
26
to
28
| <PackageReference Include="Foundatio.Extensions.Hosting" Version="13.0.0" /> | ||
| <PackageReference Include="Foundatio.JsonNet" Version="13.0.0" /> | ||
| <PackageReference Include="MiniValidation" Version="0.9.2" /> |
There was a problem hiding this comment.
This PR description says application-level Newtonsoft.Json has been removed, but Exceptionless.Core still references Foundatio.JsonNet (Newtonsoft-based). If this package is no longer required, it should be removed to fully enforce the STJ-only goal; if it is still required, the PR description should be updated to reflect the remaining Newtonsoft dependency.
- Remove Foundatio.JsonNet direct dependency from Exceptionless.Core.csproj - Remove JObject/JToken handlers from ObjectToInferredTypesConverter and DataDictionaryExtensions (dead code since NEST is replaced by new Elastic client) - Convert JObject-based tests to use Dictionary<string, object?> and List<object?> which is what ObjectToInferredTypesConverter actually produces - Add Size(0) to aggregation-only searches in CleanupOrphanedDataJob (feature branch) - Add .AddKeywordField() to EventIndex IP address copy-to mapping - Narrow bare catch blocks in DataDictionaryExtensions to specific exception types - Fix OrganizationRepository suspended=false filter: use Must (AND) instead of Should (OR) so both billing status AND IsSuspended are required - Use JsonElement.DeepEquals() instead of GetRawText() comparison in TypeHelper Newtonsoft.Json remains as a transitive dependency from Stripe.net and Foundatio.Repositories, but no application code references it.
The v8 Elastic client does support SizeField via map.Size(). The disabled comment was incorrect - restore the conditional SizeField mapping for production deployments with the plugin enabled.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 130 out of 130 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <ProjectReference | ||
| Include="..\..\..\..\Foundatio\Foundatio.Repositories\src\Foundatio.Repositories.Elasticsearch\Foundatio.Repositories.Elasticsearch.csproj" | ||
| Condition="'$(ReferenceFoundatioRepositoriesSource)' == 'true'" /> | ||
| <PackageReference Include="Foundatio.Repositories.Elasticsearch" Version="8.0.0-preview.elastic-client.0.5" Condition="'$(ReferenceFoundatioRepositoriesSource)' == '' OR '$(ReferenceFoundatioRepositoriesSource)' == 'false'" /> |
There was a problem hiding this comment.
The Foundatio.Repositories.Elasticsearch package version was changed to 8.0.0-preview.elastic-client.0.5. This repo/branch is expected to stay on the vetted preview version (see earlier conventions for this codebase); using an older preview can break API compatibility with the Elastic .NET client and Foundatio integrations. Please pin this back to the approved version used elsewhere in the repo/branch (or justify/update all dependent packages in lockstep).
Suggested change
| <PackageReference Include="Foundatio.Repositories.Elasticsearch" Version="8.0.0-preview.elastic-client.0.5" Condition="'$(ReferenceFoundatioRepositoriesSource)' == '' OR '$(ReferenceFoundatioRepositoriesSource)' == 'false'" /> | |
| <PackageReference Include="Foundatio.Repositories.Elasticsearch" Version="8.0.0-preview.elastic-client.0.7" Condition="'$(ReferenceFoundatioRepositoriesSource)' == '' OR '$(ReferenceFoundatioRepositoriesSource)' == 'false'" /> |
Comment on lines
+39
to
+42
| // XSS-safe encoder: escapes <, >, &, ' while allowing Unicode characters | ||
| // This protects against script injection when JSON is embedded in HTML/JavaScript | ||
| options.Encoder = JavaScriptEncoder.Create(new TextEncoderSettings(UnicodeRanges.All)); | ||
|
|
There was a problem hiding this comment.
JavaScriptEncoder.Create(new TextEncoderSettings(UnicodeRanges.All)) is effectively the relaxed JSON encoder behavior and may stop escaping HTML-sensitive characters (e.g., <, >, &). That would make the comment here incorrect and could introduce XSS risk anywhere JSON is embedded into HTML/JS. Please switch back to JavaScriptEncoder.Default (safe-by-default) or add explicit tests/justification proving the encoder still escapes the HTML-sensitive characters you rely on.
Comment on lines
+18
to
+28
| // For properties typed as IEnumerable (but not string), check at compile time | ||
| if (typeof(IEnumerable).IsAssignableFrom(property.PropertyType) && property.PropertyType != typeof(string)) | ||
| { | ||
| var originalShouldSerialize = property.ShouldSerialize; | ||
| property.ShouldSerialize = (obj, value) => | ||
| { | ||
| if (originalShouldSerialize is not null && !originalShouldSerialize(obj, value)) | ||
| return false; | ||
|
|
||
| return !IsEmptyCollection(value); | ||
| }; |
There was a problem hiding this comment.
EmptyCollectionModifier checks emptiness for non-ICollection enumerables by allocating an enumerator and calling MoveNext(). For properties typed as IEnumerable that are lazy/streaming (or otherwise expensive), this adds work during serialization and can have side effects depending on the enumerable implementation. Consider limiting the empty-collection skip to known countable types (e.g., ICollection/IReadOnlyCollection/arrays) and avoid probing arbitrary IEnumerable instances.
…e health check AspNetCore.HealthChecks.Elasticsearch v9.0.0 depends on Elastic.Clients.Elasticsearch 8.14.3 which uses Elastic.Transport 0.4.x API (TransportConfigurationBase<T>). Our upgrade to Elastic.Clients.Elasticsearch 8.19.18 brings Elastic.Transport 0.15.x which removed that type, causing TypeLoadException in CI integration tests. Fix: Remove the incompatible NuGet package and implement a simple health check using the Elastic.Clients.Elasticsearch client directly.
- StackRepository.GetStacksForCleanupAsync: pass string 'open' instead of StackStatus.Open to FieldEquals because Foundatio's FieldValueHelper.ToFieldValue calls .ToString() (producing 'Open') but ES stores 'open' via [JsonStringEnumMemberName]. - Admin response records (MigrationsResponse, ElasticsearchSnapshotsResponse, ElasticsearchInfoResponse): make array parameters nullable because EmptyCollectionModifier.SkipEmptyCollections omits empty arrays during serialization, and RespectNullableAnnotations causes deserialization to fail when those properties are missing from the JSON payload.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 134 out of 134 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (string.IsNullOrEmpty(connectionString)) | ||
| return new HealthCheckResult(context.Registration.FailureStatus, "Connection string not available."); | ||
|
|
||
| var client = new ElasticsearchClient(new Uri(connectionString)); |
Comment on lines
461
to
470
| Assert.NotNull(response); | ||
| Assert.NotNull(response.States); | ||
|
|
||
| foreach (var state in response.States) | ||
| if (response.States is not null) | ||
| { | ||
| Assert.NotNull(state.Id); | ||
| Assert.True(Enum.IsDefined(state.MigrationType)); | ||
| foreach (var state in response.States) | ||
| { | ||
| Assert.NotNull(state.Id); | ||
| Assert.True(Enum.IsDefined(state.MigrationType)); | ||
| } | ||
| } |
Comment on lines
506
to
+516
| // Assert | ||
| Assert.NotNull(elasticsearch); | ||
| Assert.All(elasticsearch.IndexDetails, indexDetail => | ||
| if (elasticsearch.IndexDetails is not null) | ||
| { | ||
| Assert.True(indexDetail.DocsCount >= 0); | ||
| Assert.True(indexDetail.StoreSizeInBytes >= 0); | ||
| Assert.True(indexDetail.UnassignedShards >= 0); | ||
| }); | ||
| Assert.All(elasticsearch.IndexDetails, indexDetail => | ||
| { | ||
| Assert.True(indexDetail.DocsCount >= 0); | ||
| Assert.True(indexDetail.StoreSizeInBytes >= 0); | ||
| Assert.True(indexDetail.UnassignedShards >= 0); | ||
| }); | ||
| } |
Comment on lines
527
to
538
| [Fact] | ||
| public async Task GetElasticsearchSnapshots_AsGlobalAdmin_ReturnsTypedResponse() | ||
| { | ||
| // Act | ||
| var snapshots = await SendRequestAsAsync<ElasticsearchSnapshotsResponse>(r => r | ||
| .AsGlobalAdminUser() | ||
| .AppendPaths("admin", "elasticsearch", "snapshots") | ||
| .StatusCodeShouldBeOk()); | ||
|
|
||
| // Assert | ||
| Assert.NotNull(snapshots); | ||
| Assert.NotNull(snapshots.Repositories); | ||
| Assert.NotNull(snapshots.Snapshots); | ||
| } |
# Conflicts: # src/Exceptionless.Core/Bootstrapper.cs # src/Exceptionless.Core/Models/Event.cs
The OnDeserialized() method now uses TryAdd semantics instead of unconditional assignment when merging root-level JSON extension data into the Data dictionary. This preserves values explicitly provided in the 'data' JSON property, matching the old Newtonsoft DataObjectConverter behavior where duplicate keys were preserved under modified names. Added tests for the collision scenario and root-level-only capture.
Previously NormalizeDates() modified the original JsonNode tree in place, which could corrupt upstream objects if the same node was reused after formatting. Now creates a deep clone before date normalization, keeping the source node tree pristine for downstream processing.
The GetValue<T> JSON string path was only using the primary serializer (snake_case naming policy), which cannot deserialize legacy PascalCase data stored as raw JSON strings. Now uses TryDeserializeWithFallback consistently across all paths (JsonElement, JsonNode, Dictionary, List, and JSON string). Also improved TryDeserializeWithFallback documentation explaining why the length-based heuristic is reliable for the PascalCase vs snake_case use case, and added comprehensive tests for both naming conventions across string and dictionary source formats.
- GetSlackToken: Added KeyNotFoundException to catch filter (GetValue throws it for missing keys), added Debug.WriteLine for diagnostics, documented that TryDeserializeWithFallback handles PascalCase legacy data. - TypeHelper.AreSameValue: Added Dictionary<string, object?> comparison via JSON serialization since Dictionary.Equals uses reference equality. This matches the old JToken.ToString() comparison behavior.
Replace System.Diagnostics.Debug.WriteLine with structured ILogger.LogWarning for SlackToken deserialization failures. The logger parameter is optional (null-safe) so test callers are unaffected. All production callers (ProjectController, SlackService, WebHooksJob) now pass their logger.
Add round-trip, snake_case deserialization, and DataDictionary GetValue tests for: - Method (with GenericArguments and Parameters) - InnerError (with nested inner errors and TargetMethod) - Location (record type with Unicode support) - UserDescription (with IData) - SubmissionClient (with IPv6) - ManualStackingInfo (with SignatureData dictionary) 25 new tests covering all remaining data model serialization gaps.
…ptions The CaseInsensitiveFallbackOptions (used for legacy PascalCase data) was missing ObjectToInferredTypesConverter, causing object-typed properties to remain as JsonElement instead of being inferred to native .NET types. This could produce inconsistent types between the primary and fallback deserialization paths. Also documented the ConvertJsonElement static method's int-first numeric inference behavior in Event.OnDeserialized.
PascalCase data never existed in ES — the old Newtonsoft config used LowerCaseUnderscorePropertyNamesContractResolver, so everything is snake_case. Removed: - TypeHelper dictionary serialization comparison hack - PascalCase-specific tests (GetValue_PascalCase*) - Verbose Event.cs comment about ConvertJsonElement - Overly detailed TryDeserializeWithFallback XML docs Simplified: - CaseInsensitiveFallbackOptions (reverted earlier in session) - JSON string deserialization path (reverted earlier in session) Also added // Arrange / // Act / // Assert structure to all 6 new model serializer test files.
…ides Replace custom LowerCaseUnderscoreNamingPolicy with the built-in JsonNamingPolicy.SnakeCaseLower for JSON serialization. The only model properties where the two policies differ are EnvironmentInfo.OSName and OSVersion: the custom policy produces o_s_name/o_s_version (letter-by-letter) while SnakeCaseLower produces os_name/os_version (grouped acronyms). Added [JsonPropertyName] attributes to preserve the legacy field names. The ES DefaultFieldNameInferrer still uses ToLowerUnderscoredWords, but [JsonPropertyName] overrides it via DefaultPropertyMappingProvider. Added round-trip serializer tests for Organization, Token, and SavedView (previously missing coverage for ES-persisted models). Added OSProperties_UseJsonPropertyNameOverride test to verify the o_s_name/o_s_version attribute overrides produce correct output. Full test suite: 1541 total, 1539 passed, 0 failed, 2 skipped.
|
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
Removes all Newtonsoft.Json serialization from the application layer, replacing it with System.Text.Json (STJ) exclusively. NEST still brings in Newtonsoft transitively for Elasticsearch wire protocol, but all application-level serialization now uses STJ.
Key Changes
New Files
ElasticSystemTextJsonSerializer— CustomIElasticsearchSerializerfor NEST using STJEmptyCollectionModifier— STJ type modifier that omits empty collections during serializationJsonNodeExtensions— STJ equivalents of JObject helpers for event upgradersObjectToInferredTypesConverter— Handles JObject/JToken from NEST during STJ serializationDeleted Files (Newtonsoft-specific)
DataObjectConverter,ElasticJsonNetSerializer,DynamicTypeContractResolverElasticConnectionSettingsAwareContractResolver,ExceptionlessNamingStrategyLowerCaseUnderscorePropertyNamesContractResolverCore Changes
IJsonOnDeserializedmerges[JsonExtensionData]overflow intoDatadictionaryGetValue<T>()fromJsonElementJObjecttoJsonObject(System.Text.Json.Nodes)JsonSerializerOptionsfrom DI instead ofISerializer[JsonPropertyName]attributes for PascalCase backward compatibilityPackage Changes
Foundatio.JsonNet,NEST.JsonNetSerializer(x2),FluentRest.NewtonsoftJsonFluentRest(STJ-based)Testing
JsonNode.DeepEquals)