diff --git a/CLAUDE.md b/CLAUDE.md index 8e9c7d4..b9339a3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -414,8 +414,8 @@ keyed lookup would work only because `HttpResponseMessage` happens to canonicali casing. A `Retry-After` carrying an HTTP date rather than seconds yields `null`, since an unset `ResetsAt` is better than an invented one. -**A repository from a hosting provider carries no `LocalPath`, and is addressed by the host's own -id.** `GitRepository.LocalPath` is nullable because a repository a provider enumerated has never been +**A repository from a hosting provider carries no `LocalPath`, and how it is addressed is the +provider's own decision.** `GitRepository.LocalPath` is nullable because a repository a provider enumerated has never been cloned. Both providers used to invent one under `Environment.CurrentDirectory`, which made the same remote repository yield a different record depending on when it was enumerated, and which forced a containment guard to exist purely to keep a name from a remote response from escaping that @@ -423,14 +423,36 @@ directory. That guard is gone with the reason for it, and `IGitClient.Clone(GitR reports a missing `LocalPath` rather than resurrecting the invented default — where a working copy goes is the caller's decision. -`GitRepository.HostRepositoryId` carries what a host documents its own API in terms of. Microsoft's +`GitRepository.HostRepositoryId` carries what a host documents its own API in terms of — and the two +hosts disagree about what that is, which is why the preference is `GitProvider`'s +`private protected abstract bool PrefersHostRepositoryId` rather than one shared answer. Microsoft's reference types Azure DevOps's `{repositoryId}` path parameter as `string (uuid)` and draws an explicit id-or-name distinction for the sibling `project` parameter while withholding it here, so -substituting a name there is unconfirmed against the documented schema rather than sanctioned by it. -`GetPullRequestsAsync(GitRepository)` and `CreatePullRequest(GitRepository)` therefore prefer the id; -the `GitRepositoryName`-taking overloads still pass a name, since that is all they are given. Both -route through one internal core taking the finished path segment, so the choice lives in exactly one -place and the overloads cannot answer the same question differently. +substituting a name there is unconfirmed against the documented schema rather than sanctioned by it: +`AzureDevOpsProvider` prefers the id. GitHub's `{repo}` slot in `/repos/{owner}/{repo}` takes a +repository **name** only — its id-addressed form is the separate `/repositories/{id}` route — so +`GitHubProvider` prefers the name. A single shared preference for the id used to be applied to both, +which put GitHub's numeric id in a slot that only accepts a name and made +`GetPullRequestsAsync(GitRepository)` and `CreatePullRequest(GitRepository)` answer `404` on GitHub +for a repository just enumerated from the same provider — the overload `IGitHostingProvider` +documents as the one to prefer. + +The preference only decides which form wins when a `GitRepository` carries both. Which form was +chosen travels with it, on the internal `GitRepositoryAddress` record struct both internal cores now +take, so a repository carrying only an id is still addressed correctly on GitHub — through Octokit's +`long repositoryId` overloads, which build the id-addressed route — rather than by dropping an id +into the name slot. A bare string could not carry that distinction and a provider cannot recover it +from the value's shape, since a repository may legitimately be *named* a number. +`AzureDevOpsProvider` has one slot for both forms and ignores the discriminator entirely. The +`GitRepositoryName`-taking overloads still pass a name, since that is all they are given, and both +overloads still route through one internal core, so the choice lives in exactly one place and the +two cannot answer the same question differently. + +`FakeHttpMessageHandler.RespondToPath` exists because of this bug: the test that should have caught +it asserted the wrong URL against a handler that returned `[]` for every path, so the whole method +ran its success path around a request GitHub would have refused. A path-scoped response answers an +unexpected path with `404`, the way the host does, which makes a wrong route fail rather than pass +quietly. Prefer it over plain `Respond` for anything whose route is part of what the test claims. **A field a host reported goes through `GitProvider.ToHostValue`, never through `As()` directly.** The hosting counterpart to `GitParseValues.ToSemantic`, and it exists for the same reason: a value diff --git a/GitIntegration.Test/Fakes/FakeHttpMessageHandler.cs b/GitIntegration.Test/Fakes/FakeHttpMessageHandler.cs index 0fe1de6..82766a6 100644 --- a/GitIntegration.Test/Fakes/FakeHttpMessageHandler.cs +++ b/GitIntegration.Test/Fakes/FakeHttpMessageHandler.cs @@ -66,7 +66,34 @@ internal sealed class FakeHttpMessageHandler : HttpMessageHandler /// The same handler, to allow chaining. public FakeHttpMessageHandler Respond(HttpStatusCode status, string body, params (string Name, string Value)[] headers) { - _responses.Enqueue(new QueuedResponse(status, body, headers)); + _responses.Enqueue(new QueuedResponse(status, body, headers, ExpectedPath: null)); + _totalQueued++; + + return this; + } + + /// + /// Queues the next response, to be returned only if the request that arrives asks for + /// ; any other path is answered 404. + /// + /// + /// A route assertion the code under test has to satisfy to get its scripted response at all, + /// rather than one made afterwards on a recorded URI. A test that only asserts the URI still + /// receives the success body it scripted, so everything downstream of the route — deserialization, + /// mapping, the returned model — is exercised against a request the host would have refused. That + /// is how a wrong route survived here: a test asserted a URI addressing a repository by an id + /// GitHub's name slot cannot accept, and the always-succeeding handler made the whole method look + /// correct around it. Answering an unexpected path the way the host would makes the wrong route + /// fail where it actually fails. + /// + /// The the request must carry, compared exactly. + /// The status code the response carries when the path matches. + /// The response body, sent as UTF-8 text, when the path matches. + /// Header name/value pairs to attach, routed as routes them. + /// The same handler, to allow chaining. + public FakeHttpMessageHandler RespondToPath(string expectedPath, HttpStatusCode status, string body, params (string Name, string Value)[] headers) + { + _responses.Enqueue(new QueuedResponse(status, body, headers, expectedPath)); _totalQueued++; return this; @@ -115,6 +142,21 @@ protected override async Task SendAsync(HttpRequestMessage $"{_totalQueued} queued, {_requests.Count} arrived."); } + // The scripted response is conditional on the route when the test made it so, and the + // condition is checked after the dequeue rather than before it: a request that misses stays + // one request, consuming its own queued response, so a following request still gets the + // response the test queued for it rather than this one. + if (queued.ExpectedPath is string expectedPath && !string.Equals(request.RequestUri!.AbsolutePath, expectedPath, StringComparison.Ordinal)) + { + return new HttpResponseMessage(HttpStatusCode.NotFound) + { + Content = new StringContent( + $"{{\"message\":\"Not Found\",\"expectedPath\":\"{expectedPath}\",\"requestedPath\":\"{request.RequestUri.AbsolutePath}\"}}", + Encoding.UTF8, + "application/json"), + }; + } + HttpResponseMessage response = new(queued.Status) { Content = new StringContent(queued.Body, Encoding.UTF8), @@ -163,7 +205,7 @@ protected override void Dispose(bool disposing) base.Dispose(disposing); } - private readonly record struct QueuedResponse(HttpStatusCode Status, string Body, (string Name, string Value)[] Headers); + private readonly record struct QueuedResponse(HttpStatusCode Status, string Body, (string Name, string Value)[] Headers, string? ExpectedPath); /// A single request this handler received, captured in full. /// The HTTP method used. diff --git a/GitIntegration.Test/Fakes/FakeHttpMessageHandlerTests.cs b/GitIntegration.Test/Fakes/FakeHttpMessageHandlerTests.cs index 98cb02e..505bd48 100644 --- a/GitIntegration.Test/Fakes/FakeHttpMessageHandlerTests.cs +++ b/GitIntegration.Test/Fakes/FakeHttpMessageHandlerTests.cs @@ -87,6 +87,33 @@ public async Task NamesHowManyResponsesWereQueuedAndHowManyRequestsArrivedAsync( StringAssert.Contains(exception.Message, "2 arrived"); } + [TestMethod] + public async Task ReturnsAPathScopedResponseOnlyForThatPathAsync() + { + // The route has to be a condition of getting the scripted response, not merely something a + // test inspects afterwards: a handler that answers every path alike lets a wrong route run the + // whole success path and look correct, which is how a wrong route shipped here once. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .RespondToPath("/repos/contoso/my-repo/pulls", HttpStatusCode.OK, "[]") + .RespondToPath("/repos/contoso/my-repo/pulls", HttpStatusCode.OK, "[]"); + using HttpClient client = new(handler); + + using HttpResponseMessage matched = await client.GetAsync(new Uri("https://example.invalid/repos/contoso/my-repo/pulls?state=open"), TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + using HttpResponseMessage missed = await client.GetAsync(new Uri("https://example.invalid/repos/contoso/90000001/pulls"), TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + // The query string is no part of the comparison — a route is the path. + Assert.AreEqual(HttpStatusCode.OK, matched.StatusCode); + + Assert.AreEqual(HttpStatusCode.NotFound, missed.StatusCode); + StringAssert.Contains( + await missed.Content.ReadAsStringAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false), + "/repos/contoso/90000001/pulls"); + + // A missed request still consumed its own queued response rather than the next one's, so a + // following request is answered by what the test queued for it. + Assert.AreEqual(2, handler.Requests.Count); + } + [TestMethod] public async Task AttachesResponseHeadersWhenGivenAsync() { diff --git a/GitIntegration.Test/Hosting/GitHubProviderTests.cs b/GitIntegration.Test/Hosting/GitHubProviderTests.cs index fcb5b35..812f7bc 100644 --- a/GitIntegration.Test/Hosting/GitHubProviderTests.cs +++ b/GitIntegration.Test/Hosting/GitHubProviderTests.cs @@ -174,13 +174,15 @@ public async Task ThrowsAHostingFailureWhenAReportedNameIsNotOneThisLibraryCanRe } [TestMethod] - public async Task AddressesARepositoryByItsHostIdWhenListingPullRequestsAsync() + public async Task AddressesARepositoryByNameWhenListingPullRequestsAsync() { - // The point of carrying the id: it has to reach the request path. GitHub happens to accept a - // name in this position, but both providers answer the same question the same way under one - // interface, so both prefer the host's own identifier when one is known. + // GET /repos/{owner}/{repo}/pulls takes a repository NAME in its {repo} slot — the id-addressed + // form is the separate /repositories/{id}/pulls. This provider used to send GitHub's numeric + // HostRepositoryId here, which addresses no repository at all and answers 404 for one that was + // just enumerated from the same provider. The repository below carries both forms, so this is + // the case where the preference itself decides, and the name has to win. using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() - .Respond(HttpStatusCode.OK, "[]", ("Content-Type", "application/json")); + .RespondToPath("/repos/contoso/my-repo/pulls", HttpStatusCode.OK, "[]", ("Content-Type", "application/json")); GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; GitRepository repository = new() @@ -191,7 +193,71 @@ public async Task AddressesARepositoryByItsHostIdWhenListingPullRequestsAsync() _ = await provider.GetPullRequestsAsync(repository, TestContext.CancellationTokenSource.Token).ConfigureAwait(false); - StringAssert.Contains(handler.Requests[0].Uri.AbsoluteUri, "/repos/contoso/90000001/pulls"); + Assert.AreEqual("/repos/contoso/my-repo/pulls", handler.Requests[0].Uri.AbsolutePath); + } + + [TestMethod] + public async Task AddressesARepositoryByItsHostIdOnTheIdRouteWhenNoNameIsKnownAsync() + { + // Preferring the name does not discard the id. A repository carrying only a HostRepositoryId — + // one a caller built by hand, since GetRepositoriesAsync always reports a name — is addressed + // on GitHub's id-addressed route rather than by dropping its id into the name slot. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .RespondToPath("/repositories/90000001/pulls", HttpStatusCode.OK, "[]", ("Content-Type", "application/json")); + GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + + GitRepository repository = new() { HostRepositoryId = "90000001".As() }; + + _ = await provider.GetPullRequestsAsync(repository, TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual("/repositories/90000001/pulls", handler.Requests[0].Uri.AbsolutePath); + } + + [TestMethod] + public async Task ReportsAHostRepositoryIdGitHubCannotBeAddressedByAsync() + { + // GitHostRepositoryId is unvalidated, because Azure DevOps's is a uuid and GitHub's a whole + // number, so the semantic type can enforce neither. A hand-built repository carrying a + // GitHub-impossible id is a caller's argument rather than anything a host reported — nothing is + // sent, so there is no response for a GitHostingException to describe. + using FakeHttpMessageHandler handler = new(); + GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + + GitRepository repository = new() { HostRepositoryId = "not-a-number".As() }; + + ArgumentException exception = await Assert.ThrowsExactlyAsync( + async () => await provider.GetPullRequestsAsync(repository, TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); + + StringAssert.Contains(exception.Message, "whole numbers"); + Assert.AreEqual(0, handler.Requests.Count); + } + + [TestMethod] + public async Task AddressesARepositoryByNameWhenCreatingAPullRequestAsync() + { + // CreatePullRequest(GitRepository) reaches GitHub's routes the same way GetPullRequestsAsync + // does, and carried the identical bug: Octokit's Create(owner, name, ...) builds + // POST /repos/{owner}/{repo}/pulls, so a numeric id in that slot 404s just as it does on the + // listing route. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .RespondToPath("/repos/contoso/my-repo/pulls", HttpStatusCode.Created, Fixture("github-pullrequest-created.json"), ("Content-Type", "application/json")); + GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + + GitRepository repository = new() + { + Name = "my-repo".As(), + HostRepositoryId = "90000001".As(), + }; + + _ = await provider.CreatePullRequest(repository) + .From("example-branch-1".As()) + .Into("main".As()) + .Titled("A title".As()) + .ExecuteAsync(TestContext.CancellationTokenSource.Token) + .ConfigureAwait(false); + + Assert.AreEqual("/repos/contoso/my-repo/pulls", handler.Requests[0].Uri.AbsolutePath); } [TestMethod] diff --git a/GitIntegration.Test/Hosting/GitProviderTests.cs b/GitIntegration.Test/Hosting/GitProviderTests.cs index 6bac4d2..ecfc603 100644 --- a/GitIntegration.Test/Hosting/GitProviderTests.cs +++ b/GitIntegration.Test/Hosting/GitProviderTests.cs @@ -215,10 +215,15 @@ private sealed class TestProvider : GitProvider public override Task> GetRepositoriesAsync(CancellationToken cancellationToken = default) => throw new NotSupportedException("Not exercised by these tests."); - internal override Task> GetPullRequestsCoreAsync(string repositoryIdentifier, CancellationToken cancellationToken) => + // Arbitrary, because this provider issues no request for the choice to matter. The member is + // abstract so that each real provider has to state its own host's answer rather than inherit + // one that is wrong for it — GitHub and Azure DevOps genuinely differ. + private protected override bool PrefersHostRepositoryId => false; + + internal override Task> GetPullRequestsCoreAsync(GitRepositoryAddress repositoryAddress, CancellationToken cancellationToken) => throw new NotSupportedException("Not exercised by these tests."); - internal override Task CreatePullRequestCoreAsync(string repositoryIdentifier, GitPullRequestSpecification specification, CancellationToken cancellationToken) => + internal override Task CreatePullRequestCoreAsync(GitRepositoryAddress repositoryAddress, GitPullRequestSpecification specification, CancellationToken cancellationToken) => throw new NotSupportedException("Not exercised by these tests."); public HostingCredential CallResolveCredential() => ResolveCredential(); diff --git a/GitIntegration.Test/Hosting/GitPullRequestCreateBuilderTests.cs b/GitIntegration.Test/Hosting/GitPullRequestCreateBuilderTests.cs index 296601d..0a9aaa2 100644 --- a/GitIntegration.Test/Hosting/GitPullRequestCreateBuilderTests.cs +++ b/GitIntegration.Test/Hosting/GitPullRequestCreateBuilderTests.cs @@ -192,12 +192,17 @@ private sealed class RecordingProvider : GitProvider public override Task> GetRepositoriesAsync(CancellationToken cancellationToken = default) => throw new NotSupportedException("Not exercised by the routing test."); - internal override Task> GetPullRequestsCoreAsync(string repositoryIdentifier, CancellationToken cancellationToken) => + // Arbitrary, because this provider issues no request for the choice to matter. The member is + // abstract so that each real provider has to state its own host's answer rather than inherit + // one that is wrong for it — GitHub and Azure DevOps genuinely differ. + private protected override bool PrefersHostRepositoryId => false; + + internal override Task> GetPullRequestsCoreAsync(GitRepositoryAddress repositoryAddress, CancellationToken cancellationToken) => throw new NotSupportedException("Not exercised by the routing test."); - internal override Task CreatePullRequestCoreAsync(string repositoryIdentifier, GitPullRequestSpecification specification, CancellationToken cancellationToken) + internal override Task CreatePullRequestCoreAsync(GitRepositoryAddress repositoryAddress, GitPullRequestSpecification specification, CancellationToken cancellationToken) { - Repository = repositoryIdentifier; + Repository = repositoryAddress.Value; Specification = specification; return Task.FromResult(CannedPullRequest); } diff --git a/GitIntegration/GitHubProvider.cs b/GitIntegration/GitHubProvider.cs index 36784ff..a7d21f9 100644 --- a/GitIntegration/GitHubProvider.cs +++ b/GitIntegration/GitHubProvider.cs @@ -45,6 +45,24 @@ public sealed class GitHubProvider : GitProvider /// private protected override HttpMessageHandler DefaultHandler => SharedHandler; + /// + /// + /// , because GitHub's repository-addressed routes are two routes rather + /// than one. GET /repos/{owner}/{repo}/pulls takes a repository name in its + /// {repo} slot; the id-addressed form is the separate GET /repositories/{id}/pulls. + /// A numeric id in the name slot addresses no repository at all and answers 404 — which + /// this provider surfaces as , indistinguishable from a + /// repository that really is gone. Azure DevOps, whose one {repositoryId} slot is + /// documented as taking the id, answers instead. + /// + /// Preferring the name does not discard the id: a repository carrying only a + /// still arrives here as one, and + /// and answer it + /// on the id-addressed route through Octokit's long repositoryId overloads. + /// + /// + private protected override bool PrefersHostRepositoryId => false; + /// /// /// Adds to the interface's remarks rather than restating them. @@ -84,9 +102,9 @@ public override async Task> GetRepositoriesAsync(Ca } /// - internal override async Task> GetPullRequestsCoreAsync(string repositoryIdentifier, CancellationToken cancellationToken) + internal override async Task> GetPullRequestsCoreAsync(GitRepositoryAddress repositoryAddress, CancellationToken cancellationToken) { - Ensure.NotNull(repositoryIdentifier); + Ensure.NotNull(repositoryAddress.Value); cancellationToken.ThrowIfCancellationRequested(); (GitHubClient client, IDisposable createdTransport) = CreateClient(); @@ -100,8 +118,13 @@ internal override async Task> GetPullRequestsCoreA // exists, so a future validating change to PullRequestRequest can never leak it. PullRequestRequest request = new() { State = ItemStateFilter.Open }; - IReadOnlyList pullRequests = await client.PullRequest - .GetAllForRepository(Owner.WeakString, repositoryIdentifier, request) + // Two routes, chosen by which form the address carries rather than by what the value looks + // like: the name overload builds /repos/{owner}/{repo}/pulls, the long overload builds + // /repositories/{id}/pulls. Sending either form down the other's route is what this + // provider used to do, and GitHub answers it with a 404 rather than a diagnosable failure. + IReadOnlyList pullRequests = await (repositoryAddress.IsHostRepositoryId + ? client.PullRequest.GetAllForRepository(ToOctokitRepositoryId(repositoryAddress), request) + : client.PullRequest.GetAllForRepository(Owner.WeakString, repositoryAddress.Value, request)) .ConfigureAwait(false); return [.. pullRequests.Select(ToGitPullRequest)]; @@ -113,9 +136,9 @@ internal override async Task> GetPullRequestsCoreA } /// - internal override async Task CreatePullRequestCoreAsync(string repositoryIdentifier, GitPullRequestSpecification specification, CancellationToken cancellationToken) + internal override async Task CreatePullRequestCoreAsync(GitRepositoryAddress repositoryAddress, GitPullRequestSpecification specification, CancellationToken cancellationToken) { - Ensure.NotNull(repositoryIdentifier); + Ensure.NotNull(repositoryAddress.Value); Ensure.NotNull(specification); cancellationToken.ThrowIfCancellationRequested(); @@ -134,8 +157,10 @@ internal override async Task CreatePullRequestCoreAsync(string r Draft = specification.IsDraft, }; - PullRequest created = await client.PullRequest - .Create(Owner.WeakString, repositoryIdentifier, newPullRequest) + // The same two routes GetPullRequestsCoreAsync chooses between, for the same reason. + PullRequest created = await (repositoryAddress.IsHostRepositoryId + ? client.PullRequest.Create(ToOctokitRepositoryId(repositoryAddress), newPullRequest) + : client.PullRequest.Create(Owner.WeakString, repositoryAddress.Value, newPullRequest)) .ConfigureAwait(false); return ToGitPullRequest(created); @@ -146,6 +171,31 @@ internal override async Task CreatePullRequestCoreAsync(string r } } + /// + /// Reads a holding GitHub's own repository id as the + /// Octokit's id-addressed overloads take. + /// + /// + /// is an unvalidated semantic string, because the two hosts + /// disagree about what one looks like — Azure DevOps's is a uuid, GitHub's a whole number — so + /// the type cannot enforce either and this is where GitHub's constraint is checked. A value that + /// is not a whole number reaches here only from a hand-built , since + /// fills the property from Repository.Id, which is a + /// already. That is a caller's argument rather than something a host + /// reported, so it raises and not a + /// — nothing was sent, and there is no response to describe. + /// + /// The address, whose is . + /// The repository id. + /// The id is not a whole number, so GitHub cannot be addressed by it. + private static long ToOctokitRepositoryId(GitRepositoryAddress repositoryAddress) => + long.TryParse(repositoryAddress.Value, NumberStyles.Integer, CultureInfo.InvariantCulture, out long repositoryId) + ? repositoryId + : throw new ArgumentException( + $"GitHub repository ids are whole numbers, and '{repositoryAddress.Value}' is not one. " + + "Address the repository by its Name instead.", + nameof(repositoryAddress)); + /// /// Creates an Octokit client wired to this provider's transport and credential. /// diff --git a/GitIntegration/GitProvider.cs b/GitIntegration/GitProvider.cs index 9e825fe..40225ce 100644 --- a/GitIntegration/GitProvider.cs +++ b/GitIntegration/GitProvider.cs @@ -77,81 +77,114 @@ public abstract class GitProvider : IGitHostingProvider /// public Task> GetPullRequestsAsync(GitRepositoryName repositoryName, CancellationToken cancellationToken = default) => - GetPullRequestsCoreAsync(Ensure.NotNull(repositoryName).WeakString, cancellationToken); + GetPullRequestsCoreAsync(GitRepositoryAddress.ByName(Ensure.NotNull(repositoryName).WeakString), cancellationToken); /// public Task> GetPullRequestsAsync(GitRepository repository, CancellationToken cancellationToken = default) => - GetPullRequestsCoreAsync(ToRepositoryIdentifier(repository), cancellationToken); + GetPullRequestsCoreAsync(ToRepositoryAddress(repository), cancellationToken); /// public IGitPullRequestCreateBuilder CreatePullRequest(GitRepositoryName repositoryName) { - string identifier = Ensure.NotNull(repositoryName).WeakString; + GitRepositoryAddress address = GitRepositoryAddress.ByName(Ensure.NotNull(repositoryName).WeakString); return new GitPullRequestCreateBuilder((specification, cancellationToken) => - CreatePullRequestCoreAsync(identifier, specification, cancellationToken)); + CreatePullRequestCoreAsync(address, specification, cancellationToken)); } /// public IGitPullRequestCreateBuilder CreatePullRequest(GitRepository repository) { - string identifier = ToRepositoryIdentifier(repository); + GitRepositoryAddress address = ToRepositoryAddress(repository); return new GitPullRequestCreateBuilder((specification, cancellationToken) => - CreatePullRequestCoreAsync(identifier, specification, cancellationToken)); + CreatePullRequestCoreAsync(address, specification, cancellationToken)); } /// - /// Chooses how a repository is addressed in a request path: by the host's own identifier when - /// one is known, and by name otherwise. + /// Gets a value indicating whether this host's repository-addressed routes want + /// in preference to . /// /// - /// is what a host documents its own API in terms of. - /// Azure DevOps types its {repositoryId} path parameter as string (uuid), and draws - /// an explicit id-or-name distinction for the sibling project parameter while withholding - /// it here — so substituting a name there is unconfirmed against the documented schema rather - /// than sanctioned by it. Preferring the id closes that gap for every repository a caller got - /// from GetRepositoriesAsync, which is the normal way to obtain one. /// - /// Falling back to rather than requiring the id, because a - /// caller may legitimately have constructed a by hand from a name - /// alone. That fallback is the same unconfirmed substitution the name-taking overloads make, and - /// it is no worse than what those overloads already do. + /// Per-provider rather than shared, because the two hosts genuinely disagree and a single answer + /// was wrong for one of them. Azure DevOps types its {repositoryId} path parameter as + /// string (uuid) and draws an explicit id-or-name distinction for the sibling + /// project parameter while withholding it here, so an id is what its documented schema + /// asks for. GitHub's {repo} slot in /repos/{owner}/{repo} takes a repository + /// name only — its id-addressed route is the separate /repositories/{id}, so + /// putting GitHub's numeric id in the name slot addresses nothing and answers 404 for a + /// repository that plainly exists. /// + /// + /// A preference rather than a requirement: this only decides which candidate wins when a + /// carries both. Which of the two a provider was handed reaches it on + /// , so a provider whose two routes differ + /// can still address either form correctly rather than guessing from the value's shape. + /// + /// + /// for the reason gives: every + /// provider this library ships lives in this assembly, and no externally-defined subclass can be + /// instantiated anyway. + /// + /// + /// to prefer the host's own id; to prefer the name. + private protected abstract bool PrefersHostRepositoryId { get; } + + /// + /// Chooses how a repository is addressed in a request path, in this host's preferred order. + /// + /// + /// Falls back to whichever form was not preferred rather than requiring the preferred one, + /// because a caller may legitimately have constructed a by hand + /// carrying only one of the two. The result says which form it carries, so a provider whose id + /// and name routes differ answers the fallback with the right route instead of sending one form + /// down the other's route — which is exactly the failure that made this per-provider. /// /// The repository to address. - /// The path segment identifying the repository. + /// The value identifying the repository, and which of the two forms it is. /// is . /// /// carries neither a /// nor a , so there is nothing to address it by. /// - private static string ToRepositoryIdentifier(GitRepository repository) + private GitRepositoryAddress ToRepositoryAddress(GitRepository repository) { Ensure.NotNull(repository); - return repository.HostRepositoryId?.WeakString - ?? repository.Name?.WeakString - ?? throw new ArgumentException( - "The repository carries neither a HostRepositoryId nor a Name, so there is no way to " + - "address it on the host.", - nameof(repository)); + GitRepositoryAddress? byHostId = repository.HostRepositoryId?.WeakString is string hostId + ? GitRepositoryAddress.ByHostRepositoryId(hostId) + : null; + + GitRepositoryAddress? byName = repository.Name?.WeakString is string name + ? GitRepositoryAddress.ByName(name) + : null; + + GitRepositoryAddress? chosen = PrefersHostRepositoryId + ? byHostId ?? byName + : byName ?? byHostId; + + return chosen ?? throw new ArgumentException( + "The repository carries neither a HostRepositoryId nor a Name, so there is no way to " + + "address it on the host.", + nameof(repository)); } /// - /// Retrieves the open pull requests for the repository a path segment identifies. + /// Retrieves the open pull requests for the repository an address identifies. /// /// /// The single implementation behind both public overloads, so the two cannot answer the same - /// question differently. Takes the finished path segment rather than a + /// question differently. Takes the resolved rather than a /// or a , because choosing between an - /// id and a name is a decision that belongs in one place — — - /// rather than repeated in each provider. + /// id and a name is a decision that belongs in one place — — + /// rather than repeated in each provider. The address carries which form was chosen, so a + /// provider that addresses ids and names through different routes picks the right one. /// - /// The path segment identifying the repository. + /// The address identifying the repository. /// A token to cancel the request. /// The repository's open pull requests, as reported by the host. - internal abstract Task> GetPullRequestsCoreAsync(string repositoryIdentifier, CancellationToken cancellationToken); + internal abstract Task> GetPullRequestsCoreAsync(GitRepositoryAddress repositoryAddress, CancellationToken cancellationToken); /// /// Creates the pull request a finished describes. @@ -163,14 +196,14 @@ private static string ToRepositoryIdentifier(GitRepository repository) /// this library ships lives in this assembly, so nothing outside it needs to implement this /// member. /// - /// - /// The path segment identifying the repository the pull request is opened against, already chosen - /// by or taken from a caller-supplied name. + /// + /// The address identifying the repository the pull request is opened against, already chosen by + /// or taken from a caller-supplied name. /// /// The pull request's finished configuration. /// A token to cancel the request. /// The pull request as the host reports it after creation. - internal abstract Task CreatePullRequestCoreAsync(string repositoryIdentifier, GitPullRequestSpecification specification, CancellationToken cancellationToken); + internal abstract Task CreatePullRequestCoreAsync(GitRepositoryAddress repositoryAddress, GitPullRequestSpecification specification, CancellationToken cancellationToken); /// /// Attempts to retrieve the credential for this provider from the credential cache. @@ -383,6 +416,41 @@ internal HostingCredential ResolveCredential() => }; } +/// +/// How one repository is addressed on a host: the value to send, and which of the two forms — +/// the host's own id, or the repository's name — that value is. +/// +/// +/// +/// The discriminator is the whole point of this type. A bare string cannot say which form it +/// carries, and a provider cannot recover that from the value's shape: GitHub's repository ids are +/// decimal digits, and a repository may legitimately be named one. Passing the two forms +/// interchangeably is exactly how a numeric id came to be sent where GitHub's /repos/{owner}/{repo} +/// route wants a name, which answers 404 rather than failing in any way a caller could read. +/// +/// +/// A provider whose id and name routes are the same path — Azure DevOps's {repositoryId} slot +/// — can ignore entirely and send . +/// +/// +/// The value identifying the repository, unescaped. +/// +/// when is the host's own repository id; +/// when it is the repository's name. +/// +internal readonly record struct GitRepositoryAddress(string Value, bool IsHostRepositoryId) +{ + /// Addresses a repository by the host's own repository id. + /// The host's repository id. + /// The address. + public static GitRepositoryAddress ByHostRepositoryId(string hostRepositoryId) => new(hostRepositoryId, IsHostRepositoryId: true); + + /// Addresses a repository by its name. + /// The repository's name. + /// The address. + public static GitRepositoryAddress ByName(string repositoryName) => new(repositoryName, IsHostRepositoryId: false); +} + /// /// The outcome of resolving a provider's credential: a bearer token, a username and password, or /// nothing — proceeding unauthenticated. diff --git a/GitIntegration/Hosting/AzureDevOpsProvider.cs b/GitIntegration/Hosting/AzureDevOpsProvider.cs index 0f61b82..f518469 100644 --- a/GitIntegration/Hosting/AzureDevOpsProvider.cs +++ b/GitIntegration/Hosting/AzureDevOpsProvider.cs @@ -73,6 +73,19 @@ public sealed class AzureDevOpsProvider : GitProvider /// private protected override HttpMessageHandler DefaultHandler => SharedHandler; + /// + /// + /// , and unchanged by the split that made this per-provider. Azure DevOps + /// has one repository-addressed path — .../repositories/{repositoryId}/... — and + /// Microsoft's reference types that parameter as string (uuid) while drawing an explicit + /// id-or-name distinction for the sibling project parameter without drawing one here, so + /// an id is what the documented schema asks for and a name there is unconfirmed rather than + /// sanctioned. Both forms go into that same slot, so this provider never needs to read + /// — unlike GitHub, whose id-addressed and + /// name-addressed routes are different routes. + /// + private protected override bool PrefersHostRepositoryId => true; + /// /// Gets the name of this Git provider. /// @@ -156,7 +169,7 @@ public override async Task> GetRepositoriesAsync(Ca /// needs none of this, because its endpoint documents no pagination at all. /// /// - /// {repositoryId} is filled with , which the caller's + /// {repositoryId} is filled with , which the caller's /// choice of overload decides. Microsoft's reference types that parameter as a repository /// id (GitRepository.id is a string (uuid)), and draws an explicit /// id-or-name distinction for the sibling project parameter without drawing one here — a @@ -170,9 +183,9 @@ public override async Task> GetRepositoriesAsync(Ca /// /// /// is . See 's remarks. - internal override async Task> GetPullRequestsCoreAsync(string repositoryIdentifier, CancellationToken cancellationToken) + internal override async Task> GetPullRequestsCoreAsync(GitRepositoryAddress repositoryAddress, CancellationToken cancellationToken) { - Ensure.NotNull(repositoryIdentifier); + string repositoryIdentifier = Ensure.NotNull(repositoryAddress.Value); cancellationToken.ThrowIfCancellationRequested(); EnsureProjectIsSet(); @@ -228,9 +241,9 @@ internal override async Task> GetPullRequestsCoreA /// rather than a specific status code. /// /// is . See 's remarks. - internal override async Task CreatePullRequestCoreAsync(string repositoryIdentifier, GitPullRequestSpecification specification, CancellationToken cancellationToken) + internal override async Task CreatePullRequestCoreAsync(GitRepositoryAddress repositoryAddress, GitPullRequestSpecification specification, CancellationToken cancellationToken) { - Ensure.NotNull(repositoryIdentifier); + string repositoryIdentifier = Ensure.NotNull(repositoryAddress.Value); Ensure.NotNull(specification); cancellationToken.ThrowIfCancellationRequested(); EnsureProjectIsSet();