Replace JSON.NET with System.Text.Json across the codebase#2135
Open
niemyjski wants to merge 83 commits into
Open
Replace JSON.NET with System.Text.Json across the codebase#2135niemyjski wants to merge 83 commits into
niemyjski wants to merge 83 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
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.
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.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 121 out of 121 changed files in this pull request and generated 3 comments.
💡 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 121 out of 121 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Pass JsonSerializerOptions to SerializeToNode so UserInfo/UserDescription nodes use snake_case property names instead of PascalCase defaults. Also: - Remove stale TryDeserializeWithFallback references in ProjectExtensions - Add max depth protection to EventUpgraderContext JSON parsing - Fix misleading enum storage comment in ES config (StackStatus is string) - Update 977.expected.json for snake_case @user output
- Replace raw NEST ElasticFilter with typed Foundatio expressions - Fix org abbreviation violations (org → organization) - Simplify V2_EventUpgrade (remove unused options param) - Update 977.expected.json for STJ format - Add OrganizationRepository test coverage
- Add DictionaryKeyPolicy for typed object deserialization in GetValue - Preserve user-provided keys when target is a dictionary type - Remove synthetic 1477.snake-case.json (use original expected.json) - Remove DeleteIndexesAsync hack from IntegrationTestsBase - Simplify EventUpgraderContext (remove static parse options) - Fix pipeline test assertions for snake_case keys (DictionaryKeyPolicy) - Fix serializer test assertions for normalized dictionary keys
Covers all serialization paths, data flow lifecycle, GetValue<T> dictionary extraction strategy, ObjectToInferredTypesConverter, event upgrade pipeline, model annotations, ES divergences, transitive dependencies, and production safety guarantees.
DictionaryKeyPolicy applied recursively to ALL dictionary keys at ALL nesting levels, normalizing user-provided keys like 'SomeProp' to 'some_prop' in Error.Data, QueryString, etc. This is data corruption. PropertyNamingPolicy=SnakeCaseLower alone handles typed C# property name serialization. PropertyNameCaseInsensitive=true on the deserializer handles snake_case to PascalCase property matching. Dictionary keys (user data) don't need normalization. Changes: - Remove DictionaryKeyPolicy from s_dictSerializeOptions - Remove IsDictionaryType helper and s_dictPreserveKeysOptions - Revert corrupted test assertions back to original key casing - Add snake-case fixture for WebHookDataTests (production ES format) - Fix case-sensitivity test to match STJ SnakeCaseLower behavior - Rename doc to serialization-architecture.md, remove DictionaryKeyPolicy docs
- Deleted 1477.snake-case.expected.json (should never have been created) - WebHookDataTests now uses original 1477.expected.json fixture - Added docs/foundatio-issues.md documenting 3 upstream issues CRITICAL BUG FOUND: V1 webhook data extraction is broken after STJ migration. GetError/GetRequestInfo/GetEnvironmentInfo not deserializing nested data correctly. V1 webhook output missing MachineName, IpAddress, TargetMethod, query strings. Needs investigation.
GetValue<T> failed to map multi-word PascalCase dictionary keys
(e.g., "ClientIpAddress", "MachineName", "TargetMethod") to their snake_case
typed property targets because STJ's PropertyNameCaseInsensitive only handles
case differences ("Message" ↔ "message") — it cannot structurally match
keys that differ by underscores ("ClientIpAddress" vs "client_ip_address").
This caused V1 webhooks to ship incomplete payloads (missing MachineName,
IpAddress, TargetMethod, full RequestPath) for any event whose @error,
@request, or @Environment data was stored with PascalCase keys (legacy V1
clients, pre-STJ test fixtures).
Fix: Add a recursive JsonNode key normalizer in DataDictionaryExtensions
that snake-cases dict keys ONLY at typed-property positions, walking into
typed collections and nested typed models. User-data dictionary positions
(Error.Data, RequestInfo.QueryString, RequestInfo.Cookies, etc.) are detected
via IDictionary<string, T> and left untouched — user-provided keys are
preserved exactly as submitted.
Added 6 targeted DataDictionary tests:
- PascalCase RequestInfo with multi-word keys (ClientIpAddress, etc.)
- PascalCase EnvironmentInfo with multi-word keys
- PascalCase Error with nested TargetMethod + StackFrame recursion
- PascalCase Error.Data user keys preserved exactly (SomeProp, MixedCASE_key)
- PascalCase RequestInfo.QueryString/Cookies keys preserved
- Mixed PascalCase + snake_case in same dictionary
Full suite: 1555/1557 pass (2 skipped are intentional perf tests).
…eserialization errors
Previously, all GetError/GetSimpleError/GetRequestInfo/GetEnvironmentInfo/
GetUserIdentity/GetSubmissionClient/GetLocation/GetManualStackingInfo/
GetUserDescription methods silently swallowed exceptions with `catch (Exception) { /* Ignored */ }`.
This meant deserialization failures (like the V1 webhook PascalCase bug just fixed)
were invisible in logs, making debugging nearly impossible.
Changes:
- Add optional `ILogger? logger = null` parameter to all 9 GetValue<T> extension methods
- Replace `catch (Exception)` with `catch (Exception ex)` and log as warnings with context
- Log format: "Failed to deserialize {DataKey} for event type {EventType}"
- Update webhook plugins (010_VersionOnePlugin, 005_SlackPlugin) to pass _logger
- Update event processor plugins (03_ManualStackingPlugin, 30_SimpleErrorPlugin,
40_RequestInfoPlugin, 45_EnvironmentInfoPlugin, 90_RemovePrivateInformationPlugin) to pass _logger
- Other call sites (PersistentEventExtensions, ErrorExtensions, Mailer, FormattingPluginBase)
continue to work without logger (optional parameter defaults to null)
Full suite: 1553/1555 pass, 2 skipped (intentional perf tests).
- Remove optional logger parameter (= null) from all Get* methods in EventExtensions - Update all call sites across plugins, jobs, formatters, and tests to pass logger - Add logger parameter to ToSessionStartEvent and GetIpAddresses extension methods - Add logger parameter to SlackAttachment constructor - Update Mailer.AddUserInfo to use instance logger instead of static method All tests passing (1555/1557 passed, 2 skipped by design)
- Upgrade Foundatio.Repositories.Elasticsearch to 8.0.0-beta1.3 which fixes FieldValueHelper.ToFieldValue to respect [JsonStringEnumMemberName] - Remove StackRepository workaround: use StackStatus.Open enum directly instead of string literal "open" - Add _logger parameter to all Get* test call sites (required after previous commit) - Add *.lscache to .gitignore, remove from tracking EventRepository still uses ElasticFilter workaround for ExistsQuery on dynamic fields (no upstream typed API available yet).
These regression tests were verified against origin/main before being added here. They pin the V1 webhook payload contract, legacy EnvironmentInfo OS fields, and large scientific-notation fallback behavior so the STJ branch cannot hide those differences.
- ObjectToInferredTypesConverter: Handle exponent numbers (e.g. 1e100) that overflow decimal range by falling back to double via TryGetDecimal - DataDictionaryExtensions: Map both SnakeCaseLower(PropertyName) and JsonPropertyName attribute forms in GetPropertyMap, and normalize keys to the attribute name (e.g. 'o_s_name' not 'os_name') in NormalizeKeysForType - V1 webhook fixtures: Remove null/empty fields to match STJ global options (WhenWritingNull + SkipEmptyCollections) — standard JSON convention, semantically equivalent for all webhook consumers
…exponents RCA: The static ConvertJsonElementNumber helper (called from Event.OnDeserialized for every extension data property) used unconditional element.GetDecimal() for numbers with decimal points or exponents. Decimal has max range ~±7.9×10²⁸, so values like 1e100 (common in telemetry data, scientific measurements, and JS Number.MAX_VALUE) throw OverflowException. The instance ReadNumber method already had the correct TryGetDecimal→double fallback pattern but the static helper did not share this logic — an asymmetry introduced when the static helper was extracted. Fix: Use TryGetDecimal with GetDouble fallback in both the floating-point branch AND the large-integer fallback, matching the instance method's fault tolerance. Add direct unit tests for ConvertJsonElement with out-of-decimal-range values.
RCA: JsonObject.Add throws ArgumentException if the key already exists. The Rename and RenameOrRemoveIfNullOrEmpty methods rebuild the object in property-order and substitute currentName with newName, but never checked whether newName already existed as a separate property. When event upgrade plugins rename a field to a name that malformed client data already populated, this crashed the event pipeline. Principle: Rename semantics should match Newtonsoft JObject behavior where the last-written key wins. In a rename, the source value is the intentional one (being migrated to the canonical name), so it takes precedence. Fix: Skip properties whose key matches newName during the rebuild loop. The renamed value is inserted at the original position, and the pre-existing collision property is dropped. Both Rename and RenameOrRemoveIfNullOrEmpty are patched consistently.
The Dict→JsonNode→String→T path in GetValue<T> performs three serialization steps. The JsonNode intermediate is required for NormalizeKeysForType which renames PascalCase keys in-place to match SnakeCaseLower property names. The final ToJsonString→Deserialize could be eliminated by deserializing directly from the node if ITextSerializer exposed JsonSerializerOptions. Annotated for future optimization (not a correctness issue).
…behavior The test 'Deserialize_JsonWithUnknownRootProperties_IgnoresUnknownProperties' implied that unknown root properties are discarded. In reality, they are captured via [JsonExtensionData] and merged into Event.Data by OnDeserialized(). Rename to 'MergesIntoData' and add assertions verifying the unknown properties are actually present in ev.Data with correct values.
…paths ASP.NET Core's DefaultProblemDetailsFactory adds 'traceId' (camelCase) to ProblemDetails.Extensions. The DefaultProblemDetailsWriter (status code pages) applies PropertyNamingPolicy to extension keys producing 'trace_id', but MVC's SystemTextJsonOutputFormatter writes [JsonExtensionData] keys literally, producing 'traceId'. This caused inconsistent API responses. Fix: explicitly rename the key in CustomizeProblemDetails (called for both paths) so the literal key is always 'trace_id'. Also set ProblemDetails.Instance property directly instead of polluting the Extensions dictionary — Instance is a first-class RFC 9457 member.
The @submission_client.version field reflects the FluentRest assembly version, which was bumped from 11.0.0.0 to 11.1.0.0 as part of the dependency updates.
The PostEvent_WithExtraRootProperties_CapturedInDataBag and PostEvent_WithExtraPropertiesAndKnownData_PreservesAllData tests used conditional assertions (if ContainsKey) that would pass vacuously when data was missing. Made assertions unconditional to properly validate the serialization round-trip. Also added nested object/array assertions in PostEvent_WithExtraComplexProperties_CapturedAsObjects using 42L to match ES long integer behavior.
…lity WebHooksJob now uses dedicated JsonSerializerOptions for webhook delivery that does NOT use DefaultIgnoreCondition.WhenWritingNull or the EmptyCollectionModifier. This ensures external webhook consumers continue receiving null fields (DateFixed, Description, etc.) and empty collections (Tags: []) in the payload — matching the original Newtonsoft.Json behavior. The webhook test was also updated to use the same delivery-path options, and the expected JSON files now reflect the full payload including nulls. For v2 webhooks, this is an additive change (extra null fields) which is safe for consumers that ignore unknown properties.
…Element When TrySetPropertyValue receives a raw JsonElement (e.g., from UnknownProperties in legacy PATCH endpoints), it was calling JsonSerializer.Deserialize without the configured options. This would fail for complex types like Dictionary<string, bool> that rely on the snake_case naming policy. The DeltaJsonConverter now passes options to the Delta instance for use in this fallback path.
…orruption The STJ encoder uses UnicodeRanges.All which passes non-ASCII characters through unescaped (unlike Newtonsoft which escaped to \\uXXXX). The existing Encoding.ASCII.GetBytes would corrupt these characters (e.g., in release notification messages). Also fixes the ArraySegment length parameter which used string char count instead of byte array length.
The test scope naming used 'test' for id=0 and 'test-{N}' for id>0.
When Foundatio's DeleteIndexesAsync/ConfigureIndexesAsync ran with a
'test-*' wildcard, it inadvertently matched indices from 'test-1',
'test-2', etc., causing invalid_alias_name_exception failures.
Changed to 'xtest{N}' format so no scope prefix is a substring of
another, eliminating the race condition during parallel test execution.
totalOrphanedEventCount was declared and logged at the end of both DeleteOrphanedEventsByProjectAsync and DeleteOrphanedEventsByOrganizationAsync but never incremented inside the batch loop. Operators reading the summary log line would always see 'Found 0 orphaned events' even when hundreds were deleted. The deletes themselves were unaffected. Same increment pattern already exists in DeleteOrphanedEventsByStackAsync.
The encoder was described as 'XSS-safe' that 'escapes <, >, &', but JavaScriptEncoder.Create(UnicodeRanges.All) does the opposite — it allows those characters through unescaped. The default encoder escapes them. The actual intent is to allow non-ASCII Unicode (CJK, emoji, etc.) through unescaped for readability. For JSON APIs the Content-Type header is the XSS boundary, not the encoder.
Add 21 new tests across two test classes covering CleanupDataJob and CleanupOrphanedDataJob with multi-tenant scenarios: CleanupDataJobTests (9 new): - Suspended token cleanup across multiple tenants - Soft-deleted organization/project/stack cleanup isolation - Retention period enforcement for free vs paid plans - Combined multi-tenant full cleanup scenario CleanupOrphanedDataJobTests (12 new): - Orphaned events by stack/project/organization deletion - Large volume test (5000 valid + 10000 orphaned events) - Cross-tenant isolation verification - Duplicate stack detection and cleanup - Empty database edge case - Combined all-orphan-types scenario fix: preserve webhook backwards compatibility for V1 and V2 payloads The old Newtonsoft code used DynamicTypeContractResolver which applied different serialization rules per type: - V1 (VersionOneWebHookEvent/Stack): default resolver — PascalCase via [JsonProperty], includes nulls and empty collections. - V2 (WebHookEvent/Stack): LowerCaseUnderscorePropertyNamesContractResolver — snake_case naming, omits nulls and empty collections. Replicate this in STJ with two options sets: - V1: clone standard options + DefaultIgnoreCondition.Never + DefaultJsonTypeInfoResolver (no EmptyCollectionModifier) - V2: clone standard options as-is (WhenWritingNull + EmptyCollectionModifier + SnakeCaseLower naming) Pick options at serialization time based on the runtime type of the webhook data object. fix: correct misleading encoder comment — HTML chars ARE escaped JavaScriptEncoder.Create(UnicodeRanges.All) allows non-ASCII through but still escapes HTML-sensitive characters (<, >, &, ') to their \\uXXXX forms. The old comment incorrectly claimed they were NOT escaped. Test results confirm: & serializes as \\u0026. fix: adapt test infrastructure for Elastic.Clients.Elasticsearch v8 The new Elastic.Clients.Elasticsearch v8 client differs from NEST in several ways that broke test isolation: - Does NOT split comma-separated index strings (DeleteByQuery targeted nothing) - Concurrent index creation races during parallel test runs - AppScope pool ID returned before host fully disposed (alias conflicts) - ConfigureIndexesAsync cache prevented recreation after wildcard delete Fix: use scope wildcard patterns for cleanup, add retry logic for alias races, scope index configuration cache by ID, return pool IDs after full disposal, and aggressively delete all indices for a scope on test init. refactor: remove #region directives from test files Regions add visual clutter without providing meaningful organization. Test methods are already grouped by class and attribute-driven discovery makes region-based grouping unnecessary. Fix ArgumentException when multiple stacks share the same SignatureHash in cache Dictionary.Add throws ArgumentException on duplicate keys. When multiple stacks share the same SignatureHash (e.g. duplicate stacks created with same project/type/source), AddDocumentsToCacheAsync would throw. Use indexer assignment instead so the last write wins gracefully. Fix WillExcludeOldStacksForStackNewMode test - give log events unique sources Events 1, 2, and 3 all defaulted to Type=Log and Source='Test Event', giving them identical DuplicateSignature values. The FixDuplicateStacks logic (run periodically) merges stacks with the same DuplicateSignature, marking all but one as is_deleted=true. This caused event 3's stack to be soft-deleted, returning only 1 result instead of 2. Fix: assign unique Source values to each log event so they have distinct stacks that won't be merged by the deduplication logic. fix: DeltaJsonConverter respects [JsonPropertyName], removes itself from options copy; Delta uses TryGetValue Add serialization audit tests for STJ migration verification Comprehensive integration tests that submit events, webhooks, and patches with different JSON property casings (snake_case, camelCase, PascalCase, MIXED) and capture request/elastic/response JSON to audit-output/ for cross-branch diffing. Key findings documented via test output: - Multi-word PascalCase/camelCase root properties (referenceId, ReferenceId) do not match snake_case wire names and fall into Data bag - Single-word properties match case-insensitively with any casing - NormalizeKeysForType in DataDictionaryExtensions correctly handles multi-word keys for typed models in the Data dictionary - Character escaping: &→\\u0026, +→\\u002B, <→\\u003C, >→\\u003E in output - Webhook creation requires snake_case property names (camelCase returns 400) Add audit-output/ to .gitignore Add casing compatibility and integration tests for STJ migration Reproduce behavioral differences found during serialization audit: - PascalCase/camelCase ReferenceId not binding (goes to ExtensionData) - Date-only strings being parsed as DateTimeOffset - Numeric precision, empty collection omission edge cases These tests confirm the bugs before fixes are applied. Fix PascalCase/camelCase ReferenceId binding in STJ deserialization STJ's PropertyNameCaseInsensitive with SnakeCaseLower policy only matches case-insensitively against the policy-transformed name (reference_id), so PascalCase (ReferenceId) and camelCase (referenceId) end up in ExtensionData. Add fallback in OnDeserialized to check ExtensionData for these alternate casings and bind to the ReferenceId property, preserving backwards compatibility with payloads submitted by older SDKs. Preserve date-only strings instead of parsing as DateTimeOffset Date-only strings like '2026-01-15' should not be expanded to DateTimeOffset values (e.g., '2026-01-15T00:00:00-06:00') because: 1. They may not represent dates in user data 2. The timezone expansion adds information that wasn't in the original 3. This matches the legacy Newtonsoft behavior (DateParseHandling.None) Only parse strings as dates when they contain a 'T' time separator, indicating an explicit ISO 8601 datetime with time component. Add serialization-audit skill for branch comparison workflow Documents the workflow for generating, diffing, and testing serialization behavior across branches during serializer migrations. Clean up STJ migration: fix date parsing, add empty collection tests, document ReferenceId - Remove lowercase 't' check in date parsing (ISO 8601 uses uppercase T only) - Add tests for empty collection omission (tags, references, roundtrip) - Remove decorative comment borders from test/script files - Fix serialization-audit skill YAML frontmatter for discoverability - Document ReferenceId fallback with SDK version context
…ewIndex to new ES client, fix validator types
Standardize DateTimeOffset parsing for robustness across locales by explicitly using InvariantCulture and no special DateTimeStyles. This prevents unintended date conversions. Refine API data contracts by removing nullable annotations from array properties in several record types (e.g., Elasticsearch, Migrations). These arrays are now guaranteed to be non-null (though potentially empty), simplifying consumption. Corresponding tests are updated to reflect this. Includes minor code style updates for null checks (`is not null`) and removes a redundant test assertion. Fix RandomEventGeneratorTests: use properly configured serializer from shared static instance Instead of 'new SystemTextJsonSerializer()' with default options (missing snake_case, converters, etc.), use a shared static instance configured with ConfigureExceptionlessDefaults(). This matches the DI-registered serializer behavior without requiring full DI infrastructure for this standalone unit test. Add SetError/SetSimpleError extensions and use them in plugins Adds SetError(Error) and SetSimpleError(SimpleError) extension methods to EventExtensions, matching the existing GetError/GetSimpleError pattern. Updates ErrorPlugin, SimpleErrorPlugin, and AngularPlugin to use these extensions instead of direct dictionary assignment. Fix critical data correctness issues - OrganizationRepository: Fix suspended query logic — wrap FieldEquals conditions in FieldOr inside FieldNot so NOT(Active|Trialing|Canceled) works correctly instead of always-true NOT(AND of mutually exclusive) - ObjectToInferredTypesConverter: Replace ToDictionary (throws on duplicate keys) with a loop using dict[key]= (last-wins) to prevent DoS via crafted JSON with duplicate object keys - DataDictionaryExtensions: Rename s_ prefix to _ per project convention Code quality improvements from PR review - WebHooksJob: Remove redundant V2 options copy, use DI options directly - WebHookRepository: Restore logging in MarkDisabledAsync, spell out 'organization' per AGENTS.md convention - ProjectExtensions: Use TryGetValue instead of ContainsKey+indexer, remove useless <remarks> tag - 010_VersionOnePlugin: Use TryGetValue for SignatureInfo lookups - CleanupOrphanedDataJob: Use FieldValueHelper.ToFieldValue for consistency with StackQuery pattern Minor fixes: explicit byte[] type, restore comments, fix .gitignore - WebSocketConnectionManager: Use explicit byte[] instead of var - Startup.cs: Restore removed comments about serializer/ExternalAuthInfo - .gitignore: Remove extra blank line Test cleanup: remove bare AAA comments, fix prefixes, formatting - Remove bare '// Arrange', '// Act', '// Assert' comments across all PR test files (kept contextual ones like '// Arrange - Org 1 is...') - AppWebHostFactory: Rename s_ prefix to _ per convention - SerializationAuditTests: Rename s_ prefix to _ - EventControllerTests: Remove duplicate assert, add blank lines - WebHookDataTests: Remove redundant V2 options, use DI options - Webhook expected JSON: Revert escaped \\u0026 back to & Revert 'Test cleanup: remove bare AAA comments' - user wants AAA comments This reverts commit 9400674ff. The user wants AAA comments (Arrange/Act/Assert) added to tests where missing, not removed. Restoring them now. Fix serializer instance creation and formatting per user feedback - RandomEventGeneratorTests: Remove static serializer field, create inline - ObjectToInferredTypesConverter: Add blank line before return dict Document DataDictionaryExtensions _dictSerializeOptions rationale Clarify that _dictSerializeOptions is only for intermediate Dict→JsonNode conversion during key normalization, NOT for final deserialization which uses ITextSerializer from DI. Addresses PR feedback about custom options. Remove accidental frontend and Organization.cs changes from PR These files got pulled in during the rebase conflict resolution and don't belong in the STJ migration PR. Restored to main's version. Address PR feedback: test naming, AAA comments, null safety, EmptyCollectionModifier - Rename 4-part test names to 3-part (Method_Condition_Expected) - Rename SerializationAuditTests 2-part names to 3-part - Add AAA comments to all CasingCompatibilityTests - Fix nullable dereference: use Assert.IsType instead of as-cast - Add date assertion back to EventControllerTests - Remove custom JsonSerializerOptions in EventUpgraderTests assert message - Rename _options to _innerOptions in DeltaJsonConverter for clarity - Fix EmptyCollectionModifier: null no longer treated as empty, remove IEnumerable MoveNext() side-effect risk (restrict to ICollection) - Fix XML doc comment with unescaped angle brackets ExtensibleObject.GetProperty<T>: accept JsonSerializerOptions for proper deserialization Callers with complex types can pass configured options from DI. Existing string callers work without changes (backward-compatible default null). Fix RandomEventGeneratorTests: get serializer from DI instead of manual construction Extends TestWithServices to resolve ITextSerializer via DI container. Eliminates the last 'new SystemTextJsonSerializer(new JsonSerializerOptions())' usage.
|
|
||
| Assert.NotNull(ev); | ||
| Assert.NotNull(ev.Data); | ||
| Assert.True(ev.Data.ContainsKey("date_only")); |
|
|
||
| // Verify complex properties are captured in Data via JsonExtensionData + OnDeserialized | ||
| Assert.NotNull(ev.Data); | ||
| Assert.True(ev.Data.ContainsKey("metadata"), "metadata key should be captured in Data"); |
| // Verify complex properties are captured in Data via JsonExtensionData + OnDeserialized | ||
| Assert.NotNull(ev.Data); | ||
| Assert.True(ev.Data.ContainsKey("metadata"), "metadata key should be captured in Data"); | ||
| Assert.True(ev.Data.ContainsKey("tags_list"), "tags_list key should be captured in Data"); |
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.
Replace JSON.NET with System.Text.Json
Replaces all Newtonsoft.Json serialization with System.Text.Json (STJ) and migrates from NEST to Elastic.Clients.Elasticsearch.
Key changes
Serialization
SnakeCaseLowernaming policyObjectToInferredTypesConverterhandlesobject-typed properties (replacesDataObjectConverter)[JsonExtensionData]onEventmerges root-level@error,@request, etc. intoDatadict[JsonPropertyName]overrides for legacy property names (o_s_name,o_s_version)[JsonPropertyName]for PascalCase backward compatibilityElasticsearch
Elastic.Clients.ElasticsearchthroughoutEvent processing
JObjecttoJsonObject(System.Text.Json.Nodes)GetValue<T>()simplified — removed unnecessaryTryDeserializeWithFallback(PascalCase data never existed in ES)ErrorPlugin/SimpleErrorPluginafter pipeline mutationRemoved
DataObjectConverter,ElasticJsonNetSerializer, all Newtonsoft classesFoundatio.JsonNet,NEST.JsonNetSerializer,FluentRest.NewtonsoftJsonpackagesTest results