Add ReadOnlyMemory based overload for CanReadToken #3222

henrikwidlund · 2025-05-07T08:29:07Z

Add Memory based overload for CanReadToken

You've read the Contributor Guide and Code of Conduct.
You've included unit or integration tests for your change, where applicable.
You've included inline docs for your change, where applicable.
If any gains or losses in performance are possible, you've included benchmarks for your changes. More info
There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

Added an overload for the CanReadToken in JsonWebTokenHandler and JwtSecurityTokenHandler with ReadOnlyMemory<char> to reduce allocations and adjusted the string based overload to call the new method.

New overload is only available on dotnet 8 and above as the span based regex methods used to match JWS/JWE isn't available in dotnet 6.

Fixes #3221

Fixes AzureAD#3221

henrikwidlund · 2025-05-07T08:31:10Z

@microsoft-github-policy-service agree

henrikwidlund · 2025-05-07T08:32:37Z

I did not add benchmarks, even though this is a performance improvement. The reason being that the string based API calls the span based one.

Change to ReadOnlyMemory to reduce allocations when calling ToString

kllysng · 2025-05-08T19:08:17Z

Thanks for your contribution @henrikwidlund . Are you planning to add unit tests?

henrikwidlund · 2025-05-09T07:12:03Z

@kllysng I was thinking that the regular string based ones would cover it since the string based method calls the memory based one. Let me know if you want explicit tests.

henrikwidlund · 2025-05-09T14:20:09Z

I've added tests now either way. Also added the method to JwtSecurityTokenHandler. Was thinking of adding the memory based overload to JwtSecurityTokenHandler.ReadToken, but feels out of scope.

brentschmaltz · 2025-05-12T16:41:10Z

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.cs

+            // more segments than the maximum we know how to handle.
+            int segmentCount = JwtTokenUtilities.CountJwtTokenPart(token.Span, JwtConstants.MaxJwtSegmentCount + 1);
+
+            switch (segmentCount)


this code (switch (segmentCount)) can be shared between JsonWebTokenHandler and JwtSecurityTokenHandler.

When you think about it, most of this code could be shared between JsonWebTokenHandler and JwtSecurityTokenHandler.

@brentschmaltz please resolve if you're satisfied with the solution :)

brentschmaltz

refactor and share common code between JsonWebTokenHandler and JwtSecurityTokenHandler.

henrikwidlund · 2025-05-12T19:10:36Z

Good points @brentschmaltz, I've applied your suggestions.

sruke

LGTM, thank you for taking the suggestions and making the contribution @henrikwidlund.

henrikwidlund · 2025-05-13T21:16:31Z

I see that a few unit tests are failing because of accessibility. I'll make the method internal again tomorrow

…nHandler

henrikwidlund · 2025-05-14T07:03:54Z

@sruke fixed the compiler error

… framework.

sruke · 2025-05-15T17:25:47Z

@henrikwidlund, a few unit tests are still failing. We’re planning to release IdentityModel today—would it be okay to include this PR in the next release (in a month) instead?

henrikwidlund · 2025-05-15T18:21:48Z

@sruke I believe I've fixed the test (was a real bug). I leave it up to you if it's getting to tight for your deadline (although, it would be nice to have it :))

henrikwidlund · 2025-05-19T18:04:15Z

I noticed that the PR build is still failing, but I don't understand if it's related to my code changes?

sruke · 2025-05-20T17:22:08Z

I noticed that the PR build is still failing, but I don't understand if it's related to my code changes?

The failure (Resource not accessible by integration) appears unrelated to recent changes. All tests passed. I’m rerunning the build to check if the issue resolves. This change wasn’t included in the previous IdentityModel release but will be part of the upcoming one.

henrikwidlund · 2025-05-20T18:13:52Z

I noticed that the PR build is still failing, but I don't understand if it's related to my code changes?

The failure (Resource not accessible by integration) appears unrelated to recent changes. All tests passed. I’m rerunning the build to check if the issue resolves. This change wasn’t included in the previous IdentityModel release but will be part of the upcoming one.

Notice that it didn't help 😕

henrikwidlund · 2025-05-23T18:47:19Z

@sruke @brentschmaltz ping, any ideas what the problem is and when this can be merged?

…ndler_memory # Conflicts: # src/Microsoft.IdentityModel.JsonWebTokens/PublicAPI.Unshipped.txt

henrikwidlund · 2025-06-04T17:41:10Z

@sruke @brentschmaltz ping again :/

brentschmaltz · 2025-07-01T23:35:40Z

src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.cs

        /// <param name="maxCount">The maximum number of segments to count up to.</param>
        /// <returns>The number of segments up to <paramref name="maxCount"/>.</returns>
-        internal static int CountJwtTokenPart(string token, int maxCount)
+        internal static int CountJwtTokenPart(ReadOnlySpan<char> token, int maxCount)


Why do we need this as ReadOnlySpan?

We don't like to remove the old method as that can cause issues when mixed versions are used.
We end up with a MethodNotFound exception

We need it because without it the PR would not make sense as we'd need to work with strings again. If you want I can create a method that takes string and calls this one.

