Skip to content

Commit

Permalink
dependencies: migrate from json.net to system.text.json (#1274)
Browse files Browse the repository at this point in the history
# 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 updates
`Microsoft.Identity.Client` so that the workaround for [this
issue](AzureAD/microsoft-authentication-library-for-dotnet#4108
) is no longer necessary. The third updates the current global Json.NET
dependency to `System.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 update `Trace2Message`, the Bitbucket
REST API, the GitHub REST API, the Azure DevOps API tests, and
OAuth-specific code to use `System.Text.Json` instead of Json.NET for
JSON serialization/deserialization.

# Performance comparison
Comparison runs were done using
[`dotnet-trace`](https://learn.microsoft.com/en-us/dotnet/core/diagnostics/dotnet-trace)
with the command `printf "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
| **Newtonsoft.json** | **System.Text.Json** |
|---------------------|----------------------|
| 364.08              | 304.92               |
| 366.11              | 305.23               |
| 377.31              | 313.09               |
| 381.3               | 313.51               |
| 373.25              | 297.71               |
| 373.98              | 312.78               |
| 370.84              | 317.86               |
| 368.25              | 311.78               |
| 369.79              | 307.69               |

## Run Summary
|             | **Newtonsoft.json** | **System.Text.Json** |
|-------------|---------------------|----------------------|
| **Average** | 371.66          | 309.40          |
| **Median**  | 370.84              | 311.78               |

## 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.
  • Loading branch information
ldennington committed Jun 6, 2023
2 parents d66558b + 2e153ce commit b550293
Show file tree
Hide file tree
Showing 49 changed files with 430 additions and 292 deletions.
4 changes: 2 additions & 2 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"request": "launch",
"preLaunchTask": "build",
// If you have changed target frameworks, make sure to update the program path.
"program": "${workspaceFolder}/out/shared/Git-Credential-Manager/bin/Debug/net6.0/git-credential-manager.dll",
"program": "${workspaceFolder}/out/shared/Git-Credential-Manager/bin/Debug/net7.0/git-credential-manager.dll",
"args": ["get"],
"cwd": "${workspaceFolder}/out/shared/Git-Credential-Manager",
"console": "integratedTerminal",
Expand All @@ -22,7 +22,7 @@
"request": "launch",
"preLaunchTask": "build",
// If you have changed target frameworks, make sure to update the program path.
"program": "${workspaceFolder}/out/shared/Git-Credential-Manager/bin/Debug/net6.0/git-credential-manager.dll",
"program": "${workspaceFolder}/out/shared/Git-Credential-Manager/bin/Debug/net7.0/git-credential-manager.dll",
"args": ["store"],
"cwd": "${workspaceFolder}/out/shared/Git-Credential-Manager",
"console": "integratedTerminal",
Expand Down
4 changes: 2 additions & 2 deletions .vscode/tasks.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
"type": "shell",
"group": "test",
"args": [
"~/.nuget/packages/reportgenerator/*/*/net6.0/ReportGenerator.dll",
"~/.nuget/packages/reportgenerator/*/*/net7.0/ReportGenerator.dll",
"-reports:${workspaceFolder}/**/TestResults/**/coverage.cobertura.xml",
"-targetdir:${workspaceFolder}/out/code-coverage"
],
Expand All @@ -71,7 +71,7 @@
"type": "shell",
"group": "test",
"args": [
"${env:USERROFILE}/.nuget/packages/reportgenerator/*/*/net6.0/ReportGenerator.dll",
"${env:USERROFILE}/.nuget/packages/reportgenerator/*/*/net7.0/ReportGenerator.dll",
"-reports:${workspaceFolder}/**/TestResults/**/coverage.cobertura.xml",
"-targetdir:${workspaceFolder}/out/code-coverage"
],
Expand Down
7 changes: 3 additions & 4 deletions Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,9 @@
<GenerateWindowsAppManifest Condition="'$(GenerateWindowsAppManifest)' == '' AND '$(OSPlatform)' == 'windows' AND '$(_IsExeProject)' == 'true'">true</GenerateWindowsAppManifest>
</PropertyGroup>

<ItemGroup>
<!-- Workaround https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/4108 -->
<PackageReference Include="Newtonsoft.Json">
<Version>13.0.1</Version>
<ItemGroup Condition = "'$(TargetFramework)' == 'net472'">
<PackageReference Include="System.Text.Json">
<Version>7.0.2</Version>
</PackageReference>
</ItemGroup>

Expand Down
2 changes: 1 addition & 1 deletion build/GCM.MSBuild.csproj
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>net6.0</TargetFramework>
<TargetFramework>net7.0</TargetFramework>
<IncludeBuildOutput>false</IncludeBuildOutput>
</PropertyGroup>

Expand Down
4 changes: 2 additions & 2 deletions docs/development.md
Original file line number Diff line number Diff line change
Expand Up @@ -209,13 +209,13 @@ HTML reports can be generated using ReportGenerator, this should be installed
during the build process, from the command line:

```shell
dotnet ~/.nuget/packages/reportgenerator/*/*/net6.0/ReportGenerator.dll -reports:./**/TestResults/**/coverage.cobertura.xml -targetdir:./out/code-coverage
dotnet ~/.nuget/packages/reportgenerator/*/*/net7.0/ReportGenerator.dll -reports:./**/TestResults/**/coverage.cobertura.xml -targetdir:./out/code-coverage
```

or

```shell
dotnet {$env:USERPROFILE}/.nuget/packages/reportgenerator/*/*/net6.0/ReportGenerator.dll -reports:./**/TestResults/**/coverage.cobertura.xml -targetdir:./out/code-coverage
dotnet {$env:USERPROFILE}/.nuget/packages/reportgenerator/*/*/net7.0/ReportGenerator.dll -reports:./**/TestResults/**/coverage.cobertura.xml -targetdir:./out/code-coverage
```

Or via VSCode Terminal/Run Task:
Expand Down
2 changes: 1 addition & 1 deletion src/linux/Packaging.Linux/Packaging.Linux.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" />

<PropertyGroup>
<TargetFramework>net6.0</TargetFramework>
<TargetFramework>net7.0</TargetFramework>
<GenerateAssemblyInfo>false</GenerateAssemblyInfo>
</PropertyGroup>

Expand Down
2 changes: 1 addition & 1 deletion src/linux/Packaging.Linux/layout.sh
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ GCM_SRC="$SRC/shared/Git-Credential-Manager"
PROJ_OUT="$OUT/linux/Packaging.Linux"

# Build parameters
FRAMEWORK=net6.0
FRAMEWORK=net7.0
RUNTIME=linux-x64

# Perform pre-execution checks
Expand Down
2 changes: 1 addition & 1 deletion src/osx/Installer.Mac/Installer.Mac.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" />

<PropertyGroup>
<TargetFramework>net6.0</TargetFramework>
<TargetFramework>net7.0</TargetFramework>
<GenerateAssemblyInfo>false</GenerateAssemblyInfo>
</PropertyGroup>

Expand Down
2 changes: 1 addition & 1 deletion src/osx/Installer.Mac/layout.sh
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ GCM_SRC="$SRC/shared/Git-Credential-Manager"
GCM_UI_SRC="$SRC/shared/Git-Credential-Manager.UI.Avalonia"

# Build parameters
FRAMEWORK=net6.0
FRAMEWORK=net7.0

# Parse script arguments
for i in "$@"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>net6.0</TargetFramework>
<TargetFramework>net7.0</TargetFramework>
<IsPackable>false</IsPackable>
<IsTestProject>true</IsTestProject>
<LangVersion>latest</LangVersion>
Expand Down
18 changes: 8 additions & 10 deletions src/shared/Atlassian.Bitbucket.Tests/BitbucketHostProviderTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public Mock<IBitbucketRestApi> GetBitbucketApi()
}
MockStoredAccount(context, input, token);
MockRemoteAccessTokenValid(input, token);

