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

Implement GetAllAsync so we can inspect the status code #240

Closed
shiftkey opened this issue Nov 26, 2013 · 5 comments · Fixed by #288
Closed

Implement GetAllAsync so we can inspect the status code #240

shiftkey opened this issue Nov 26, 2013 · 5 comments · Fixed by #288
Assignees

Comments

@shiftkey
Copy link
Member

Some parts of the API should show a specific error message when you:

  • interact with an API that uses a GetAll
  • the status code returned when the input cannot be found is 404
  • we should handle this and return a human-friendly error message

Relates to #238

@ghost ghost assigned shiftkey Nov 26, 2013
@hnrkndrssn
Copy link
Contributor

Let's see if I have understood this one correctly...

For example in the ReferencesClient we have GetAll and GetAllForSubNamespace, which call ApiConnection.GetAll<>.
Now, instead of calling ApiConnection.GetAll<> we'll create a new GetAllAsync<> on ApiConnection which will, in addition to what the existing call to ApiConnection.GetPage<> does, BUT also check the HttpStatusCode of the response and if it's a 404 return a human friendly error message?

And this should be done for all the GetAll type methods?

@shiftkey
Copy link
Member Author

It's kinda fiddly, which is why I put it aside but here's the abridged version:

  • internally, ApiConnection.GetAll<T> just calls ApiPagination.GetAllPages

return _pagination.GetAllPages(async () => await GetPage<T>(uri, parameters, accepts)

  • ApiPagination.GetAllPages will enumerate all pages and likely return an empty collection

Perhaps we should have a consistent response inside ApiPagination.GetAllPages when you get a 404

The example I'd like to get passing as part of this is #242 - because when you specify a namespace that is not valid, you should get a specific error message rather than an empty collection.

https://github.com/octokit/octokit.net/blob/master/Octokit.Tests.Integration/Clients/ReferencesClientTests.cs#L66

@hnrkndrssn
Copy link
Contributor

OK, I'll see if I can come up with some ingenious solution to said problem 😆

@hnrkndrssn
Copy link
Contributor

We could pass in the URI being called to the ApiPagination.GetAllPages and in the event of a 404, we return a NotFoundException with an error message of _[URI] was not found._?

That would at least inform the user specifically what was not found, instead of just _Not Found_.

New signature of ApiPagination.GetAllPages would be:

GetAllPages<T>(Func<Task<IReadOnlyPagedCollection<T>>> getFirstPage, Uri uri)

So the example test for #242 would look like:

[IntegrationTest]
public async Task CanGetErrorForInvalidNamespace()
{
    var owner = "octokit";
    var repo = "octokit.net";
    var subNamespace = "666";

    var result = await AssertEx.Throws<NotFoundException>(
        async () => { await _fixture.GetAllForSubNamespace(owner, repo, subNamespace); });
    Assert.Equal(string.Format("{0} was not found.", ApiUrls.Reference(owner, repo, subNamespace)), result.Message);
}

Happy to discuss, I have made changes locally with the above scenario in mind, I can send it through PR for review if you like.

@shiftkey
Copy link
Member Author

@alfhenrik I like it

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