[RFC][WIP] Step 1: Move The World

[shiftkey]

This is the first step in me rewriting the HTTP internals of Octokit as discussed in #984 - I've gone through this exercise a few times, and I think this is now simple enough to share with the world and actually commit to.

This is targeting a branch named http-client-milestone instead of master so I can work on this alongside other stuff - and others can get involved to hash out the API in parallel with Important Things™.

What this PR is about:

• showing what the new flow for creating a GitHubClient looks like
• rewriting the internals to then use the new HTTP bits
• getting tests passing again

What this PR isn't about:

• determining the obsoletion steps - I've used [Obsolete] here to help with refactoring, however nothing is set in stone
• new features - I've used Timeout as an example property because it came up in Provided a httpclient timeout option #965 and I'd really like to avoid adding more ctor parameters to GitHubClient

What I'm looking for feedback on:

• is setup too verbose? I'm going back and forth with @haacked on how much of the simpler ctors for GitHubClient we keep around, but see the Monkey test for what the new setup looks like right now
• how much backwards compatibility do others care about here?

Checklist:

• gather feedback
  • Ensure ClientInfo values are copied when creating HttpClient
  • Port existing setup for now, don't introduce new concepts like Timeout
  • Move required properties into ctor overloads
• reflect, update samples
• rewrite intro docs?

[M-Zuber]

If what I am saying is irrelevant, I will delete the comment to avoid clutter.

Would this be the time to try and introduce a way to use the lib without any auth - as can be done if using the api directly?
(I guess minimally you need PHV?)

[shiftkey]

Would this be the time to try and introduce a way to use the lib without any auth - as can be done if using the api directly?

To be honest, I'd make it hard to use this library without auth. The API is very restrictive unless you're using an Authorization header, and I don't really see much value in fighting against the API on this front.

In the earlier samples, I had a snippet like this:

var info = new ClientInfo
{
    AppName = "my-cool-app",
    Credentials = new Credentials("my-token"),
    Server = "https://enterprise.my-work.com",
    UseDefaultProxy = true
};

var http = HttpClientFactory.Create(info);
var client = new GitHubClient(http);

We don't currently do this, but I'd like for the Credentials to be an option you pass into ClientInfo - for most scenarios, I think this will satisfy their use case.

[M-Zuber]

I know it might be a lot to ask, but is it possible to put the smallest, most relevant code sample at the end of the first comment?
Or am I the only one that feels it would be useful?

[shiftkey]

@M-Zuber okay, so this is what I've currently got:

[IntegrationTest]
public void Setup()
{
    // TODO: what other options may we get here
    var info = new ClientInfo
    {
       Timeout = TimeSpan.MaxValue
    };

     // generate the client you need
     var client = HttpClientFactory.Create(info);

     // TODO: we probably need to guard here that we have enough
     //       information to use this client against the GitHub API
     var github = new GitHubClient(new Connection(new ProductHeaderValue("The Future?"), client));
}

The goal here (eventually?) is to find that right balance between convenience and customization

[M-Zuber]

To be honest, I'd make it hard to use this library without auth. The API is very restrictive unless you're using an Authorization header, and I don't really see much value in fighting against the API on this front

There are some use cases though where no auth is needed and it just gets in the way.
Today for example I was having trouble searching through the projects I had starred.
In 2 minutes I had a simple script that downloaded all my stars and filtered - with no auth needed.

But its not a big enough problem to make a real issue out of it, so bowing out on this one

[shiftkey]

There are some use cases though where no auth is needed and it just gets in the way.

Sure, and the rate-limiting will kick in quick enough for you to know. Anyway, nice feedback to know ✨

[M-Zuber]

Looks simple enough

// generate the client you need
var client = HttpClientFactory.Create(info);

This would need to be done before creating any new GitHubClient?
Or is there room for an overload that just takes the info and calls HttpClientFactory internally?

[shiftkey]

This would need to be done before creating any new GitHubClient?
Or is there room for an overload that just takes the info and calls HttpClientFactory internally?

Not necessarily - this is what @haacked and I are probably going to debate about in here.

On the one hand, most consumers won't care about the HTTP stack and just want to supply enough data to get going - a user-agent and perhaps some credentials. That scenario probably favours calling HttpClientFactory internally, like we did before.