var provider = new BitbucketHostProvider(context, bitbucketAuthentication.Object, MockRestApiRegistry(input, bitbucketApi).Object);

var credential = await provider.GetCredentialAsync(input);
Expand Down Expand Up @@ -363,7 +363,7 @@ private void MockDCSSOEnabled()
// Cloud - supports Basic, OAuth
[InlineData("https", "bitbucket.org", "oauth", AuthenticationModes.OAuth)]
[InlineData("https", "bitbucket.org", "basic", AuthenticationModes.Basic)]
[InlineData("https", "bitbucket.org", "NOT-A-REAL-VALUE", CloudConstants.DotOrgAuthenticationModes)]
[InlineData("https", "bitbucket.org", "NOT-A-REAL-VALUE", CloudConstants.DotOrgAuthenticationModes)]
[InlineData("https", "bitbucket.org", "none", CloudConstants.DotOrgAuthenticationModes)]
[InlineData("https", "bitbucket.org", null, CloudConstants.DotOrgAuthenticationModes)]
public async Task BitbucketHostProvider_GetSupportedAuthenticationModes(string protocol, string host, string bitbucketAuthModes, AuthenticationModes expectedModes)
Expand Down Expand Up @@ -497,10 +497,9 @@ private void MockPromptOAuth(InputArguments input)
.ReturnsAsync(new CredentialsPromptResult(AuthenticationModes.OAuth));
}

