From ad00fbd15a02fc8c70bc79020bacb403dd134212 Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Mon, 13 Jan 2025 15:24:54 +0100 Subject: [PATCH 1/3] Drop superfluous client parameter --- Sources/App/Commands/Ingestion.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/App/Commands/Ingestion.swift b/Sources/App/Commands/Ingestion.swift index 14a25c861..c330c6a33 100644 --- a/Sources/App/Commands/Ingestion.swift +++ b/Sources/App/Commands/Ingestion.swift @@ -181,7 +181,7 @@ enum Ingestion { } let (metadata, license, readme) = try await run { - try await fetchMetadata(client: client, package: package.model, owner: owner, repository: repository) + try await fetchMetadata(package: package.model, owner: owner, repository: repository) } rethrowing: { Ingestion.Error(packageId: package.model.id!, underlyingError: .fetchMetadataFailed(owner: owner, name: repository, details: "\($0)")) @@ -256,7 +256,7 @@ enum Ingestion { } - static func fetchMetadata(client: Client, package: Package, owner: String, repository: String) async throws(Github.Error) -> (Github.Metadata, Github.License?, Github.Readme?) { + static func fetchMetadata(package: Package, owner: String, repository: String) async throws(Github.Error) -> (Github.Metadata, Github.License?, Github.Readme?) { @Dependency(\.environment) var environment if environment.shouldFail(failureMode: .fetchMetadataFailed) { throw Github.Error.requestFailed(.internalServerError) From 457b05a4b4a51d002e5308ab207cd10791a0a717 Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Mon, 13 Jan 2025 15:25:08 +0100 Subject: [PATCH 2/3] Fix test_issue_761_no_license --- Tests/AppTests/IngestionTests.swift | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/Tests/AppTests/IngestionTests.swift b/Tests/AppTests/IngestionTests.swift index 57559d578..41c260da7 100644 --- a/Tests/AppTests/IngestionTests.swift +++ b/Tests/AppTests/IngestionTests.swift @@ -636,20 +636,27 @@ class IngestionTests: AppTestCase { // https://github.com/SwiftPackageIndex/SwiftPackageIndex-Server/issues/761 try await withDependencies { // use live fetch request for fetchLicense, whose behaviour we want to test ... - $0.github.fetchLicense = { @Sendable owner, repo in await Github.fetchLicense(owner: owner, repository: repo) } + $0.github.fetchLicense = GithubClient.liveValue.fetchLicense // use mock for metadata request which we're not interested in ... $0.github.fetchMetadata = { @Sendable _, _ in .init() } $0.github.fetchReadme = { @Sendable _, _ in nil } + $0.httpClient.get = { @Sendable url, _ in + if url.hasSuffix("/license") { + return .notFound + } else { + XCTFail("unexpected url \(url)") + struct TestError: Error { } + throw TestError() + } + } } operation: { // setup let pkg = Package(url: "https://github.com/foo/1") try await pkg.save(on: app.db) - // and simulate its underlying request returning a 404 (by making all requests - // return a 404, but it's the only one we're sending) - let client = MockClient { _, resp in resp.status = .notFound } + Current.githubToken = { "token" } // MUT - let (_, license, _) = try await Ingestion.fetchMetadata(client: client, package: pkg, owner: "foo", repository: "1") + let (_, license, _) = try await Ingestion.fetchMetadata(package: pkg, owner: "foo", repository: "1") // validate XCTAssertEqual(license, nil) From 3f88739b0cb6482a7aec4c713dd183faa29395fe Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Wed, 15 Jan 2025 11:46:28 +0100 Subject: [PATCH 3/3] =?UTF-8?q?Move=20Current.githubToken=20=E2=86=92=20gi?= =?UTF-8?q?thub.token?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Sources/App/Core/AppEnvironment.swift | 2 -- .../App/Core/Dependencies/GithubClient.swift | 8 +++-- Sources/App/Core/Github.swift | 14 +++++--- Tests/AppTests/GithubTests.swift | 34 ++++++------------- Tests/AppTests/IngestionTests.swift | 2 +- .../AppTests/Mocks/AppEnvironment+mock.swift | 1 - 6 files changed, 27 insertions(+), 34 deletions(-) diff --git a/Sources/App/Core/AppEnvironment.swift b/Sources/App/Core/AppEnvironment.swift index a8de72aa8..7de880203 100644 --- a/Sources/App/Core/AppEnvironment.swift +++ b/Sources/App/Core/AppEnvironment.swift @@ -27,7 +27,6 @@ struct AppEnvironment: Sendable { var fileManager: FileManager var getStatusCount: @Sendable (_ client: Client, _ status: Gitlab.Builder.Status) async throws -> Int var git: Git - var githubToken: @Sendable () -> String? var gitlabApiToken: @Sendable () -> String? var gitlabPipelineToken: @Sendable () -> String? var gitlabPipelineLimit: @Sendable () -> Int @@ -73,7 +72,6 @@ extension AppEnvironment { maxPageCount: 5) }, git: .live, - githubToken: { Environment.get("GITHUB_TOKEN") }, gitlabApiToken: { Environment.get("GITLAB_API_TOKEN") }, gitlabPipelineToken: { Environment.get("GITLAB_PIPELINE_TOKEN") }, gitlabPipelineLimit: { diff --git a/Sources/App/Core/Dependencies/GithubClient.swift b/Sources/App/Core/Dependencies/GithubClient.swift index 531078a97..5dd5364fd 100644 --- a/Sources/App/Core/Dependencies/GithubClient.swift +++ b/Sources/App/Core/Dependencies/GithubClient.swift @@ -15,6 +15,7 @@ import Dependencies import IssueReporting +import Vapor // We currently cannot use @DependencyClient here due to @@ -24,6 +25,7 @@ struct GithubClient { var fetchLicense: @Sendable (_ owner: String, _ repository: String) async -> Github.License? var fetchMetadata: @Sendable (_ owner: String, _ repository: String) async throws(Github.Error) -> Github.Metadata = { _,_ in reportIssue("fetchMetadata"); return .init() } var fetchReadme: @Sendable (_ owner: String, _ repository: String) async -> Github.Readme? + var token: @Sendable () -> String? } @@ -32,7 +34,8 @@ extension GithubClient: DependencyKey { .init( fetchLicense: { owner, repo in await Github.fetchLicense(owner: owner, repository: repo) }, fetchMetadata: { owner, repo throws(Github.Error) in try await Github.fetchMetadata(owner: owner, repository: repo) }, - fetchReadme: { owner, repo in await Github.fetchReadme(owner: owner, repository: repo) } + fetchReadme: { owner, repo in await Github.fetchReadme(owner: owner, repository: repo) }, + token: { Environment.get("GITHUB_TOKEN") } ) } } @@ -43,7 +46,8 @@ extension GithubClient: TestDependencyKey { .init( fetchLicense: { _, _ in unimplemented("fetchLicense"); return nil }, fetchMetadata: { _, _ in unimplemented("fetchMetadata"); return .init() }, - fetchReadme: { _, _ in unimplemented("fetchReadme"); return nil } + fetchReadme: { _, _ in unimplemented("fetchReadme"); return nil }, + token: { unimplemented("token"); return nil } ) } } diff --git a/Sources/App/Core/Github.swift b/Sources/App/Core/Github.swift index 55b643f8c..1cde1459d 100644 --- a/Sources/App/Core/Github.swift +++ b/Sources/App/Core/Github.swift @@ -87,11 +87,13 @@ extension Github { } static func fetch(url: String, headers: [(String, String)] = []) async throws -> (content: String, etag: String?) { - guard let token = Current.githubToken() else { + @Dependency(\.github) var github + @Dependency(\.httpClient) var httpClient + + guard let token = github.token() else { throw Error.missingToken } - @Dependency(\.httpClient) var httpClient let response = try await httpClient.get(url: url, headers: defaultHeaders(with: token).adding(contentsOf: headers)) @@ -114,7 +116,9 @@ extension Github { } static func fetchResource(_ type: T.Type, url: String) async throws -> T { - guard let token = Current.githubToken() else { + @Dependency(\.github) var github + + guard let token = github.token() else { throw Error.missingToken } @@ -176,7 +180,9 @@ extension Github { } static func fetchResource(_ type: T.Type, query: GraphQLQuery) async throws(Github.Error) -> T { - guard let token = Current.githubToken() else { + @Dependency(\.github) var github + + guard let token = github.token() else { throw Error.missingToken } diff --git a/Tests/AppTests/GithubTests.swift b/Tests/AppTests/GithubTests.swift index 748229d36..b49857def 100644 --- a/Tests/AppTests/GithubTests.swift +++ b/Tests/AppTests/GithubTests.swift @@ -103,7 +103,6 @@ class GithubTests: AppTestCase { } func test_fetchResource() async throws { - Current.githubToken = { "secr3t" } struct Response: Decodable, Equatable { var data: Data struct Data: Decodable, Equatable { @@ -116,6 +115,7 @@ class GithubTests: AppTestCase { let q = Github.GraphQLQuery(query: "query { viewer { login } }") try await withDependencies { + $0.github.token = { "secr3t" } $0.httpClient.post = { @Sendable _, _, _ in .ok(body: #"{"data":{"viewer":{"login":"finestructure"}}}"#) } @@ -126,10 +126,10 @@ class GithubTests: AppTestCase { } func test_fetchMetadata() async throws { - Current.githubToken = { "secr3t" } let iso8601 = ISO8601DateFormatter() try await withDependencies { + $0.github.token = { "secr3t" } $0.httpClient.post = { @Sendable _, _, _ in try .ok(fixture: "github-graphql-resource.json") } @@ -181,9 +181,8 @@ class GithubTests: AppTestCase { } func test_fetchMetadata_badRequest() async throws { - Current.githubToken = { "secr3t" } - await withDependencies { + $0.github.token = { "secr3t" } $0.httpClient.post = { @Sendable _, _, _ in .badRequest } } operation: { do { @@ -200,10 +199,8 @@ class GithubTests: AppTestCase { } func test_fetchMetadata_badData() async throws { - // setup - Current.githubToken = { "secr3t" } - await withDependencies { + $0.github.token = { "secr3t" } $0.httpClient.post = { @Sendable _, _, _ in .ok(body: "bad data") } } operation: { // MUT @@ -225,10 +222,8 @@ class GithubTests: AppTestCase { func test_fetchMetadata_rateLimiting_429() async throws { // Github doesn't actually send a 429 when you hit the rate limit - // setup - Current.githubToken = { "secr3t" } - await withDependencies { + $0.github.token = { "secr3t" } $0.httpClient.post = { @Sendable _, _, _ in .tooManyRequests } } operation: { // MUT @@ -278,10 +273,8 @@ class GithubTests: AppTestCase { // X-RateLimit-Limit: 60 // X-RateLimit-Remaining: 56 // Ensure we record it as a rate limit error and raise a Rollbar item - // setup - Current.githubToken = { "secr3t" } - await withDependencies { + $0.github.token = { "secr3t" } $0.httpClient.post = { @Sendable _, _, _ in .init(status: .forbidden, headers: ["X-RateLimit-Remaining": "0"]) } @@ -313,10 +306,8 @@ class GithubTests: AppTestCase { } func test_fetchLicense() async throws { - // setup - Current.githubToken = { "secr3t" } - await withDependencies { + $0.github.token = { "secr3t" } $0.httpClient.get = { @Sendable _, _ in try .ok(fixture: "github-license-response.json") } @@ -331,10 +322,8 @@ class GithubTests: AppTestCase { func test_fetchLicense_notFound() async throws { // https://github.com/SwiftPackageIndex/SwiftPackageIndex-Server/issues/761 - // setup - Current.githubToken = { "secr3t" } - await withDependencies { + $0.github.token = { "secr3t" } $0.httpClient.get = { @Sendable _, _ in .notFound } } operation: { // MUT @@ -346,10 +335,9 @@ class GithubTests: AppTestCase { } func test_fetchReadme() async throws { - // setup - Current.githubToken = { "secr3t" } let requestCount = QueueIsolated(0) await withDependencies { + $0.github.token = { "secr3t" } $0.httpClient.get = { @Sendable _, headers in requestCount.increment() switch headers[.accept] { @@ -383,10 +371,8 @@ class GithubTests: AppTestCase { } func test_fetchReadme_notFound() async throws { - // setup - Current.githubToken = { "secr3t" } - await withDependencies { + $0.github.token = { "secr3t" } $0.httpClient.get = { @Sendable _, headers in .notFound } } operation: { // MUT diff --git a/Tests/AppTests/IngestionTests.swift b/Tests/AppTests/IngestionTests.swift index 41c260da7..dcb499eed 100644 --- a/Tests/AppTests/IngestionTests.swift +++ b/Tests/AppTests/IngestionTests.swift @@ -640,6 +640,7 @@ class IngestionTests: AppTestCase { // use mock for metadata request which we're not interested in ... $0.github.fetchMetadata = { @Sendable _, _ in .init() } $0.github.fetchReadme = { @Sendable _, _ in nil } + $0.github.token = { "token" } $0.httpClient.get = { @Sendable url, _ in if url.hasSuffix("/license") { return .notFound @@ -653,7 +654,6 @@ class IngestionTests: AppTestCase { // setup let pkg = Package(url: "https://github.com/foo/1") try await pkg.save(on: app.db) - Current.githubToken = { "token" } // MUT let (_, license, _) = try await Ingestion.fetchMetadata(package: pkg, owner: "foo", repository: "1") diff --git a/Tests/AppTests/Mocks/AppEnvironment+mock.swift b/Tests/AppTests/Mocks/AppEnvironment+mock.swift index 11e831979..3a8e6f1cf 100644 --- a/Tests/AppTests/Mocks/AppEnvironment+mock.swift +++ b/Tests/AppTests/Mocks/AppEnvironment+mock.swift @@ -26,7 +26,6 @@ extension AppEnvironment { fileManager: .mock, getStatusCount: { _, _ in 100 }, git: .mock, - githubToken: { nil }, gitlabApiToken: { nil }, gitlabPipelineToken: { nil }, gitlabPipelineLimit: { Constants.defaultGitlabPipelineLimit },