Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Paging seems broken for Milestone.GetForRepository #169

Closed
SimonCropp opened this issue Nov 5, 2013 · 6 comments
Closed

Paging seems broken for Milestone.GetForRepository #169

SimonCropp opened this issue Nov 5, 2013 · 6 comments

Comments

@SimonCropp
Copy link
Contributor

This

[Test]
[Explicit]
public async void OctokitTests()
{
    var githubUsername = Environment.GetEnvironmentVariable("OCTOKIT_GITHUBUSERNAME");
    var githubPassword = Environment.GetEnvironmentVariable("OCTOKIT_GITHUBPASSWORD");

    var gitHubClient = new GitHubClient(new ProductHeaderValue("Foo"))
    {
        Credentials = new Credentials(githubUsername, githubPassword)
    };

    var stopwatch = Stopwatch.StartNew();
    try
    {
        await gitHubClient.Issue.Milestone.GetForRepository("Particular", "NServiceBus", new MilestoneRequest { State = ItemState.Closed });
    }
    finally
    {
        Debug.WriteLine(stopwatch.ElapsedMilliseconds);
    }
}

causes, according to fiddler, looping requests on https://api.github.com/repositories/1056713/milestones?state=closed&sort=due_date&direction=asc

I killed mine after ~40 requests sinve i did not want o max my rate limit

@shiftkey
Copy link
Member

shiftkey commented Nov 5, 2013

It's to do with the ApplyParameters extension we use. Here's a failing test:

[Fact]
public void DoesNotDropPageFromQueryStringWhenUsingParameters()
{
    var uri = new Uri("https://api.github.com/repositories/1/milestones?state=closed&sort=due_date&direction=asc&page=2");

    var parameters = new Dictionary<string, string> { { "state", "open" }, { "sort", "other"} };

    var actual = uri.ApplyParameters(parameters);

    Assert.True(actual.Query.Contains("page=2"));
}

cc @haacked if he wants to look into this fix

@shiftkey
Copy link
Member

shiftkey commented Nov 5, 2013

I've started a PR for this fix here

https://github.com/octokit/octokit.net/tree/milestones-kill-your-rate-limit

I haven't confirmed the milestones client works as expected, but that's easy to chase down now...

@SimonCropp
Copy link
Contributor Author

@haacked @shiftkey i notice 0.1.3 is out. did this one make it into the release?

@SimonCropp
Copy link
Contributor Author

https://github.com/octokit/octokit.net/releases/tag/v0.1.3

"Fixed bug in applying query parameters that could cause paging to continually request the same page"

looks like it. so shall we close this?

@haacked
Copy link
Contributor

haacked commented Nov 6, 2013

@SimonCropp Can you try it out?

@haacked haacked closed this as completed Nov 6, 2013
@SimonCropp
Copy link
Contributor Author

Works on my machine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants