diff --git a/src/GitHub.App/Services/ModelService.cs b/src/GitHub.App/Services/ModelService.cs index e50eae69ec..94002c4aed 100644 --- a/src/GitHub.App/Services/ModelService.cs +++ b/src/GitHub.App/Services/ModelService.cs @@ -87,10 +87,10 @@ public IObservable GetLicenses() public IObservable> GetAccounts() { - return Observable.Zip( + return Observable.Concat( GetUser(), - GetUserOrganizations(), - (user, orgs) => user.Concat(orgs)) + GetUserOrganizations()) + .ToList() .ToReadOnlyList(Create); } @@ -108,49 +108,51 @@ IObservable GetGitIgnoreTemplatesFromApi() .Select(GitIgnoreCacheItem.Create); } - IObservable> GetUser() + IObservable GetUser() { - return hostCache.GetAndRefreshObject("user", - () => ApiClient.GetUser().Select(AccountCacheItem.Create), TimeSpan.FromMinutes(5), TimeSpan.FromDays(7)) - .TakeLast(1) - .ToList(); - } - - IObservable> GetUserOrganizations() - { - return GetUserFromCache().SelectMany(user => - hostCache.GetAndRefreshObject(user.Login + "|orgs", - () => ApiClient.GetOrganizations().Select(AccountCacheItem.Create).ToList(), - TimeSpan.FromMinutes(2), TimeSpan.FromDays(7))) - // TODO: Akavache returns the cached version followed by the fresh version if > 2 - // minutes have expired from the last request. Here we make sure the latest value is - // returned but it's a hack. We really need a better way to cache this stuff. - .TakeLast(1) - .Catch, KeyNotFoundException>( - // This could in theory happen if we try to call this before the user is logged in. - e => + return Observable.Defer(() => hostCache.GetAndRefreshObject("user", + () => ApiClient.GetUser().Select(AccountCacheItem.Create), TimeSpan.FromMinutes(5), TimeSpan.FromDays(7)) + .Catch(e => + { + log.Error(e, "Retrieving user failed because user is not stored in the cache"); + return Observable.Return(null); + }) + .Catch(e => { - log.Error(e, "Retrieve user organizations failed because user is not stored in the cache"); - return Observable.Return(Enumerable.Empty()); + log.Error(e, "Retrieving user failed"); + return Observable.Return(null); }) - .Catch, Exception>(e => - { - log.Error(e, "Retrieve user organizations failed"); - return Observable.Return(Enumerable.Empty()); - }); + .WhereNotNull() + .TakeLast(1) + ); } - public IObservable> GetRepositories() + IObservable GetUserOrganizations() { - return GetUserRepositories(RepositoryType.Owner) - .TakeLast(1) - .Concat(GetUserRepositories(RepositoryType.Member).TakeLast(1)) - .Concat(GetAllRepositoriesForAllOrganizations()); + return + GetUserFromCache() + .SelectMany(user => + hostCache.GetAndRefreshObject(user.Login + "|orgs", + () => ApiClient.GetOrganizations().Select(AccountCacheItem.Create).ToList(), + TimeSpan.FromMinutes(2), TimeSpan.FromDays(7)) + ) + + // TODO: Akavache returns the cached version followed by the fresh version if > 2 + // minutes have expired from the last request. Here we make sure the latest value is + // returned + .Catch, Exception>(e => + { + log.Error(e, "Retrieve user organizations failed"); + return Observable.Return>(null); + }) + .WhereNotNull() + .TakeLast(1) + .SelectMany(x => x); } IObservable GetUserFromCache() { - return Observable.Defer(() => hostCache.GetObject("user")); + return GetUser(); } /// @@ -244,7 +246,13 @@ public ITrackingCollection GetRepositories(ITrackingColl .SelectMany(key => hostCache.GetAndFetchLatestFromIndex(key, () => ApiClient.GetRepositories() - .Select(RepositoryCacheItem.Create), + .Catch((Exception ex) => + { + log.Error(ex, "GetRepositories"); + return Observable.Return(null); + }) + .WhereNotNull() + .Select(RepositoryCacheItem.Create), item => { if (collection.Disposed) return; @@ -312,60 +320,6 @@ public IObservable GetFileContents(IRepositoryModel repo, string commitS })); } - IObservable> GetUserRepositories(RepositoryType repositoryType) - { - return Observable.Defer(() => GetUserFromCache().SelectMany(user => - hostCache.GetAndRefreshObject(string.Format(CultureInfo.InvariantCulture, "{0}|{1}:repos", user.Login, repositoryType), - () => GetUserRepositoriesFromApi(repositoryType), - TimeSpan.FromMinutes(2), - TimeSpan.FromDays(7))) - .ToReadOnlyList(Create)) - .Catch, KeyNotFoundException>( - // This could in theory happen if we try to call this before the user is logged in. - e => - { - log.Error(e, - "Retrieving {RepositoryType} user repositories failed because user is not stored in the cache", - repositoryType); - return Observable.Return(new IRemoteRepositoryModel[] {}); - }); - } - - IObservable> GetUserRepositoriesFromApi(RepositoryType repositoryType) - { - return ApiClient.GetUserRepositories(repositoryType) - .WhereNotNull() - .Select(RepositoryCacheItem.Create) - .ToList() - .Catch, Exception>(_ => Observable.Return(Enumerable.Empty())); - } - - IObservable> GetAllRepositoriesForAllOrganizations() - { - return GetUserOrganizations() - .SelectMany(org => org.ToObservable()) - .SelectMany(org => GetOrganizationRepositories(org.Login).TakeLast(1)); - } - - IObservable> GetOrganizationRepositories(string organization) - { - return Observable.Defer(() => GetUserFromCache().SelectMany(user => - hostCache.GetAndRefreshObject(string.Format(CultureInfo.InvariantCulture, "{0}|{1}|repos", user.Login, organization), - () => ApiClient.GetRepositoriesForOrganization(organization).Select( - RepositoryCacheItem.Create).ToList(), - TimeSpan.FromMinutes(2), - TimeSpan.FromDays(7))) - .ToReadOnlyList(Create)) - .Catch, KeyNotFoundException>( - // This could in theory happen if we try to call this before the user is logged in. - e => - { - log.Error(e, "Retrieveing {Organization} org repositories failed because user is not stored in the cache", - organization); - return Observable.Return(new IRemoteRepositoryModel[] {}); - }); - } - public IObservable GetBranches(IRepositoryModel repo) { var keyobs = GetUserFromCache() diff --git a/test/UnitTests/GitHub.App/Caches/BlobCacheTests.cs b/test/UnitTests/GitHub.App/Caches/BlobCacheTests.cs new file mode 100644 index 0000000000..580cbefb56 --- /dev/null +++ b/test/UnitTests/GitHub.App/Caches/BlobCacheTests.cs @@ -0,0 +1,68 @@ +using System; +using System.Linq; +using System.Reactive.Linq; +using System.Threading.Tasks; +using Akavache; +using GitHub.Api; +using GitHub.Caches; +using GitHub.Collections; +using GitHub.Models; +using GitHub.Services; +using NSubstitute; +using NUnit.Framework; + +namespace BlobCacheTests +{ + [TestFixture] + public class CacheCorruptionTests : TestBaseClass + { + [Test] + public async Task RecoversFromInvalidJsonInIndexEntries() + { + int count = 1; + var octokitRepos = new[] + { + CreateRepository("haacked", "seegit", id: count++), + CreateRepository("haacked", "codehaacks", id: count++), + CreateRepository("mojombo", "semver", id: count++), + CreateRepository("ninject", "ninject", id: count++), + CreateRepository("jabbr", "jabbr", id: count++), + CreateRepository("fody", "nullguard", id: count++), + CreateRepository("github", "visualstudio", id: count++), + CreateRepository("octokit", "octokit.net", id: count++), + CreateRepository("octokit", "octokit.rb", id: count++), + CreateRepository("octokit", "octokit.objc", id: count++), + }; + var expectedRepos = octokitRepos.Select(x => new RemoteRepositoryModel(x) as IRemoteRepositoryModel).ToList(); + + var apiClient = Substitute.For(); + apiClient.GetRepositories().Returns(octokitRepos.ToObservable()); + + var cache = new InMemoryBlobCache(); + var aRepoCacheEntry = new ModelService.RepositoryCacheItem(); + var modelService = new ModelService(apiClient, cache, Substitute.For()); + + await modelService.InsertUser(new AccountCacheItem { Login = "opus" }); + await cache.InsertObject(CacheIndex.RepoPrefix + "|opus", aRepoCacheEntry); + + var repositories = new TrackingCollection(); + modelService.GetRepositories(repositories); + repositories.Subscribe(); + await repositories.OriginalCompleted; + + // the first time we do this, it should fetch from the API + await apiClient.Received().GetRepositories(); + apiClient.ClearReceivedCalls(); + + repositories = new TrackingCollection(); + modelService.GetRepositories(repositories); + repositories.Subscribe(); + await repositories.OriginalCompleted; + // the second time we do this, it should not fetch + await apiClient.DidNotReceive().GetRepositories(); + + Assert.That(expectedRepos, Is.EquivalentTo(repositories)); + } + } + +} diff --git a/test/UnitTests/GitHub.App/Models/ModelServiceTests.cs b/test/UnitTests/GitHub.App/Models/ModelServiceTests.cs index ed938565c3..e699df0276 100644 --- a/test/UnitTests/GitHub.App/Models/ModelServiceTests.cs +++ b/test/UnitTests/GitHub.App/Models/ModelServiceTests.cs @@ -14,11 +14,9 @@ using NUnit.Framework; using System.Globalization; using System.Reactive.Subjects; -using System.Threading; using GitHub.Models; using GitHub.Primitives; using GitHub.Collections; -using ReactiveUI; using static GitHub.Services.ModelService; public class ModelServiceTests @@ -158,20 +156,19 @@ public async Task CanRetrieveAndCacheUserAndAccounts() apiClient.GetOrganizations().Returns(orgs.ToObservable()); var cache = new InMemoryBlobCache(); var modelService = new ModelService(apiClient, cache, Substitute.For()); - await modelService.InsertUser(new AccountCacheItem { Login = "snoopy" }); var fetched = await modelService.GetAccounts(); - Assert.That(3, Is.EqualTo(fetched.Count)); - Assert.That("snoopy", Is.EqualTo(fetched[0].Login)); - Assert.That("github", Is.EqualTo(fetched[1].Login)); - Assert.That("fake", Is.EqualTo(fetched[2].Login)); + Assert.That(fetched.Count, Is.EqualTo(3)); + Assert.That(fetched[0].Login, Is.EqualTo("snoopy")); + Assert.That(fetched[1].Login, Is.EqualTo("github")); + Assert.That(fetched[2].Login, Is.EqualTo("fake")); var cachedOrgs = await cache.GetObject>("snoopy|orgs"); - Assert.That(2, Is.EqualTo(cachedOrgs.Count)); - Assert.That("github", Is.EqualTo(cachedOrgs[0].Login)); - Assert.That("fake", Is.EqualTo(cachedOrgs[1].Login)); + Assert.That(cachedOrgs.Count, Is.EqualTo(2)); + Assert.That(cachedOrgs[0].Login, Is.EqualTo("github")); + Assert.That(cachedOrgs[1].Login, Is.EqualTo("fake")); var cachedUser = await cache.GetObject("user"); - Assert.That("snoopy", Is.EqualTo(cachedUser.Login)); + Assert.That(cachedUser.Login, Is.EqualTo("snoopy")); } [Test] @@ -227,91 +224,36 @@ public async Task OnlyRetrievesOneUserEvenIfCacheOrApiReturnsMoreThanOne() public class TheGetRepositoriesMethod : TestBaseClass { [Test] - public async Task CanRetrieveAndCacheRepositoriesForUserAndOrganizations() + public async Task CanRetrieveAndCacheRepositories() { - var orgs = new[] - { - CreateOctokitOrganization("github"), - CreateOctokitOrganization("octokit") - }; - var ownedRepos = new[] - { - CreateRepository("haacked", "seegit"), - CreateRepository("haacked", "codehaacks") - }; - var memberRepos = new[] - { - CreateRepository("mojombo", "semver"), - CreateRepository("ninject", "ninject"), - CreateRepository("jabbr", "jabbr"), - CreateRepository("fody", "nullguard") - }; - var githubRepos = new[] - { - CreateRepository("github", "visualstudio") - }; + int count = 1; var octokitRepos = new[] { - CreateRepository("octokit", "octokit.net"), - CreateRepository("octokit", "octokit.rb"), - CreateRepository("octokit", "octokit.objc") + CreateRepository("haacked", "seegit", id: count++), + CreateRepository("haacked", "codehaacks", id: count++), + CreateRepository("mojombo", "semver", id: count++), + CreateRepository("ninject", "ninject", id: count++), + CreateRepository("jabbr", "jabbr", id: count++), + CreateRepository("fody", "nullguard", id: count++), + CreateRepository("github", "visualstudio", id: count++), + CreateRepository("octokit", "octokit.net", id: count++), + CreateRepository("octokit", "octokit.rb", id: count++), + CreateRepository("octokit", "octokit.objc", id: count++), }; + var expectedRepos = octokitRepos.Select(x => new RemoteRepositoryModel(x) as IRemoteRepositoryModel).ToList(); + var apiClient = Substitute.For(); - apiClient.GetOrganizations().Returns(orgs.ToObservable()); - apiClient.GetUserRepositories(RepositoryType.Owner).Returns(ownedRepos.ToObservable()); - apiClient.GetUserRepositories(RepositoryType.Member).Returns(memberRepos.ToObservable()); - apiClient.GetRepositoriesForOrganization("github").Returns(githubRepos.ToObservable()); - apiClient.GetRepositoriesForOrganization("octokit").Returns(octokitRepos.ToObservable()); + apiClient.GetRepositories().Returns(octokitRepos.ToObservable()); var cache = new InMemoryBlobCache(); var modelService = new ModelService(apiClient, cache, Substitute.For()); await modelService.InsertUser(new AccountCacheItem { Login = "opus" }); - var fetched = await modelService.GetRepositories().ToList(); - - Assert.That(4, Is.EqualTo(fetched.Count)); - Assert.That(2, Is.EqualTo(fetched[0].Count)); - Assert.That(4, Is.EqualTo(fetched[1].Count)); - Assert.That(1, Is.EqualTo(fetched[2].Count)); - Assert.That(3, Is.EqualTo(fetched[3].Count)); - Assert.That("seegit", Is.EqualTo(fetched[0][0].Name)); - Assert.That("codehaacks", Is.EqualTo(fetched[0][1].Name)); - Assert.That("semver", Is.EqualTo(fetched[1][0].Name)); - Assert.That("ninject", Is.EqualTo(fetched[1][1].Name)); - Assert.That("jabbr", Is.EqualTo(fetched[1][2].Name)); - Assert.That("nullguard", Is.EqualTo(fetched[1][3].Name)); - Assert.That("visualstudio", Is.EqualTo(fetched[2][0].Name)); - Assert.That("octokit.net", Is.EqualTo(fetched[3][0].Name)); - Assert.That("octokit.rb", Is.EqualTo(fetched[3][1].Name)); - Assert.That("octokit.objc", Is.EqualTo(fetched[3][2].Name)); - var cachedOwnerRepositories = await cache.GetObject>("opus|Owner:repos"); - Assert.That(2, Is.EqualTo(cachedOwnerRepositories.Count)); - Assert.That("seegit", Is.EqualTo(cachedOwnerRepositories[0].Name)); - Assert.That("haacked", Is.EqualTo(cachedOwnerRepositories[0].Owner.Login)); - Assert.That("codehaacks", Is.EqualTo(cachedOwnerRepositories[1].Name)); - Assert.That("haacked", Is.EqualTo(cachedOwnerRepositories[1].Owner.Login)); - var cachedMemberRepositories = await cache.GetObject>("opus|Member:repos"); - Assert.That(4, Is.EqualTo(cachedMemberRepositories.Count)); - Assert.That("semver", Is.EqualTo(cachedMemberRepositories[0].Name)); - Assert.That("mojombo", Is.EqualTo(cachedMemberRepositories[0].Owner.Login)); - Assert.That("ninject", Is.EqualTo(cachedMemberRepositories[1].Name)); - Assert.That("ninject", Is.EqualTo(cachedMemberRepositories[1].Owner.Login)); - Assert.That("jabbr", Is.EqualTo(cachedMemberRepositories[2].Name)); - Assert.That("jabbr", Is.EqualTo(cachedMemberRepositories[2].Owner.Login)); - Assert.That("nullguard", Is.EqualTo(cachedMemberRepositories[3].Name)); - Assert.That("fody", Is.EqualTo(cachedMemberRepositories[3].Owner.Login)); - var cachedGitHubRepositories = await cache.GetObject>("opus|github|repos"); - Assert.That(1, Is.EqualTo(cachedGitHubRepositories.Count)); - Assert.That("seegit", Is.EqualTo(cachedOwnerRepositories[0].Name)); - Assert.That("haacked", Is.EqualTo(cachedOwnerRepositories[0].Owner.Login)); - Assert.That("codehaacks", Is.EqualTo(cachedOwnerRepositories[1].Name)); - Assert.That("haacked", Is.EqualTo(cachedOwnerRepositories[1].Owner.Login)); - var cachedOctokitRepositories = await cache.GetObject>("opus|octokit|repos"); - Assert.That("octokit.net", Is.EqualTo(cachedOctokitRepositories[0].Name)); - Assert.That("octokit", Is.EqualTo(cachedOctokitRepositories[0].Owner.Login)); - Assert.That("octokit.rb", Is.EqualTo(cachedOctokitRepositories[1].Name)); - Assert.That("octokit", Is.EqualTo(cachedOctokitRepositories[1].Owner.Login)); - Assert.That("octokit.objc", Is.EqualTo(cachedOctokitRepositories[2].Name)); - Assert.That("octokit", Is.EqualTo(cachedOctokitRepositories[2].Owner.Login)); + var repositories = new TrackingCollection(); + modelService.GetRepositories(repositories); + repositories.Subscribe(); + await repositories.OriginalCompleted; + + CollectionAssert.AreEquivalent(expectedRepos, repositories.OrderBy(x => x.Name)); } [Test] @@ -320,9 +262,12 @@ public async Task WhenNotLoggedInReturnsEmptyCollection() var apiClient = Substitute.For(); var modelService = new ModelService(apiClient, new InMemoryBlobCache(), Substitute.For()); - var repos = await modelService.GetRepositories(); + var repositories = new TrackingCollection(); + modelService.GetRepositories(repositories); + repositories.Subscribe(); + await repositories.OriginalCompleted; - Assert.That(0, Is.EqualTo(repos.Count)); + CollectionAssert.AreEqual(new IRemoteRepositoryModel[] { }, repositories.ToArray()); } [Test] @@ -330,12 +275,16 @@ public async Task WhenLoggedInDoesNotBlowUpOnUnexpectedNetworkProblems() { var apiClient = Substitute.For(); var modelService = new ModelService(apiClient, new InMemoryBlobCache(), Substitute.For()); - apiClient.GetOrganizations() - .Returns(Observable.Throw(new NotFoundException("Not Found", HttpStatusCode.NotFound))); + await modelService.InsertUser(new AccountCacheItem { Login = "opus" }); + apiClient.GetRepositories() + .Returns(Observable.Throw(new NotFoundException("Not Found", HttpStatusCode.NotFound))); - var repos = await modelService.GetRepositories(); + var repositories = new TrackingCollection(); + modelService.GetRepositories(repositories); + repositories.Subscribe(); + await repositories.OriginalCompleted; - Assert.That(0, Is.EqualTo(repos.Count)); + CollectionAssert.AreEquivalent(new IRemoteRepositoryModel[] { }, repositories.ToArray()); } } @@ -348,11 +297,11 @@ public async Task InvalidatesTheCache() var cache = new InMemoryBlobCache(); var modelService = new ModelService(apiClient, cache, Substitute.For()); var user = await modelService.InsertUser(new AccountCacheItem(CreateOctokitUser("octocat"))); - //Assert.Single((await cache.GetAllObjects())); + Assert.That((await cache.GetAllObjects()), Has.Count.EqualTo(1)); await modelService.InvalidateAll(); - //Assert.That((cache.GetAllObjects(), Is.Empty)); + Assert.That((await cache.GetAllObjects()), Is.Empty); } [Test] @@ -425,7 +374,7 @@ public async Task NonExpiredIndexReturnsCache() await col.OriginalCompleted.Timeout(TimeSpan.FromMilliseconds(Timeout));; Assert.That(expected, Is.EqualTo(col.Count)); - //Assert.Collection(col, col.Select(x => new Action(t => Assert.That("Cache", StartsWith(x.Title)))).ToArray()); + Assert.That(col, Has.All.Matches(Has.Property(nameof(IPullRequestModel.Title)).StartsWith("Cache"))); } [Test] @@ -493,7 +442,7 @@ public async Task ExpiredIndexReturnsLive() await done; - //Assert.Collection(col, col.Select(x => new Action(t => Assert.StartsWith("Live", x.Title))).ToArray()); + Assert.That(col, Has.All.Matches(Has.Property(nameof(IPullRequestModel.Title)).StartsWith("Live"))); } [Test] diff --git a/test/UnitTests/GitHub.App/ViewModels/Dialog/RepositoryCloneViewModelTests.cs b/test/UnitTests/GitHub.App/ViewModels/Dialog/RepositoryCloneViewModelTests.cs index 7a82160aa4..7488fd3431 100644 --- a/test/UnitTests/GitHub.App/ViewModels/Dialog/RepositoryCloneViewModelTests.cs +++ b/test/UnitTests/GitHub.App/ViewModels/Dialog/RepositoryCloneViewModelTests.cs @@ -189,7 +189,7 @@ public async Task IsFalseWhenLoadingAndCompletedWithRepository() var col = (ITrackingCollection)vm.Repositories; await col.OriginalCompleted.Timeout(TimeSpan.FromMilliseconds(Timeout)); - //Assert.Single(vm.Repositories); + Assert.That(vm.Repositories, Has.Count.EqualTo(1)); Assert.False(vm.NoRepositoriesFound); } diff --git a/test/UnitTests/GitHub.App/ViewModels/GitHubPane/NavigationViewModelTests.cs b/test/UnitTests/GitHub.App/ViewModels/GitHubPane/NavigationViewModelTests.cs index 7db3f5f844..cc83b34600 100644 --- a/test/UnitTests/GitHub.App/ViewModels/GitHubPane/NavigationViewModelTests.cs +++ b/test/UnitTests/GitHub.App/ViewModels/GitHubPane/NavigationViewModelTests.cs @@ -243,7 +243,7 @@ public void CloseRequestedShouldRemovePage() target.NavigateTo(second); close.OnNext(Unit.Default); - //Assert.Single(target.History); + Assert.That(target.History, Has.Count.EqualTo(1)); Assert.That(first, Is.SameAs(target.History[0])); } @@ -358,7 +358,7 @@ public void RemovesAllInstancesOfPage() target.NavigateTo(second); target.RemoveAll(second); - //Assert.Single(target.History); + Assert.That(target.History, Has.Count.EqualTo(1)); } [Test] @@ -376,7 +376,7 @@ public void RemovingItemAfterCurrentWorks() Assert.That(first, Is.SameAs(target.Content)); Assert.That(first, Is.SameAs(target.History[0])); Assert.That(0, Is.EqualTo(target.Index)); - //Assert.Single(target.History); + Assert.That(target.History, Has.Count.EqualTo(1)); } [Test] @@ -393,7 +393,7 @@ public void RemovingCurrentItemSetsContentToPrevious() Assert.That(first, Is.SameAs(target.Content)); Assert.That(first, Is.SameAs(target.History[0])); Assert.That(0, Is.EqualTo(target.Index)); - //Assert.Single(target.History); + Assert.That(target.History, Has.Count.EqualTo(1)); } [Test] diff --git a/test/UnitTests/UnitTests.csproj b/test/UnitTests/UnitTests.csproj index 6426dc4779..32ca6dc137 100644 --- a/test/UnitTests/UnitTests.csproj +++ b/test/UnitTests/UnitTests.csproj @@ -232,6 +232,7 @@ +