-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
dependencies: migrate from json.net to system.text.json
#1274
dependencies: migrate from json.net to system.text.json
#1274
Conversation
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.
Looks good! Happy to see even using reflection we're seeing a 10% improvement 🎉
I notice there's a warning from the build in OAuthClient.cs:
The using directive for 'System.Text.Json' appeared previously in this namespace
src/shared/Atlassian.Bitbucket/BitbucketTokenEndpointResponseJson.cs
Outdated
Show resolved
Hide resolved
Update to .NET 7.0 to take advantage of certain new features of System.Text.Json (e.g. JsonRequired).
Update Microsoft.Identity.Client and Microsoft.Identity.Broker to version 4.54.0 in anticipation of removing Json.NET dependency.
2914963
to
92018e6
Compare
@mjcheetham - I decided that it would be a better idea to run performance comparisons against |
Update global reference to Json.NET (Newtonsoft.Json) to System.Text.Json. Note that this is only required for .NET Framework and can be removed when GCM migrates off it.
92018e6
to
b8fd4d5
Compare
The Bitbucket authority returns the non-standard 'scopes' property with token endpoint results. With Json.NET, it was possible to handle this differentiation from the 'scope' property returned by other providers using inheritance. However, while this somewhat works in System.Text.Json, there is an issue with overwriting the value with the final property. So, for example, if 'scopes' is the final property in the JSON returned from Bitbucket's OAuth token endpoint, then the scopes property is correctly used in deserialization. However, if 'scope' is the final property, this value overwrites the scope value. For this reason, we use a custom converter to explicitly deserialize Bitbucket OAuth responses. For simplicity in working with System.Text.Json, we also store the 'expires_in' in a long rather than a TimeSpan.
Update the Trace2Message class to use System.Text.Json instead of Json.NET for event format target messages. Note that ordering was purposefully added to ensure the expected format of the new JSON tests.
b8fd4d5
to
d87b245
Compare
Update Bitbucket REST API-related code to use System.Text.Json rather than Json.NET for JSON deserialization. This change also removes Bitbucket API properties (for Cloud and DC) that we are not using.
Update GitHub REST API-related code to use System.Text.Json rather than Json.NET for JSON deserialization.
Update OAuth-related code to use System.Text.Json rather than Json.NET for JSON serialization.
Update Azure DevOps API tests to use System.Text.Json rather than Json.NET for JSON serialization.
d87b245
to
2e153ce
Compare
**Changes:** - Use in-proc methods for getting OS version number (#1240, #1264) - Update System.CommandLine (#1265) - Suppress GUI from command-line argument (#1267) - Add github (login|logout|list) commands (#1267) - cURL Cookie file support (#1251) - Update target framework on Mac/Linux to .NET 7 (#1274, #1282) - Replace JSON.NET with System.Text.Json (#1274) - Preserve exact redirect URI formatting in OAuth requests (#1281) - Use IP localhost redirect for GitHub (#1286) - Use WWW-Authenticate headers from Git for Azure Repos authority (#1288) - Better GitHub Enterprise Managed User (EMU) account support (#1190)
Overview
Migrate GCM from JSON.NET to
System.Text.Json
.The first commit updates to .NET 7.0 to take advantage of certain features like
JsonRequired
. The second updatesMicrosoft.Identity.Client
so that the workaround for this issue is no longer necessary. The third updates the current global Json.NET dependency toSystem.Text.Json
and ensures it is only used for .NET Framework. The fourth adds a custom converter to handle Bitbucket's use of the non-standard 'scopes' property to provide token endpoint results. The fifth through ninth commits updateTrace2Message
, the Bitbucket REST API, the GitHub REST API, the Azure DevOps API tests, and OAuth-specific code to useSystem.Text.Json
instead of Json.NET for JSON serialization/deserialization.Performance comparison
Comparison runs were done using
dotnet-trace
with the commandprintf "protocol=https\nhost=github.com" | git-credential-manager get
. The machine used was a Mac with a 2.4 GHz 8-Core Intel Core i9 processor running Ventura 13.4. Times are in milliseconds.Individual Run Times
Run Summary
Analysis
Per the above, the transition to
System.Text.Json
and .NET 7.0 decreased end-to-end execution times by about 17% on average.