Skip to content

Commit

Permalink
Handle corner case #82
Browse files Browse the repository at this point in the history 
* Improve error handling with Http errors
* Introduce IsInvalid flag
  • Loading branch information
@CodyRay
CodyRay committed Apr 24, 2016
1 parent 0465d8a commit ac932ba
Show file tree
Hide file tree
Showing 7 changed files with 70 additions and 47 deletions.
2 changes: 1 addition & 1 deletion SemDiff.Core/Analysis.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ private static IEnumerable<Pull> 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))
Expand Down
8 changes: 6 additions & 2 deletions SemDiff.Core/PullRequest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ public class PullRequest
public string CacheDirectory => Path.Combine(ParentRepo.CacheDirectory, Number.ToString());

public IList<RepoFile> Files { get; set; }

[JsonIgnore]
public IEnumerable<RepoFile> ValidFiles => Files.Where(f => !f.IsInvalid);

public HeadBase Head { get; set; }
public DateTime LastWrite { get; set; } = DateTime.MinValue;
public int Number { get; set; }
Expand All @@ -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;
}
}
Expand Down
29 changes: 17 additions & 12 deletions SemDiff.Core/Repo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,15 +147,15 @@ 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;
}

if (VerifyPullRequestCache(p))
{
PullRequests.Add(p);
foreach (var r in p.Files)
foreach (var r in p.ValidFiles)
{
r.LoadFromCache();
}
Expand All @@ -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;
Expand Down Expand Up @@ -386,7 +386,7 @@ internal async Task<string> HttpGetAsync(string url, Ref<string> 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<string> headerVal;
if (response.Headers.TryGetValues("X-RateLimit-Limit", out headerVal))
{
Expand All @@ -402,22 +402,24 @@ internal async Task<string> HttpGetAsync(string url, Ref<string> etag = null, Re
{
case HttpStatusCode.Unauthorized:
var unauth = await response.Content.ReadAsStringAsync();
var unauthorizedError = DeserializeWithErrorHandling<GitHubError>(unauth);
Logger.Error($"{nameof(GitHubAuthenticationFailureException)}: {unauthorizedError.Message}");
var unauthorizedError = DeserializeWithErrorHandling<GitHubError>(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<GitHubError>(forbid);
Logger.Error($"{nameof(GitHubRateLimitExceededException)}: {forbidError.Message}");
var forbidError = DeserializeWithErrorHandling<GitHubError>(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
return null;

default:
var str = await response.Content.ReadAsStringAsync();
var error = DeserializeWithErrorHandling<GitHubError>(str);
throw error.ToException();
if (str == "Not Found")
throw new GitHubUnknownErrorException("Not Found");
var error = DeserializeWithErrorHandling<GitHubError>(str, supress_error: true);
throw error?.ToException() ?? new GitHubUnknownErrorException(str);
}
}
if (etag != null && response.Headers.TryGetValues("ETag", out headerVal))
Expand Down Expand Up @@ -459,7 +461,7 @@ private static Repo AddRepo(string repoDir)
}
}

private static T DeserializeWithErrorHandling<T>(string content)
private static T DeserializeWithErrorHandling<T>(string content, bool supress_error = false)
{
try
{
Expand All @@ -468,7 +470,10 @@ private static T DeserializeWithErrorHandling<T>(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);
}
}

Expand Down
58 changes: 36 additions & 22 deletions SemDiff.Core/RepoFile.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -41,6 +42,11 @@ public enum StatusEnum

public string Filename { get; set; }

/// <summary>
/// There are cases where GitHub's diff messes up; See #82
/// </summary>
public bool IsInvalid { get; set; }

/// <summary>
/// The current file from the open pull request
/// </summary>
Expand All @@ -58,32 +64,40 @@ 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}");

var text = await Task.WhenAll(baseTsk, headTsk);
var baseText = text[0];
var headText = text[1];

if (ParentRepo.LineEndings == LineEndingType.crlf)
try
{
//Area for speed improvement
baseText = baseText.Replace("\r\n", "\n").Replace("\r", "\n").Replace("\n", "\r\n");
headText = headText.Replace("\r\n", "\n").Replace("\r", "\n").Replace("\n", "\r\n");
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];

if (ParentRepo.LineEndings == LineEndingType.crlf)
{
//Area for speed improvement
baseText = baseText.Replace("\r\n", "\n").Replace("\r", "\n").Replace("\n", "\r\n");
headText = headText.Replace("\r\n", "\n").Replace("\r", "\n").Replace("\n", "\r\n");
}
else
{
//Area for speed improvement
baseText = baseText.Replace("\r\n", "\n").Replace("\r", "\n");
headText = headText.Replace("\r\n", "\n").Replace("\r", "\n");
}

BaseTree = CSharpSyntaxTree.ParseText(baseText);
HeadTree = CSharpSyntaxTree.ParseText(headText);

Directory.CreateDirectory(Path.GetDirectoryName(CachePathBase));
File.WriteAllText(CachePathBase, baseText);
File.WriteAllText(CachePathHead, headText);
}
else
catch (GitHubUnknownErrorException ex) when (ex.Message == "Not Found")
{
//Area for speed improvement
baseText = baseText.Replace("\r\n", "\n").Replace("\r", "\n");
headText = headText.Replace("\r\n", "\n").Replace("\r", "\n");
//See #82
IsInvalid = true;
}

BaseTree = CSharpSyntaxTree.ParseText(baseText);
HeadTree = CSharpSyntaxTree.ParseText(headText);

Directory.CreateDirectory(Path.GetDirectoryName(CachePathBase));
File.WriteAllText(CachePathBase, baseText);
File.WriteAllText(CachePathHead, headText);
}

internal void LoadFromCache()
Expand Down
2 changes: 1 addition & 1 deletion SemDiff.Test/FalsePositiveAnalysisTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
14 changes: 7 additions & 7 deletions SemDiff.Test/PullRequestListTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,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);
}
Expand Down Expand Up @@ -88,7 +88,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]
Expand All @@ -106,7 +106,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();

Expand Down Expand Up @@ -157,7 +157,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]
Expand All @@ -169,7 +169,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();
Expand Down Expand Up @@ -208,7 +208,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);
}
Expand Down
4 changes: 2 additions & 2 deletions SemDiff.Test/RepoTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,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);
Expand Down

0 comments on commit ac932ba

Please sign in to comment.