Produce deprecated property in JsonSchema for obsolete types#130665
Produce deprecated property in JsonSchema for obsolete types#130665Youssef1313 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the System.Text.Json JSON Schema exporter to emit the JSON Schema deprecated keyword when exporting schemas for [Obsolete] .NET types, and adds a regression test covering the new behavior.
Changes:
- Detect
[Obsolete]on exported types and setdeprecated: truein the generated schema. - Add
Deprecatedsupport to the internalJsonSchemamodel and include it in JSON output. - Add a unit test validating
deprecatedis emitted for an obsolete type.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/libraries/System.Text.Json/tests/Common/JsonSchemaExporterTests.cs | Adds a test case validating deprecated is produced for an [Obsolete] type. |
| src/libraries/System.Text.Json/src/System/Text/Json/Schema/JsonSchemaExporter.cs | Marks exported schemas as deprecated when the associated .NET type has [Obsolete]. |
| src/libraries/System.Text.Json/src/System/Text/Json/Schema/JsonSchema.cs | Adds a Deprecated keyword to the schema model, counts it, and emits it during JSON serialization. |
2e895bf to
a5d6c2d
Compare
a5d6c2d to
0460d8b
Compare
|
Tagging subscribers to this area: @dotnet/area-system-text-json |
0460d8b to
76a2de7
Compare
76a2de7 to
3c313d5
Compare
| public int? MaxLength { get => _maxLength; set { VerifyMutable(); _maxLength = value; } } | ||
| private int? _maxLength; | ||
|
|
||
| public bool Deprecated { get => _deprecated; set { VerifyMutable(); _deprecated = value; } } |
There was a problem hiding this comment.
Other keywords model both all possible values plus the absense of a value. Correspondingly, this should be bool? and we should include it in the generated schema document if non-null.
There was a problem hiding this comment.
I'd argue it should only be serialized when explicitly set to true, and ignored otherwise, based on the keyword definition
Omitting this keyword has the same behavior as a value of false.
There was a problem hiding this comment.
It's an internal model. So we could make it nullable bool, but I think we will never set it to false. So it will either be true (serialized with value true) or will be null (not serialized at all).
This will be behaviourly identical to what I have now with it being a bool that's only serialized when true.
There was a problem hiding this comment.
I don't disagree, but that should be the decision of the mapping layer. The model itself should be neutral in representing all possible states.
|
|
||
| private static bool HasObsoleteAttribute(ICustomAttributeProvider? attributeProvider) | ||
| { | ||
| if (attributeProvider is null) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Identify ObsoleteAttribute using its full type name rather than typeof(ObsoleteAttribute). | ||
| // On downlevel targets System.Text.Json compiles in an internal ObsoleteAttribute polyfill | ||
| // that would otherwise shadow the framework type, causing the typeof comparison to never match | ||
| // the ObsoleteAttribute applied by user code. | ||
| foreach (object attribute in attributeProvider.GetCustomAttributes(inherit: true)) | ||
| { | ||
| if (attribute.GetType().FullName == "System.ObsoleteAttribute") | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
| AssertDeprecated(properties["MyString"], expectedDeprecated: false); | ||
|
|
||
| // Property annotated with [Obsolete] reports deprecated. | ||
| AssertDeprecated(properties["MyObsoleteString"], expectedDeprecated: true); |
There was a problem hiding this comment.
I would recommend having the test asserting the exact shape of the generated schema, rather than interrogating individual aspects of it. There's a built-in theory you can use to drive this comparison.
|
|
||
| JsonSchema CompleteSchema(ref GenerationState state, JsonSchema schema) | ||
| { | ||
| if (HasObsoleteAttribute(typeInfo.Type) || |
There was a problem hiding this comment.
If the type itself is marked obsolete, wouldn't this result in every single contained property also being marked obsolete?
Spec: https://json-schema.org/draft/2020-12/draft-bhutton-json-schema-validation-01#section-9.3