Add ReadOnlyMemory based overload for CanReadToken

[henrikwidlund]

trafficstars

Add Memory based overload for CanReadToken

• [x] You've read the Contributor Guide and Code of Conduct.
• [x] You've included unit or integration tests for your change, where applicable.
• [x] 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
• [x] 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

[henrikwidlund]

@microsoft-github-policy-service agree

[henrikwidlund]

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.

[kllysng]

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

[henrikwidlund]

@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]

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.

[henrikwidlund]

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

[henrikwidlund]

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

[henrikwidlund]

@sruke fixed the compiler error

[sruke]

@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]

@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]

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

[sruke]

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]

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]

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

[henrikwidlund]

@sruke @brentschmaltz ping again :/

[henrikwidlund]

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

[pmaytak]

@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.