private void MockRemoteBasicValid(InputArguments input, string password, bool twoFactor = true)
private void MockRemoteBasicValid(InputArguments input, string password)
{
var userInfo = new Mock<IUserInfo>(MockBehavior.Strict);
userInfo.Setup(ui => ui.IsTwoFactorAuthenticationEnabled).Returns(twoFactor);
var userInfo = new Mock<IUserInfo>(MockBehavior.Strict);
userInfo.Setup(ui => ui.UserName).Returns(input.UserName);

// Basic
Expand All @@ -515,10 +514,9 @@ private void MockRemoteAccessTokenExpired(InputArguments input, string token)
.ReturnsAsync(new RestApiResult<IUserInfo>(System.Net.HttpStatusCode.Unauthorized));
}

private void MockRemoteAccessTokenValid(InputArguments input, string token, bool twoFactor = true)
private void MockRemoteAccessTokenValid(InputArguments input, string token)
{
var userInfo = new Mock<IUserInfo>(MockBehavior.Strict);
userInfo.Setup(ui => ui.IsTwoFactorAuthenticationEnabled).Returns(twoFactor);
var userInfo = new Mock<IUserInfo>(MockBehavior.Strict);
userInfo.Setup(ui => ui.UserName).Returns(input.UserName);

// OAuth
Expand Down Expand Up @@ -565,9 +563,9 @@ private void VerifyOAuthRefreshTokenStored(TestCommandContext context, InputArgu
private static Mock<IRegistry<IBitbucketRestApi>> MockRestApiRegistry(InputArguments input, Mock<IBitbucketRestApi> bitbucketApi)
{
var restApiRegistry = new Mock<IRegistry<IBitbucketRestApi>>(MockBehavior.Strict);

restApiRegistry.Setup(rar => rar.Get(input)).Returns(bitbucketApi.Object);

return restApiRegistry;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,32 @@
using Newtonsoft.Json;
using System;
using System.Text.Json;
using Xunit;

namespace Atlassian.Bitbucket.Tests
{
public class BitbucketTokenEndpointResponseJsonTest
{
[Fact]
public void BitbucketTokenEndpointResponseJson_Deserialize_Scopes_Not_Scope()
public void BitbucketTokenEndpointResponseJson_Deserialize_Uses_Scopes()
{
var scopesString = "a,b,c";
var json = "{access_token: '', token_type: '', scopes:'" + scopesString + "', scope: 'x,y,z'}";
var accessToken = "123";
var tokenType = "Bearer";
var expiresIn = 1000;
var scopesString = "x,y,z";
var scopeString = "a,b,c";

var result = JsonConvert.DeserializeObject<BitbucketTokenEndpointResponseJson>(json);
var json = $"{{\"access_token\": \"{accessToken}\", \"token_type\": \"{tokenType}\", \"expires_in\": {expiresIn}, \"scopes\": \"{scopesString}\", \"scope\": \"{scopeString}\"}}";

var result = JsonSerializer.Deserialize<BitbucketTokenEndpointResponseJson>(json,
new JsonSerializerOptions
{
PropertyNameCaseInsensitive = true
});

Assert.Equal(accessToken, result.AccessToken);
Assert.Equal(tokenType, result.TokenType);
Assert.Equal(expiresIn, result.ExpiresIn);
Assert.Equal(scopesString, result.Scope);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,16 @@ public class BitbucketRestApiTest
[Theory]
[InlineData("jsquire", "token", true)]
[InlineData("jsquire", "password", false)]
public async Task BitbucketRestApi_GetUserInformationAsync_ReturnsUserInfo_ForSuccessfulRequest(string username, string password, bool isBearerToken)
public async Task BitbucketRestApi_GetUserInformationAsync_ReturnsUserInfo_ForSuccessfulRequest(string username, string password, bool isBearerToken)
{
var twoFactorAuthenticationEnabled = false;
var uuid = Guid.NewGuid();
var accountId = "1234";

var context = new TestCommandContext();

var expectedRequestUri = new Uri("https://api.bitbucket.org/2.0/user");

var userinfoResponseJson = $"{{ \"username\": \"{username}\" , \"has_2fa_enabled\": \"{twoFactorAuthenticationEnabled}\", \"account_id\": \"{accountId}\", \"uuid\": \"{uuid}\"}}";
var userinfoResponseJson = $"{{\"username\":\"{username}\",\"has_2fa_enabled\":false,\"account_id\":\"{accountId}\",\"uuid\":\"{uuid}\"}}";

var httpResponse = new HttpResponseMessage(HttpStatusCode.OK)
{
Expand Down Expand Up @@ -52,11 +51,8 @@ public async Task BitbucketRestApi_GetUserInformationAsync_ReturnsUserInfo_ForSu

Assert.NotNull(result);
Assert.Equal(username, result.Response.UserName);
Assert.Equal(twoFactorAuthenticationEnabled, result.Response.IsTwoFactorAuthenticationEnabled);
Assert.Equal(accountId, ((UserInfo)result.Response).AccountId);
Assert.Equal(uuid, ((UserInfo)result.Response).Uuid);

httpHandler.AssertRequest(HttpMethod.Get, expectedRequestUri, 1);
}
}
}
}
29 changes: 21 additions & 8 deletions src/shared/Atlassian.Bitbucket.Tests/Cloud/UserInfoTest.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
using System;
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Threading.Tasks;
using Atlassian.Bitbucket.Cloud;
using Xunit;
Expand All @@ -9,19 +12,29 @@ public class UserInfoTest
[Fact]
public void UserInfo_Set()
{
var uuid = System.Guid.NewGuid();
var userInfo = new UserInfo()
{
AccountId = "abc",
IsTwoFactorAuthenticationEnabled = false,
UserName = "123",
Uuid = uuid
};

Assert.Equal("abc", userInfo.AccountId);
Assert.False(userInfo.IsTwoFactorAuthenticationEnabled);
Assert.Equal("123", userInfo.UserName);
Assert.Equal(uuid, userInfo.Uuid);
}

[Fact]
public void Deserialize_UserInfo()
{
var uuid = "{bef4bd75-03fe-4f19-9c6c-ed57b05ab6f6}";
var userName = "bob";
var accountId = "123abc";

var json = $"{{\"uuid\": \"{uuid}\", \"has_2fa_enabled\": null, \"username\": \"{userName}\", \"account_id\": \"{accountId}\"}}";

var result = JsonSerializer.Deserialize<UserInfo>(json, new JsonSerializerOptions()
{
PropertyNameCaseInsensitive = true,
});

Assert.Equal(userName, result.UserName);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,8 @@ namespace Atlassian.Bitbucket.Tests.DataCenter
public class BitbucketRestApiTest
{
[Fact]
public async Task BitbucketRestApi_GetUserInformationAsync_ReturnsUserInfo_ForSuccessfulRequest_DoesNothing()
public async Task BitbucketRestApi_GetUserInformationAsync_ReturnsUserInfo_ForSuccessfulRequest_DoesNothing()
{
var twoFactorAuthenticationEnabled = false;

var context = new TestCommandContext();

var expectedRequestUri = new Uri("http://example.com/rest/api/1.0/users");
Expand All @@ -26,22 +24,21 @@ public async Task BitbucketRestApi_GetUserInformationAsync_ReturnsUserInfo_ForSu
return httpResponse;
});
context.HttpClientFactory.MessageHandler = httpHandler;

context.Settings.RemoteUri = new Uri("http://example.com");

var api = new BitbucketRestApi(context);
var result = await api.GetUserInformationAsync("never used", "never used", false);

Assert.NotNull(result);
Assert.Equal(DataCenterConstants.OAuthUserName, result.Response.UserName);
Assert.Equal(twoFactorAuthenticationEnabled, result.Response.IsTwoFactorAuthenticationEnabled);

httpHandler.AssertRequest(HttpMethod.Get, expectedRequestUri, 1);
}

[Theory]
[InlineData(HttpStatusCode.Unauthorized, true)]
[InlineData(HttpStatusCode.NotFound, false)]
[InlineData(HttpStatusCode.Unauthorized, true)]
[InlineData(HttpStatusCode.NotFound, false)]
public async Task BitbucketRestApi_IsOAuthInstalledAsync_ReflectsBitbucketAuthenticationResponse(HttpStatusCode responseCode, bool impliedSupport)
{
var context = new TestCommandContext();
Expand Down Expand Up @@ -75,7 +72,7 @@ public async Task BitbucketRestApi_GetAuthenticationMethodsAsync_ReflectRestApiR
var httpHandler = new TestHttpMessageHandler();

var expectedRequestUri = new Uri("http://example.com/rest/authconfig/1.0/login-options");

var httpResponse = new HttpResponseMessage(HttpStatusCode.OK)
{
Content = new StringContent(loginOptionResponseJson)
Expand All @@ -99,19 +96,19 @@ public async Task BitbucketRestApi_GetAuthenticationMethodsAsync_ReflectRestApiR
Assert.Equal(authMethods.Count, impliedSupportedMethods.Count);
Assert.Contains(authMethods, m => impliedSupportedMethods.Contains(m));
Assert.DoesNotContain(authMethods, m => impliedUnsupportedMethods.Contains(m));
}
}

public static IEnumerable<object[]> GetAuthenticationMethodsAsyncData =>
public static IEnumerable<object[]> GetAuthenticationMethodsAsyncData =>
new List<object[]>
{
new object[] { $"{{ \"results\":[ {{ \"type\":\"LOGIN_FORM\"}}]}}",
new List<AuthenticationMethod>{AuthenticationMethod.BasicAuth},
new object[] { $"{{ \"results\":[ {{ \"type\":\"LOGIN_FORM\"}}]}}",
new List<AuthenticationMethod>{AuthenticationMethod.BasicAuth},
new List<AuthenticationMethod>{AuthenticationMethod.Sso}},
new object[] { $"{{ \"results\":[{{\"type\":\"IDP\"}}]}}",
new List<AuthenticationMethod>{AuthenticationMethod.Sso},
new object[] { $"{{ \"results\":[{{\"type\":\"IDP\"}}]}}",
new List<AuthenticationMethod>{AuthenticationMethod.Sso},
new List<AuthenticationMethod>{AuthenticationMethod.BasicAuth}},
new object[] { $"{{ \"results\":[{{\"type\":\"IDP\"}}, {{ \"type\":\"LOGIN_FORM\"}}, {{ \"type\":\"UNDEFINED\"}}]}}",
new List<AuthenticationMethod>{AuthenticationMethod.Sso, AuthenticationMethod.BasicAuth},
new object[] { $"{{ \"results\":[{{\"type\":\"IDP\"}}, {{ \"type\":\"LOGIN_FORM\"}}, {{ \"type\":\"UNDEFINED\"}}]}}",
new List<AuthenticationMethod>{AuthenticationMethod.Sso, AuthenticationMethod.BasicAuth},
new List<AuthenticationMethod>()},
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,12 @@ public class UserInfoTest
[Fact]
public void UserInfo_Set()
{
var uuid = System.Guid.NewGuid();
var userInfo = new UserInfo()
{
UserName = "123"
};

Assert.False(userInfo.IsTwoFactorAuthenticationEnabled);
Assert.Equal("123", userInfo.UserName);
}
}
}
}
4 changes: 2 additions & 2 deletions src/shared/Atlassian.Bitbucket/Atlassian.Bitbucket.csproj
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFrameworks>net6.0</TargetFrameworks>
<TargetFrameworks Condition="'$(OSPlatform)'=='windows'">net6.0;net472</TargetFrameworks>
<TargetFrameworks>net7.0</TargetFrameworks>
<TargetFrameworks Condition="'$(OSPlatform)'=='windows'">net7.0;net472</TargetFrameworks>
<AssemblyName>Atlassian.Bitbucket</AssemblyName>
<RootNamespace>Atlassian.Bitbucket</RootNamespace>
<IsTestProject>false</IsTestProject>
Expand Down
Loading

0 comments on commit b550293

Please sign in to comment.