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

[Feature Request] Add an override of TryGetClaimValue with a JsonTypeInfo parameter #2255

Open
jmprieur opened this issue Aug 24, 2023 · 7 comments
Labels
Internal Indicates issue was opened by the IdentityModel team

Comments

@jmprieur
Copy link
Contributor

jmprieur commented Aug 24, 2023

Is your feature request related to a problem? Please describe.
Developers might want to deserialize claims in ways we can't anticipate, and are ready to direct the .NET 7/8 compiler to generate the code for the serializers/deserializers.

Describe the solution you'd like
Have an override of JsonWebToken.TryGetClaim that takes a JsonTypeInfo parameter in addition to the name of the claim and the out parameter

bool JsonWebToken.TryGetClaimValue<TValue>(string claimName, JsonTypeInfo<TValue> jsonTypeInfo, out TValue result)

Describe alternatives you've considered
Trying to guess all the types that need to be deserialized

Additional context
The only way, today, to make JsonSerializer work with trimming/AOT is to use the JSON source generator - which produces these JsonTypeInfo classes. Then when you need to serialize/deserialize to/from objects, you pass the correct JsonTypeInfo you got from the source generator into the API.
Any call to JsonSerializer that doesn't pass the JsonTypeInfo will result in trimming and AOT warnings (and probably will fail at runtime when you AOT your app)

Resources:

@jmprieur jmprieur changed the title [Feature Request] Add an override of TryGetClaim with a JsonTypeInfo parameter [Feature Request] Add an override of TryGetClaimValue with a JsonTypeInfo parameter Aug 24, 2023
@jennyf19 jennyf19 added this to the 7.0.0-preview5 milestone Aug 24, 2023
@kevinchalet
Copy link
Contributor

Note: this scenario is already possible/supported using something like this:

if (token.TryGetPayloadValue("claim", out JsonElement element) &&
    element.Deserialize<ClrType>(jsonTypeInfo))
{
    // ...
}

@brentschmaltz
Copy link
Member

@kevinchalet we currently only keep the JsonElement when we hit a JsonArray or JsonObject, all others are deserialized into C# types.

@brentschmaltz
Copy link
Member

@keegan-caruso if this was available when the token was being deserialized the type would be created at read time and the cpu cycles used to create the JsonElement then back to the type would be avoided.

@kevinchalet
Copy link
Contributor

@kevinchalet we currently only keep the JsonElement when we hit a JsonArray or JsonObject, all others are deserialized into C# types.

Wait, really, @brentschmaltz? That sounds like a weird design to me. Any particular reason why not everything can be represented as a JsonElement? It will make using TryGetPayloadValue() unnecessarily painful to use 😱

@brentschmaltz
Copy link
Member

@kevinchalet There are performance reasons why we don't want to store everything as a JsonElement.
When we need to validate a token, we will need to process the json.
Consider the validating the issuer, we are already sitting on the iss property.

We could easily have a switch where we could store the JsonElement.

@kevinchalet
Copy link
Contributor

@kevinchalet There are performance reasons why we don't want to store everything as a JsonElement.

JsonElement in itself is a just a struct pointer to a node in a JsonDocument that doesn't materialize the actual value as a CLR instance so it's very cheap, but it's likely more overhead than just working with Utf8JsonReader, indeed.

When we need to validate a token, we will need to process the json.
Consider the validating the issuer, we are already sitting on the iss property.

I had not realized you were materializing all the claims as strongly-typed CLR objects upfront: when we mentioned JsonElement, I thought all this stuff was delayed until each node/claim accessed.

Now, this raises a question regarding the API suggested by @jmprieur: won't you end up deserializing claims twice with this API? (once by JsonClaimSet and a second time by the call to TryGetClaimValue<T>)?

@brentschmaltz
Copy link
Member

@kevinchalet when we are processing the json (base64urldecoding, reading, etc...) we are working in an ArrayPool buffer and we want to get as much work done as possible.

The reason for the TryGetClaimValue with JsonTypeInfo is so that a user could obtain a complicated custom type.
AOT support prevents us from using JsonSerializer as that requires reflection.

We only support a limited number of types: Collection, Dictionary<string, string> etc...
We are looking for a natural way for a users to have extended control.

@jmprieur jmprieur removed this from the 7.0.0-preview5 milestone Aug 29, 2023
@brentschmaltz brentschmaltz added the Internal Indicates issue was opened by the IdentityModel team label Sep 21, 2023
@brentschmaltz brentschmaltz added this to the December refresh milestone Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Indicates issue was opened by the IdentityModel team
Projects
None yet
Development

No branches or pull requests

4 participants