BrighterCommand / BrighterCommand/Brighter
ConfigureJsonSerialisation mutates the shared JsonSerializerOptions with no read-only guard
- Dominant language
- C#
- Stars
- 2.5k
- Forks
- 296
- Avg merge
- 1d 11h
- Merged PRs (30d)
- 21
Description
## Summary
`ConfigureJsonSerialisation` mutates Brighter's process-global `JsonSerializerOptions` with **no guard
against the instance already being read-only**. Once anything in the process has serialized through
`JsonSerialisationOptions.Options`, System.Text.Json freezes it and *every* mutation throws — so the
user's configure callback throws `InvalidOperationException` out of their DI setup.
```csharp
// src/Paramore.Brighter.Extensions.DependencyInjection/ServiceCollectionExtensions.cs:788-793
public static IBrighterBuilder ConfigureJsonSerialisation(this IBrighterBuilder brighterBuilder,
Action configure)
{
configure.Invoke(JsonSerialisationOptions.Options);
return brighterBuilder;
}
```
This is the same design flaw as #4327 (fixed for the JustSaying adaptor by #4328), but in core and
**without even the `IsReadOnly` check** that #4327 had.
## Why it matters
It is not only `Converters.Add` that throws. Plain property setters do too — verified on net9.0:
```
IsReadOnly=True
WriteIndented THREW: InvalidOperationException: This JsonSerializerOptions instance is read-only or has already been used in serialization or deserialization.
NamingPolicy THREW: InvalidOperationException: This JsonSerializerOptions instance is read-only or has already been used in serialization or deserialization.
```
So *any* realistic callback fails, including the one in our own samples
(`samples/WebAPI/WebAPI_Dynamo/SalutationAnalytics/Program.cs:110`, which sets
`PropertyNameCaseInsensitive`) and the one in our own test (`WriteIndented`).
It usually works today only because `ConfigureJsonSerialisation` normally runs during DI registration,
before anything has serialized. It breaks when that ordering does not hold, for example:
- the host serializes anything through `JsonSerialisationOptions.Options` during startup before
`AddBrighter(...)` runs;
- a second Brighter registration, or a re-registration, later in the process lifetime;
- test hosts that build a service provider more than once in one process.
## Related: the existing test is vulnerable to the same ordering
`tests/Paramore.Brighter.Extensions.Tests/When_configuring_json_serialisation.cs` mutates
`WriteIndented` through the real extension method. It passes today only because nothing else in that
assembly has serialized first. xUnit runs test classes in parallel, so this is a latent flake of
exactly the family that produced #4327 — where the same pattern flaked intermittently in CI for over
a year before being diagnosed.
## Suggested direction
The underlying problem is that `JsonSerialisationOptions.Options` is a mutable, publicly settable,
process-global that everything shares (170 references across 70 files in `src/`). Options worth
considering, roughly in increasing order of ambition:
1. **Fail loudly and early.** Throw a Brighter exception explaining that serialisation must be
configured before first use, rather than surfacing a bare System.Text.Json message. Cheapest, and
at least makes the constraint discoverable.
2. **Copy-on-configure.** Build a new `JsonSerializerOptions` from the current one, apply `configure`
to the copy, and assign it back to `JsonSerialisationOptions.Options`. Copy-constructing from a
read-only instance is always legal — this is what #4328 relies on. Callers that captured the old
instance would keep using it, which needs thought.
3. **Stop sharing a mutable global.** Make the options an injected, immutable dependency built once
during configuration. Correct, and much the largest change.
Note that (2) interacts with the public setter on `JsonSerialisationOptions.Options`: anything that
cached the previous instance silently keeps the old settings. That setter probably deserves review in
its own right.
## Not urgent, but not theoretical
No known production report — this is a latent defect found while diagnosing #4327. Raising it so it is
not lost, and because #4327 demonstrates this failure mode can hide for a long time and then present
as an unexplained intermittent CI failure.
Contributor guide
Assessment
This issue has not been assessed yet.