-
Notifications
You must be signed in to change notification settings - Fork 432
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:
|
Fixes #2982
Updated perf results: #2981 (comment)
This pull request introduces several updates and improvements to the
Microsoft.IdentityModel.JsonWebTokenslibrary, focusing on enhancing the handling of token payloads and custom claims. The most important changes include adding new constructors to theJsonWebTokenclass, updating theJsonClaimSetinitialization, and introducing a new delegate for reading custom token payload values.Enhancements to
JsonWebToken:JsonWebTokenclass to support initializing tokens with custom delegates for reading token payload values. (src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs) [1] [2] [3]ReadTokenPayloadValueDelegatesproperty to theJsonWebTokenclass, allowing custom handling of specific claim names during token reading. (src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs)Updates to
TokenValidationParameters:ReadTokenPayloadValueDelegatesproperty to theTokenValidationParametersclass to support custom claim handling during token validation. (src/Microsoft.IdentityModel.Tokens/TokenValidationParameters.cs) [1] [2]TokenValidationParametersto include the newReadTokenPayloadValueDelegatesproperty. (src/Microsoft.IdentityModel.Tokens/TokenValidationParameters.cs)Refactoring and cleanup:
ReadPayloadValuemethod inJsonWebToken.PayloadClaimSetto simplify the handling of standard claims and integrate custom delegate handling. (src/Microsoft.IdentityModel.JsonWebTokens/Json/JsonWebToken.PayloadClaimSet.cs) [1] [2]CustomJsonWebTokenclass from the tests, as its functionality is now covered by the new delegate-based approach. (test/Microsoft.IdentityModel.JsonWebTokens.Tests/CustomJsonWebToken.cs)Delegate introduction:
ReadTokenPayloadValueDelegatedelegate to handle custom claim reading during token payload processing. (src/Microsoft.IdentityModel.Tokens/Delegates.cs)