RepositoriesClient.GetAllPublic() fails on GitHub Enterprise due to incorrect endpoint Uri

[ryangribble]

Due to shenanigans with the way dotnet joinsUri objects together, if any relative Uri endpoint starts with a leading slash, this "wipes out" the /api/v3/ component of the BaseAddress for GitHub Enterprise.

There were 2 occurrences of endpoint Uri's being set in ApiUrls helper class that had incorrect leading slashes on them, causing these calls (both for RepositoriesClient.GetAllPublic()) to return HTTP 406 "NotAccepted" errors when run against GitHub Enterprise due to the Url being incorrect. These have now been corrected.

Additionally I have added a safety in HttpClientAdapter class, when constructing the full uri, to trim any leading slashes off relative Uri endpoint before joining it to BaseAddress.
@shiftkey is this OK or is there another way you'd prefer to guard against this?

[shiftkey]

is this OK or is there another way you'd prefer to guard against this?

I'm a bit 👎 on the guard clause as-is but I'll sleep on it - or we can discuss it in a separate PR.

Everything else is 💎

[ryangribble]

I'm a bit 👎 on the guard clause as-is but I'll sleep on it - or we can discuss it in a separate PR.
Yeah it was a bit clunky due to code analysis not allowing combining a Uri and string and also the StartsWith() needing to have StringCompareInfo set etc.

I'm not a huge fan of the guard either... but given that we aren't running integration tests against github enterprise it pretty much means that we could break any "normal" call from working against GHE if we miss that innocuous leading slash when reviewing PRs.

Other things I considered were doing something in the FormatUri() extension method, or having a convention test that checked the endpoint Uris in ApiArls.cs helper class... but in both cases that didn't protect against a contribution that doesnt use the helper class or extension method (assuming we dont pull up the contributor in the PR and ask them to use the helper class and extension method).

Then we have the other situation of any consumers of octokit that are using the client.Connection.Get<T> etc methods, and passing in their own endpoint Uri. Perhaps we dont need to "stop" them since they can fix their code (although whether they would know that the HTTP 406 is because the Uri fragments got eaten due to a leading slash, and not being able to easily debug right down into the guts of the octokit plumbing, it would lead to much hair pulling!!).

But yeah my main concern was the fact we can inadvertently break API methods from working on Enterprise and it doesnt get picked up in tests or at runtime. We could throw an exception I guess (again, that's too late if we've shipped the release though!). So that's how I came to the conclusion that there doesnt seem to be a reason NOT to trim leading slashes off. The check that it isnt an AboluteUri is needed because when dealing with multple paginated calls, the subsequent requests have the full absolute Uri in the endpoint...

[shiftkey]

@ryangribble I'd love to script out something so we can verify all these URIs are correctly formatted, rather that bake that behaviour into HttpClientAdapter which may go away if we move to having HttpClient at our core. I think we could probably hack something quick and dirty together with reflection to cover ourselves...

[ryangribble]

OK ive reverted the change in HttpClientAdapter for now, and will just push this PR in with the correction to the URI's

Will raise a new issue for the

script out something so we can verify all these URIs are correctly formatted

part...

[shiftkey]

@ryangribble thanks!

[shiftkey]

release_notes: RepositoriesClient.GetAllPublic() failed on GitHub Enterprise usage due to incorrect Uri formatting