diff --git a/SemDiff.Core/Analysis.cs b/SemDiff.Core/Analysis.cs index 97034f0..0d8af99 100644 --- a/SemDiff.Core/Analysis.cs +++ b/SemDiff.Core/Analysis.cs @@ -148,7 +148,7 @@ private static IEnumerable GetPulls(Repo repo, string relativePath) } relativePath = relativePath.ToStandardPath(); return repo.PullRequests - .SelectMany(p => p.Files + .SelectMany(p => p.ValidFiles .Select(f => new { n = f.Filename, f, p })) .Where(a => a.n == relativePath) .Select(a => new Pull(a.f, a.p)) diff --git a/SemDiff.Core/PullRequest.cs b/SemDiff.Core/PullRequest.cs index ba87a1c..eb63859 100644 --- a/SemDiff.Core/PullRequest.cs +++ b/SemDiff.Core/PullRequest.cs @@ -20,6 +20,10 @@ public class PullRequest public string CacheDirectory => Path.Combine(ParentRepo.CacheDirectory, Number.ToString()); public IList Files { get; set; } + + [JsonIgnore] + public IEnumerable ValidFiles => Files.Where(f => !f.IsInvalid); + public HeadBase Head { get; set; } public DateTime LastWrite { get; set; } = DateTime.MinValue; public int Number { get; set; } @@ -44,14 +48,14 @@ public async Task GetFilesAsync() { if (LastWrite >= Updated) { - foreach (var f in Files) + foreach (var f in ValidFiles) { f.LoadFromCache(); } } else { - await Task.WhenAll(Files.Select(current => current.DownloadFileAsync())); + await Task.WhenAll(ValidFiles.Select(current => current.DownloadFileAsync())); LastWrite = DateTime.UtcNow; } } diff --git a/SemDiff.Core/Repo.cs b/SemDiff.Core/Repo.cs index d8bf751..e8b06d9 100644 --- a/SemDiff.Core/Repo.cs +++ b/SemDiff.Core/Repo.cs @@ -147,7 +147,7 @@ public void GetCurrentSaved() { //Restore Self-Referential Loops p.ParentRepo = this; - foreach (var r in p.Files) + foreach (var r in p.ValidFiles) { r.ParentPullRequst = p; } @@ -155,7 +155,7 @@ public void GetCurrentSaved() if (VerifyPullRequestCache(p)) { PullRequests.Add(p); - foreach (var r in p.Files) + foreach (var r in p.ValidFiles) { r.LoadFromCache(); } @@ -180,7 +180,7 @@ private static bool VerifyPullRequestCache(PullRequest p) if (!Directory.Exists(p.CacheDirectory)) return false; - foreach (var f in p.Files) + foreach (var f in p.ValidFiles) { if (!File.Exists(f.CachePathBase)) return false; @@ -386,7 +386,7 @@ internal async Task HttpGetAsync(string url, Ref etag = null, Re { Client.DefaultRequestHeaders.IfNoneMatch.Add(EntityTagHeaderValue.Parse(etag.Value)); } - var response = await Extensions.RetryOnceAsync(() => Client.GetAsync(url), TimeSpan.FromMinutes(5)); + var response = await Client.GetAsync(url); IEnumerable headerVal; if (response.Headers.TryGetValues("X-RateLimit-Limit", out headerVal)) { @@ -402,13 +402,13 @@ internal async Task HttpGetAsync(string url, Ref etag = null, Re { case HttpStatusCode.Unauthorized: var unauth = await response.Content.ReadAsStringAsync(); - var unauthorizedError = DeserializeWithErrorHandling(unauth); - Logger.Error($"{nameof(GitHubAuthenticationFailureException)}: {unauthorizedError.Message}"); + var unauthorizedError = DeserializeWithErrorHandling(unauth, supress_error: true); + Logger.Error($"{nameof(GitHubAuthenticationFailureException)}: {unauthorizedError?.Message ?? unauth}"); throw new GitHubAuthenticationFailureException(); case HttpStatusCode.Forbidden: var forbid = await response.Content.ReadAsStringAsync(); - var forbidError = DeserializeWithErrorHandling(forbid); - Logger.Error($"{nameof(GitHubRateLimitExceededException)}: {forbidError.Message}"); + var forbidError = DeserializeWithErrorHandling(forbid, supress_error: true); + Logger.Error($"{nameof(GitHubRateLimitExceededException)}: {forbidError?.Message ?? forbid}"); throw new GitHubRateLimitExceededException(); case HttpStatusCode.NotModified: //Returns null because we have nothing to update if nothing was modified @@ -416,8 +416,10 @@ internal async Task HttpGetAsync(string url, Ref etag = null, Re default: var str = await response.Content.ReadAsStringAsync(); - var error = DeserializeWithErrorHandling(str); - throw error.ToException(); + if (str == "Not Found") + throw new GitHubUnknownErrorException("Not Found"); + var error = DeserializeWithErrorHandling(str, supress_error: true); + throw error?.ToException() ?? new GitHubUnknownErrorException(str); } } if (etag != null && response.Headers.TryGetValues("ETag", out headerVal)) @@ -459,7 +461,7 @@ private static Repo AddRepo(string repoDir) } } - private static T DeserializeWithErrorHandling(string content) + private static T DeserializeWithErrorHandling(string content, bool supress_error = false) { try { @@ -468,7 +470,10 @@ private static T DeserializeWithErrorHandling(string content) catch (Exception ex) { Logger.Error($"{nameof(GitHubDeserializationException)}: {ex.Message}"); - throw new GitHubDeserializationException(ex); + if (!supress_error) + throw new GitHubDeserializationException(ex); + else + return default(T); } } diff --git a/SemDiff.Core/RepoFile.cs b/SemDiff.Core/RepoFile.cs index 0b3a33f..291e2e1 100644 --- a/SemDiff.Core/RepoFile.cs +++ b/SemDiff.Core/RepoFile.cs @@ -4,6 +4,7 @@ using Microsoft.CodeAnalysis.CSharp; using Newtonsoft.Json; using Newtonsoft.Json.Converters; +using SemDiff.Core.Exceptions; using System.IO; using System.Threading.Tasks; @@ -41,6 +42,11 @@ public enum StatusEnum public string Filename { get; set; } + /// + /// There are cases where GitHub's diff messes up; See #82 + /// + public bool IsInvalid { get; set; } + /// /// The current file from the open pull request /// @@ -58,19 +64,27 @@ public enum StatusEnum internal async Task DownloadFileAsync() { - var baseTsk = ParentRepo.HttpGetAsync($@"https://github.com/{ParentRepo.Owner}/{ParentRepo.RepoName}/raw/{ParentPullRequst.Base.Sha}/{Filename}"); - var headTsk = ParentRepo.HttpGetAsync($@"https://github.com/{ParentRepo.Owner}/{ParentRepo.RepoName}/raw/{ParentPullRequst.Head.Sha}/{Filename}"); + try + { + var baseTsk = ParentRepo.HttpGetAsync($@"https://github.com/{ParentRepo.Owner}/{ParentRepo.RepoName}/raw/{ParentPullRequst.Base.Sha}/{Filename}"); + var headTsk = ParentRepo.HttpGetAsync($@"https://github.com/{ParentRepo.Owner}/{ParentRepo.RepoName}/raw/{ParentPullRequst.Head.Sha}/{Filename}"); - var text = await Task.WhenAll(baseTsk, headTsk); - var baseText = text[0]; - var headText = text[1]; + var text = await Task.WhenAll(baseTsk, headTsk); + var baseText = text[0]; + var headText = text[1]; - BaseTree = CSharpSyntaxTree.ParseText(baseText); - HeadTree = CSharpSyntaxTree.ParseText(headText); + BaseTree = CSharpSyntaxTree.ParseText(baseText); + HeadTree = CSharpSyntaxTree.ParseText(headText); - Directory.CreateDirectory(Path.GetDirectoryName(CachePathBase)); - File.WriteAllText(CachePathBase, baseText); - File.WriteAllText(CachePathHead, headText); + Directory.CreateDirectory(Path.GetDirectoryName(CachePathBase)); + File.WriteAllText(CachePathBase, baseText); + File.WriteAllText(CachePathHead, headText); + } + catch (GitHubUnknownErrorException ex) when (ex.Message == "Not Found") + { + //See #82 + IsInvalid = true; + } } internal void LoadFromCache() diff --git a/SemDiff.Test/FalsePositiveAnalysisTests.cs b/SemDiff.Test/FalsePositiveAnalysisTests.cs index 5805e4b..14f7b2e 100644 --- a/SemDiff.Test/FalsePositiveAnalysisTests.cs +++ b/SemDiff.Test/FalsePositiveAnalysisTests.cs @@ -61,7 +61,7 @@ private static void OneSide(Repo repo, SyntaxTree local, SyntaxTree remote, Synt }); var pr = repo.PullRequests.First(); pr.ParentRepo = repo; - pr.Files.First().ParentPullRequst = pr; + pr.ValidFiles.First().ParentPullRequst = pr; var res = Analysis.ForFalsePositive(repo, local); Assert.IsTrue(res.Any()); diff --git a/SemDiff.Test/PullRequestListTests.cs b/SemDiff.Test/PullRequestListTests.cs index 17a9d49..de77e2a 100644 --- a/SemDiff.Test/PullRequestListTests.cs +++ b/SemDiff.Test/PullRequestListTests.cs @@ -50,8 +50,8 @@ public void PullRequestFromTestRepo() if (r.Number == 4) { Assert.AreEqual(r.State, "open"); - Assert.AreEqual(r.Files.Count, 1); - foreach (var f in r.Files) + Assert.AreEqual(r.ValidFiles.Count(), 1); + foreach (var f in r.ValidFiles) { Assert.AreEqual("Curly-Broccoli/Curly/Logger.cs", f.Filename); } @@ -100,7 +100,7 @@ public void FilesPagination() var PRs = github.GetPullRequestsAsync().Result; Assert.AreEqual(1, PRs.Count); var pr = PRs.First(); - Assert.AreEqual(84, pr.Files.Count); + Assert.AreEqual(84, pr.ValidFiles.Count()); } [TestMethod] @@ -121,7 +121,7 @@ public void GetFilesFromGitHub() var counter = 0; string[] directoryTokens; var dir = ""; - var f = r.Files.Last(); + var f = r.ValidFiles.Last(); directoryTokens = f.Filename.Split('/'); dir = Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData) + "/SemDiff/semdiffdotnet/curly-broccoli/".ToLocalPath(); @@ -171,7 +171,7 @@ public void UpdateLocalSaved() Assert.AreEqual(local.Url, gPR.Url); Assert.IsNotNull(local.Head); Assert.IsNotNull(local.Base); - Assert.IsNotNull(local.Files); + Assert.IsNotNull(local.ValidFiles); } [TestMethod] @@ -182,7 +182,7 @@ public void RemoveUnusedLocalFiles() var prZero = requests.OrderBy(p => p.Number).First().Clone(); prZero.Number = 0; prZero.ParentRepo = github; - foreach (var f in prZero.Files) //Add files to fool it! + foreach (var f in prZero.ValidFiles) //Add files to fool it! { f.ParentPullRequst = prZero; new FileInfo(f.CachePathBase).Directory.Create(); @@ -219,7 +219,7 @@ public void NoUnnecessaryDownloading() r.GetFilesAsync().Wait(); if (r.Number == 1) { - foreach (var files in r.Files) + foreach (var files in r.ValidFiles) { path = files.Filename.Replace('/', Path.DirectorySeparatorChar); } diff --git a/SemDiff.Test/RepoTests.cs b/SemDiff.Test/RepoTests.cs index f5519e3..741c393 100644 --- a/SemDiff.Test/RepoTests.cs +++ b/SemDiff.Test/RepoTests.cs @@ -37,10 +37,10 @@ public void RepoGetChangedFiles() Assert.AreEqual(5, pulls.Count); foreach (var p in pulls) { - Assert.IsNotNull(p.Files); + Assert.IsNotNull(p.ValidFiles); Assert.IsNotNull(p.Title); Assert.AreNotEqual(default(DateTime), p.Updated); - foreach (var f in p.Files) + foreach (var f in p.ValidFiles) { Assert.IsNotNull(f.BaseTree); Assert.IsNotNull(f.HeadTree);