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

Expose ElementType and KeyType on JsonTypeInfo metadata. #77306

Closed
Tracked by #71967 ...
krwq opened this issue Oct 21, 2022 · 11 comments · Fixed by #102902
Closed
Tracked by #71967 ...

Expose ElementType and KeyType on JsonTypeInfo metadata. #77306

krwq opened this issue Oct 21, 2022 · 11 comments · Fixed by #102902
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json partner-impact This issue impacts a partner who needs to be kept updated
Milestone

Comments

@krwq
Copy link
Member

krwq commented Oct 21, 2022

public class partial JsonTypeInfo
{
    public Type? KeyType { get; } // relevant for JsonTypeInfoKind.Dictionary
    public Type? ElementType { get; } // relevant for JsonTypeInfoKind.Enumerable/Dictionary
}

These APIs are useful for querying the model, i.e.: generating docs, schema. It's not exactly trivial to implement them but we already store this data internally.

cc: @captainsafia

@krwq krwq added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json labels Oct 21, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 21, 2022
@ghost
Copy link

ghost commented Oct 21, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details
public class partial JsonTypeInfo
{
    public Type KeyType { get; } // relevant for JsonTypeInfoKind.Dictionary
    public Type ElementType { get; } // relevant for JsonTypeInfoKind.Enumerable/Dictionary
}

These APIs are useful for querying the model, i.e.: generating docs, schema. It's not exactly trivial to implement them but we already store this data internally.

cc: @captainsafia

Author: krwq
Assignees: -
Labels:

api-suggestion, area-System.Text.Json

Milestone: -

@krwq
Copy link
Member Author

krwq commented Oct 21, 2022

consider how this interacts with custom element converters and element polymorphic serialization

@eiriktsarpalis
Copy link
Member

Should these be nullable?

@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Oct 21, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Oct 21, 2022
@krwq krwq added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Oct 21, 2022
@krwq krwq self-assigned this Oct 21, 2022
@jeffhandley jeffhandley added the partner-impact This issue impacts a partner who needs to be kept updated label Nov 21, 2022
@krwq krwq changed the title Expose ElementType and KeyType Expose ElementType and KeyType (converters) Jan 25, 2023
@eiriktsarpalis eiriktsarpalis added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 23, 2023
@eiriktsarpalis
Copy link
Member

@captainsafia is this blocking you for .NET 8? We're moving it to Future if not.

@eiriktsarpalis eiriktsarpalis modified the milestones: 8.0.0, Future May 31, 2023
@captainsafia
Copy link
Member

@eiriktsarpalis It's no longer blocking for .NET 8 so it's sufficient to move it back to Future.

@krwq krwq removed their assignment Jun 7, 2023
@eiriktsarpalis eiriktsarpalis self-assigned this Mar 13, 2024
@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 9.0.0 Mar 13, 2024
@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Mar 13, 2024
@eiriktsarpalis eiriktsarpalis changed the title Expose ElementType and KeyType (converters) Expose ElementType and KeyType on JsonTypeInfo metadata. Mar 13, 2024
@bartonjs
Copy link
Member

bartonjs commented Mar 19, 2024

Video

Looks good as proposed.

public partial class JsonTypeInfo
{
    public Type? KeyType { get; } // relevant for JsonTypeInfoKind.Dictionary
    public Type? ElementType { get; } // relevant for JsonTypeInfoKind.Enumerable/Dictionary
}

@bartonjs bartonjs removed the blocking Marks issues that we want to fast track in order to unblock other important work label Mar 19, 2024
@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Mar 19, 2024
@Neme12
Copy link

Neme12 commented Mar 19, 2024

I don't have much knowledge about this area, but I wanted to ask anyway: shouldn't those properties return JsonTypeInfo? That's what I would expect if I saw those properties.

@Neme12
Copy link

Neme12 commented Mar 19, 2024

Also, given the previous API review about TypeName... in that case, it was decided that rather than having nullable properties (like ElementType etc), those would be methods instead that would throw if called on the wrong type. @terrajobst mentioned that is better because it would be stupid if someone checked that the type is an array but would then have to check if the ElementType is null anyway after having checked that it's an array. Is there a reason why the consideration is different here?

Maybe the same argument applies here as well, because people will check whether the JsonTypeInfoKind is Dictionary or Enumerable anyway before getting the KeyType or ElementType?

@eiriktsarpalis
Copy link
Member

shouldn't those properties return JsonTypeInfo?

No. The simple reason is that resolving the JsonTypeInfo for each of the element types and key types is delayed and happens only after the instance has been made read-only and its caches are being configured. We want these properties to be consistent regardless of what the serializer is doing under the hood.

Maybe the same argument applies here as well, because people will check whether the JsonTypeInfoKind is Dictionary or Enumerable anyway before getting the KeyType or ElementType?

The current behavior is consistent with the other public APIs on JsonTypeInfo. We only throw in property setters where the kind doesn't match.

@Neme12
Copy link

Neme12 commented Mar 22, 2024

If it's locally consistent, then I understand. Although now I'm confused when you said:

We only throw in property setters where the kind doesn't match.

Isn't this the same thing? The property would throw if the kind doesn't match (disregarding the fact that properties shouldn't throw at all, and if it needs to, it should be a method instead. But you said there's prior art for this).

@eiriktsarpalis
Copy link
Member

We only throw in property setters where the kind doesn't match.

Isn't this the same thing? The property would throw if the kind doesn't match.

There are no setters in the new properties.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json partner-impact This issue impacts a partner who needs to be kept updated
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants