Swap Newtonsoft.Json for System.Text.Json in query logging (#294)#327
Merged
Conversation
…ry logging (#294) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
FR12 prerequisite for the FR10 IAssemblyFixture<T> log-capture mechanism. - Pin xunit.v3 3.2.2 in central package management (replaces xunit 2.9.3); xunit.analyzers kept at 1.27.0 (restore clean, no NU1605/NU1107). - Repoint the four xunit-referencing test csprojs to xunit.v3 (Core.Tests, Extensions.Tests, Tests.AOT, Test.Helpers); Benchmarks untouched. - Drop `using Xunit.Abstractions;` (v3 moves ITestOutputHelper into Xunit). - Rework TestClassBase<T> to source the current test from TestContext.Current.Test as Xunit.v3.IXunitTest, replacing the v2 private-`test`-field reflection (and its IL2075 suppression); TestQualifiedName now uses IXunitTest.TestDisplayName. Pinned by a new behavioural test (FR12 item 6). Full suite green on net8.0 and net9.0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
FR2/FR3/FR6/FR14. Introduces the System.Text.Json configuration surface the
query logging decorators will read at serialise time.
- Add System.Text.Json 10.0.8 (CPM) as a direct dependency of Paramore.Darker
across netstandard2.0;net8.0;net9.0.
- Add public static QueryLoggingJsonOptions with a settable Options property,
defaulting to a JsonSerializerOptions { ReferenceHandler = IgnoreCycles } (FR3)
and a null-guarded setter (FR2). Class-init performs no Serialize call, so the
options are not locked at startup (FR14).
Pinned by three tests in a DisableParallelization collection (C5): default
content, null-guard, and the lossy direct-assignment contract.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ueries (#294) FR10 log-capture infrastructure for the decorator-rewrite tests. - Add LoggerCaptureFixture/CapturingLoggerProvider/CapturingLogger/CapturedLogEntry to both Core.Tests and Extensions.Tests. The fixture swaps ApplicationLogging.LoggerFactory for a capturing one and restores it on dispose. CapturedLogEntry retains the message template ({OriginalFormat}), the rendered message, the structured KVP arguments, and any exception. - Register via [assembly: AssemblyFixture(typeof(LoggerCaptureFixture))]: xunit.v3 3.2.2 has no IAssemblyFixture<T> interface, and the attribute's "initialized before any test in the assembly runs" contract is the equivalent install-before-touch guarantee FR10 requires (the static readonly Logger per closed generic must be cached against the capturing factory). - Add disjoint CoreLoggingTestQuery / ExtensionsLoggingTestQuery (+ sync and async handlers) so each assembly's QueryLoggingDecorator<,> closed generic is a separate cache cell. Also fix QueryLoggingJsonOptions to use an explicit initializer (target-typed new is unavailable under the netstandard2.0 C# 7.3 default). Full suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
FR1/FR8/FR9/FR13. QueryLoggingDecorator drops the JsonSerializerSettings
constructor parameter and the ConfigurationException path, and serialises the
query via System.Text.Json reading QueryLoggingJsonOptions.Options.
- Use the runtime-type overload JsonSerializer.Serialize(value, value.GetType(),
options): the pipeline closes the decorator over IQuery<TResult>
(PipelineBuilder.cs:214), so the generic one-arg overload would serialise the
bare marker interface and emit "{}". value.GetType() restores Newtonsoft's
runtime-type behaviour and composes with a source-gen TypeInfoResolver (FR11).
ADR 0012 amended to record this.
- FR13 IL2026/IL3050 UnconditionalSuppressMessage pair on Serialize<T>, guarded
#if NET8_0_OR_GREATER (the attribute is internal in the netstandard2.0 BCL; the
warnings only fire under the net8/net9 trim/AOT analysers).
- Preserve the start/completion templates verbatim and the runtime-concatenated
" (with fallback)" suffix.
Tests (Core.Tests, throwaway-instance lock isolation): assert STJ body via
captured logs, and the with-fallback {OriginalFormat} template. Deleted the two
obsolete Newtonsoft-era decorator tests (referenced the removed ctor).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
FR1/FR8/FR9/FR13. Mirror of the sync decorator change on
QueryLoggingDecoratorAsync: drop the JsonSerializerSettings ctor parameter and
the ConfigurationException path; serialise via System.Text.Json's runtime-type
overload reading QueryLoggingJsonOptions.Options; FR13 IL2026/IL3050
suppressions guarded #if NET8_0_OR_GREATER; async start/completion templates and
the " (with fallback)" suffix preserved.
Tests assert the STJ body via captured logs and the async with-fallback
{OriginalFormat} template, using throwaway-instance lock isolation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
#294) FR4/FR5/FR8. The AddJsonQueryLogging / JsonQueryLogging callback changes from Action<JsonSerializerSettings> to Action<JsonSerializerOptions>. - Canonical AddJsonQueryLogging<TBuilder> invokes configure against QueryLoggingJsonOptions.Options and registers the decorators; the DI extension is a thin forwarder and no longer registers a JsonSerializerSettings singleton (FR8). JsonQueryLogging(IBuildTheQueryProcessor) still casts to QueryProcessorBuilder and throws NotSupportedException for custom builders. - Drop the Newtonsoft.Json usings from both extension classes. - Delete the obsolete When_AddJsonQueryLogging_called_should_register_serializer_settings test; add the rewrite (callback config + System.Text.Json smoke) and a NotSupportedException pin (CustomQueryProcessorBuilder). The smoke test routes DI logging through the fixture's capturing provider (AddDarker resets ApplicationLogging.LoggerFactory from the container) and runs in a DisableParallelization "DarkerHostBootstrap" collection to avoid racing on that global (C6 mitigation). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
FR5/AC1. Drop the Newtonsoft.Json PackageReference from Paramore.Darker.csproj and the PackageVersion from Directory.Packages.props now that the query logging decorator serialises via System.Text.Json. No source references remain. dotnet list package --include-transitive shows no Newtonsoft.Json for Paramore.Darker or the sample on any TFM. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Step 9 swaps the AOT verification mechanism from an xunit test host to a plain native-AOT console application. The former Paramore.Darker.Tests.AOT xunit project referenced two test executables (Core.Tests, Test.Helpers) purely for shared types, which a self-contained AOT publish cannot reference (NETSDK1150); AOT-publishing a reflection-driven test runner is also fraught. The test framework need not be AOT-compiled — only the product code — so a console harness that references only the product libraries, runs the two FR11 scenarios, and exits non-zero on mismatch is the correct AC4 proof. Consequences recorded across ADR 0012, tasks.md Step 9/12/13, and the FR12 item-6 note in requirements.md: - Paramore.Test.Helpers is removed in full; its FR12-item-6 meta-test is dropped (not relocated) — TestClassBase's only consumer was the deleted AOT host, so the log-naming hazard cannot occur and FR12 item 6 is satisfied vacuously. No AC depends on it. - <IsAotCompatible> stays on core to validate the FR13 suppressions; the ~28 pre-existing pipeline trim/AOT warnings (outside Logging/) are documented as a known limitation, not silenced with NoWarn. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
) Replaces the xunit-based Paramore.Darker.Tests.AOT test host with a plain console application so it can be published under PublishAot=true. The xunit host referenced two test executables (Core.Tests, Test.Helpers) for shared types, which a self-contained AOT publish cannot reference (NETSDK1150). - AOT csproj: OutputType=Exe, PublishAot=true, TrimMode=full; references only the product libraries (Paramore.Darker, Extensions.DI, Testing). Drops xunit, Test.Sdk, Shouldly and the two test-project references. - Deletes AOTTestClassBase and AOTQueryProcessorTests (depended on the removed test-exe types; their functional coverage is duplicated in Core/Extensions tests). Adds a placeholder Program.cs; the FR11 scenarios land next. - Removes Paramore.Test.Helpers in full (its only consumer was the AOT host) and prunes it from Darker.slnx / Darker.Filter.slnf. The FR12-item-6 meta-test is dropped, not relocated — TestClassBase has no remaining consumer, so the log-naming hazard cannot occur; no AC depends on it. - Adds <IsAotCompatible> to Paramore.Darker (net8/net9 only) to validate the FR13 Logging/ suppressions at the library's own build. No product behaviour change. Verified: core + filter build succeed; full suite green (Core 75, Extensions 8 on net8.0 + net9.0); core build emits zero IL warnings under Logging/ (only the pre-existing pipeline/registry warnings, outside AC4 scope); AOT publish (net9.0, osx-arm64) produces a native binary that runs to exit 0 — NETSDK1150 resolved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The native-AOT harness now runs the first FR11 scenario: a property-bearing
[QueryLogging] query whose {Query} log argument must equal the exact
source-generated JSON. Without a source-generated TypeInfoResolver the
decorator's serialization throws under native AOT (reflection-based
serialization disabled); installing AotTestJsonContext.Default makes the
logging path AOT-safe.
Verified: net8.0 + net9.0 publish -r osx-arm64 with no IL warnings under
Logging/ (only the pre-existing pipeline/registry sites), both binaries
run to exit 0. A harness-side [DynamicDependency] roots the handler methods
the pipeline resolves reflectively (consumer-side compensation for the
documented pipeline AOT limitation; not a logging-path change).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds a native-AOT harness scenario proving a [QueryLogging] query whose object graph contains a Parent -> Child -> Parent reference cycle does not throw when serialised, because the shared QueryLoggingJsonOptions.Options carries ReferenceHandler.IgnoreCycles by default (FR3). Case 2 chains after Case 1 in Program.cs and does not re-install the source-gen resolver (the shared Options is already locked by Case 1's first serialize). Both net8.0 and net9.0 publish (-r osx-arm64) with zero IL warnings under Logging/ and run to exit 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pins the FR13 allow-list empirically after a clean native-AOT publish of the harness on both TFMs: zero IL2xxx/IL3xxx warnings under src/Paramore.Darker/Logging/, both binaries exit 0, caller-propagation contingency did not fire. The only Logging/ suppression sites are the IL2026/IL3050 pairs on QueryLoggingDecorator.Serialize and QueryLoggingDecoratorAsync.Serialize. CI wiring intentionally skipped; AC4 stays a design-time verification. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…294) Adds the AC3-shaped OrderingTestQuery (public sealed, Marker="x") with a single [QueryLogging] handler reserved for the FR14 lock-after-use ordering test, plus a dedicated DisableParallelization collection (QueryLoggingJsonOptionsOrdering) so the irreversible options lock that test triggers stays isolated. Build green; Core.Tests 75 pass on both TFMs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds the FR14 ordering test: a single sequential [Fact] proving
QueryLoggingJsonOptions.Options is mutable before the first query is logged
(MaxDepth set, AddJsonQueryLogging configure callback applies WriteIndented),
serialises the indented {Query} on execution, then throws
InvalidOperationException when mutated after the lock. GREEN-on-arrival pin;
no production code. The irreversible lock is isolated on a throwaway options
instance (C5) so it never touches the shared default, removing the run-last
ordering hazard. Core.Tests 76 pass on both TFMs.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author specs/006-json_serializer/release-notes-draft.md covering the System.Text.Json swap: callback type change, decorator ctor change, DI singleton removal, Newtonsoft removal, both migration snippets, the custom-decorator escape hatch, the AOT suppression/TypeInfoResolver note, the pre-existing pipeline trim warnings, and the builder-surface and parallel-WebApplicationFactory limitations (Step 12 DoD artefact). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Step 11 (AC5): sample app emits System.Text.Json {Query} log lines for
GET /people ({}) and the with-fallback completion variant for an unknown
GET /people/{id}; no Newtonsoft in the sample's transitive closure.
Step 13 (gate): full Darker.Filter.slnf build/test green (Core 76,
Extensions 8 x2 TFMs), AOT publish+run exit 0 on net8.0/net9.0 with no
Logging/ IL warnings, no Newtonsoft in Paramore.Darker or the sample,
and CPM pins verified. All of AC1-AC5 + DoD satisfied.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #294. Implements ADR 0012 — replaces
Newtonsoft.JsonwithSystem.Text.Jsonin the built-in query logging decorator and removesNewtonsoft.Jsonas a dependency ofParamore.Darker. Brings Darker into parity with Brighter (which migrated in BrighterCommand/Brighter#1470).Developed via the spec workflow (
specs/006-json_serializer): Requirements → ADR 0012 → adversarial review → tasks → TDD implementation.What changed
{Query}argument withJsonSerializer.Serialize(value, value.GetType(), QueryLoggingJsonOptions.Options)(runtime-type serialize so a decorator closed overIQuery<TResult>still emits real properties, not{}).QueryLoggingJsonOptions.Options, defaulting toReferenceHandler.IgnoreCycles(keeps EF Core navigation-cycle graphs from throwing on the logging path). Mirrors Brighter'sJsonSerialisationOptions.Action<JsonSerializerSettings>→Action<JsonSerializerOptions>; decorator constructor no longer takes a serialiser; no DI singleton registered.Newtonsoft.Jsonremoved fromParamore.Darker(and from CPM).System.Text.Json 10.0.8pinned.xunit.v3 3.2.2; assembly-scopedLoggerCaptureFixture; AOT proof converted to a native-AOT console harness (PublishAot=true,TrimMode=full) running both FR11 scenarios.Breaking changes (consumer-facing)
Callback type change, decorator ctor change, DI singleton removal, and Newtonsoft removal. Migration guidance — including snippets that preserve (or warn about dropping)
ReferenceHandler.IgnoreCycles, the custom-decorator escape hatch, and the AOTTypeInfoResolveropt-in — is inspecs/006-json_serializer/release-notes-draft.mdfor the V5 release notes.Acceptance criteria — all verified
Newtonsoft.JsoninParamore.Darker(any TFM) or the sample; CPM pins correct.src/Paramore.Darker/Logging/(only the two FR13 allow-listSerialize<T>suppressions).{Query}log lines ({}forGET /people;(with fallback)completion for an unknownGET /people/{id}).Known limitation (pre-existing, documented, not introduced here)
Enabling AOT/trim analysis surfaces ~28 pre-existing trim warnings in Darker's reflection pipeline (
PipelineBuilder,QueryHandlerRegistry*), outside the logging path. These are not suppressed and remain a follow-up; consumers AOT-publishing an app that resolves Darker handlers should root handlers with[DynamicDependency(PublicMethods, typeof(TheHandler))]. Detailed in the release-notes draft.🤖 Generated with Claude Code