From cbe65117f531f399c4d2087f7e333a4993440c30 Mon Sep 17 00:00:00 2001 From: BovineOx <73857041+BovineOx@users.noreply.github.com> Date: Tue, 12 Mar 2024 15:44:36 +0100 Subject: [PATCH] CNX-8920 use account migration when selecting accounts in sdk (#3216) * AccountManager and StreamWrapper Changes to ensure they are aware of the movedFrom property added for server migration * test(core): [CNX-9138] Unit tests for AccountManager server migration (#3215) * Unit tests * teardown * More test cases * polish * removing dupes removing duplicate accounts from those returned from GetAccounts(), where the account ID matches but is essentially the same as the account coming from the upgraded account. * Updated summary comment Updated summary comment * TrimEnd('/'); Added .TrimEnd('/') when setting the ServerInfo.url * Fixed account test (#3218) Account unit tests --------- Co-authored-by: Jedd Morgan <45512892+JR-Morgan@users.noreply.github.com> --- Core/Core/Credentials/AccountManager.cs | 27 +++++- Core/Core/Credentials/StreamWrapper.cs | 10 +-- .../AccountServerMigrationTests.cs | 87 +++++++++++++++++++ .../Credentials/Accounts.cs | 63 +++++++------- .../Tests/Speckle.Core.Tests.Unit/Fixtures.cs | 2 +- 5 files changed, 150 insertions(+), 39 deletions(-) create mode 100644 Core/Tests/Speckle.Core.Tests.Unit/Credentials/AccountServerMigrationTests.cs diff --git a/Core/Core/Credentials/AccountManager.cs b/Core/Core/Credentials/AccountManager.cs index 2a3d708f96..c6fb027a6f 100644 --- a/Core/Core/Credentials/AccountManager.cs +++ b/Core/Core/Credentials/AccountManager.cs @@ -253,20 +253,41 @@ public static void UpgradeAccount(string id) account.serverInfo.migration.movedTo = null; account.serverInfo.migration.movedFrom = new Uri(account.serverInfo.url); - account.serverInfo.url = upgradeUri.ToString(); + account.serverInfo.url = upgradeUri.ToString().TrimEnd('/'); account.serverInfo.frontend2 = true; s_accountStorage.UpdateObject(account.id, JsonConvert.SerializeObject(account)); } /// - /// Gets all the accounts for a given server. + /// Returns all unique accounts matching the serverUrl provided. If an account exists on more than one server, + /// typically because it has been migrated, then only the upgraded account (and therefore server) are returned. + /// Accounts are deemed to be the same when the Account.Id matches. /// /// /// public static IEnumerable GetAccounts(string serverUrl) { - return GetAccounts().Where(acc => acc.serverInfo.url == serverUrl); + var accounts = GetAccounts().ToList(); + List filtered = new(); + + foreach (var acc in accounts) + { + if (acc.serverInfo?.migration?.movedFrom == new Uri(serverUrl)) + { + filtered.Add(acc); + } + } + + foreach (var acc in accounts) + { + if (acc.serverInfo.url == serverUrl && !filtered.Any(x => x.id != acc.id)) + { + filtered.Add(acc); + } + } + + return filtered; } /// diff --git a/Core/Core/Credentials/StreamWrapper.cs b/Core/Core/Credentials/StreamWrapper.cs index 0370480b85..7c9d49caa2 100644 --- a/Core/Core/Credentials/StreamWrapper.cs +++ b/Core/Core/Credentials/StreamWrapper.cs @@ -375,11 +375,6 @@ public bool Equals(StreamWrapper? wrapper) /// Verification of the current state of the stream wrapper with provided was unsuccessful. The could be invalid, or lack permissions for the , or the or are invalid public async Task ValidateWithAccount(Account acc) { - if (ServerUrl != acc.serverInfo.url) - { - throw new ArgumentException($"Account is not from server {ServerUrl}", nameof(acc)); - } - Uri url; try { @@ -390,6 +385,11 @@ public async Task ValidateWithAccount(Account acc) throw new ArgumentException("Server Url is improperly formatted", nameof(acc), ex); } + if (ServerUrl != acc.serverInfo.url && url != acc.serverInfo.migration?.movedFrom) + { + throw new ArgumentException($"Account is not from server {ServerUrl}", nameof(acc)); + } + try { await Http.HttpPing(url).ConfigureAwait(false); diff --git a/Core/Tests/Speckle.Core.Tests.Unit/Credentials/AccountServerMigrationTests.cs b/Core/Tests/Speckle.Core.Tests.Unit/Credentials/AccountServerMigrationTests.cs new file mode 100644 index 0000000000..b7b807d49e --- /dev/null +++ b/Core/Tests/Speckle.Core.Tests.Unit/Credentials/AccountServerMigrationTests.cs @@ -0,0 +1,87 @@ +using NUnit.Framework; +using Speckle.Core.Api; +using Speckle.Core.Credentials; + +namespace Speckle.Core.Tests.Unit.Credentials; + +public class AccountServerMigrationTests +{ + private readonly List _accountsToCleanUp = new(); + + public static IEnumerable MigrationTestCase() + { + const string OLD_URL = "https://old.example.com"; + const string NEW_URL = "https://new.example.com"; + const string OTHER_URL = "https://other.example.com"; + Account oldAccount = CreateTestAccount(OLD_URL, null, new(NEW_URL)); + Account newAccount = CreateTestAccount(NEW_URL, new(OLD_URL), null); + Account otherAccount = CreateTestAccount(OTHER_URL, null, null); + + List givenAccounts = new() { oldAccount, newAccount, otherAccount }; + + yield return new TestCaseData(givenAccounts, NEW_URL, new[] { newAccount }) + .SetName("Get New") + .SetDescription("When requesting for new account, ensure only this account is returned"); + + yield return new TestCaseData(givenAccounts, OLD_URL, new[] { newAccount }) + .SetName("Get New via Old") + .SetDescription("When requesting for old account, ensure migrated account is returned first"); + + var reversed = Enumerable.Reverse(givenAccounts).ToList(); + + yield return new TestCaseData(reversed, OLD_URL, new[] { newAccount }) + .SetName("Get New via Old (Reversed order)") + .SetDescription("Account order shouldn't matter"); + } + + [Test] + [TestCaseSource(nameof(MigrationTestCase))] + public void TestServerMigration(IList accounts, string requestedUrl, IList expectedSequence) + { + AddAccounts(accounts); + + var result = AccountManager.GetAccounts(requestedUrl).ToList(); + + Assert.That(result, Is.EquivalentTo(expectedSequence)); + } + + [TearDown] + public void TearDown() + { + //Clean up any of the test accounts we made + foreach (var acc in _accountsToCleanUp) + { + Fixtures.DeleteLocalAccount(acc.id); + } + _accountsToCleanUp.Clear(); + } + + private static Account CreateTestAccount(string url, Uri movedFrom, Uri movedTo) + { + return new Account + { + token = "myToken", + serverInfo = new ServerInfo + { + url = url, + name = "myServer", + migration = new ServerMigration { movedTo = movedTo, movedFrom = movedFrom } + }, + userInfo = new UserInfo + { + id = new Guid().ToString(), + email = "user@example.com", + name = "user" + } + }; + } + + private void AddAccounts(IEnumerable accounts) + { + foreach (Account account in accounts) + { + _accountsToCleanUp.Add(account); + Fixtures.UpdateOrSaveAccount(account); + } + } +} diff --git a/Core/Tests/Speckle.Core.Tests.Unit/Credentials/Accounts.cs b/Core/Tests/Speckle.Core.Tests.Unit/Credentials/Accounts.cs index 1f78ee3c3a..ba0b1aec09 100644 --- a/Core/Tests/Speckle.Core.Tests.Unit/Credentials/Accounts.cs +++ b/Core/Tests/Speckle.Core.Tests.Unit/Credentials/Accounts.cs @@ -7,29 +7,29 @@ namespace Speckle.Core.Tests.Unit.Credentials; [TestFixture] public class CredentialInfrastructure { - [SetUp] - public void SetUp() + [OneTimeSetUp] + public static void SetUp() { - _testAccount1 = new Account + s_testAccount1 = new Account { refreshToken = "bla", token = "bla", - serverInfo = new ServerInfo { url = "bla", company = "bla" }, + serverInfo = new ServerInfo { url = "https://bla.example.com", company = "bla" }, userInfo = new UserInfo { email = "one@two.com" } }; - _testAccount2 = new Account + s_testAccount2 = new Account { refreshToken = "foo", token = "bar", - serverInfo = new ServerInfo { url = "baz", company = "qux" }, + serverInfo = new ServerInfo { url = "https://baz.example.com", company = "qux" }, userInfo = new UserInfo { email = "three@four.com" } }; - _testAccount3 = new Account + s_testAccount3 = new Account { token = "secret", - serverInfo = new ServerInfo { url = "https://sample.com", name = "qux" }, + serverInfo = new ServerInfo { url = "https://example.com", name = "qux" }, userInfo = new UserInfo { email = "six@five.com", @@ -38,22 +38,22 @@ public void SetUp() } }; - Fixtures.UpdateOrSaveAccount(_testAccount1); - Fixtures.UpdateOrSaveAccount(_testAccount2); - Fixtures.SaveLocalAccount(_testAccount3); + Fixtures.UpdateOrSaveAccount(s_testAccount1); + Fixtures.UpdateOrSaveAccount(s_testAccount2); + Fixtures.SaveLocalAccount(s_testAccount3); } - [TearDown] - public void TearDown() + [OneTimeTearDown] + public static void TearDown() { - Fixtures.DeleteLocalAccount(_testAccount1.id); - Fixtures.DeleteLocalAccount(_testAccount2.id); + Fixtures.DeleteLocalAccount(s_testAccount1.id); + Fixtures.DeleteLocalAccount(s_testAccount2.id); Fixtures.DeleteLocalAccountFile(); } - private Account _testAccount1, - _testAccount2, - _testAccount3; + private static Account s_testAccount1, + s_testAccount2, + s_testAccount3; [Test] public void GetAllAccounts() @@ -62,24 +62,27 @@ public void GetAllAccounts() Assert.That(accs, Has.Count.GreaterThanOrEqualTo(3)); // Tests are adding three accounts, you might have extra accounts on your machine when testing :D } - [Test] - public void GetAccountsForServer() + public static IEnumerable TestCases() { - var accs = AccountManager.GetAccounts("baz").ToList(); - - Assert.That(accs, Has.Count.EqualTo(1)); - Assert.That(accs[0].serverInfo.company, Is.EqualTo("qux")); - Assert.That(accs[0].serverInfo.url, Is.EqualTo("baz")); - Assert.That(accs[0].refreshToken, Is.EqualTo("foo")); + SetUp(); + return new[] { s_testAccount1, s_testAccount2, s_testAccount3 }; } [Test] - public void GetLocalAccount() + [TestCaseSource(nameof(TestCases))] + public void GetAccountsForServer(Account target) { - var acc = AccountManager.GetAccounts().FirstOrDefault(x => x.userInfo.id == "123345"); + var accs = AccountManager.GetAccounts(target.serverInfo.url).ToList(); + + Assert.That(accs, Has.Count.EqualTo(1)); + + var acc = accs[0]; - Assert.That(acc.serverInfo.url, Is.EqualTo("https://sample.com")); - Assert.That(acc.token, Is.EqualTo("secret")); + Assert.That(acc, Is.Not.SameAs(target), "We expect new objects (no reference equality)"); + Assert.That(acc.serverInfo.company, Is.EqualTo(target.serverInfo.company)); + Assert.That(acc.serverInfo.url, Is.EqualTo(target.serverInfo.url)); + Assert.That(acc.refreshToken, Is.EqualTo(target.refreshToken)); + Assert.That(acc.token, Is.EqualTo(target.token)); } [Test] diff --git a/Core/Tests/Speckle.Core.Tests.Unit/Fixtures.cs b/Core/Tests/Speckle.Core.Tests.Unit/Fixtures.cs index 3fbb6bca21..c6423ee967 100644 --- a/Core/Tests/Speckle.Core.Tests.Unit/Fixtures.cs +++ b/Core/Tests/Speckle.Core.Tests.Unit/Fixtures.cs @@ -32,7 +32,7 @@ public abstract class Fixtures public static void UpdateOrSaveAccount(Account account) { - s_accountStorage.DeleteObject(account.id); + DeleteLocalAccount(account.id); string serializedObject = JsonConvert.SerializeObject(account); s_accountStorage.SaveObjectSync(account.id, serializedObject); }