Skip to content

Commit 1abd9a2

Browse files
ZachNagengastZachNagengast
authored
Fix inverted logic on registry cache expiration (#9144)
1 parent d8ae00b commit 1abd9a2

File tree

2 files changed

+219
-17
lines changed

2 files changed

+219
-17
lines changed

‎Sources/PackageRegistry/RegistryClient.swift‎

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -397,8 +397,8 @@ public final class RegistryClient: AsyncCancellable {
397397
timeout: DispatchTimeInterval?,
398398
observabilityScope: ObservabilityScope
399399
) async throws -> Serialization.VersionMetadata {
400-
let cacheKey = MetadataCacheKey(registry: registry, package: package)
401-
if let cached = self.metadataCache[cacheKey], cached.expires < .now() {
400+
let cacheKey = MetadataCacheKey(registry: registry, package: package, version: version)
401+
if let cached = self.metadataCache[cacheKey], cached.expires > .now() {
402402
return cached.metadata
403403
}
404404

@@ -1403,21 +1403,9 @@ public final class RegistryClient: AsyncCancellable {
14031403
}
14041404
}
14051405

1406-
private func unwrapRegistry(from package: PackageIdentity) throws -> (PackageIdentity.RegistryIdentity, Registry) {
1407-
guard let registryIdentity = package.registry else {
1408-
throw RegistryError.invalidPackageIdentity(package)
1409-
}
1410-
1411-
guard let registry = self.configuration.registry(for: registryIdentity.scope) else {
1412-
throw RegistryError.registryNotConfigured(scope: registryIdentity.scope)
1413-
}
1414-
1415-
return (registryIdentity, registry)
1416-
}
1417-
14181406
// If the registry is available, the function returns, otherwise an error
14191407
// explaining why the registry is unavailable is thrown.
1420-
private func withAvailabilityCheck(
1408+
func withAvailabilityCheck(
14211409
registry: Registry,
14221410
observabilityScope: ObservabilityScope
14231411
) async throws {
@@ -1438,7 +1426,7 @@ public final class RegistryClient: AsyncCancellable {
14381426
}
14391427
}
14401428

1441-
if let cached = self.availabilityCache[registry.url], cached.expires < .now() {
1429+
if let cached = self.availabilityCache[registry.url], cached.expires > .now() {
14421430
return try availabilityHandler(cached.status)
14431431
}
14441432

@@ -1453,6 +1441,18 @@ public final class RegistryClient: AsyncCancellable {
14531441
return try availabilityHandler(result)
14541442
}
14551443

1444+
private func unwrapRegistry(from package: PackageIdentity) throws -> (PackageIdentity.RegistryIdentity, Registry) {
1445+
guard let registryIdentity = package.registry else {
1446+
throw RegistryError.invalidPackageIdentity(package)
1447+
}
1448+
1449+
guard let registry = self.configuration.registry(for: registryIdentity.scope) else {
1450+
throw RegistryError.registryNotConfigured(scope: registryIdentity.scope)
1451+
}
1452+
1453+
return (registryIdentity, registry)
1454+
}
1455+
14561456
private func unexpectedStatusError(
14571457
_ response: HTTPClientResponse,
14581458
expectedStatus: [Int]
@@ -1495,6 +1495,7 @@ public final class RegistryClient: AsyncCancellable {
14951495
private struct MetadataCacheKey: Hashable {
14961496
let registry: Registry
14971497
let package: PackageIdentity.RegistryIdentity
1498+
let version: Version
14981499
}
14991500
}
15001501

‎Tests/PackageRegistryTests/RegistryClientTests.swift‎

Lines changed: 202 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
//
1111
//===----------------------------------------------------------------------===//
1212

13-
import Basics
13+
@testable import Basics
1414
import _Concurrency
1515
import Foundation
1616
import PackageFingerprint
@@ -440,6 +440,126 @@ fileprivate var availabilityURL = URL("\(registryURL)/availability")
440440
assert(metadataSync)
441441
}
442442

443+
@Test func getPackageVersionMetadataInCache() async throws {
444+
let checksumAlgorithm: HashAlgorithm = MockHashAlgorithm()
445+
let expectedChecksums: [Version: String] = [
446+
Version("1.1.1"): "a2ac54cf25fbc1ad0028f03f0aa4b96833b83bb05a14e510892bb27dea4dc812",
447+
Version("1.1.0"): checksumAlgorithm.hash(emptyZipFile).hexadecimalRepresentation
448+
]
449+
450+
let counter = SendableBox(0)
451+
let handler: HTTPClient.Implementation = { request, _ in
452+
await counter.increment()
453+
switch (request.method, request.url) {
454+
case (.get, releasesURL.appending(component: "1.1.1")):
455+
let expectedChecksum = expectedChecksums[Version("1.1.1")]!
456+
#expect(request.headers.get("Accept").first == "application/vnd.swift.registry.v1+json")
457+
458+
let data = """
459+
{
460+
"id": "mona.LinkedList",
461+
"version": "1.1.1",
462+
"resources": [
463+
{
464+
"name": "source-archive",
465+
"type": "application/zip",
466+
"checksum": "\(expectedChecksum)"
467+
}
468+
],
469+
"metadata": {
470+
"author": {
471+
"name": "J. Appleseed"
472+
},
473+
"licenseURL": "https://github.com/mona/LinkedList/license",
474+
"readmeURL": "https://github.com/mona/LinkedList/readme",
475+
"repositoryURLs": [
476+
"https://github.com/mona/LinkedList",
477+
"ssh://git@github.com:mona/LinkedList.git",
478+
"git@github.com:mona/LinkedList.git"
479+
]
480+
}
481+
}
482+
""".data(using: .utf8)!
483+
484+
return .init(
485+
statusCode: 200,
486+
headers: .init([
487+
.init(name: "Content-Length", value: "\(data.count)"),
488+
.init(name: "Content-Type", value: "application/json"),
489+
.init(name: "Content-Version", value: "1"),
490+
]),
491+
body: data
492+
)
493+
case (.get, releasesURL.appending(component: "1.1.0")):
494+
let expectedChecksum = expectedChecksums[Version("1.1.0")]!
495+
#expect(request.headers.get("Accept").first == "application/vnd.swift.registry.v1+json")
496+
497+
let data = """
498+
{
499+
"id": "mona.LinkedList",
500+
"version": "1.1.0",
501+
"resources": [
502+
{
503+
"name": "source-archive",
504+
"type": "application/zip",
505+
"checksum": "\(expectedChecksum)",
506+
}
507+
],
508+
"metadata": {
509+
"author": {
510+
"name": "J. Appleseed"
511+
},
512+
"licenseURL": "https://github.com/mona/LinkedList/license",
513+
"readmeURL": "https://github.com/mona/LinkedList/readme",
514+
"repositoryURLs": [
515+
"https://github.com/mona/LinkedList",
516+
"ssh://git@github.com:mona/LinkedList.git",
517+
"git@github.com:mona/LinkedList.git"
518+
]
519+
}
520+
}
521+
""".data(using: .utf8)!
522+
523+
return .init(
524+
statusCode: 200,
525+
headers: .init([
526+
.init(name: "Content-Length", value: "\(data.count)"),
527+
.init(name: "Content-Type", value: "application/json"),
528+
.init(name: "Content-Version", value: "1"),
529+
]),
530+
body: data
531+
)
532+
default:
533+
throw StringError("method and url should match")
534+
}
535+
}
536+
537+
let httpClient = HTTPClient(implementation: handler)
538+
var configuration = RegistryConfiguration()
539+
configuration.defaultRegistry = Registry(url: registryURL, supportsAvailability: false)
540+
541+
let registryClient = makeRegistryClient(configuration: configuration, httpClient: httpClient)
542+
543+
var expectedRequestCount = 0
544+
try await check(version: Version("1.1.1"), expectCached: false)
545+
try await check(version: Version("1.1.0"), expectCached: false)
546+
try await check(version: Version("1.1.1"), expectCached: true)
547+
try await check(version: Version("1.1.0"), expectCached: true)
548+
549+
func check(version: Version, expectCached: Bool) async throws {
550+
let metadata = try await registryClient.getPackageVersionMetadata(package: identity, version: version)
551+
552+
if !expectCached {
553+
expectedRequestCount += 1
554+
}
555+
556+
let count = await counter.value
557+
#expect(count == expectedRequestCount)
558+
#expect(metadata.author?.name == "J. Appleseed")
559+
#expect(metadata.resources[0].checksum == expectedChecksums[version]!)
560+
}
561+
}
562+
443563
func getPackageVersionMetadata_404() async throws {
444564
let serverErrorHandler = ServerErrorHandler(
445565
method: .get,
@@ -3701,6 +3821,87 @@ fileprivate var availabilityURL = URL("\(registryURL)/availability")
37013821
try await registryClient.checkAvailability(registry: registry)
37023822
}
37033823
}
3824+
3825+
@Test func withAvailabilityCheck() async throws {
3826+
let handler: HTTPClient.Implementation = { request, _ in
3827+
switch (request.method, request.url) {
3828+
case (.get, availabilityURL):
3829+
return .okay()
3830+
default:
3831+
throw StringError("method and url should match")
3832+
}
3833+
}
3834+
3835+
let httpClient = HTTPClient(implementation: handler)
3836+
let registry = Registry(url: registryURL, supportsAvailability: true)
3837+
3838+
let registryClient = makeRegistryClient(
3839+
configuration: .init(),
3840+
httpClient: httpClient
3841+
)
3842+
3843+
try await registryClient.withAvailabilityCheck(
3844+
registry: registry,
3845+
observabilityScope: ObservabilitySystem.NOOP
3846+
)
3847+
}
3848+
3849+
@Test func withAvailabilityCheckServerError() async throws {
3850+
let handler: HTTPClient.Implementation = { request, _ in
3851+
switch (request.method, request.url) {
3852+
case (.get, availabilityURL):
3853+
return .serverError(reason: "boom")
3854+
default:
3855+
throw StringError("method and url should match")
3856+
}
3857+
}
3858+
3859+
let httpClient = HTTPClient(implementation: handler)
3860+
let registry = Registry(url: registryURL, supportsAvailability: true)
3861+
3862+
let registryClient = makeRegistryClient(
3863+
configuration: .init(),
3864+
httpClient: httpClient
3865+
)
3866+
3867+
await #expect(throws: StringError("unknown server error (500)")) {
3868+
try await registryClient.withAvailabilityCheck(
3869+
registry: registry,
3870+
observabilityScope: ObservabilitySystem.NOOP
3871+
)
3872+
}
3873+
}
3874+
3875+
@Test func withAvailabilityCheckInCache() async throws {
3876+
let counter = SendableBox(0)
3877+
let handler: HTTPClient.Implementation = { request, _ in
3878+
await counter.increment()
3879+
switch (request.method, request.url) {
3880+
case (.get, availabilityURL):
3881+
return .okay()
3882+
default:
3883+
throw StringError("method and url should match")
3884+
}
3885+
}
3886+
3887+
let httpClient = HTTPClient(implementation: handler)
3888+
let registry = Registry(url: registryURL, supportsAvailability: true)
3889+
3890+
let registryClient = makeRegistryClient(
3891+
configuration: .init(),
3892+
httpClient: httpClient
3893+
)
3894+
3895+
// Request count should not increase after first check
3896+
for _ in 0..<5 {
3897+
try await registryClient.withAvailabilityCheck(
3898+
registry: registry,
3899+
observabilityScope: ObservabilitySystem.NOOP
3900+
)
3901+
let count = await counter.value
3902+
#expect(count == 1)
3903+
}
3904+
}
37043905
}
37053906

37063907
// MARK: - Sugar

0 commit comments

Comments
 (0)