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

Removal of dependency on Newtonsoft #2233

Merged
merged 5 commits into from
Aug 21, 2023
Merged

Conversation

brentschmaltz
Copy link
Member

@brentschmaltz brentschmaltz commented Aug 17, 2023

This PR removes the use of Newtonsoft from IdentityModel by using Utf8JsonReader/Writer
JsonWebTokens were previously populated with JsonDocument.Parse, which resulted Claims and Values being parsed from a JsonElement each time properties were accessed.
This PR introduces a new model of reading with a Utf8JsonReader and creating a collection of values that have been deserialized from Json. This will improve performance when accessing either a System.Claim or a C# property. There is more work to do as it will be helpful if the JsonElement is remembered when parsing as when creating a Claim, the serialized Json is needed in the claim.

A first cut at improving creation of JWT's when a reduced number of string -> Utf8bytes -> string + string -> Utf8bytes transformations were taking place. The goal is to use buffer pooling and write into the buffer (Span) once, perform all work (signature, compression, encryption) on the buffer.

Standardized roundtrip serialization was improved with the goal of providing a deterministic model for 'additional' data found on some objects.

@brentschmaltz brentschmaltz force-pushed the brentsch/RemoveNewtonSoft branch 2 times, most recently from b941969 to 6d5cf6e Compare August 17, 2023 23:29
Replaced JsonDocument.Parse with Utf8JsonReader
Standardized roundtrip serialization
@brentschmaltz brentschmaltz force-pushed the brentsch/RemoveNewtonSoft branch from 6d5cf6e to f5a7cb7 Compare August 17, 2023 23:40
Remove some debugging code.
@jennyf19
Copy link
Collaborator

    public void TryGetClaim()

getting this error locally:

str1 != str2, StringComparison: 'Ordinal'
http://www.w3.org/2001/XMLSchema#double
http://www.w3.org/2001/XMLSchema#integer32


Refers to: test/Microsoft.IdentityModel.JsonWebTokens.Tests/JsonWebTokenTests.cs:160 in 40be4d0. [](commit_id = 40be4d0, deletion_comment = False)

@jennyf19
Copy link
Collaborator

    public void GetClaim()

str1 != str2, StringComparison: 'Ordinal'
http://www.w3.org/2001/XMLSchema#double
http://www.w3.org/2001/XMLSchema#integer32


Refers to: test/Microsoft.IdentityModel.JsonWebTokens.Tests/JsonWebTokenTests.cs:134 in 40be4d0. [](commit_id = 40be4d0, deletion_comment = False)

@jmprieur
Copy link
Contributor

jmprieur commented Aug 20, 2023

One breaking change in Wilson7 that we might want to fix (but in preview4) is:

jsonWebToken.TryGetPayload(JwtClaimsTypes.Roles, out roles) 

used to accept that roles is a string[]. I now needs to be a IList<string>

@brentschmaltz:

  • is it expected?
  • do we agree that we would provide something in preview4? (not preview 3)?

@jennyf19
Copy link
Collaborator

One breaking change in Wilson7 that we might want to fix (but in preview4) is:

jsonWebToken.TryGetPayload(JwtClaimsTypes.Roles, out roles) 

used to accept that roles is a string[]. I now needs to be a IList<string>

@brentschmaltz:

  • is it expected?
  • do we agree that we would provide something in preview4? (not preview 3)?

Thoughts @kevinchalet ?

@brentschmaltz brentschmaltz merged commit 55dbe85 into dev7 Aug 21, 2023
@brentschmaltz brentschmaltz deleted the brentsch/RemoveNewtonSoft branch August 21, 2023 02:04
@kevinchalet
Copy link
Contributor

Thoughts @kevinchalet ?

Thanks for tagging me, @jennyf19!

I'm assuming TryGetPayload actually means TryGetPayloadValue. If so, OpenIddict uses it in 3 places: twice with string and once with ImmutableDictionary<string, string[]>. Chances things are broken with string are low, but with ImmutableDictionary<string, string[]>, it's not unreasonable so I'll give it a try 😄

Was this PR included in the preview that shipped yesterday?

@jennyf19
Copy link
Collaborator

Thoughts @kevinchalet ?

Thanks for tagging me, @jennyf19!

I'm assuming TryGetPayload actually means TryGetPayloadValue. If so, OpenIddict uses it in 3 places: twice with string and once with ImmutableDictionary<string, string[]>. Chances things are broken with string are low, but with ImmutableDictionary<string, string[]>, it's not unreasonable so I'll give it a try 😄

Was this PR included in the preview that shipped yesterday?

Yes, this is in preview3, which is out on NuGet as of Sunday (8/20) night. Would appreciate your feedback on this one.

@jennyf19 jennyf19 added this to the 7.0.0-preview3 milestone Aug 21, 2023
@kevinchalet
Copy link
Contributor

Would appreciate your feedback on this one.

Sadly, I'm seeing important regressions when moving to preview3. I'll open dedicated tickets.

@kevinchalet
Copy link
Contributor

Sadly, I'm seeing important regressions when moving to preview3. I'll open dedicated tickets.

Here's the first one: #2244

@kevinchalet
Copy link
Contributor

And the second one: #2245.

@kevinchalet
Copy link
Contributor

And the third one: #2246.

It seems the migration to System.Text.Json has caused a lot of side effects/regressions. More agressive E2E testing would be a good idea. Having such bugs in the final version would be absolutely terrible 😅

@jennyf19
Copy link
Collaborator

And the third one: #2246.

It seems the migration to System.Text.Json has caused a lot of side effects/regressions. More agressive E2E testing would be a good idea. Having such bugs in the final version would be absolutely terrible 😅

We agree. Appreciate the bugs and all the thorough testing. We will improve our coverage as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants