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 <[email protected]>

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));
   }
 
   /// <summary>
-  /// 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.
   /// </summary>
   /// <param name="serverUrl"></param>
   /// <returns></returns>
   public static IEnumerable<Account> GetAccounts(string serverUrl)
   {
-    return GetAccounts().Where(acc => acc.serverInfo.url == serverUrl);
+    var accounts = GetAccounts().ToList();
+    List<Account> 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;
   }
 
   /// <summary>

Core/Core/Credentials/StreamWrapper.cs

@@ -375,11 +375,6 @@ public bool Equals(StreamWrapper? wrapper)
   /// <exception cref="SpeckleException">Verification of the current state of the stream wrapper with provided <paramref name="acc"/> was unsuccessful. The <paramref name="acc"/> could be invalid, or lack permissions for the <see cref="StreamId"/>, or the <see cref="StreamId"/> or <see cref="BranchName"/> are invalid</exception>
   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);

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<Account> _accountsToCleanUp = new();
+
+  public static IEnumerable<TestCaseData> 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<Account> 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<Account> accounts, string requestedUrl, IList<Account> 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 = "[email protected]",
+        name = "user"
+      }
+    };
+  }
+
+  private void AddAccounts(IEnumerable<Account> accounts)
+  {
+    foreach (Account account in accounts)
+    {
+      _accountsToCleanUp.Add(account);
+      Fixtures.UpdateOrSaveAccount(account);
+    }
+  }
+}

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 = "[email protected]" }
     };
 
-    _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 = "[email protected]" }
     };
 
-    _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 = "[email protected]",
@@ -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<Account> 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]

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);
   }