-
Notifications
You must be signed in to change notification settings - Fork 425
Replace JsonWebToken ReadPayloadValue with a delegate #2981
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
Conversation
3613363
to
3cbc31c
Compare
Edit: These results are outdated. Looks like when using delegates there're extra allocations because of: string claimName = reader.GetString();
claims[claimName] = ReadTokenPayloadValueDelegate(ref reader, claimName);
|
3cbc31c
to
4d5347b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 9 changed files in this pull request and generated no comments.
Files not reviewed (4)
- src/Microsoft.IdentityModel.JsonWebTokens/InternalAPI.Unshipped.txt: Language not supported
- src/Microsoft.IdentityModel.Tokens/InternalAPI.Unshipped.txt: Language not supported
- test/Microsoft.IdentityModel.JsonWebTokens.Tests/CustomJsonWebToken.cs: Evaluated as low risk
- src/Microsoft.IdentityModel.JsonWebTokens/Json/JsonWebToken.PayloadClaimSet.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)
src/Microsoft.IdentityModel.JsonWebTokens/Json/JsonClaimSet.cs:31
- The initialization of _jsonClaims should be 'new Dictionary<string, object>()' instead of '[]'.
_jsonClaims = [];
Edit: These results are outdated. Updated results comparing delegates which have a dictionary as a parameter. Ran JsonWebTokenHandler_ValidateTokenAsyncWithTVP and ReadJWS_FromMemory with extended claims. These test using the default delegate implemenation.
BenchmarkDotNet v0.13.12, Windows 11 (10.0.26100.2605) (Hyper-V) |
add645b
to
89a8c8a
Compare
89a8c8a
to
442baa3
Compare
e810ae2
to
aecf790
Compare
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
src/Microsoft.IdentityModel.JsonWebTokens/Json/JsonWebToken.PayloadClaimSet.cs
Outdated
Show resolved
Hide resolved
It would be great to have the same design when reading the header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a way to allow the user to parse all claims in a performant manner.
This design skips claims that are parsed.
Allowing the user to parse all claims allows for an implementation where validation could fail fast.
Failing fast needs to have a design that can propagate a detailed error result to upper layers for error reporting.
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
Edit: These results are outdated. See update in the most recent post. The three scenarios compared below:
|
Edit: These results are outdated. See update in the most recent post. The difference in code from the previous perf results is that this time the delegates are null by default (and not set to new dictionary) which reduced the allocations from the previous run. Latest commit: 3c2fbbc The three scenarios compared below:
|
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
Edit: These results are outdated. See update in the most recent post. The difference from the previous commit is that there's now one delegate instead of a dictionary. The three scenarios compared below:
|
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
|
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- src/Microsoft.IdentityModel.JsonWebTokens/PublicAPI/net6.0/InternalAPI.Unshipped.txt: Language not supported
Comments suppressed due to low confidence (1)
src/Microsoft.IdentityModel.JsonWebTokens/Json/JsonClaimSet.cs:38
- [nitpick] The dictionary initialization using '[]' deviates from the conventional explicit syntax. If this new collection expression syntax for dictionaries is intended for the target framework version, please ensure that it is clear and supported; otherwise, revert to 'new Dictionary<string, object>()' for clarity and compatibility.
_jsonClaims = [];
src/Microsoft.IdentityModel.JsonWebTokens/Json/JsonWebToken.PayloadClaimSet.cs
Show resolved
Hide resolved
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
src/Microsoft.IdentityModel.JsonWebTokens/Json/JsonWebToken.HeaderClaimSet.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few small questions, but otherwise LGTM
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
Fixes #2982
Updated perf results: #2981 (comment)
This pull request introduces several updates and improvements to the
Microsoft.IdentityModel.JsonWebTokens
library, focusing on enhancing the handling of token payloads and custom claims. The most important changes include adding new constructors to theJsonWebToken
class, updating theJsonClaimSet
initialization, and introducing a new delegate for reading custom token payload values.Enhancements to
JsonWebToken
:JsonWebToken
class to support initializing tokens with custom delegates for reading token payload values. (src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs
) [1] [2] [3]ReadTokenPayloadValueDelegates
property to theJsonWebToken
class, allowing custom handling of specific claim names during token reading. (src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs
)Updates to
TokenValidationParameters
:ReadTokenPayloadValueDelegates
property to theTokenValidationParameters
class to support custom claim handling during token validation. (src/Microsoft.IdentityModel.Tokens/TokenValidationParameters.cs
) [1] [2]TokenValidationParameters
to include the newReadTokenPayloadValueDelegates
property. (src/Microsoft.IdentityModel.Tokens/TokenValidationParameters.cs
)Refactoring and cleanup:
ReadPayloadValue
method inJsonWebToken.PayloadClaimSet
to simplify the handling of standard claims and integrate custom delegate handling. (src/Microsoft.IdentityModel.JsonWebTokens/Json/JsonWebToken.PayloadClaimSet.cs
) [1] [2]CustomJsonWebToken
class from the tests, as its functionality is now covered by the new delegate-based approach. (test/Microsoft.IdentityModel.JsonWebTokens.Tests/CustomJsonWebToken.cs
)Delegate introduction:
ReadTokenPayloadValueDelegate
delegate to handle custom claim reading during token payload processing. (src/Microsoft.IdentityModel.Tokens/Delegates.cs
)