Skip to content

Register default serializers lazily so a custom serializer can replace them without loading System.Text.Json - #2403

Open
FirmaSpring wants to merge 1 commit into
restsharp:devfrom
FirmaSpring:fix/lazy-default-serializers
Open

Register default serializers lazily so a custom serializer can replace them without loading System.Text.Json#2403
FirmaSpring wants to merge 1 commit into
restsharp:devfrom
FirmaSpring:fix/lazy-default-serializers

Conversation

@FirmaSpring

Copy link
Copy Markdown

Problem

RestClient constructs the default SystemTextJsonSerializer eagerly in SerializerConfig.UseDefaultSerializers(), before the caller's configureSerialization callback gets a chance to replace it.

For applications that use RestSharp.Serializers.NewtonsoftJson (or any other custom JSON serializer) exclusively, this forces System.Text.Json to be loaded even though it is never used for serialization. On .NET Framework 4.x in shared AppDomain plugin scenarios, this can trigger FileNotFoundException when the host has a different strong-named System.Text.Json version bound.

Reported in #2402.

Solution

UseDefaultSerializers() now registers SerializerRecord entries that carry a serializer factory (Func<IRestSerializer>) instead of a pre-constructed instance. The serializers are only constructed when GetSerializer() is first called, so a custom serializer installed via configureSerialization fully replaces the default before SystemTextJsonSerializer is ever instantiated.

The accepted content types and content type matching functions registered for the defaults mirror the implementations on SystemTextJsonSerializer and XmlRestSerializer, so serializer resolution behavior is unchanged.

Verification

  • Reproduced the reported failure on the dev branch with a net48 probe that removes System.Text.Json.dll from the output directory and hooks AssemblyResolve: constructing a client with UseOnlySerializer triggers RESOLVE-FAILED: System.Text.Json and exits with code 2.
  • With the change applied, the same probe prints OK: client constructed without loading the assembly.
  • Existing serializer configuration tests pass:
    • RestSharp.Tests net8.0 serializer filter: 4/4 passed.
    • RestSharp.Tests.Serializers.Json net8.0 full suite: 13/13 passed.

Fixes #2402

… them without loading System.Text.Json

Fixes restsharp#2402

Co-authored-by: FirmamentalSpring <287222957+FirmaSpring@users.noreply.github.com>
@sonarqubecloud

Copy link
Copy Markdown

@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Lazy-register default serializers so custom JSON serializers avoid loading System.Text.Json

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Register default JSON/XML serializers via factories to defer instantiation until first use.
• Allow configureSerialization overrides to replace defaults before any System.Text.Json loading.
• Add tests ensuring NewtonsoftJson/custom serializers replace defaults without breaking XML
 registration.
Diagram

graph TD
  A["RestClient"] --> B["SerializerConfig"] --> C["SerializerRecord factories"] --> E["RestSerializers.GetSerializer"] --> F["IRestSerializer instance"]
  B --> D["configureSerialization"] --> C
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Cache instantiated default serializers (Lazy)
  • ➕ Avoids creating multiple serializer instances if GetSerializer is called repeatedly
  • ➕ Keeps lazy-load benefit while stabilizing per-client serializer identity
  • ➖ Adds synchronization/complexity to SerializerRecord storage
  • ➖ Potentially changes subtle expectations if callers rely on fresh instances
2. Late-bind System.Text.Json via reflection/Type.GetType
  • ➕ Can avoid hard reference paths in some plugin/assembly-binding scenarios
  • ➕ Defers both type resolution and instantiation until actually needed
  • ➖ More complex and less type-safe
  • ➖ Worse debuggability and potential runtime failures when types move/rename

Recommendation: The PR’s factory-based registration is the simplest way to prevent eager System.Text.Json loading while preserving existing serializer resolution behavior. Consider caching only if repeated instantiation becomes a measurable issue; otherwise keep the current approach for minimal risk and complexity.

Files changed (3) +50 / -1

Bug fix (1) +17 / -1
SerializerConfig.csRegister default JSON/XML serializers via lazy factories +17/-1

Register default JSON/XML serializers via lazy factories

• Changes UseDefaultSerializers to store SerializerRecord entries with serializer factories instead of eagerly constructing SystemTextJsonSerializer/XmlRestSerializer. Duplicates the default AcceptedContentTypes and SupportsContentType logic to keep serializer resolution behavior unchanged while deferring System.Text.Json loading.

src/RestSharp/Serializers/SerializerConfig.cs

Tests (2) +33 / -0
IntegratedSimpleTests.csAdd test asserting NewtonsoftJson replaces the default JSON serializer +8/-0

Add test asserting NewtonsoftJson replaces the default JSON serializer

• Adds an integration test ensuring cfg.UseNewtonsoftJson() results in JsonNetSerializer being used for DataFormat.Json. Also asserts the XML serializer registration remains present.

test/RestSharp.Tests.Serializers.Json/NewtonsoftJson/IntegratedSimpleTests.cs

RestClientTests.csAdd regression test for lazy default registration with UseOnlySerializer +25/-0

Add regression test for lazy default registration with UseOnlySerializer

