From edf8d16297649eb6fed6f63640a740779d1e61b2 Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Tue, 21 Jan 2025 05:45:42 +0100 Subject: [PATCH 1/8] Move Current.fetchS3Readme to S3Client.fetchS3Readme --- .../PackageController+routes.swift | 3 +- Sources/App/Core/AppEnvironment.swift | 2 - Sources/App/Core/Dependencies/S3Client.swift | 47 +++++++ .../AppTests/Mocks/AppEnvironment+mock.swift | 1 - .../PackageController+routesTests.swift | 119 ++++++++++-------- 5 files changed, 115 insertions(+), 57 deletions(-) create mode 100644 Sources/App/Core/Dependencies/S3Client.swift diff --git a/Sources/App/Controllers/PackageController+routes.swift b/Sources/App/Controllers/PackageController+routes.swift index 7e7829b2e..dae7c8251 100644 --- a/Sources/App/Controllers/PackageController+routes.swift +++ b/Sources/App/Controllers/PackageController+routes.swift @@ -306,7 +306,8 @@ enum PackageController { } do { - let readme = try await Current.fetchS3Readme(req.client, owner, repository) + @Dependency(\.s3Client) var s3Client + let readme = try await s3Client.fetchS3Readme(req.client, owner, repository) guard let branch = pkg.repository?.defaultBranch else { return PackageReadme.View(model: .cacheLookupFailed(url: readmeHtmlUrl)).document() } diff --git a/Sources/App/Core/AppEnvironment.swift b/Sources/App/Core/AppEnvironment.swift index 2e780efb0..e25bf96ec 100644 --- a/Sources/App/Core/AppEnvironment.swift +++ b/Sources/App/Core/AppEnvironment.swift @@ -23,7 +23,6 @@ import FoundationNetworking struct AppEnvironment: Sendable { - var fetchS3Readme: @Sendable (_ client: Client, _ owner: String, _ repository: String) async throws -> String var fileManager: FileManager var getStatusCount: @Sendable (_ client: Client, _ status: Gitlab.Builder.Status) async throws -> Int var git: Git @@ -53,7 +52,6 @@ extension AppEnvironment { nonisolated(unsafe) static var logger: Logger! static let live = AppEnvironment( - fetchS3Readme: { client, owner, repo in try await S3Readme.fetchReadme(client:client, owner: owner, repository: repo) }, fileManager: .live, getStatusCount: { client, status in try await Gitlab.Builder.getStatusCount(client: client, diff --git a/Sources/App/Core/Dependencies/S3Client.swift b/Sources/App/Core/Dependencies/S3Client.swift new file mode 100644 index 000000000..27fa4bb36 --- /dev/null +++ b/Sources/App/Core/Dependencies/S3Client.swift @@ -0,0 +1,47 @@ +// Copyright Dave Verwer, Sven A. Schmidt, and other contributors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import Dependencies +import DependenciesMacros +import Vapor + + +@DependencyClient +struct S3Client { +#warning("drop client parameter") + var fetchS3Readme: @Sendable (_ client: Client, _ owner: String, _ repository: String) async throws -> String +} + +extension S3Client: DependencyKey { + static var liveValue: Self { + .init( + fetchS3Readme: { client, owner, repo in + try await S3Readme.fetchReadme(client:client, owner: owner, repository: repo) + } + ) + } +} + +extension S3Client: TestDependencyKey { + static var testValue: Self { Self() } +} + +extension DependencyValues { + var s3Client: S3Client { + get { self[S3Client.self] } + set { self[S3Client.self] = newValue } + } +} + + diff --git a/Tests/AppTests/Mocks/AppEnvironment+mock.swift b/Tests/AppTests/Mocks/AppEnvironment+mock.swift index 431a89238..b2e1ad262 100644 --- a/Tests/AppTests/Mocks/AppEnvironment+mock.swift +++ b/Tests/AppTests/Mocks/AppEnvironment+mock.swift @@ -22,7 +22,6 @@ import Vapor extension AppEnvironment { static func mock(eventLoop: EventLoop) -> Self { .init( - fetchS3Readme: { _, _, _ in "" }, fileManager: .mock, getStatusCount: { _, _ in 100 }, git: .mock, diff --git a/Tests/AppTests/PackageController+routesTests.swift b/Tests/AppTests/PackageController+routesTests.swift index 3d84274b1..e0026c2f5 100644 --- a/Tests/AppTests/PackageController+routesTests.swift +++ b/Tests/AppTests/PackageController+routesTests.swift @@ -167,75 +167,87 @@ class PackageController_routesTests: SnapshotTestCase { func test_readme_basic() async throws { // Test readme fragment happy path - // setup - let pkg = try await savePackage(on: app.db, "1") - try await Repository(package: pkg, defaultBranch: "main", name: "package", owner: "owner", readmeHtmlUrl: "html url") - .save(on: app.db) - try await Version(package: pkg, latest: .defaultBranch).save(on: app.db) - let req = Request(application: app, on: app.eventLoopGroup.next()) - req.parameters.set("owner", to: "owner") - req.parameters.set("repository", to: "package") - Current.fetchS3Readme = { _, _, _ in #"
readme content
"# } + try await withDependencies { + $0.s3Client.fetchS3Readme = { @Sendable _, _, _ in + #"
readme content
"# + } + } operation: { + // setup + let pkg = try await savePackage(on: app.db, "1") + try await Repository(package: pkg, defaultBranch: "main", name: "package", owner: "owner", readmeHtmlUrl: "html url") + .save(on: app.db) + try await Version(package: pkg, latest: .defaultBranch).save(on: app.db) + let req = Request(application: app, on: app.eventLoopGroup.next()) + req.parameters.set("owner", to: "owner") + req.parameters.set("repository", to: "package") - // MUT - let node = try await PackageController.readme(req: req) + // MUT + let node = try await PackageController.readme(req: req) - // validate - XCTAssertEqual(node.render(indentedBy: .spaces(2)), + // validate + XCTAssertEqual(node.render(indentedBy: .spaces(2)), """ readme content """) + } } func test_readme_no_readmeHtmlUrl() async throws { // Test readme fragment when there's no readme html url - // setup - let pkg = try await savePackage(on: app.db, "1") - try await Repository(package: pkg, name: "package", owner: "owner", readmeHtmlUrl: nil) - .save(on: app.db) - try await Version(package: pkg, latest: .defaultBranch).save(on: app.db) - let req = Request(application: app, on: app.eventLoopGroup.next()) - req.parameters.set("owner", to: "owner") - req.parameters.set("repository", to: "package") - Current.fetchS3Readme = { _, _, _ in - XCTFail("must not be called") - return "" - } + try await withDependencies { + $0.s3Client.fetchS3Readme = { @Sendable _, _, _ in + XCTFail("must not be called") + return "" + } + } operation: { + // setup + let pkg = try await savePackage(on: app.db, "1") + try await Repository(package: pkg, name: "package", owner: "owner", readmeHtmlUrl: nil) + .save(on: app.db) + try await Version(package: pkg, latest: .defaultBranch).save(on: app.db) + let req = Request(application: app, on: app.eventLoopGroup.next()) + req.parameters.set("owner", to: "owner") + req.parameters.set("repository", to: "package") - // MUT - let node = try await PackageController.readme(req: req) + // MUT + let node = try await PackageController.readme(req: req) - // validate - XCTAssertEqual(node.render(indentedBy: .spaces(2)), + // validate + XCTAssertEqual(node.render(indentedBy: .spaces(2)), """

This package does not have a README file.

""") + } } func test_readme_error() async throws { // Test readme fragment when fetchS3Readme throws - // setup - struct Error: Swift.Error { } - Current.fetchS3Readme = { _, _, _ in throw Error() } - let pkg = try await savePackage(on: app.db, "1") - try await Repository(package: pkg, - name: "package", - owner: "owner", - readmeHtmlUrl: "html url", - s3Readme: .cached(s3ObjectUrl: "", githubEtag: "") - ).save(on: app.db) - try await Version(package: pkg, latest: .defaultBranch).save(on: app.db) - let req = Request(application: app, on: app.eventLoopGroup.next()) - req.parameters.set("owner", to: "owner") - req.parameters.set("repository", to: "package") - - // MUT - let node = try await PackageController.readme(req: req) - - // validate - XCTAssertEqual(node.render(indentedBy: .spaces(2)), + try await withDependencies { + $0.s3Client.fetchS3Readme = { @Sendable _, _, _ in + struct Error: Swift.Error { } + throw Error() + } + } operation: { + // setup + let pkg = try await savePackage(on: app.db, "1") + try await Repository(package: pkg, + name: "package", + owner: "owner", + readmeHtmlUrl: "html url", + s3Readme: .cached(s3ObjectUrl: "", githubEtag: "") + ).save(on: app.db) + try await Version(package: pkg, latest: .defaultBranch).save(on: app.db) + let req = Request(application: app, on: app.eventLoopGroup.next()) + req.parameters.set("owner", to: "owner") + req.parameters.set("repository", to: "package") + + // MUT + let node = try await PackageController.readme(req: req) + + // validate + XCTAssertEqual(node.render(indentedBy: .spaces(2)), """

This package's README file couldn't be loaded. Try @@ -243,10 +255,11 @@ class PackageController_routesTests: SnapshotTestCase {

""") - let app = self.app! - try await XCTAssertEqualAsync(try await Repository.query(on: app.db).count(), 1) - let s3Readme = try await XCTUnwrapAsync(try await Repository.query(on: app.db).first()?.s3Readme) - XCTAssert(s3Readme.isError) + let app = self.app! + try await XCTAssertEqualAsync(try await Repository.query(on: app.db).count(), 1) + let s3Readme = try await XCTUnwrapAsync(try await Repository.query(on: app.db).first()?.s3Readme) + XCTAssert(s3Readme.isError) + } } func test_releases() async throws { From 235a1c3a6484e130ccd8000224855400a8dcf88b Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Tue, 21 Jan 2025 06:02:33 +0100 Subject: [PATCH 2/8] Renames --- Sources/App/Controllers/PackageController+routes.swift | 4 ++-- Sources/App/Core/Dependencies/S3Client.swift | 6 +++--- Tests/AppTests/PackageController+routesTests.swift | 10 +++++----- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Sources/App/Controllers/PackageController+routes.swift b/Sources/App/Controllers/PackageController+routes.swift index dae7c8251..d3f3882ca 100644 --- a/Sources/App/Controllers/PackageController+routes.swift +++ b/Sources/App/Controllers/PackageController+routes.swift @@ -306,8 +306,8 @@ enum PackageController { } do { - @Dependency(\.s3Client) var s3Client - let readme = try await s3Client.fetchS3Readme(req.client, owner, repository) + @Dependency(\.s3) var s3 + let readme = try await s3.fetchReadme(req.client, owner, repository) guard let branch = pkg.repository?.defaultBranch else { return PackageReadme.View(model: .cacheLookupFailed(url: readmeHtmlUrl)).document() } diff --git a/Sources/App/Core/Dependencies/S3Client.swift b/Sources/App/Core/Dependencies/S3Client.swift index 27fa4bb36..b50f7c550 100644 --- a/Sources/App/Core/Dependencies/S3Client.swift +++ b/Sources/App/Core/Dependencies/S3Client.swift @@ -20,13 +20,13 @@ import Vapor @DependencyClient struct S3Client { #warning("drop client parameter") - var fetchS3Readme: @Sendable (_ client: Client, _ owner: String, _ repository: String) async throws -> String + var fetchReadme: @Sendable (_ client: Client, _ owner: String, _ repository: String) async throws -> String } extension S3Client: DependencyKey { static var liveValue: Self { .init( - fetchS3Readme: { client, owner, repo in + fetchReadme: { client, owner, repo in try await S3Readme.fetchReadme(client:client, owner: owner, repository: repo) } ) @@ -38,7 +38,7 @@ extension S3Client: TestDependencyKey { } extension DependencyValues { - var s3Client: S3Client { + var s3: S3Client { get { self[S3Client.self] } set { self[S3Client.self] = newValue } } diff --git a/Tests/AppTests/PackageController+routesTests.swift b/Tests/AppTests/PackageController+routesTests.swift index e0026c2f5..429bb604c 100644 --- a/Tests/AppTests/PackageController+routesTests.swift +++ b/Tests/AppTests/PackageController+routesTests.swift @@ -168,7 +168,7 @@ class PackageController_routesTests: SnapshotTestCase { func test_readme_basic() async throws { // Test readme fragment happy path try await withDependencies { - $0.s3Client.fetchS3Readme = { @Sendable _, _, _ in + $0.s3.fetchReadme = { @Sendable _, _, _ in #"
readme content
"# } } operation: { @@ -195,7 +195,7 @@ class PackageController_routesTests: SnapshotTestCase { func test_readme_no_readmeHtmlUrl() async throws { // Test readme fragment when there's no readme html url try await withDependencies { - $0.s3Client.fetchS3Readme = { @Sendable _, _, _ in + $0.s3.fetchReadme = { @Sendable _, _, _ in XCTFail("must not be called") return "" } @@ -225,7 +225,7 @@ class PackageController_routesTests: SnapshotTestCase { func test_readme_error() async throws { // Test readme fragment when fetchS3Readme throws try await withDependencies { - $0.s3Client.fetchS3Readme = { @Sendable _, _, _ in + $0.s3.fetchReadme = { @Sendable _, _, _ in struct Error: Swift.Error { } throw Error() } @@ -242,10 +242,10 @@ class PackageController_routesTests: SnapshotTestCase { let req = Request(application: app, on: app.eventLoopGroup.next()) req.parameters.set("owner", to: "owner") req.parameters.set("repository", to: "package") - + // MUT let node = try await PackageController.readme(req: req) - + // validate XCTAssertEqual(node.render(indentedBy: .spaces(2)), """ From e64a1b7755a79dae5d23c1e9c4692a97996da16f Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Tue, 21 Jan 2025 06:27:33 +0100 Subject: [PATCH 3/8] Move Current.storeS3Readme to S3Client.storeS3Readme --- Sources/App/Commands/Ingestion.swift | 3 ++- Sources/App/Core/AppEnvironment.swift | 6 ----- Sources/App/Core/Dependencies/S3Client.swift | 19 ++++++++++++--- Tests/AppTests/IngestionTests.swift | 24 +++++++++---------- .../AppTests/Mocks/AppEnvironment+mock.swift | 1 - 5 files changed, 30 insertions(+), 23 deletions(-) diff --git a/Sources/App/Commands/Ingestion.swift b/Sources/App/Commands/Ingestion.swift index c330c6a33..2e88c247a 100644 --- a/Sources/App/Commands/Ingestion.swift +++ b/Sources/App/Commands/Ingestion.swift @@ -240,12 +240,13 @@ enum Ingestion { static func storeS3Readme(client: Client, repository: Repository, metadata: Github.Metadata, readme: Github.Readme?) async throws(S3Readme.Error) -> S3Readme? { + @Dependency(\.s3) var s3 if let upstreamEtag = readme?.etag, repository.s3Readme?.needsUpdate(upstreamEtag: upstreamEtag) ?? true, let owner = metadata.repositoryOwner, let repository = metadata.repositoryName, let html = readme?.html { - let objectUrl = try await Current.storeS3Readme(owner, repository, html) + let objectUrl = try await s3.storeS3Readme(owner, repository, html) if let imagesToCache = readme?.imagesToCache, imagesToCache.isEmpty == false { try await Current.storeS3ReadmeImages(client, imagesToCache) } diff --git a/Sources/App/Core/AppEnvironment.swift b/Sources/App/Core/AppEnvironment.swift index e25bf96ec..9a6b78345 100644 --- a/Sources/App/Core/AppEnvironment.swift +++ b/Sources/App/Core/AppEnvironment.swift @@ -32,9 +32,6 @@ struct AppEnvironment: Sendable { var logger: @Sendable () -> Logger var setLogger: @Sendable (Logger) -> Void var shell: Shell - var storeS3Readme: @Sendable (_ owner: String, - _ repository: String, - _ readme: String) async throws(S3Readme.Error) -> String var storeS3ReadmeImages: @Sendable (_ client: Client, _ imagesToCache: [Github.Readme.ImageToCache]) async throws(S3Readme.Error) -> Void var triggerBuild: @Sendable (_ client: Client, @@ -70,9 +67,6 @@ extension AppEnvironment { logger: { logger }, setLogger: { logger in Self.logger = logger }, shell: .live, - storeS3Readme: { owner, repo, readme throws(S3Readme.Error) in - try await S3Readme.storeReadme(owner: owner, repository: repo, readme: readme) - }, storeS3ReadmeImages: { client, images throws(S3Readme.Error) in try await S3Readme.storeReadmeImages(client: client, imagesToCache: images) }, diff --git a/Sources/App/Core/Dependencies/S3Client.swift b/Sources/App/Core/Dependencies/S3Client.swift index b50f7c550..4f59670ef 100644 --- a/Sources/App/Core/Dependencies/S3Client.swift +++ b/Sources/App/Core/Dependencies/S3Client.swift @@ -13,14 +13,19 @@ // limitations under the License. import Dependencies -import DependenciesMacros +import IssueReporting import Vapor -@DependencyClient +// We currently cannot use @DependencyClient here due to +// https://github.com/pointfreeco/swift-dependencies/discussions/324 +//@DependencyClient struct S3Client { #warning("drop client parameter") var fetchReadme: @Sendable (_ client: Client, _ owner: String, _ repository: String) async throws -> String + var storeS3Readme: @Sendable (_ owner: String, _ repository: String, _ readme: String) async throws(S3Readme.Error) -> String = { _, _, _ in + reportIssue("storeS3Readme"); return "" + } } extension S3Client: DependencyKey { @@ -28,13 +33,21 @@ extension S3Client: DependencyKey { .init( fetchReadme: { client, owner, repo in try await S3Readme.fetchReadme(client:client, owner: owner, repository: repo) + }, + storeS3Readme: { owner, repo, readme throws(S3Readme.Error) in + try await S3Readme.storeReadme(owner: owner, repository: repo, readme: readme) } ) } } extension S3Client: TestDependencyKey { - static var testValue: Self { Self() } + static var testValue: Self { + .init( + fetchReadme: { _, _, _ in unimplemented(); return "" }, + storeS3Readme: { _, _, _ in unimplemented("storeS3Readme"); return "" } + ) + } } extension DependencyValues { diff --git a/Tests/AppTests/IngestionTests.swift b/Tests/AppTests/IngestionTests.swift index 28ea32859..367a00c44 100644 --- a/Tests/AppTests/IngestionTests.swift +++ b/Tests/AppTests/IngestionTests.swift @@ -466,6 +466,7 @@ class IngestionTests: AppTestCase { func test_ingest_storeS3Readme() async throws { let fetchCalls = QueueIsolated(0) + let storeCalls = QueueIsolated(0) try await withDependencies { $0.date.now = .now $0.github.fetchLicense = { @Sendable _, _ in nil } @@ -484,13 +485,7 @@ class IngestionTests: AppTestCase { imagesToCache: []) } } - } operation: { - // setup - let app = self.app! - let pkg = Package(url: "https://github.com/foo/bar".url, processingStage: .reconciliation) - try await pkg.save(on: app.db) - let storeCalls = QueueIsolated(0) - Current.storeS3Readme = { owner, repo, html in + $0.s3.storeS3Readme = { owner, repo, html in storeCalls.increment() XCTAssertEqual(owner, "foo") XCTAssertEqual(repo, "bar") @@ -501,6 +496,11 @@ class IngestionTests: AppTestCase { } return "objectUrl" } + } operation: { + // setup + let app = self.app! + let pkg = Package(url: "https://github.com/foo/bar".url, processingStage: .reconciliation) + try await pkg.save(on: app.db) do { // first ingestion, no readme has been saved // MUT @@ -553,7 +553,6 @@ class IngestionTests: AppTestCase { let pkg = Package(url: "https://github.com/foo/bar".url, processingStage: .reconciliation) try await pkg.save(on: app.db) - Current.storeS3Readme = { _, _, _ in "objectUrl" } let storeS3ReadmeImagesCalls = QueueIsolated(0) Current.storeS3ReadmeImages = { _, imagesToCache in storeS3ReadmeImagesCalls.increment() @@ -586,6 +585,7 @@ class IngestionTests: AppTestCase { path: "/foo/bar/with-jwt-2.jpg")) ]) } + $0.s3.storeS3Readme = { _, _, _ in "objectUrl" } } operation: { // MUT try await Ingestion.ingest(client: app.client, database: app.db, mode: .limit(1)) @@ -601,10 +601,6 @@ class IngestionTests: AppTestCase { let pkg = Package(url: "https://github.com/foo/bar".url, processingStage: .reconciliation) try await pkg.save(on: app.db) let storeCalls = QueueIsolated(0) - Current.storeS3Readme = { owner, repo, html throws(S3Readme.Error) in - storeCalls.increment() - throw .storeReadmeFailed - } do { // first ingestion, no readme has been saved try await withDependencies { @@ -617,6 +613,10 @@ class IngestionTests: AppTestCase { htmlUrl: "readme url", imagesToCache: []) } + $0.s3.storeS3Readme = { owner, repo, html throws(S3Readme.Error) in + storeCalls.increment() + throw .storeReadmeFailed + } } operation: { // MUT let app = self.app! diff --git a/Tests/AppTests/Mocks/AppEnvironment+mock.swift b/Tests/AppTests/Mocks/AppEnvironment+mock.swift index b2e1ad262..47849e727 100644 --- a/Tests/AppTests/Mocks/AppEnvironment+mock.swift +++ b/Tests/AppTests/Mocks/AppEnvironment+mock.swift @@ -31,7 +31,6 @@ extension AppEnvironment { logger: { logger }, setLogger: { logger in Self.logger = logger }, shell: .mock, - storeS3Readme: { _, _, _ in "s3ObjectUrl" }, storeS3ReadmeImages: { _, _ in }, triggerBuild: { _, _, _, _, _, _, _, _ in .init(status: .ok, webUrl: "http://web_url") } ) From 2cc308ee71aad66934d4f08b30f2f8d278790c33 Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Tue, 21 Jan 2025 06:30:05 +0100 Subject: [PATCH 4/8] Rename --- Sources/App/Commands/Ingestion.swift | 2 +- Sources/App/Core/Dependencies/S3Client.swift | 6 +++--- Tests/AppTests/IngestionTests.swift | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Sources/App/Commands/Ingestion.swift b/Sources/App/Commands/Ingestion.swift index 2e88c247a..5d50b3be5 100644 --- a/Sources/App/Commands/Ingestion.swift +++ b/Sources/App/Commands/Ingestion.swift @@ -246,7 +246,7 @@ enum Ingestion { let owner = metadata.repositoryOwner, let repository = metadata.repositoryName, let html = readme?.html { - let objectUrl = try await s3.storeS3Readme(owner, repository, html) + let objectUrl = try await s3.storeReadme(owner, repository, html) if let imagesToCache = readme?.imagesToCache, imagesToCache.isEmpty == false { try await Current.storeS3ReadmeImages(client, imagesToCache) } diff --git a/Sources/App/Core/Dependencies/S3Client.swift b/Sources/App/Core/Dependencies/S3Client.swift index 4f59670ef..ee06349f8 100644 --- a/Sources/App/Core/Dependencies/S3Client.swift +++ b/Sources/App/Core/Dependencies/S3Client.swift @@ -23,7 +23,7 @@ import Vapor struct S3Client { #warning("drop client parameter") var fetchReadme: @Sendable (_ client: Client, _ owner: String, _ repository: String) async throws -> String - var storeS3Readme: @Sendable (_ owner: String, _ repository: String, _ readme: String) async throws(S3Readme.Error) -> String = { _, _, _ in + var storeReadme: @Sendable (_ owner: String, _ repository: String, _ readme: String) async throws(S3Readme.Error) -> String = { _, _, _ in reportIssue("storeS3Readme"); return "" } } @@ -34,7 +34,7 @@ extension S3Client: DependencyKey { fetchReadme: { client, owner, repo in try await S3Readme.fetchReadme(client:client, owner: owner, repository: repo) }, - storeS3Readme: { owner, repo, readme throws(S3Readme.Error) in + storeReadme: { owner, repo, readme throws(S3Readme.Error) in try await S3Readme.storeReadme(owner: owner, repository: repo, readme: readme) } ) @@ -45,7 +45,7 @@ extension S3Client: TestDependencyKey { static var testValue: Self { .init( fetchReadme: { _, _, _ in unimplemented(); return "" }, - storeS3Readme: { _, _, _ in unimplemented("storeS3Readme"); return "" } + storeReadme: { _, _, _ in unimplemented("storeS3Readme"); return "" } ) } } diff --git a/Tests/AppTests/IngestionTests.swift b/Tests/AppTests/IngestionTests.swift index 367a00c44..6d6f91363 100644 --- a/Tests/AppTests/IngestionTests.swift +++ b/Tests/AppTests/IngestionTests.swift @@ -485,7 +485,7 @@ class IngestionTests: AppTestCase { imagesToCache: []) } } - $0.s3.storeS3Readme = { owner, repo, html in + $0.s3.storeReadme = { owner, repo, html in storeCalls.increment() XCTAssertEqual(owner, "foo") XCTAssertEqual(repo, "bar") @@ -585,7 +585,7 @@ class IngestionTests: AppTestCase { path: "/foo/bar/with-jwt-2.jpg")) ]) } - $0.s3.storeS3Readme = { _, _, _ in "objectUrl" } + $0.s3.storeReadme = { _, _, _ in "objectUrl" } } operation: { // MUT try await Ingestion.ingest(client: app.client, database: app.db, mode: .limit(1)) @@ -613,7 +613,7 @@ class IngestionTests: AppTestCase { htmlUrl: "readme url", imagesToCache: []) } - $0.s3.storeS3Readme = { owner, repo, html throws(S3Readme.Error) in + $0.s3.storeReadme = { owner, repo, html throws(S3Readme.Error) in storeCalls.increment() throw .storeReadmeFailed } From f5dac45304df4aabaaf31ddde75d40f3c8636488 Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Tue, 21 Jan 2025 06:41:12 +0100 Subject: [PATCH 5/8] Move Current.storeS3ReadmeImages to S3Client.storeS3ReadmeImages --- Sources/App/Commands/Ingestion.swift | 2 +- Sources/App/Core/AppEnvironment.swift | 5 ----- Sources/App/Core/Dependencies/S3Client.swift | 8 +++++++- Tests/AppTests/IngestionTests.swift | 9 ++++----- Tests/AppTests/Mocks/AppEnvironment+mock.swift | 1 - 5 files changed, 12 insertions(+), 13 deletions(-) diff --git a/Sources/App/Commands/Ingestion.swift b/Sources/App/Commands/Ingestion.swift index 5d50b3be5..9586994c9 100644 --- a/Sources/App/Commands/Ingestion.swift +++ b/Sources/App/Commands/Ingestion.swift @@ -248,7 +248,7 @@ enum Ingestion { let html = readme?.html { let objectUrl = try await s3.storeReadme(owner, repository, html) if let imagesToCache = readme?.imagesToCache, imagesToCache.isEmpty == false { - try await Current.storeS3ReadmeImages(client, imagesToCache) + try await s3.storeS3ReadmeImages(client, imagesToCache) } return .cached(s3ObjectUrl: objectUrl, githubEtag: upstreamEtag) } else { diff --git a/Sources/App/Core/AppEnvironment.swift b/Sources/App/Core/AppEnvironment.swift index 9a6b78345..4dd628ba6 100644 --- a/Sources/App/Core/AppEnvironment.swift +++ b/Sources/App/Core/AppEnvironment.swift @@ -32,8 +32,6 @@ struct AppEnvironment: Sendable { var logger: @Sendable () -> Logger var setLogger: @Sendable (Logger) -> Void var shell: Shell - var storeS3ReadmeImages: @Sendable (_ client: Client, - _ imagesToCache: [Github.Readme.ImageToCache]) async throws(S3Readme.Error) -> Void var triggerBuild: @Sendable (_ client: Client, _ buildId: Build.Id, _ cloneURL: String, @@ -67,9 +65,6 @@ extension AppEnvironment { logger: { logger }, setLogger: { logger in Self.logger = logger }, shell: .live, - storeS3ReadmeImages: { client, images throws(S3Readme.Error) in - try await S3Readme.storeReadmeImages(client: client, imagesToCache: images) - }, triggerBuild: { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in try await Gitlab.Builder.triggerBuild(client: client, buildId: buildId, diff --git a/Sources/App/Core/Dependencies/S3Client.swift b/Sources/App/Core/Dependencies/S3Client.swift index ee06349f8..655437c6e 100644 --- a/Sources/App/Core/Dependencies/S3Client.swift +++ b/Sources/App/Core/Dependencies/S3Client.swift @@ -26,6 +26,8 @@ struct S3Client { var storeReadme: @Sendable (_ owner: String, _ repository: String, _ readme: String) async throws(S3Readme.Error) -> String = { _, _, _ in reportIssue("storeS3Readme"); return "" } +#warning("drop client parameter") + var storeS3ReadmeImages: @Sendable (_ client: Client, _ imagesToCache: [Github.Readme.ImageToCache]) async throws(S3Readme.Error) -> Void } extension S3Client: DependencyKey { @@ -36,6 +38,9 @@ extension S3Client: DependencyKey { }, storeReadme: { owner, repo, readme throws(S3Readme.Error) in try await S3Readme.storeReadme(owner: owner, repository: repo, readme: readme) + }, + storeS3ReadmeImages: { client, images throws(S3Readme.Error) in + try await S3Readme.storeReadmeImages(client: client, imagesToCache: images) } ) } @@ -45,7 +50,8 @@ extension S3Client: TestDependencyKey { static var testValue: Self { .init( fetchReadme: { _, _, _ in unimplemented(); return "" }, - storeReadme: { _, _, _ in unimplemented("storeS3Readme"); return "" } + storeReadme: { _, _, _ in unimplemented("storeS3Readme"); return "" }, + storeS3ReadmeImages: { _, _ throws(S3Readme.Error) in unimplemented("storeS3ReadmeImages") } ) } } diff --git a/Tests/AppTests/IngestionTests.swift b/Tests/AppTests/IngestionTests.swift index 6d6f91363..e7051dfe4 100644 --- a/Tests/AppTests/IngestionTests.swift +++ b/Tests/AppTests/IngestionTests.swift @@ -554,11 +554,6 @@ class IngestionTests: AppTestCase { processingStage: .reconciliation) try await pkg.save(on: app.db) let storeS3ReadmeImagesCalls = QueueIsolated(0) - Current.storeS3ReadmeImages = { _, imagesToCache in - storeS3ReadmeImagesCalls.increment() - - XCTAssertEqual(imagesToCache.count, 2) - } try await withDependencies { $0.date.now = .now @@ -586,6 +581,10 @@ class IngestionTests: AppTestCase { ]) } $0.s3.storeReadme = { _, _, _ in "objectUrl" } + $0.s3.storeS3ReadmeImages = { _, imagesToCache in + storeS3ReadmeImagesCalls.increment() + XCTAssertEqual(imagesToCache.count, 2) + } } operation: { // MUT try await Ingestion.ingest(client: app.client, database: app.db, mode: .limit(1)) diff --git a/Tests/AppTests/Mocks/AppEnvironment+mock.swift b/Tests/AppTests/Mocks/AppEnvironment+mock.swift index 47849e727..e17d4fdc2 100644 --- a/Tests/AppTests/Mocks/AppEnvironment+mock.swift +++ b/Tests/AppTests/Mocks/AppEnvironment+mock.swift @@ -31,7 +31,6 @@ extension AppEnvironment { logger: { logger }, setLogger: { logger in Self.logger = logger }, shell: .mock, - storeS3ReadmeImages: { _, _ in }, triggerBuild: { _, _, _, _, _, _, _, _ in .init(status: .ok, webUrl: "http://web_url") } ) } From 24e28fedb0392a0a13918258b2ca6336867432b3 Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Tue, 21 Jan 2025 06:42:15 +0100 Subject: [PATCH 6/8] Rename --- Sources/App/Commands/Ingestion.swift | 2 +- Sources/App/Core/Dependencies/S3Client.swift | 6 +++--- Tests/AppTests/IngestionTests.swift | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Sources/App/Commands/Ingestion.swift b/Sources/App/Commands/Ingestion.swift index 9586994c9..7370a2ca6 100644 --- a/Sources/App/Commands/Ingestion.swift +++ b/Sources/App/Commands/Ingestion.swift @@ -248,7 +248,7 @@ enum Ingestion { let html = readme?.html { let objectUrl = try await s3.storeReadme(owner, repository, html) if let imagesToCache = readme?.imagesToCache, imagesToCache.isEmpty == false { - try await s3.storeS3ReadmeImages(client, imagesToCache) + try await s3.storeReadmeImages(client, imagesToCache) } return .cached(s3ObjectUrl: objectUrl, githubEtag: upstreamEtag) } else { diff --git a/Sources/App/Core/Dependencies/S3Client.swift b/Sources/App/Core/Dependencies/S3Client.swift index 655437c6e..3d984a343 100644 --- a/Sources/App/Core/Dependencies/S3Client.swift +++ b/Sources/App/Core/Dependencies/S3Client.swift @@ -27,7 +27,7 @@ struct S3Client { reportIssue("storeS3Readme"); return "" } #warning("drop client parameter") - var storeS3ReadmeImages: @Sendable (_ client: Client, _ imagesToCache: [Github.Readme.ImageToCache]) async throws(S3Readme.Error) -> Void + var storeReadmeImages: @Sendable (_ client: Client, _ imagesToCache: [Github.Readme.ImageToCache]) async throws(S3Readme.Error) -> Void } extension S3Client: DependencyKey { @@ -39,7 +39,7 @@ extension S3Client: DependencyKey { storeReadme: { owner, repo, readme throws(S3Readme.Error) in try await S3Readme.storeReadme(owner: owner, repository: repo, readme: readme) }, - storeS3ReadmeImages: { client, images throws(S3Readme.Error) in + storeReadmeImages: { client, images throws(S3Readme.Error) in try await S3Readme.storeReadmeImages(client: client, imagesToCache: images) } ) @@ -51,7 +51,7 @@ extension S3Client: TestDependencyKey { .init( fetchReadme: { _, _, _ in unimplemented(); return "" }, storeReadme: { _, _, _ in unimplemented("storeS3Readme"); return "" }, - storeS3ReadmeImages: { _, _ throws(S3Readme.Error) in unimplemented("storeS3ReadmeImages") } + storeReadmeImages: { _, _ throws(S3Readme.Error) in unimplemented("storeS3ReadmeImages") } ) } } diff --git a/Tests/AppTests/IngestionTests.swift b/Tests/AppTests/IngestionTests.swift index e7051dfe4..96e8cbeac 100644 --- a/Tests/AppTests/IngestionTests.swift +++ b/Tests/AppTests/IngestionTests.swift @@ -581,7 +581,7 @@ class IngestionTests: AppTestCase { ]) } $0.s3.storeReadme = { _, _, _ in "objectUrl" } - $0.s3.storeS3ReadmeImages = { _, imagesToCache in + $0.s3.storeReadmeImages = { _, imagesToCache in storeS3ReadmeImagesCalls.increment() XCTAssertEqual(imagesToCache.count, 2) } From 0c4ce91ec8861a7cf6555dd6e4b7a6c491dcf612 Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Tue, 21 Jan 2025 06:57:31 +0100 Subject: [PATCH 7/8] Drop Client parameter from fetchReadme --- .../App/Controllers/PackageController+routes.swift | 2 +- Sources/App/Core/Dependencies/HTTPClient.swift | 2 ++ Sources/App/Core/Dependencies/S3Client.swift | 12 +++++++----- Sources/App/Core/Extensions/S3Readme+ext.swift | 7 ++++--- Tests/AppTests/PackageController+routesTests.swift | 6 +++--- 5 files changed, 17 insertions(+), 12 deletions(-) diff --git a/Sources/App/Controllers/PackageController+routes.swift b/Sources/App/Controllers/PackageController+routes.swift index d3f3882ca..8140d6ffc 100644 --- a/Sources/App/Controllers/PackageController+routes.swift +++ b/Sources/App/Controllers/PackageController+routes.swift @@ -307,7 +307,7 @@ enum PackageController { do { @Dependency(\.s3) var s3 - let readme = try await s3.fetchReadme(req.client, owner, repository) + let readme = try await s3.fetchReadme(owner, repository) guard let branch = pkg.repository?.defaultBranch else { return PackageReadme.View(model: .cacheLookupFailed(url: readmeHtmlUrl)).document() } diff --git a/Sources/App/Core/Dependencies/HTTPClient.swift b/Sources/App/Core/Dependencies/HTTPClient.swift index f7c5acbe3..af00e6d25 100644 --- a/Sources/App/Core/Dependencies/HTTPClient.swift +++ b/Sources/App/Core/Dependencies/HTTPClient.swift @@ -66,6 +66,8 @@ extension HTTPClient: DependencyKey { } ) } + + func get(url: String) async throws -> Response { try await get(url: url, headers: .init()) } } diff --git a/Sources/App/Core/Dependencies/S3Client.swift b/Sources/App/Core/Dependencies/S3Client.swift index 3d984a343..fac00a69c 100644 --- a/Sources/App/Core/Dependencies/S3Client.swift +++ b/Sources/App/Core/Dependencies/S3Client.swift @@ -21,8 +21,7 @@ import Vapor // https://github.com/pointfreeco/swift-dependencies/discussions/324 //@DependencyClient struct S3Client { -#warning("drop client parameter") - var fetchReadme: @Sendable (_ client: Client, _ owner: String, _ repository: String) async throws -> String + var fetchReadme: @Sendable (_ owner: String, _ repository: String) async throws -> String var storeReadme: @Sendable (_ owner: String, _ repository: String, _ readme: String) async throws(S3Readme.Error) -> String = { _, _, _ in reportIssue("storeS3Readme"); return "" } @@ -30,11 +29,12 @@ struct S3Client { var storeReadmeImages: @Sendable (_ client: Client, _ imagesToCache: [Github.Readme.ImageToCache]) async throws(S3Readme.Error) -> Void } + extension S3Client: DependencyKey { static var liveValue: Self { .init( - fetchReadme: { client, owner, repo in - try await S3Readme.fetchReadme(client:client, owner: owner, repository: repo) + fetchReadme: { owner, repo in + try await S3Readme.fetchReadme(owner: owner, repository: repo) }, storeReadme: { owner, repo, readme throws(S3Readme.Error) in try await S3Readme.storeReadme(owner: owner, repository: repo, readme: readme) @@ -46,16 +46,18 @@ extension S3Client: DependencyKey { } } + extension S3Client: TestDependencyKey { static var testValue: Self { .init( - fetchReadme: { _, _, _ in unimplemented(); return "" }, + fetchReadme: { _, _ in unimplemented(); return "" }, storeReadme: { _, _, _ in unimplemented("storeS3Readme"); return "" }, storeReadmeImages: { _, _ throws(S3Readme.Error) in unimplemented("storeS3ReadmeImages") } ) } } + extension DependencyValues { var s3: S3Client { get { self[S3Client.self] } diff --git a/Sources/App/Core/Extensions/S3Readme+ext.swift b/Sources/App/Core/Extensions/S3Readme+ext.swift index 99aab319d..e657720c6 100644 --- a/Sources/App/Core/Extensions/S3Readme+ext.swift +++ b/Sources/App/Core/Extensions/S3Readme+ext.swift @@ -27,11 +27,12 @@ extension S3Readme { case storeImagesFailed } - static func fetchReadme(client: Client, owner: String, repository: String) async throws(S3Readme.Error) -> String { + static func fetchReadme(owner: String, repository: String) async throws(S3Readme.Error) -> String { let key = try S3Store.Key.readme(owner: owner, repository: repository) - let response: ClientResponse + @Dependency(\.httpClient) var httpClient + let response: HTTPClient.Response do { - response = try await client.get(URI(string: key.objectUrl)) + response = try await httpClient.get(url: key.objectUrl) } catch { throw .requestFailed(key: key, error: error) } diff --git a/Tests/AppTests/PackageController+routesTests.swift b/Tests/AppTests/PackageController+routesTests.swift index 429bb604c..1741fc8a4 100644 --- a/Tests/AppTests/PackageController+routesTests.swift +++ b/Tests/AppTests/PackageController+routesTests.swift @@ -168,7 +168,7 @@ class PackageController_routesTests: SnapshotTestCase { func test_readme_basic() async throws { // Test readme fragment happy path try await withDependencies { - $0.s3.fetchReadme = { @Sendable _, _, _ in + $0.s3.fetchReadme = { @Sendable _, _ in #"
readme content
"# } } operation: { @@ -195,7 +195,7 @@ class PackageController_routesTests: SnapshotTestCase { func test_readme_no_readmeHtmlUrl() async throws { // Test readme fragment when there's no readme html url try await withDependencies { - $0.s3.fetchReadme = { @Sendable _, _, _ in + $0.s3.fetchReadme = { @Sendable _, _ in XCTFail("must not be called") return "" } @@ -225,7 +225,7 @@ class PackageController_routesTests: SnapshotTestCase { func test_readme_error() async throws { // Test readme fragment when fetchS3Readme throws try await withDependencies { - $0.s3.fetchReadme = { @Sendable _, _, _ in + $0.s3.fetchReadme = { @Sendable _, _ in struct Error: Swift.Error { } throw Error() } From fc1efa9a88be6104e912b1f30a9917ed923f795e Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Tue, 21 Jan 2025 07:04:05 +0100 Subject: [PATCH 8/8] Drop Client parameter from storeReadmeImages --- Sources/App/Commands/Ingestion.swift | 6 +++--- Sources/App/Core/Dependencies/S3Client.swift | 10 ++++------ Sources/App/Core/Extensions/S3Readme+ext.swift | 5 +++-- Tests/AppTests/IngestionTests.swift | 2 +- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/Sources/App/Commands/Ingestion.swift b/Sources/App/Commands/Ingestion.swift index 7370a2ca6..d9ad102ce 100644 --- a/Sources/App/Commands/Ingestion.swift +++ b/Sources/App/Commands/Ingestion.swift @@ -190,7 +190,7 @@ enum Ingestion { let s3Readme: S3Readme? do throws(S3Readme.Error) { - s3Readme = try await storeS3Readme(client: client, repository: repo, metadata: metadata, readme: readme) + s3Readme = try await storeS3Readme(repository: repo, metadata: metadata, readme: readme) } catch { // We don't want to fail ingestion in case storing the readme fails - warn and continue. Current.logger().warning("storeS3Readme failed: \(error)") @@ -239,7 +239,7 @@ enum Ingestion { } - static func storeS3Readme(client: Client, repository: Repository, metadata: Github.Metadata, readme: Github.Readme?) async throws(S3Readme.Error) -> S3Readme? { + static func storeS3Readme(repository: Repository, metadata: Github.Metadata, readme: Github.Readme?) async throws(S3Readme.Error) -> S3Readme? { @Dependency(\.s3) var s3 if let upstreamEtag = readme?.etag, repository.s3Readme?.needsUpdate(upstreamEtag: upstreamEtag) ?? true, @@ -248,7 +248,7 @@ enum Ingestion { let html = readme?.html { let objectUrl = try await s3.storeReadme(owner, repository, html) if let imagesToCache = readme?.imagesToCache, imagesToCache.isEmpty == false { - try await s3.storeReadmeImages(client, imagesToCache) + try await s3.storeReadmeImages(imagesToCache) } return .cached(s3ObjectUrl: objectUrl, githubEtag: upstreamEtag) } else { diff --git a/Sources/App/Core/Dependencies/S3Client.swift b/Sources/App/Core/Dependencies/S3Client.swift index fac00a69c..d0079943d 100644 --- a/Sources/App/Core/Dependencies/S3Client.swift +++ b/Sources/App/Core/Dependencies/S3Client.swift @@ -14,7 +14,6 @@ import Dependencies import IssueReporting -import Vapor // We currently cannot use @DependencyClient here due to @@ -25,8 +24,7 @@ struct S3Client { var storeReadme: @Sendable (_ owner: String, _ repository: String, _ readme: String) async throws(S3Readme.Error) -> String = { _, _, _ in reportIssue("storeS3Readme"); return "" } -#warning("drop client parameter") - var storeReadmeImages: @Sendable (_ client: Client, _ imagesToCache: [Github.Readme.ImageToCache]) async throws(S3Readme.Error) -> Void + var storeReadmeImages: @Sendable (_ imagesToCache: [Github.Readme.ImageToCache]) async throws(S3Readme.Error) -> Void } @@ -39,8 +37,8 @@ extension S3Client: DependencyKey { storeReadme: { owner, repo, readme throws(S3Readme.Error) in try await S3Readme.storeReadme(owner: owner, repository: repo, readme: readme) }, - storeReadmeImages: { client, images throws(S3Readme.Error) in - try await S3Readme.storeReadmeImages(client: client, imagesToCache: images) + storeReadmeImages: { images throws(S3Readme.Error) in + try await S3Readme.storeReadmeImages(imagesToCache: images) } ) } @@ -52,7 +50,7 @@ extension S3Client: TestDependencyKey { .init( fetchReadme: { _, _ in unimplemented(); return "" }, storeReadme: { _, _, _ in unimplemented("storeS3Readme"); return "" }, - storeReadmeImages: { _, _ throws(S3Readme.Error) in unimplemented("storeS3ReadmeImages") } + storeReadmeImages: { _ throws(S3Readme.Error) in unimplemented("storeS3ReadmeImages") } ) } } diff --git a/Sources/App/Core/Extensions/S3Readme+ext.swift b/Sources/App/Core/Extensions/S3Readme+ext.swift index e657720c6..7d222d3cb 100644 --- a/Sources/App/Core/Extensions/S3Readme+ext.swift +++ b/Sources/App/Core/Extensions/S3Readme+ext.swift @@ -57,8 +57,9 @@ extension S3Readme { return key.objectUrl } - static func storeReadmeImages(client: Client, imagesToCache: [Github.Readme.ImageToCache]) async throws(S3Readme.Error) { + static func storeReadmeImages(imagesToCache: [Github.Readme.ImageToCache]) async throws(S3Readme.Error) { @Dependency(\.environment) var environment + @Dependency(\.httpClient) var httpClient guard let accessKeyId = environment.awsAccessKeyId() else { throw .envVariableNotSet("AWS_ACCESS_KEY_ID") } guard let secretAccessKey = environment.awsSecretAccessKey() else { throw .envVariableNotSet("AWS_SECRET_ACCESS_KEY")} @@ -66,7 +67,7 @@ extension S3Readme { for imageToCache in imagesToCache { Current.logger().debug("Copying readme image to \(imageToCache.s3Key.s3Uri) ...") do { - let response = try await client.get(URI(stringLiteral: imageToCache.originalUrl)) + let response = try await httpClient.get(url: imageToCache.originalUrl) if var body = response.body, let imageData = body.readData(length: body.readableBytes) { try await store.save(payload: imageData, to: imageToCache.s3Key) } diff --git a/Tests/AppTests/IngestionTests.swift b/Tests/AppTests/IngestionTests.swift index 96e8cbeac..f75509815 100644 --- a/Tests/AppTests/IngestionTests.swift +++ b/Tests/AppTests/IngestionTests.swift @@ -581,7 +581,7 @@ class IngestionTests: AppTestCase { ]) } $0.s3.storeReadme = { _, _, _ in "objectUrl" } - $0.s3.storeReadmeImages = { _, imagesToCache in + $0.s3.storeReadmeImages = { imagesToCache in storeS3ReadmeImagesCalls.increment() XCTAssertEqual(imagesToCache.count, 2) }