Replace JSON.NET with System.Text.Json across the codebase#2135
Open
Replace JSON.NET with System.Text.Json across the codebase#2135
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
src/Exceptionless.Core/Repositories/Configuration/ExceptionlessElasticConfiguration.cs
Fixed
Show fixed
Hide fixed
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.
src/Exceptionless.Core/Serialization/ElasticSystemTextJsonSerializer.cs
Outdated
Show resolved
Hide resolved
src/Exceptionless.Core/Serialization/ElasticSystemTextJsonSerializer.cs
Outdated
Show resolved
Hide resolved
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.
src/Exceptionless.Core/Serialization/ElasticSystemTextJsonSerializer.cs
Outdated
Show resolved
Hide resolved
src/Exceptionless.Core/Serialization/ObjectToInferredTypesConverter.cs
Outdated
Show resolved
Hide resolved
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
src/Exceptionless.Core/Plugins/EventParser/Default/JsonEventParserPlugin.cs
Fixed
Show fixed
Hide fixed
src/Exceptionless.Core/Plugins/EventParser/Default/JsonEventParserPlugin.cs
Fixed
Show fixed
Hide fixed
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.
src/Exceptionless.Core/Repositories/Configuration/ExceptionlessElasticConfiguration.cs
Fixed
Show fixed
Hide fixed
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.
|
|
||
| // Assert | ||
| Assert.NotNull(ev?.Data); | ||
| Assert.Equal(4, ev.Data.Count); |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 127 out of 127 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
+104
to
+111
| JsonValueKind.Null => null, | ||
| JsonValueKind.Array => element.EnumerateArray() | ||
| .Select(JsonElementToObject) | ||
| .ToList(), | ||
| JsonValueKind.Object => element.EnumerateObject() | ||
| .ToDictionary(p => p.Name, p => JsonElementToObject(p.Value)), | ||
| _ => element.GetRawText() | ||
| }; |
There was a problem hiding this comment.
JsonElementToObject converts objects via EnumerateObject().ToDictionary(...). This will throw if the JSON object contains duplicate property names (duplicate keys are legal in JSON and can be user-supplied), which would make event deserialization fail and can be used as a denial-of-service vector. Consider building the dictionary via a loop and assigning dict[name] = ... (optionally with StringComparer.OrdinalIgnoreCase, matching DataDictionary/ObjectToInferredTypesConverter behavior) so duplicates are handled deterministically instead of throwing.
The isSnakeCase heuristic only checked top-level dictionary keys, missing snake_case in nested objects like Error.Inner (which has stack_trace, target_method). This caused the deserializer to use PascalCase-only mode, leaving Inner.StackTrace empty and breaking error chain traversal. Solution: Try both SnakeCaseOptions and CaseInsensitiveOptions exhaustively, use deep property counting to pick whichever populates more fields. Added short-circuit optimization when one deserializer returns null. Also tightened ErrorExtensions.GetStackingTarget() to check for non-empty StackTrace (was checking non-null, but collections initialize to new()). Fixes SummaryDataTests.EventSummaryData for event3.json.
…nd, ES nullable annotations - Event.OnDeserialized delegates to ObjectToInferredTypesConverter.ConvertJsonElement for case-insensitive dictionary creation (was case-sensitive) - Iso8601DateTimeConverter preserves DateTime.MinValue identity through ES round-trip (prevents Kind=Unspecified→Utc mismatch breaking != default comparisons) - ElasticSystemTextJsonSerializer sets RespectNullableAnnotations=false to prevent JsonException on legacy ES documents with null non-nullable fields - ObjectToInferredTypesConverter gains public ConvertJsonElement for shared use - Clean up duplicate PackageReference entries in csproj files
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 126 out of 126 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
18
to
+35
| @@ -32,7 +32,7 @@ public void Upgrade(EventUpgraderContext ctx) | |||
| extendedData.Rename("TraceInfo", "TraceLog"); | |||
| } | |||
|
|
|||
| current = current["Inner"] as JObject; | |||
| current = current["Inner"] as JsonObject; | |||
There was a problem hiding this comment.
In the inner-error traversal loop, the upgrader always reads doc["ExtendedData"] instead of current["ExtendedData"]. As a result, only the outermost error's ExtendedData is updated and nested Inner errors are skipped, even though the loop walks current = current["Inner"]. Use current for the ExtendedData lookup (or drop the loop if only the root should be modified).
Comment on lines
14
to
36
| public TokenIndex(ExceptionlessElasticConfiguration configuration) : base(configuration, configuration.Options.ScopePrefix + "tokens", 1) | ||
| { | ||
| _configuration = configuration; | ||
| } | ||
|
|
||
| public override TypeMappingDescriptor<Models.Token> ConfigureIndexMapping(TypeMappingDescriptor<Models.Token> map) | ||
| public override void ConfigureIndexMapping(TypeMappingDescriptor<Models.Token> map) | ||
| { | ||
| return map | ||
| .Dynamic(false) | ||
| map | ||
| .Dynamic(DynamicMapping.False) | ||
| .Properties(p => p | ||
| .SetupDefaults() | ||
| .Date(f => f.Name(e => e.ExpiresUtc)) | ||
| .Keyword(f => f.Name(e => e.OrganizationId)) | ||
| .Keyword(f => f.Name(e => e.ProjectId)) | ||
| .Keyword(f => f.Name(e => e.DefaultProjectId)) | ||
| .Keyword(f => f.Name(e => e.UserId)) | ||
| .Keyword(f => f.Name(u => u.CreatedBy)) | ||
| .Keyword(f => f.Name(e => e.Refresh)) | ||
| .Keyword(f => f.Name(e => e.Scopes)) | ||
| .Boolean(f => f.Name(e => e.IsDisabled)) | ||
| .Boolean(f => f.Name(e => e.IsSuspended)) | ||
| .Number(f => f.Name(e => e.Type).Type(NumberType.Byte))); | ||
| .Date(e => e.ExpiresUtc) | ||
| .Keyword(e => e.OrganizationId) | ||
| .Keyword(e => e.ProjectId) | ||
| .Keyword(e => e.DefaultProjectId) | ||
| .Keyword(e => e.UserId) | ||
| .Keyword(e => e.CreatedBy) | ||
| .Keyword(e => e.Refresh) | ||
| .Keyword(e => e.Scopes) | ||
| .Boolean(e => e.IsDisabled) | ||
| .Boolean(e => e.IsSuspended) | ||
| .IntegerNumber(e => e.Type)); | ||
| } |
There was a problem hiding this comment.
TokenIndex still uses version 1, but the mapping for Type changed from a byte-sized number (previous NEST mapping) to an integer (.IntegerNumber(e => e.Type)). Elasticsearch field types can’t be changed in-place; if this index already exists, applying the new mapping can fail or require reindexing. Either keep the same numeric type as before, or bump the index version (and handle migration/reindex) to make this change safe.
…ue<T> Replaced deep reflection traversal and property cache with simple JSON length comparison for dual-deserializer fallback. Reduces code from ~325 to ~200 lines while handling both snake_case (API/Elasticsearch) and PascalCase (in-memory construction) data dictionary entries. Root cause: PropertyNameCaseInsensitive with snake_case naming policy cannot match multi-word PascalCase keys due to policy transformation happening first. Dual-deserialize remains necessary but now uses JSON output length as a lightweight heuristic instead of recursive reflection.
Member
Author
DataDictionary.GetValue Simplification (716533b)Eliminated reflection-based property counting infrastructure from \GetValue: Removed (~100 lines):
Replaced with: Simple JSON length comparison for dual-deserialize fallback (snake_case vs case-insensitive). Why dual-deserialize is still needed: \PropertyNameCaseInsensitive=true\ with snake_case naming policy cannot match multi-word PascalCase keys (e.g., \HttpMethod) — the policy transforms first (\http_method), then case-insensitive matching fails. Dictionary entries from in-memory C# code have PascalCase keys, while API/ES entries have snake_case. Tests: All 1378 tests pass (SerializerTests, DataDictionaryTests, EventPipelineTests, WebHookDataTests, SummaryDataTests, EventUpgraderTests, EventParserTests). |
|
Deleted the custom 260-line ElasticSystemTextJsonSerializer in favor of Foundatio's DefaultSourceSerializer. The custom serializer was duplicating functionality already provided by the framework. Changes: - ExceptionlessElasticConfiguration now uses DefaultSourceSerializer with config callback - ConfigureExceptionlessDefaults + ConfigureFoundatioRepositoryDefaults provide base options - ES-specific overrides preserved: RespectNullableAnnotations=false, ObjectToInferredTypesConverter(preferInt64=true) - DateTime converters extracted to DateTimeConverters.cs - DataDictionaryExtensions: extracted TryDeserializeWithFallback helper Test Coverage: 102 serialization tests passed
src/Exceptionless.Core/Repositories/Configuration/ExceptionlessElasticConfiguration.cs
Fixed
Show fixed
Hide fixed
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 126 out of 126 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+23
to
+36
| private static void RemoveNullAndEmptyProperties(JsonNode? node) | ||
| { | ||
| if (node is JsonObject obj) | ||
| { | ||
| var keysToRemove = new List<string>(); | ||
| foreach (var prop in obj) | ||
| { | ||
| if (prop.Value is null) | ||
| keysToRemove.Add(prop.Key); | ||
| else if (prop.Value is JsonArray arr && arr.Count == 0) | ||
| keysToRemove.Add(prop.Key); | ||
| else | ||
| RemoveNullAndEmptyProperties(prop.Value); | ||
| } |
There was a problem hiding this comment.
JsonAssert claims to ignore "empty collections", but RemoveNullAndEmptyProperties only removes empty arrays and never removes empty objects (including objects that become empty after recursively removing null/empty children). This can make semantic comparisons still fail when a dictionary/object is empty in one JSON and omitted in the other. Consider also pruning JsonObject properties with Count == 0 after recursion (and updating the traversal accordingly).
Comment on lines
30
to
+35
| _upgrader.Upgrade(ctx); | ||
| string expectedContent = File.ReadAllText(Path.ChangeExtension(errorFilePath, ".expected.json")); | ||
| Assert.Equal(expectedContent, ctx.Documents.First?.ToString()); | ||
| var expected = JsonNode.Parse(expectedContent); | ||
| var actual = JsonNode.Parse(ctx.Documents.First().ToFormattedString(_jsonOptions)); | ||
| Assert.True(JsonNode.DeepEquals(expected, actual), | ||
| $"File: {Path.GetFileName(errorFilePath)}\nExpected:\n{expected?.ToJsonString(new JsonSerializerOptions { WriteIndented = true })}\n\nActual:\n{actual?.ToJsonString(new JsonSerializerOptions { WriteIndented = true })}"); |
There was a problem hiding this comment.
Similar to SummaryDataTests, this test re-implements JSON semantic comparison and formatting. Since JsonAssert.AssertJsonEquivalent exists specifically to handle STJ vs Newtonsoft null/empty-collection differences, consider using it here to avoid duplicated logic and keep normalization behavior consistent across tests.
Root cause: The suspended=false filter used Must (AND) instead of Should (OR) for billing statuses, causing the query to never match organizations where BillingStatus is Active, Trialing, or Canceled. Changed the inner BoolQuery from Must to Should with MinimumShouldMatch=1, so the query now correctly matches 'BillingStatus is Active OR Trialing OR Canceled' rather than requiring all three simultaneously (impossible). Also refactored to use ElasticFilter/FieldEquals where appropriate, consistent with other repository query patterns.
…f ITextSerializer
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 126 out of 126 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
20
to
25
| var current = doc; | ||
| while (current is not null) | ||
| { | ||
| if (doc["ExtendedData"] is JObject extendedData) | ||
| if (doc["ExtendedData"] is JsonObject extendedData) | ||
| { | ||
| if (extendedData["ExtraExceptionProperties"] is not null) |
There was a problem hiding this comment.
In the inner-error traversal loop, the upgrader always reads ExtendedData from the root doc instead of the current inner object (current). This means only the top-level error gets upgraded; nested Inner errors' ExtendedData is skipped. Use current["ExtendedData"] (and operate on that) inside the while loop so each nested error is processed.
Comment on lines
25
to
31
| // This shouldn't hurt using DateTimeOffset to try and parse a date. It insures you won't lose any info. | ||
| if (DateTimeOffset.TryParse(clientInfo["InstallDate"]!.ToString(), out var date)) | ||
| if (DateTimeOffset.TryParse(clientInfo["InstallDate"]?.ToString(), out var date)) | ||
| { | ||
| clientInfo.Remove("InstallDate"); | ||
| clientInfo.Add("InstallDate", new JValue(date)); | ||
| // Format date as ISO 8601 with offset (matching Newtonsoft behavior) | ||
| clientInfo.Add("InstallDate", JsonValue.Create(date.ToString("yyyy-MM-ddTHH:mm:ss.FFFFFFFzzz"))); | ||
| } |
There was a problem hiding this comment.
date.ToString("yyyy-MM-ddTHH:mm:ss.FFFFFFFzzz") uses the current culture, which can emit non-ASCII digits or other culture-specific formatting. Since this value is written back into JSON and later parsed/indexed, it should be culture-invariant. Specify CultureInfo.InvariantCulture (or another invariant approach) when formatting the ISO8601 string.
Comment on lines
+102
to
+107
| var identity = ev.GetUserIdentity(serializer); | ||
| Assert.NotNull(identity); | ||
| Assert.Equal("Test user", identity.Identity); | ||
| Assert.Null(identity.Name); | ||
| Assert.Null(identity.Name); | ||
| Assert.Null(ev.GetUserDescription(jsonOptions)); | ||
| Assert.Null(ev.GetUserDescription(serializer)); |
There was a problem hiding this comment.
There are two identical Assert.Null(identity.Name); statements in a row, which is redundant and makes the test harder to scan. Remove the duplicate assertion.
STJ default ISO 8601 serialization is sufficient for Elasticsearch — the custom Iso8601DateTimeConverter/Iso8601DateTimeOffsetConverter only added verbose .0000000 fractional seconds which ES handles identically. All 74 serialization unit tests pass without the converters. Also restores Assert.NotNull guards in AdminControllerTests that were weakened to ?? [] null coalescing during the migration.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 124 out of 124 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
+7
to
+10
| /// <summary> | ||
| /// Compares two JSON strings semantically, ignoring null properties and empty collections | ||
| /// that differ between Newtonsoft and STJ serialization. | ||
| /// </summary> |
There was a problem hiding this comment.
The XML summary says this helper ignores “empty collections”, but the implementation only removes empty JSON arrays (and nulls) and does not remove empty JSON objects ({}). Either update the documentation to match the current behavior (e.g., “empty arrays”), or extend the pruning logic to also remove empty objects so comparisons align with EmptyCollectionModifier skipping empty dictionaries.
Comment on lines
+85
to
+106
| var settings = new ElasticsearchClientSettings( | ||
| connectionPool, | ||
| sourceSerializer: (_, clientSettings) => | ||
| new DefaultSourceSerializer(clientSettings, options => | ||
| { | ||
| // Base defaults from DI + Foundatio | ||
| options.ConfigureExceptionlessDefaults(); | ||
| options.ConfigureFoundatioRepositoryDefaults(); | ||
|
|
||
| // ES-specific overrides (legacy data compatibility) | ||
| options.RespectNullableAnnotations = false; | ||
|
|
||
| // ES needs all integers as long to match the old JSON.NET DataObjectConverter behavior. | ||
| // Remove existing ObjectToInferredTypesConverter instances (from both Configure calls) | ||
| // and insert preferInt64: true version at position 0 so STJ picks it first. | ||
| for (int i = options.Converters.Count - 1; i >= 0; i--) | ||
| { | ||
| if (options.Converters[i] is Exceptionless.Core.Serialization.ObjectToInferredTypesConverter) | ||
| options.Converters.RemoveAt(i); | ||
| } | ||
| options.Converters.Insert(0, new Exceptionless.Core.Serialization.ObjectToInferredTypesConverter(preferInt64: true)); | ||
| })); |
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)