BrighterCommand / BrighterCommand/Brighter

ConfigureJsonSerialisation mutates the shared JsonSerializerOptions with no read-only guard

Open
#4,329 0 comments 0 reactions 1 assignee Claimed by @iancooper View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.