• Adds a test that configures RestClient with UseOnlySerializer and verifies only the custom JSON serializer is present and resolved. Introduces a minimal JsonNetSerializerStub to validate replacement without depending on SystemTextJsonSerializer instantiation.

test/RestSharp.Tests/RestClientTests.cs

@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Duplicated default matching rules 🐞 Bug ⚙ Maintainability
Description
SerializerConfig.UseDefaultSerializers now hard-codes the default JSON/XML AcceptedContentTypes and
SupportsContentType predicates, duplicating logic that already exists on SystemTextJsonSerializer
and XmlRestSerializer. This creates a second source of truth, so future changes to serializer
matching behavior can silently diverge from the defaults registered in SerializerConfig and lead to
inconsistent serializer resolution.
Code

src/RestSharp/Serializers/SerializerConfig.cs[R45-48]

+            DataFormat.Json,
+            ContentType.JsonAccept,
+            contentType => contentType.Value.EndsWith("json", StringComparison.InvariantCultureIgnoreCase),
+            static () => new SystemTextJsonSerializer()
Evidence
The PR adds hard-coded JSON/XML AcceptedContentTypes and SupportsContentType lambdas in
SerializerConfig.UseDefaultSerializers. The same matching logic already exists in the concrete
serializer implementations, so this change introduces duplication and potential drift between
registration-time metadata and serializer behavior.

src/RestSharp/Serializers/SerializerConfig.cs[40-56]
src/RestSharp/Serializers/Json/SystemTextJsonSerializer.cs[39-46]
src/RestSharp/Serializers/Xml/XmlRestSerializer.cs[23-28]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`SerializerConfig.UseDefaultSerializers()` duplicates the default serializers' `AcceptedContentTypes` and `SupportsContentType` logic. This increases drift risk: if `SystemTextJsonSerializer` or `XmlRestSerializer` updates its matching rules/content types later, the defaults registered in `SerializerConfig` may no longer match actual serializer behavior.

## Issue Context
The PR intentionally avoids constructing serializers in `UseDefaultSerializers()` to prevent eager loading of `System.Text.Json`. A fix should preserve that laziness while keeping matching metadata in one place.

## Fix approach (keep lazy loading)
- Introduce a shared, dependency-free helper (e.g., internal static class or methods on `ContentType`) that defines:
 - `JsonAcceptedContentTypes` / `XmlAcceptedContentTypes` (can just reference `ContentType.JsonAccept` / `ContentType.XmlAccept`), and
 - `SupportsJson(ContentType ct)` / `SupportsXml(ContentType ct)` predicates.
- Update both `SerializerConfig.UseDefaultSerializers()` and the serializer classes’ `SupportsContentType` implementations to call the shared helper.

## Fix Focus Areas
- src/RestSharp/Serializers/SerializerConfig.cs[40-55]
- src/RestSharp/Serializers/Json/SystemTextJsonSerializer.cs[39-46]
- src/RestSharp/Serializers/Xml/XmlRestSerializer.cs[23-28]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment on lines +45 to +48
DataFormat.Json,
ContentType.JsonAccept,
contentType => contentType.Value.EndsWith("json", StringComparison.InvariantCultureIgnoreCase),
static () => new SystemTextJsonSerializer()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Duplicated default matching rules 🐞 Bug ⚙ Maintainability

SerializerConfig.UseDefaultSerializers now hard-codes the default JSON/XML AcceptedContentTypes and
SupportsContentType predicates, duplicating logic that already exists on SystemTextJsonSerializer
and XmlRestSerializer. This creates a second source of truth, so future changes to serializer
matching behavior can silently diverge from the defaults registered in SerializerConfig and lead to
inconsistent serializer resolution.
Agent Prompt
## Issue description
`SerializerConfig.UseDefaultSerializers()` duplicates the default serializers' `AcceptedContentTypes` and `SupportsContentType` logic. This increases drift risk: if `SystemTextJsonSerializer` or `XmlRestSerializer` updates its matching rules/content types later, the defaults registered in `SerializerConfig` may no longer match actual serializer behavior.

## Issue Context
The PR intentionally avoids constructing serializers in `UseDefaultSerializers()` to prevent eager loading of `System.Text.Json`. A fix should preserve that laziness while keeping matching metadata in one place.

## Fix approach (keep lazy loading)
- Introduce a shared, dependency-free helper (e.g., internal static class or methods on `ContentType`) that defines:
  - `JsonAcceptedContentTypes` / `XmlAcceptedContentTypes` (can just reference `ContentType.JsonAccept` / `ContentType.XmlAccept`), and
  - `SupportsJson(ContentType ct)` / `SupportsXml(ContentType ct)` predicates.
- Update both `SerializerConfig.UseDefaultSerializers()` and the serializer classes’ `SupportsContentType` implementations to call the shared helper.

## Fix Focus Areas
- src/RestSharp/Serializers/SerializerConfig.cs[40-55]
- src/RestSharp/Serializers/Json/SystemTextJsonSerializer.cs[39-46]
- src/RestSharp/Serializers/Xml/XmlRestSerializer.cs[23-28]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RestClient configured with Newtonsoft.Json serializer still instantiates System.Text.Json

1 participant