On the other, if you're wanting to tweak some settings (proxy, timeout, or even some extensibility that the HttpMessageHandler pattern supports) then you're probably going to be happy with doing the setup yourself - or perhaps bringing your own HttpClient implementation.

[M-Zuber]

From a first glance, it looks nice,
Assuming I have time to contribute with code also - where you suggest I start?

[shiftkey]

Assuming I have time to contribute with code also - where you suggest I start?

Let's make sure this is the right direction, then I'll extract the tasks necessary to move on from there...

[shiftkey]

I've identified some properties on IGitHubClient and IConnection that may be worth removing while we're in here making these changes - see 0581dcb

[shiftkey]

And this is what composing a GitHubClient currently looks like:

var info = new ClientInfo
{
    UserAgent = "my-cool-app",
    // TODO: make this of type `Credentials` - no need to keep the store here
    Credentials = new InMemoryCredentialStore(new Credentials("my-token-here")),
    Server = new Uri("https://github.my-cool-enterprise.com"),
    Timeout = TimeSpan.MaxValue,
};

var github = new GitHubClient(info);

[distantcam]

👎

So my problem here is that certain properties (timeout for example) I would want to set per endpoint I use (long for uploading an attachment to a release, short default for the rest) rather than setting at a global connection level.

From an API point of view I'd like to see an optional RequestOptions object that I can pass, which modifies the behaviour of that web call, but doesn't affect all calls.

As for HttpClient, it would be nice if I can create my own instance, passing in a custom HttpMessageHandler that wraps the Octokit's RedirectHandler.

[M-Zuber]

public GitHubClient(ProductHeaderValue productInformation) { } 
public GitHubClient(ClientInfo info) { }
public GitHubClient(HttpClient httpClient) { } 

This is what the final situation will be once the dust settles, and there is no need for back compat (v1 maybe)?

If so, it looks good to me.
OTOH, I agree with @distantcam that there needs to be a way to adjust the values on a per request basis.

[M-Zuber]

OTOH, I agree with @distantcam that there needs to be a way to adjust the values on a per request basis.

I have given it some more thought, and I think this is a separate point.

I woulds like to be able to do something like the following:

var info = new ClientInfo
{
    // Defaults for most endpoints.
};

var github = new GitHubClient(info);

var releaseInfo = new ClientInfo
{
  // Any overrides specific for this endpoint.
}

github.Releases.SetOptions(releaseInfo);

As I believe such a scenario is for more advanced users, it is okay to ask them to perform the extra steps.
obviously if the api is fluent then its nicer:

var info = new ClientInfo
{
    // Defaults for most endpoints.
};

var releaseInfo = new ClientInfo
{
  // Any overrides specific for this endpoint.
}

// How exactly this should work  I am not sure
// Either separate methods for each endpoint, or some game with genrics/enum - just no magic strings 
var github = new GitHubClient(info).SetOptionsForReleases(releaseInfo);

[shiftkey]

@distantcam

As discussed in #963 we suspect the per-endpoint override (as in uploading release assets) will be ignored by the global setting - I'd love to get some more insight into that scenario as I know you've dealt with that in the past.

