Skip to content

Report a friendly error for snapshot branches missing on swift.org#547

Open
adityasingh2400 wants to merge 2 commits into
swiftlang:mainfrom
adityasingh2400:fix-snapshot-branch-not-found-http-error
Open

Report a friendly error for snapshot branches missing on swift.org#547
adityasingh2400 wants to merge 2 commits into
swiftlang:mainfrom
adityasingh2400:fix-snapshot-branch-not-found-http-error

Conversation

@adityasingh2400
Copy link
Copy Markdown

When you ask swiftly for a snapshot branch that isn't published on swift.org, you currently get a raw, low-level HTTP error rather than something you can act on:

$ swiftly list-available 9.9-snapshot
Error: Unexpected response, expected status code: ok, response: undocumented(statusCode: 404, OpenAPIRuntime.UndocumentedPayload(...))

The same thing happens for swiftly install <branch>-snapshot and when updating a snapshot whose branch has aged out.

The root cause is that HTTPRequestExecutorImpl.getSnapshotToolchains forwarded the response straight through response.ok.body.json. swift.org returns a 404 for the dev-toolchains endpoint when the branch doesn't exist (or is older than the supported main and previous x.y releases), and .ok then throws the generic OpenAPI runtime error.

Interestingly, install, list-available, and update already catch SwiftlyHTTPClient.SnapshotBranchNotFoundError and turn it into a clear, actionable message. But nothing ever threw that error, so all three catch blocks were dead code.

This change detects the 404 from listDevToolchains and throws SnapshotBranchNotFoundError, reconstructing the branch from the SourceBranch that was requested so the message names the branch the user actually typed. The reconstruction is the inverse of the SourceBranch mapping already done in getSnapshotToolchains. Non-404 responses (for example a 5xx server error) keep propagating as before, so a transient server problem is not misreported as a missing branch.

With the fix, the same command now reports:

$ swiftly list-available 9.9-snapshot
Error: The snapshot branch 9.9 development was not found on swift.org. Note that snapshot toolchains are only available for the current `main` release and the previous x.y (major.minor) release.

Verification: added SnapshotBranchNotFoundTests, which feeds a synthetic 404 listDevToolchains response through the response handler and asserts it maps to SnapshotBranchNotFoundError with the correct branch (both main and a release branch), that a 200 still returns the toolchains untouched, that a 500 is not reported as a missing branch, and that the SourceBranch to branch reconstruction round-trips. The new tests fail before the change (the raw 404 error escapes) and pass after. The rest of the suite (171 tests across 21 suites, excluding the network-tagged HTTPClientTests) continues to pass, and swiftformat reports no changes for the touched files.

Resolves #539

When a snapshot branch isn't published on swift.org, the dev-toolchains
endpoint responds with a 404. swiftly was forwarding that straight
through `response.ok.body.json`, so `list-available 9.9-snapshot`,
`install 6.3-snapshot`, and snapshot updates surfaced the raw OpenAPI
failure ("Unexpected response, expected status code: ok, response:
undocumented(statusCode: 404, ...)") instead of something actionable.

`install`, `list-available`, and `update` already catch
`SwiftlyHTTPClient.SnapshotBranchNotFoundError` and turn it into a clear
message, but nothing ever threw it, so those catch blocks were dead. Map
a 404 from `listDevToolchains` to that error, reconstructing the branch
from the requested `SourceBranch` so the message names the branch the
user asked for. Non-404 responses (for example a 5xx) keep propagating
as before so a server error isn't misreported as a missing branch.

Resolves: swiftlang#539
Comment thread Sources/SwiftlyCore/HTTPClient.swift Outdated
/// supported `main` and previous x.y releases). That response is surfaced as a typed
/// `SnapshotBranchNotFoundError` so callers can present an actionable message instead of the
/// raw, low-level HTTP failure.
static func devToolchains(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this function is just wrapping an if-statement. Lets inline the if-statement back up into the original function.

Per review, fold the SnapshotBranchNotFoundError mapping back into
getSnapshotToolchains instead of a standalone devToolchains helper. Keep the
branch-reconstruction round-trip test, which covers the part with real logic;
the response-mapping tests are dropped because they relied on the extracted
helper as their seam and the suite has no transport mock to drive the public
method offline.
@adityasingh2400
Copy link
Copy Markdown
Author

Done in de066ad. Inlined the 404 check straight into getSnapshotToolchains and removed the devToolchains helper.

One heads-up on the tests: that helper was also the seam the unit tests used to exercise the 404 to SnapshotBranchNotFoundError mapping offline, and the suite has no mock ClientTransport to drive getSnapshotToolchains without a network call. So with the helper gone I dropped the response-mapping tests and kept branchReconstructionRoundTrips, which covers the part with actual logic (rebuilding the requested branch so the error carries it). The 404 check itself is now a one-line inline pattern match.

If you would rather keep that path covered, I am happy to add a small mock-transport test that returns a 404 and asserts getSnapshotToolchains throws the typed error. Let me know your preference.

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

Successfully merging this pull request may close these issues.

swiftly list-available for an unknown branch returns a low-level http error

2 participants