Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properties for a class that extends Dictionary are not present in the JSON schema #56707

Closed
1 task done
mikekistler opened this issue Jul 9, 2024 · 10 comments
Closed
1 task done
Labels
area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-openapi

Comments

@mikekistler
Copy link
Contributor

mikekistler commented Jul 9, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

The JSON schema produced for a class typically has properties for each public class property. But if the class is defined to extend a Dictionary, then the schema only defines the additionalProperties and not the named properties.

Expected Behavior

Class properties should be represented in the JSON schema for a class as properties even if the class extends a Dictionary.

Steps To Reproduce

The "extends-dict" request and response body of my aspnet-openapi-examples project illustrate the problem.

Use branch bug-56707.

Exceptions (if any)

No response

.NET Version

9.0.100-preview.7.24358.8

Anything else?

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Jul 9, 2024
@captainsafia
Copy link
Member

@eiriktsarpalis It strikes me that this is something that we might want to resolve in the JsonSchemaExporter layer? So far, I've gotten bug reports about two things in relation to additionalProperties:

  • The scenario @mikekistler shared here around classes that extend Dictionary<,>.
  • The scenario where properties annotated with [JsonExtensionData] don't seem to produce a schema with additionalProperties.

Here's the behavior I am seeing:

class ExtendsDictionary : Dictionary<string, Guid>
{
    public required string Name { get; set; }
    public required bool IsEnabled { get; set; }
}

// {"type":["object","null"],"additionalProperties":{"type":"string","format":"uuid"}}

class TypeWithExtensionData
{
    public required string Name { get; set; }
    public required bool IsEnabled { get; set; }

    [JsonExtensionData]
    public Dictionary<string, object>? AdditionalData { get; set; }

}

// {"type":["object","null"],"properties":{"Name":{"type":"string"},"IsEnabled":{"type":"boolean"}},"required":["Name","IsEnabled"]}

You can find a repro with the current behavior over at https://github.com/captainsafia/tests/tree/jschema-addl-props.

@eiriktsarpalis
Copy link
Member

Isn't this just reflecting the behaviour of the serializer though? Properties on types implementing IEnumerable are always being ignored since the serializer uses the data provided from the enumeration to populate the JSON properties.

@captainsafia
Copy link
Member

Isn't this just reflecting the behaviour of the serializer though? Properties on types implementing IEnumerable are always being ignored since the serializer uses the data provided from the enumeration to populate the JSON properties.

I suppose that makes sense for the ExtendsDictionary scenario, but what about JsonExtensionData?

@eiriktsarpalis
Copy link
Member

What would be the expected schema for types defining extension data? It's just a property bag accumulating unbounded properties so it shouldn't have any impact from a schematization perspective.

@captainsafia
Copy link
Member

What would be the expected schema for types defining extension data? It's just a property bag accumulating unbounded properties so it shouldn't have any impact from a schematization perspective.

Mmmm...I think I've realized the gap for me here. I had assumed that additionalProperties was false by default in all schemas. But the spec indicates otherwise.

@mikekistler
Copy link
Contributor Author

I am surprised by this behavior of the STJ serializer:

image

Why are Title and Priority not included in the serialized object?

@eiriktsarpalis
Copy link
Member

Why are Title and Priority not included in the serialized object?

It's the same reason why a Count property doesn't show when you serialize a dictionary. A type can be serialized using an object view or an IEnumerable view but not both.

@mikekistler
Copy link
Contributor Author

Please excuse the newbie question, but why? This is clearly losing data. Don't we want the serialized form to contain all the information from the C# object?

@eiriktsarpalis
Copy link
Member

Don't we want the serialized form to contain all the information from the C# object?

That's almost never the case. For example, STJ doesn't serialize non-public members and it doesn't members defined on the runtime type. This is simply a matter of what view is being adopted when deriving a contract for a particular type. Arrays, List<T> and Dictionary<TKey,TValue> all expose public properties however we do not consider them to be part of the data contract and use their enumerations instead. This is done for a number of reasons:

  1. Principle of least surprise; avoid emitting metadata properties such as Length, Count, Keys and Values on the wire.
  2. Hard to reconcile schematization of properties of array-like collection types that serialize as JSON arrays.
  3. Ambiguity when deserializing: in the cited example should the JSON property "IsEnabled" : "00000000-0000-0000-0000-000000000000" fail deserialization or be appended as an entry to the underlying dictionary?
  4. Simplicity and performance. The built-in object converters use very different implementations compared to the collection converters and attempting to reconcile both into one is bound to make things more difficult to maintain and slower.

This is reflected by the JsonTypeInfoKind enumeration. Every nontrivial JSON contract can either be an object, enumerable or dictionary and these kinds are discrete.

@mikekistler
Copy link
Contributor Author

I guess this is "working as designed". I will check the docs to make sure it's clear there and open an issue if not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-openapi
Projects
None yet
Development

No branches or pull requests

3 participants