Putting that aside (it's still To Be Resolved™) I'm not planning to remove that behaviour around setting per-request overrides (especially for uploading release assets) from Octokit, but we still lack a way for users to set a timeout globally. This change makes it easier to achieve this, but isn't the only reason why we're going down this path.

From an API point of view I'd like to see an optional RequestOptions object that I can pass, which modifies the behaviour of that web call, but doesn't affect all calls.

This feels not unlike #760 which was around giving more control to pagination options for endpoints that return collections. What other parts of a request might you looking to modify, generally speaking?

[shiftkey]

@distantcam

As for HttpClient, it would be nice if I can create my own instance, passing in a custom HttpMessageHandler that wraps the Octokit's RedirectHandler.

I think this will be infinitely easier to do after this. I have some rough ideas in mind around extending the stack, but I may need to re-jig the System.Net.Http abstractions I'm using to get to a point where scenarios like yours are supported.

[distantcam]

I'd love to get some more insight into that scenario as I know you've been dealt with that in the past.

Ok the scenario was this - we have automatic scripts to deploy releases to GitHub. The script does various things:

• Find matching milestone for release version
• Create Release for the tag, with compiled notes from the milestones issues.
• Attach compiled release to the GitHub release
• Close the milestone
• Publish the release

We hit a problem with attaching the compiled binary to the release. If the binary was too large the timeout would cause the upload to fail. So we upped the timeout to several hours.

The problem is, I don't want any of the other tasks to wait several hours if they're going to timeout. For most of the communication a 60s timeout is fine, and only for the upload would I want to change that timeout.

If you're interested, our release project that uses Octokit.net is https://github.com/Particular/GitHubReleaseNotes

What other parts of a request might you looking to modify, generally speaking?

Well, I was looking into handling the request caching to avoid using up the API limit when polling a repo for changes. So I was trying to support #352.

So I guess what I'm really after here is a way to poll a repo for changes, and then have some automation happen because of that change. And suddenly as I write this I realized webhooks is a much better solution to this problem. Oh well.

[shiftkey]

For most of the communication a 60s timeout is fine, and only for the upload would I want to change that timeout.

So you're not seeing an issue with the current 100s limit we have? That's something I want to confirm - or investigate further because I'm rather puzzled by it all.

cc @naveensrinivasan

[distantcam]

So you're not seeing an issue with the current 100s limit we have?

Not currently no. The upload was bigger but we've trimmed it down and it hasn't been a problem in a while.

[naveensrinivasan]

@shiftkey Clone this repo https://github.com/naveensrinivasan/octokit-test which has the testing-data folder. Then run this octokit-release-upload-concurrent.linq in linqpad. We could reproduce the exception where the Http time's out. In this sample the code is trying to upload 11 files concurrently to GitHub Release.

[shiftkey]

@naveensrinivasan @distantcam thanks for the details. I think there needs to be some changes here around how Octokit supports timeout that satisfies both the global and specific requests:

1. if the endpoint specifies a timeout, convert this to a cancellation token and apply to the request
2. if a global setting is specified, convert into a cancellation token and apply to the request
3. whatever the provided HttpClient has configured for Timeout

We don't support that second scenario (perhaps that's the way out of this) and now I'm really grumpy at HttpClient.Timeout because I think it can still ruin this plan (not either of your faults).

Will think on a solution some more.

[haacked]

I think we could probably trim the constructors list down to...

public GitHubClient(ProductHeaderValue productInformation) { } 
public GitHubClient(ProductHeaderValue productInformation, ICredentialStore credentialStore) { } 
public GitHubClient(ProductHeaderValue productInformation, ICredentialStore credentialStore, Uri baseAddress) { } 
public GitHubClient(ClientInfo info) { }
public GitHubClient(HttpClient httpClient) { } 

My thinking is that given that ProductHeaderValue is that the only reason to specify a baseAddress is if you're calling GitHub Enterprise in which case, you must provide credentials. Thus the GitHubClient(ProductHeaderValue, Uri) is kind of pointless.

To make this even easier to use for one off scripts, we could even allow string overloads for product header where we would parse them and throw if they're invalid.

For example:

var client = new GitHubClient("Haackinator");
or
var client = new GitHubClient("Haackinator/3000");

Though I like that the ProductHeaderValue argument makes it impossible to get wrong, and this has the potential to throw exceptions.

[shiftkey]

Closing this out to take this off my plate. Don't worry, it'll return in some way, shape or colour.

[devkhan]

As @haacked mentioned, these are the ctors which are actually required:

public GitHubClient(ProductHeaderValue productInformation) { } 
public GitHubClient(ProductHeaderValue productInformation, ICredentialStore credentialStore) { } 
public GitHubClient(ProductHeaderValue productInformation, ICredentialStore credentialStore, Uri baseAddress) { } 
public GitHubClient(ClientInfo info) { }
public GitHubClient(HttpClient httpClient) { } 

But I think that if we only keep these:

public GitHubClient(ClientInfo info) { }
public GitHubClient(HttpClient httpClient) { }

with throwing exceptions in case ProductHeaderValue is not provided in info, it will be easier to maintain and forces user to follow a convention for providing config info. We can make a handful of ctors for ClientInfo. What do you think?