Fix transformer handling of boolean schemas in JsonSchemaExporter.#109954
Fix transformer handling of boolean schemas in JsonSchemaExporter.#109954eiriktsarpalis merged 2 commits intodotnet:mainfrom
Conversation
eiriktsarpalis
requested review from
PranavSenthilnathan,
jozkee and
stephentoub
ghost
added
the
src/libraries/System.Text.Json/src/System/Text/Json/Schema/JsonSchema.cs
Outdated
Show resolved
Hide resolved
| public static JsonSchema False { get; } = new(false); | ||
| public static JsonSchema True { get; } = new(true); | ||
| public static JsonSchema CreateFalseSchema() => new(false); | ||
| public static JsonSchema CreateTrueSchema() => new(true); |
There was a problem hiding this comment.
And we don't just want to use new(true) at call sites?
There was a problem hiding this comment.
The constructor accepting boolean is marked private since boolean schemas are special and do not admit keywords. It's better if the false and true schemas are created explicitly. If I were to redesign this type today, I would have made booleans and objects separate subclasses -- the current arrangement has been the source of most bugs we've seen in JsonSchemaExporter.
There was a problem hiding this comment.
The constructor accepting boolean is marked private since boolean schemas are special and do not admit keywords.
But aren't these factories now 100% equivalent to the constructor? If it's ok for these factories to exist, I don't understand why the constructor is any more problematic.
There was a problem hiding this comment.
I don't think JsonSchema schema = new(false); adequately conveys that a false schema is being created. I would rather assign a distinct factory for that case.
|
/backport to release/9.0-staging |
|
Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/11920223361 |
…otnet#109954) * Fix transformer handling of boolean schemas in JsonSchemaExporter. * Address feedback.
2 participants
The
JsonSchemaExporterimplementation uses singletons to represent boolean schema nodes, however this approach does not account for the possibility of a transformer context registering itself to the singleton. Should be backported to 9.Fix #109868.