From 4dd6db886f0af4fe7904bb51a097df1d9af7512d Mon Sep 17 00:00:00 2001 From: Adam Hathcock Date: Wed, 23 Jul 2025 13:16:08 +0100 Subject: [PATCH] insert or replace always...don't use ignore or insert (#363) * SaveObject is always insert or replace. Never use insert or ignore * add/fix tests * always replace even for bulk --- .github/git-commit-instructions.md | 2 +- src/Speckle.Sdk/Credentials/AccountManager.cs | 4 +- .../Credentials/AccountManagerTests.cs | 420 ++++++++++++++++++ 3 files changed, 423 insertions(+), 3 deletions(-) create mode 100644 tests/Speckle.Sdk.Tests.Unit/Credentials/AccountManagerTests.cs diff --git a/.github/git-commit-instructions.md b/.github/git-commit-instructions.md index 048149c4..48e6a906 100644 --- a/.github/git-commit-instructions.md +++ b/.github/git-commit-instructions.md @@ -13,7 +13,7 @@ To ensure high-quality and consistent commits, please follow these guidelines: 3. **Test your changes** - Run all unit tests before committing. - Add or update xUnit tests as needed. - - Use FluentAssertions for assertions and Moq for mocking in tests. + - Use AwesomeAssertions for assertions and Moq for mocking in tests. 4. **Review your changes** - Double-check for accidental debug code or commented-out code. diff --git a/src/Speckle.Sdk/Credentials/AccountManager.cs b/src/Speckle.Sdk/Credentials/AccountManager.cs index 7cfd2875..d4433ef1 100644 --- a/src/Speckle.Sdk/Credentials/AccountManager.cs +++ b/src/Speckle.Sdk/Credentials/AccountManager.cs @@ -174,7 +174,7 @@ public sealed class AccountManager( account.id = null!; //TODO this is gross so remove when id is nullable RemoveAccount(id); - _accountStorage.SaveObject(account.id.NotNull(), JsonConvert.SerializeObject(account)); + _accountStorage.UpdateObject(account.id.NotNull(), JsonConvert.SerializeObject(account)); } public IEnumerable GetAccounts(string serverUrl) @@ -407,7 +407,7 @@ public sealed class AccountManager( { account.isDefault = true; } - _accountStorage.SaveObject(account.id, JsonConvert.SerializeObject(account)); + _accountStorage.UpdateObject(account.id, JsonConvert.SerializeObject(account)); } } diff --git a/tests/Speckle.Sdk.Tests.Unit/Credentials/AccountManagerTests.cs b/tests/Speckle.Sdk.Tests.Unit/Credentials/AccountManagerTests.cs new file mode 100644 index 00000000..9b23cae1 --- /dev/null +++ b/tests/Speckle.Sdk.Tests.Unit/Credentials/AccountManagerTests.cs @@ -0,0 +1,420 @@ +using Microsoft.Extensions.Logging; +using Moq; +using Speckle.Newtonsoft.Json; +using Speckle.Sdk.Api.GraphQL.Models; +using Speckle.Sdk.Credentials; +using Speckle.Sdk.Helpers; +using Speckle.Sdk.SQLite; +using Speckle.Sdk.Testing; + +namespace Speckle.Sdk.Tests.Unit.Credentials; + +public class AccountManagerTests : MoqTest +{ + private class TestAccountFactory : IAccountFactory + { + public Task CreateAccount( + Uri serverUrl, + string speckleToken, + string? refreshToken = default, + CancellationToken cancellationToken = default + ) => throw new NotImplementedException(); + + public Task GetUserServerInfo( + Uri serverUrl, + string? authToken, + CancellationToken ct + ) => throw new NotImplementedException(); + } + + private readonly Mock _mockApplication; + private readonly Mock> _mockLogger; + private readonly Mock _mockGraphQLClientFactory; + private readonly Mock _mockSpeckleHttp; + private readonly IAccountFactory _mockAccountFactory; + private readonly Mock _mockSqLiteJsonCacheManagerFactory; + private readonly Mock _mockAccountStorage; + private readonly Mock _mockAccountAddLockStorage; + + private readonly AccountManager _accountManager; + + public AccountManagerTests() + { + _mockApplication = Create(); + _mockLogger = Create>(MockBehavior.Loose); + _mockGraphQLClientFactory = Create(); + _mockSpeckleHttp = Create(); + _mockAccountFactory = new TestAccountFactory(); + _mockSqLiteJsonCacheManagerFactory = Create(); + + _mockAccountStorage = Create(); + _mockAccountAddLockStorage = Create(); + + _mockSqLiteJsonCacheManagerFactory.Setup(f => f.CreateForUser("Accounts")).Returns(_mockAccountStorage.Object); + _mockSqLiteJsonCacheManagerFactory + .Setup(f => f.CreateForUser("AccountAddFlow")) + .Returns(_mockAccountAddLockStorage.Object); + + _accountManager = new AccountManager( + _mockApplication.Object, + _mockLogger.Object, + _mockGraphQLClientFactory.Object, + _mockSpeckleHttp.Object, + _mockAccountFactory, + _mockSqLiteJsonCacheManagerFactory.Object + ); + } + + [Fact] + public void GetDefaultServerUrl_ReturnsDefaultUrl_WhenNoCustomUrlProvided() + { + // Act + var result = _accountManager.GetDefaultServerUrl(); + + // Assert + Assert.Equal(new Uri(AccountManager.DEFAULT_SERVER_URL), result); + } + + [Fact] + public void GetAccount_ReturnsAccount_WhenExists() + { + // Arrange + var accountId = "test-account-id"; + var account = CreateTestAccount(accountId); + + _mockAccountStorage + .Setup(s => s.GetAllObjects()) + .Returns(new[] { (accountId, JsonConvert.SerializeObject(account)) }); + + // Act + var result = _accountManager.GetAccount(accountId); + + // Assert + Assert.Equal(accountId, result.id); + Assert.Equal(account.userInfo.name, result.userInfo.name); + } + + [Fact] + public void GetAccount_ThrowsException_WhenNotExists() + { + // Arrange + var accountId = "non-existent-id"; + + _mockAccountStorage.Setup(s => s.GetAllObjects()).Returns([]); + + // Act & Assert + var exception = Assert.Throws(() => _accountManager.GetAccount(accountId)); + Assert.Equal($"Account {accountId} not found", exception.Message); + } + + [Fact] + public void GetAccounts_StringParameter_CallsUriOverload() + { + // Arrange + var serverUrl = "https://test.speckle.systems"; + var account = CreateTestAccount("test-account-id"); + account.serverInfo.url = serverUrl; + + _mockAccountStorage + .Setup(s => s.GetAllObjects()) + .Returns(new[] { (account.id, JsonConvert.SerializeObject(account)) }); + + // Act + var result = _accountManager.GetAccounts(serverUrl).ToList(); + + // Assert + Assert.Single(result); + Assert.Equal(serverUrl, result[0].serverInfo.url); + } + + [Fact] + public void GetAccounts_UriParameter_ReturnsMatchingAccounts() + { + // Arrange + var serverUri = new Uri("https://test.speckle.systems"); + var account = CreateTestAccount("test-account-id"); + account.serverInfo.url = serverUri.ToString(); + + _mockAccountStorage + .Setup(s => s.GetAllObjects()) + .Returns(new[] { (account.id, JsonConvert.SerializeObject(account)) }); + + // Act + var result = _accountManager.GetAccounts(serverUri).ToList(); + + // Assert + Assert.Single(result); + Assert.Equal(serverUri.ToString(), result[0].serverInfo.url); + } + + [Fact] + public void GetDefaultAccount_ReturnsMarkedDefaultAccount_WhenExists() + { + // Arrange + var defaultAccount = CreateTestAccount("default-account"); + defaultAccount.isDefault = true; + + var regularAccount = CreateTestAccount("regular-account"); + + _mockAccountStorage + .Setup(s => s.GetAllObjects()) + .Returns( + new[] + { + (defaultAccount.id, JsonConvert.SerializeObject(defaultAccount)), + (regularAccount.id, JsonConvert.SerializeObject(regularAccount)), + } + ); + + // Act + var result = _accountManager.GetDefaultAccount(); + + // Assert + Assert.NotNull(result); + Assert.Equal("default-account", result!.id); + Assert.True(result.isDefault); + } + + [Fact] + public void GetDefaultAccount_ReturnsFirstAccount_WhenNoDefaultExists() + { + // Arrange + var account1 = CreateTestAccount("account-1"); + var account2 = CreateTestAccount("account-2"); + + _mockAccountStorage + .Setup(s => s.GetAllObjects()) + .Returns( + new[] + { + (account1.id, JsonConvert.SerializeObject(account1)), + (account2.id, JsonConvert.SerializeObject(account2)), + } + ); + + // Act + var result = _accountManager.GetDefaultAccount(); + + // Assert + Assert.NotNull(result); + Assert.Equal("account-1", result!.id); + } + + [Fact] + public void GetDefaultAccount_ReturnsNull_WhenNoAccounts() + { + // Arrange + _mockAccountStorage.Setup(s => s.GetAllObjects()).Returns([]); + + // Act + var result = _accountManager.GetDefaultAccount(); + + // Assert + Assert.Null(result); + } + + [Fact] + public void GetAccounts_SkipsInvalidAccounts() + { + // Arrange + var validAccount = CreateTestAccount("valid-account"); + validAccount.isDefault = true; + + var invalidAccount = new Account { id = "invalid-account" }; + + var deleteCalled = false; + + _mockAccountStorage + .Setup(s => s.GetAllObjects()) + .Returns(() => + { + if (deleteCalled) + { + return [(validAccount.id, JsonConvert.SerializeObject(validAccount))]; + } + return + [ + (validAccount.id, JsonConvert.SerializeObject(validAccount)), + (invalidAccount.id, JsonConvert.SerializeObject(invalidAccount)), + ]; + }); + + _mockAccountStorage.Setup(s => s.DeleteObject(invalidAccount.id)).Callback(() => deleteCalled = true); + // Act + var result = _accountManager.GetAccounts().ToList(); + + // Assert + Assert.Single(result); + Assert.Equal("valid-account", result[0].id); + _mockAccountStorage.Verify(s => s.DeleteObject(invalidAccount.id), Times.Once); + } + + [Fact] + public void RemoveAccount_RemovesAccount() + { + // Arrange + var accountId = "account-to-remove"; + + _mockAccountStorage.Setup(s => s.DeleteObject(accountId)); + _mockAccountStorage.Setup(s => s.GetAllObjects()).Returns([]); + + // Act + _accountManager.RemoveAccount(accountId); + + // Assert + _mockAccountStorage.Verify(s => s.DeleteObject(accountId), Times.Once); + } + + [Fact] + public void RemoveAccount_SetsNewDefaultAccount_WhenDefaultRemoved() + { + // Arrange + var defaultAccountId = "default-account"; + var regularAccountId = "regular-account"; + + var regularAccount = CreateTestAccount(regularAccountId); + + _mockAccountStorage.Setup(s => s.DeleteObject(defaultAccountId)); + _mockAccountStorage + .Setup(s => s.GetAllObjects()) + .Returns(new[] { (regularAccountId, JsonConvert.SerializeObject(regularAccount)) }); + _mockAccountStorage.Setup(s => s.UpdateObject(regularAccountId, It.IsAny())); + + // Act + _accountManager.RemoveAccount(defaultAccountId); + + // Assert + _mockAccountStorage.Verify(s => s.DeleteObject(defaultAccountId), Times.Once); + _mockAccountStorage.Verify( + s => s.UpdateObject(regularAccountId, It.Is(json => json.Contains("\"isDefault\":true"))), + Times.Once + ); + } + + [Fact] + public void ChangeDefaultAccount_UpdatesDefaultAccount() + { + // Arrange + var account1 = CreateTestAccount("account-1"); + account1.isDefault = true; + + var account2 = CreateTestAccount("account-2"); + + _mockAccountStorage + .Setup(s => s.GetAllObjects()) + .Returns( + new[] + { + (account1.id, JsonConvert.SerializeObject(account1)), + (account2.id, JsonConvert.SerializeObject(account2)), + } + ); + + _mockAccountStorage.Setup(s => s.UpdateObject(account1.id, It.IsAny())); + _mockAccountStorage.Setup(s => s.UpdateObject(account2.id, It.IsAny())); + + // Act + _accountManager.ChangeDefaultAccount(account2.id); + + // Assert + _mockAccountStorage.Verify( + s => s.UpdateObject(account1.id, It.Is(json => json.Contains("\"isDefault\":false"))), + Times.Once + ); + _mockAccountStorage.Verify( + s => s.UpdateObject(account2.id, It.Is(json => json.Contains("\"isDefault\":true"))), + Times.Once + ); + } + + [Fact] + public void GetLocalIdentifierForAccount_ReturnsIdentifier_WhenAccountExists() + { + // Arrange + var account = CreateTestAccount("test-account"); + var expectedUri = new Uri($"{account.serverInfo.url}?id={account.userInfo.id}"); + + _mockAccountStorage.Setup(s => s.GetAllObjects()).Returns(new[] { ("bad", JsonConvert.SerializeObject(account)) }); + + // Act + var result = _accountManager.GetLocalIdentifierForAccount(account); + + // Assert + Assert.NotNull(result); + Assert.Equal(expectedUri, result); + } + + [Fact] + public void GetLocalIdentifierForAccount_ReturnsNull_WhenAccountDoesNotExist() + { + // Arrange + var account = CreateTestAccount("non-existent-account"); + + _mockAccountStorage.Setup(s => s.GetAllObjects()).Returns([]); + + // Act + var result = _accountManager.GetLocalIdentifierForAccount(account); + + // Assert + Assert.Null(result); + } + + [Fact] + public void GetAccountForLocalIdentifier_ReturnsAccount_WhenMatches() + { + // Arrange + var account = CreateTestAccount("test-account"); + var localIdentifier = new Uri($"{account.serverInfo.url}?id={account.userInfo.id}"); + + _mockAccountStorage.Setup(s => s.GetAllObjects()).Returns(new[] { ("bad", JsonConvert.SerializeObject(account)) }); + + // Act + var result = _accountManager.GetAccountForLocalIdentifier(localIdentifier); + + // Assert + Assert.NotNull(result); + Assert.Equal(account.id, result!.id); + } + + [Fact] + public void GetAccountForLocalIdentifier_ReturnsNull_WhenNoMatch() + { + // Arrange + var account = CreateTestAccount("test-account"); + var localIdentifier = new Uri("https://different.url?u=different-user"); + + _mockAccountStorage.Setup(s => s.GetAllObjects()).Returns(new[] { ("bad", JsonConvert.SerializeObject(account)) }); + + // Act + var result = _accountManager.GetAccountForLocalIdentifier(localIdentifier); + + // Assert + Assert.Null(result); + } + + // Helper method to create a test account + private static Account CreateTestAccount(string id) + { + return new Account + { + id = id, + token = "test-token", + refreshToken = "refresh-token", + isDefault = false, + isOnline = true, + userInfo = new UserInfo + { + id = "user-id", + name = "Test User", + email = "test@example.com", + company = "Test Company", + }, + serverInfo = new ServerInfo + { + name = "Test Server", + url = "https://test.speckle.systems", + company = "Speckle", + }, + }; + } +}