@henrikwidlund i was thinking why ReadOnlySpan and not ReadOnlyMemory?

Only reasons are passing a smaller object and that that's what the method works with. If that's what you're thinking about I don't understand your comment about method not found as that would happen regardless.

@brentschmaltz I believe older versions of .NET does not support regular expressions for ReadOnlySpan, and this library targets older versions of .NET. This is the reason ReadOnlyMemory is used in the CanReadToken method.

ReadOnlySpan is more efficient and what libraries should use/support whenever possible.

I agreee that supporting string would be useful, even if it is basically just a wrapper for the new method.

So what's your suggestion, add back the string based one and have that one call the span one, or close the PR?

So what's your suggestion, add back the string based one and have that one call the span one, or close the PR?

Yes, that is what we are thinking.

Which of them? I can do both, but it would be a bit of a waste 😅

add back the string based one and have that one call the span one,

Okay, done :)

henrikwidlund · 2025-09-23T09:49:33Z

Is this PR still wanted? It's been open for four months now. @brentschmaltz @sruke @jennyf19

pmaytak · 2025-09-26T07:51:19Z

@henrikwidlund Sorry about the delay. I'm on holiday next week. I'll bring this up with the team, if there's available bandwidth; otherwise, I'll follow up on this after I'm back.

Add Span based overload for CanReadToken

80b6a82

Fixes AzureAD#3221

henrikwidlund requested a review from a team as a code owner May 7, 2025 08:29

Always include memory based overload to make shipped apis work

b7c5319

Change to ReadOnlyMemory to reduce allocations when calling ToString

Add to JwtSecurityTokenHandler + tests

d6659a4

henrikwidlund changed the title ~~Add Span based overload for CanReadToken~~ Add ReadOnlyMemory based overload for CanReadToken May 9, 2025

brentschmaltz reviewed May 12, 2025

View reviewed changes

brentschmaltz suggested changes May 12, 2025

View reviewed changes

PR feedback

e5387a2

Merge branch 'dev' into feature/jsonwebtokenhandler_memory

4804b14

sruke approved these changes May 13, 2025

View reviewed changes

henrikwidlund added 2 commits May 14, 2025 09:02

Make CountJwtTokenPart internal again as it's used in JwtSecurityToke…

febd873

…nHandler

Merge branch 'dev' into feature/jsonwebtokenhandler_memory

02326f2

henrikwidlund and others added 2 commits May 15, 2025 09:20

Fix compiler issues and remove in keyword (might not work with dotnet…

0475358

… framework.

Merge branch 'dev' into feature/jsonwebtokenhandler_memory

7b31bb0

Simplify logic and fix bug

a0fe604

Merge branch 'dev' into feature/jsonwebtokenhandler_memory

6dedd3d

Merge remote-tracking branch 'origin/dev' into feature/jsonwebtokenha…

4c05215

…ndler_memory # Conflicts: # src/Microsoft.IdentityModel.JsonWebTokens/PublicAPI.Unshipped.txt

henrikwidlund requested a review from brentschmaltz May 27, 2025 18:36

Merge branch 'dev' into feature/jsonwebtokenhandler_memory

f3d1d0e

brentschmaltz reviewed Jul 1, 2025

View reviewed changes

henrikwidlund added 2 commits July 8, 2025 15:15

Add back string based version of CountJwtTokenPart for back compat

5446f5f

Merge branch 'dev' into feature/jsonwebtokenhandler_memory

646fb17

Add ReadOnlyMemory based overload for CanReadToken #3222

Are you sure you want to change the base?

Add ReadOnlyMemory based overload for CanReadToken #3222

Uh oh!

Conversation

henrikwidlund commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Add Memory based overload for CanReadToken

Description

Uh oh!

henrikwidlund commented May 7, 2025

Uh oh!

henrikwidlund commented May 7, 2025

Uh oh!

kllysng commented May 8, 2025

Uh oh!

henrikwidlund commented May 9, 2025

Uh oh!

henrikwidlund commented May 9, 2025

Uh oh!

brentschmaltz May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brentschmaltz left a comment

Choose a reason for hiding this comment

Uh oh!

henrikwidlund commented May 12, 2025

Uh oh!

sruke left a comment

Choose a reason for hiding this comment

Uh oh!

henrikwidlund commented May 13, 2025

Uh oh!

henrikwidlund commented May 14, 2025

Uh oh!

sruke commented May 15, 2025

Uh oh!

henrikwidlund commented May 15, 2025

Uh oh!

henrikwidlund commented May 19, 2025

Uh oh!

sruke commented May 20, 2025

Uh oh!

henrikwidlund commented May 20, 2025

Uh oh!

henrikwidlund commented May 23, 2025

Uh oh!

henrikwidlund commented Jun 4, 2025

Uh oh!

brentschmaltz Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrikwidlund Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrikwidlund commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmaytak commented Sep 26, 2025

henrikwidlund commented May 7, 2025 •

edited

Loading

brentschmaltz May 12, 2025 •

edited

Loading

brentschmaltz Jul 1, 2025 •

edited

Loading

henrikwidlund Jul 2, 2025 •

edited

Loading

henrikwidlund commented Sep 23, 2025 •

edited

Loading