Skip to content

Throw fingerprint write errors #3932

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

Merged
merged 1 commit into from
Dec 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions Sources/PackageRegistry/RegistryClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -432,10 +432,16 @@ public final class RegistryClient {
switch storageResult {
case .success:
completion(.success(checksum))
case .failure(PackageFingerprintStorageError.conflict(_, let existing)):
switch self.fingerprintCheckingMode {
case .strict:
completion(.failure(RegistryError.checksumChanged(latest: checksum, previous: existing.value)))
case .warn:
observabilityScope.emit(warning: "The checksum \(checksum) from \(registry.url.absoluteString) does not match previously recorded value \(existing.value) from \(String(describing: existing.origin.url?.absoluteString))")
completion(.success(checksum))
}
case .failure(let error):
// Don't throw write errors
observabilityScope.emit(warning: "Failed to save checksum '\(checksum) from \(registry.url) to fingerprints storage: \(error)")
completion(.success(checksum))
completion(.failure(error))
}
}
} else {
Expand Down
10 changes: 8 additions & 2 deletions Sources/Workspace/SourceControlPackageContainer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,14 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
callback: $0
)
}
} catch {
observabilityScope.emit(warning: "Failed to save revision '\(revision.identifier) from \(sourceControlURL) to fingerprints storage: \(error)")
} catch PackageFingerprintStorageError.conflict(_, let existing) {
let message = "Revision \(revision.identifier) for \(self.package) version \(version) does not match previously recorded value \(existing.value) from \(String(describing: existing.origin.url?.absoluteString))"
switch self.fingerprintCheckingMode {
case .strict:
throw StringError(message)
case .warn:
observabilityScope.emit(warning: message)
}
}
} catch {
self.observabilityScope.emit(error: "Failed to get source control fingerprint for \(self.package) version \(version) from storage: \(error)")
Expand Down
77 changes: 73 additions & 4 deletions Tests/PackageRegistryTests/RegistryClientTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,74 @@ final class RegistryClientTests: XCTestCase {
var configuration = RegistryConfiguration()
configuration.defaultRegistry = Registry(url: URL(string: registryURL)!)

let fingerprintStorage = MockPackageFingerprintStorage([
identity: [
version: [.registry: Fingerprint(origin: .registry(URL(string: registryURL)!), value: "non-matching checksum")],
],
])
let registryClient = makeRegistryClient(configuration: configuration,
httpClient: httpClient,
fingerprintStorage: fingerprintStorage,
fingerprintCheckingMode: .strict)

XCTAssertThrowsError(try registryClient.fetchSourceArchiveChecksum(package: identity, version: version)) { error in
guard case RegistryError.checksumChanged = error else {
return XCTFail("Expected RegistryError.checksumChanged, got \(error)")
}
}
}

func testFetchSourceArchiveChecksum_storageConflict_fingerprintChecking_warn() throws {
let registryURL = "https://packages.example.com"
let identity = PackageIdentity.plain("mona.LinkedList")
let (scope, name) = identity.scopeAndName!
let version = Version("1.1.1")
let metadataURL = URL(string: "\(registryURL)/\(scope)/\(name)/\(version)")!
let checksum = "a2ac54cf25fbc1ad0028f03f0aa4b96833b83bb05a14e510892bb27dea4dc812"

let handler: HTTPClient.Handler = { request, _, completion in
switch (request.method, request.url) {
case (.get, metadataURL):
XCTAssertEqual(request.headers.get("Accept").first, "application/vnd.swift.registry.v1+json")

let data = """
{
"id": "mona.LinkedList",
"version": "1.1.1",
"resources": [
{
"name": "source-archive",
"type": "application/zip",
"checksum": "\(checksum)"
}
],
"metadata": {
"description": "One thing links to another."
}
}
""".data(using: .utf8)!

completion(.success(.init(
statusCode: 200,
headers: .init([
.init(name: "Content-Length", value: "\(data.count)"),
.init(name: "Content-Type", value: "application/json"),
.init(name: "Content-Version", value: "1"),
]),
body: data
)))
default:
completion(.failure(StringError("method and url should match")))
}
}

var httpClient = HTTPClient(handler: handler)
httpClient.configuration.circuitBreakerStrategy = .none
httpClient.configuration.retryStrategy = .none

var configuration = RegistryConfiguration()
configuration.defaultRegistry = Registry(url: URL(string: registryURL)!)

let storedChecksum = "non-matching checksum"
let fingerprintStorage = MockPackageFingerprintStorage([
identity: [
Expand All @@ -359,21 +427,22 @@ final class RegistryClientTests: XCTestCase {
let registryClient = makeRegistryClient(configuration: configuration,
httpClient: httpClient,
fingerprintStorage: fingerprintStorage,
fingerprintCheckingMode: .strict)
fingerprintCheckingMode: .warn)

let observability = ObservabilitySystem.makeForTesting()

// The checksum differs from that in storage, but we don't throw write errors
// The checksum differs from that in storage, but error is not thrown
// because fingerprintCheckingMode=.warn
let checksumResponse = try registryClient.fetchSourceArchiveChecksum(
package: identity,
version: version,
observabilityScope: observability.topScope
)
XCTAssertEqual(checksum, checksumResponse)

// There should be a warning though
// But there should be a warning
testDiagnostics(observability.diagnostics) { result in
result.check(diagnostic: .contains("different from previously recorded value"), severity: .warning)
result.check(diagnostic: .contains("does not match previously recorded value"), severity: .warning)
}

// Storage should NOT be updated
Expand Down