diff --git a/Sources/App/Commands/Ingestion.swift b/Sources/App/Commands/Ingestion.swift index e68b83659..14edba154 100644 --- a/Sources/App/Commands/Ingestion.swift +++ b/Sources/App/Commands/Ingestion.swift @@ -258,12 +258,13 @@ enum Ingestion { static func fetchMetadata(client: Client, package: Package, owner: String, repository: String) async throws(Github.Error) -> (Github.Metadata, Github.License?, Github.Readme?) { @Dependency(\.environment) var environment + @Dependency(\.github) var github if environment.shouldFail(failureMode: .fetchMetadataFailed) { throw Github.Error.requestFailed(.internalServerError) } async let metadata = try await Current.fetchMetadata(client, owner, repository) - async let license = await Current.fetchLicense(client, owner, repository) + async let license = await github.fetchLicense(owner, repository) async let readme = await Current.fetchReadme(client, owner, repository) do { diff --git a/Sources/App/Core/AppEnvironment.swift b/Sources/App/Core/AppEnvironment.swift index ae8ed942a..519e430d6 100644 --- a/Sources/App/Core/AppEnvironment.swift +++ b/Sources/App/Core/AppEnvironment.swift @@ -23,7 +23,6 @@ import FoundationNetworking struct AppEnvironment: Sendable { - var fetchLicense: @Sendable (_ client: Client, _ owner: String, _ repository: String) async -> Github.License? var fetchMetadata: @Sendable (_ client: Client, _ owner: String, _ repository: String) async throws(Github.Error) -> Github.Metadata var fetchReadme: @Sendable (_ client: Client, _ owner: String, _ repository: String) async -> Github.Readme? var fetchS3Readme: @Sendable (_ client: Client, _ owner: String, _ repository: String) async throws -> String @@ -66,7 +65,6 @@ extension AppEnvironment { nonisolated(unsafe) static var logger: Logger! static let live = AppEnvironment( - fetchLicense: { client, owner, repo in await Github.fetchLicense(client:client, owner: owner, repository: repo) }, fetchMetadata: { client, owner, repo throws(Github.Error) in try await Github.fetchMetadata(client:client, owner: owner, repository: repo) }, fetchReadme: { client, owner, repo in await Github.fetchReadme(client:client, owner: owner, repository: repo) }, fetchS3Readme: { client, owner, repo in try await S3Readme.fetchReadme(client:client, owner: owner, repository: repo) }, diff --git a/Sources/App/Core/Dependencies/GithubClient.swift b/Sources/App/Core/Dependencies/GithubClient.swift new file mode 100644 index 000000000..479074783 --- /dev/null +++ b/Sources/App/Core/Dependencies/GithubClient.swift @@ -0,0 +1,45 @@ +// 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 + + +@DependencyClient +struct GithubClient { + var fetchLicense: @Sendable (_ owner: String, _ repository: String) async -> Github.License? +} + + +extension GithubClient: DependencyKey { + static var liveValue: Self { + .init( + fetchLicense: { owner, repo in await Github.fetchLicense(owner: owner, repository: repo) } + ) + } +} + + +extension GithubClient: TestDependencyKey { + static var testValue: Self { Self() } +} + + +extension DependencyValues { + var github: GithubClient { + get { self[GithubClient.self] } + set { self[GithubClient.self] = newValue } + } +} diff --git a/Sources/App/Core/Dependencies/HTTPClient.swift b/Sources/App/Core/Dependencies/HTTPClient.swift index ec63151c1..532079ade 100644 --- a/Sources/App/Core/Dependencies/HTTPClient.swift +++ b/Sources/App/Core/Dependencies/HTTPClient.swift @@ -23,7 +23,9 @@ struct HTTPClient { typealias Request = Vapor.HTTPClient.Request typealias Response = Vapor.HTTPClient.Response + var get: @Sendable (_ url: String, _ headers: HTTPHeaders) async throws -> Response var post: @Sendable (_ url: String, _ headers: HTTPHeaders, _ body: Data?) async throws -> Response + var fetchDocumentation: @Sendable (_ url: URI) async throws -> Response var fetchHTTPStatusCode: @Sendable (_ url: String) async throws -> HTTPStatus var mastodonPost: @Sendable (_ message: String) async throws -> Void @@ -33,6 +35,10 @@ struct HTTPClient { extension HTTPClient: DependencyKey { static var liveValue: HTTPClient { .init( + get: { url, headers in + let req = try Request(url: url, method: .GET, headers: headers) + return try await Vapor.HTTPClient.shared.execute(request: req).get() + }, post: { url, headers, body in let req = try Request(url: url, method: .POST, headers: headers, body: body.map({.data($0)})) return try await Vapor.HTTPClient.shared.execute(request: req).get() diff --git a/Sources/App/Core/Github.swift b/Sources/App/Core/Github.swift index d4f0c0cfc..a63ceca83 100644 --- a/Sources/App/Core/Github.swift +++ b/Sources/App/Core/Github.swift @@ -12,9 +12,10 @@ // See the License for the specific language governing permissions and // limitations under the License. -import Vapor -import SwiftSoup +import Dependencies import S3Store +import SwiftSoup +import Vapor enum Github { @@ -35,6 +36,7 @@ enum Github { return decoder } + @available(*, deprecated) static func rateLimit(response: ClientResponse) -> Int? { guard let header = response.headers.first(name: "X-RateLimit-Remaining"), @@ -43,12 +45,27 @@ enum Github { return limit } + static func rateLimit(response: HTTPClient.Response) -> Int? { + guard + let header = response.headers.first(name: "X-RateLimit-Remaining"), + let limit = Int(header) + else { return nil } + return limit + } + + @available(*, deprecated) static func isRateLimited(_ response: ClientResponse) -> Bool { guard let limit = rateLimit(response: response) else { return false } AppMetrics.githubRateLimitRemainingCount?.set(limit) return response.status == .forbidden && limit == 0 } + static func isRateLimited(_ response: HTTPClient.Response) -> Bool { + guard let limit = rateLimit(response: response) else { return false } + AppMetrics.githubRateLimitRemainingCount?.set(limit) + return response.status == .forbidden && limit == 0 + } + static func parseOwnerName(url: String) throws(Github.Error) -> (owner: String, name: String) { let parts = url .droppingGithubComPrefix @@ -77,6 +94,7 @@ extension Github { case readme } + @available(*, deprecated) static func apiUri(owner: String, repository: String, resource: Resource) -> URI { switch resource { case .license, .readme: @@ -84,6 +102,14 @@ extension Github { } } + static func apiURL(owner: String, repository: String, resource: Resource) -> String { + switch resource { + case .license, .readme: + return "https://api.github.com/repos/\(owner)/\(repository)/\(resource.rawValue)" + } + } + + @available(*, deprecated) static func fetch(client: Client, uri: URI, headers: [(String, String)] = []) async throws -> (content: String, etag: String?) { guard let token = Current.githubToken() else { throw Error.missingToken @@ -109,6 +135,7 @@ extension Github { return (body.asString(), response.headers.first(name: .eTag)) } + @available(*, deprecated) static func fetchResource(_ type: T.Type, client: Client, uri: URI) async throws -> T { guard let token = Current.githubToken() else { throw Error.missingToken @@ -128,9 +155,29 @@ extension Github { return try response.content.decode(T.self, using: decoder) } - static func fetchLicense(client: Client, owner: String, repository: String) async -> License? { - let uri = Github.apiUri(owner: owner, repository: repository, resource: .license) - return try? await Github.fetchResource(Github.License.self, client: client, uri: uri) + static func fetchResource(_ type: T.Type, url: String) async throws -> T { + guard let token = Current.githubToken() else { + throw Error.missingToken + } + + @Dependency(\.httpClient) var httpClient + + let response = try await httpClient.get(url: url, headers: defaultHeaders(with: token)) + + guard !isRateLimited(response) else { + Current.logger().critical("rate limited while fetching resource \(url)") + throw Error.requestFailed(.tooManyRequests) + } + + guard response.status == .ok else { throw Error.requestFailed(response.status) } + guard let body = response.body else { throw Github.Error.noBody } + + return try decoder.decode(T.self, from: body) + } + + static func fetchLicense(owner: String, repository: String) async -> License? { + let url = Github.apiURL(owner: owner, repository: repository, resource: .license) + return try? await Github.fetchResource(Github.License.self, url: url) } static func fetchReadme(client: Client, owner: String, repository: String) async -> Readme? { @@ -466,7 +513,7 @@ extension Github { } } } - + struct Parent: Decodable, Equatable { var url: String? } diff --git a/Tests/AppTests/GithubTests.swift b/Tests/AppTests/GithubTests.swift index 87c6c54af..6adeddc3b 100644 --- a/Tests/AppTests/GithubTests.swift +++ b/Tests/AppTests/GithubTests.swift @@ -261,28 +261,28 @@ class GithubTests: AppTestCase { func test_isRateLimited() throws { do { - let res = ClientResponse(status: .forbidden, - headers: .init([("X-RateLimit-Remaining", "0")])) + let res = HTTPClient.Response(status: .forbidden, + headers: .init([("X-RateLimit-Remaining", "0")])) XCTAssertTrue(Github.isRateLimited(res)) } do { - let res = ClientResponse(status: .forbidden, - headers: .init([("x-ratelimit-remaining", "0")])) + let res = HTTPClient.Response(status: .forbidden, + headers: .init([("x-ratelimit-remaining", "0")])) XCTAssertTrue(Github.isRateLimited(res)) } do { - let res = ClientResponse(status: .forbidden, - headers: .init([("X-RateLimit-Remaining", "1")])) + let res = HTTPClient.Response(status: .forbidden, + headers: .init([("X-RateLimit-Remaining", "1")])) XCTAssertFalse(Github.isRateLimited(res)) } do { - let res = ClientResponse(status: .forbidden, - headers: .init([("unrelated", "0")])) + let res = HTTPClient.Response(status: .forbidden, + headers: .init([("unrelated", "0")])) XCTAssertFalse(Github.isRateLimited(res)) } do { - let res = ClientResponse(status: .ok, - headers: .init([("X-RateLimit-Remaining", "0")])) + let res = HTTPClient.Response(status: .ok, + headers: .init([("X-RateLimit-Remaining", "0")])) XCTAssertFalse(Github.isRateLimited(res)) } } @@ -318,40 +318,44 @@ class GithubTests: AppTestCase { } } - func test_apiUri() throws { - XCTAssertEqual(Github.apiUri(owner: "foo", repository: "bar", resource: .license).string, + func test_apiURL() throws { + XCTAssertEqual(Github.apiURL(owner: "foo", repository: "bar", resource: .license), "https://api.github.com/repos/foo/bar/license") - XCTAssertEqual(Github.apiUri(owner: "foo", repository: "bar", resource: .readme).string, + XCTAssertEqual(Github.apiURL(owner: "foo", repository: "bar", resource: .readme), "https://api.github.com/repos/foo/bar/readme") } func test_fetchLicense() async throws { // setup Current.githubToken = { "secr3t" } - let data = try XCTUnwrap(try fixtureData(for: "github-license-response.json")) - let client = MockClient { _, resp in - resp.status = .ok - resp.body = makeBody(data) - } - // MUT - let res = await Github.fetchLicense(client: client, owner: "PSPDFKit", repository: "PSPDFKit-SP") + await withDependencies { + $0.httpClient.get = { @Sendable _, _ in + try .init(status: .ok, body: .fixture(named: "github-license-response.json")) + } + } operation: { + // MUT + let res = await Github.fetchLicense(owner: "PSPDFKit", repository: "PSPDFKit-SP") - // validate - XCTAssertEqual(res?.htmlUrl, "https://github.com/PSPDFKit/PSPDFKit-SP/blob/master/LICENSE") + // validate + XCTAssertEqual(res?.htmlUrl, "https://github.com/PSPDFKit/PSPDFKit-SP/blob/master/LICENSE") + } } func test_fetchLicense_notFound() async throws { // https://github.com/SwiftPackageIndex/SwiftPackageIndex-Server/issues/761 // setup Current.githubToken = { "secr3t" } - let client = MockClient { _, resp in resp.status = .notFound } - // MUT - let res = await Github.fetchLicense(client: client, owner: "foo", repository: "bar") + await withDependencies { + $0.httpClient.get = { @Sendable _, _ in .notFound } + } operation: { + // MUT + let res = await Github.fetchLicense(owner: "foo", repository: "bar") - // validate - XCTAssertEqual(res, nil) + // validate + XCTAssertEqual(res, nil) + } } func test_fetchReadme() async throws { @@ -488,3 +492,10 @@ class GithubTests: AppTestCase { } } + + +private extension ByteBuffer { + static func fixture(named filename: String) throws -> Self { + .init(data: try fixtureData(for: filename)) + } +} diff --git a/Tests/AppTests/IngestionTests.swift b/Tests/AppTests/IngestionTests.swift index a0b915530..f0cd97a02 100644 --- a/Tests/AppTests/IngestionTests.swift +++ b/Tests/AppTests/IngestionTests.swift @@ -36,6 +36,7 @@ class IngestionTests: AppTestCase { try await withDependencies { $0.date.now = .now + $0.github.fetchLicense = { @Sendable _, _ in nil } } operation: { // MUT try await Ingestion.ingest(client: app.client, database: app.db, mode: .limit(10)) @@ -64,30 +65,33 @@ class IngestionTests: AppTestCase { func test_ingest_continue_on_error() async throws { // Test completion of ingestion despite early error - // setup - let packages = try await savePackages(on: app.db, ["https://github.com/foo/1", - "https://github.com/foo/2"], processingStage: .reconciliation) - .map(Joined.init(model:)) - Current.fetchMetadata = { _, owner, repository throws(Github.Error) in - if owner == "foo" && repository == "1" { - throw Github.Error.requestFailed(.badRequest) + try await withDependencies { + $0.github.fetchLicense = { @Sendable _, _ in Github.License(htmlUrl: "license") } + } operation: { + // setup + let packages = try await savePackages(on: app.db, ["https://github.com/foo/1", + "https://github.com/foo/2"], processingStage: .reconciliation) + .map(Joined.init(model:)) + Current.fetchMetadata = { _, owner, repository throws(Github.Error) in + if owner == "foo" && repository == "1" { + throw Github.Error.requestFailed(.badRequest) + } + return .mock(owner: owner, repository: repository) } - return .mock(owner: owner, repository: repository) - } - Current.fetchLicense = { _, _, _ in Github.License(htmlUrl: "license") } - // MUT - await Ingestion.ingest(client: app.client, database: app.db, packages: packages) - - do { - // validate the second package's license is updated - let repo = try await Repository.query(on: app.db) - .filter(\.$name == "2") - .first() - .unwrap() - XCTAssertEqual(repo.licenseUrl, "license") - for pkg in try await Package.query(on: app.db).all() { - XCTAssertEqual(pkg.processingStage, .ingestion, "\(pkg.url) must be in ingestion") + // MUT + await Ingestion.ingest(client: app.client, database: app.db, packages: packages) + + do { + // validate the second package's license is updated + let repo = try await Repository.query(on: app.db) + .filter(\.$name == "2") + .first() + .unwrap() + XCTAssertEqual(repo.licenseUrl, "license") + for pkg in try await Package.query(on: app.db).all() { + XCTAssertEqual(pkg.processingStage, .ingestion, "\(pkg.url) must be in ingestion") + } } } } @@ -308,6 +312,7 @@ class IngestionTests: AppTestCase { try await withDependencies { $0.date.now = .now + $0.github.fetchLicense = { @Sendable _, _ in nil } } operation: { // MUT try await Ingestion.ingest(client: app.client, database: app.db, mode: .limit(testUrls.count)) @@ -335,6 +340,7 @@ class IngestionTests: AppTestCase { try await withDependencies { $0.date.now = .now + $0.github.fetchLicense = { @Sendable _, _ in nil } } operation: { // MUT try await Ingestion.ingest(client: app.client, database: app.db, mode: .limit(10)) @@ -387,6 +393,7 @@ class IngestionTests: AppTestCase { try await withDependencies { $0.date.now = .now + $0.github.fetchLicense = { @Sendable _, _ in nil } } operation: { // MUT try await Ingestion.ingest(client: app.client, database: app.db, mode: .limit(10)) @@ -454,6 +461,7 @@ class IngestionTests: AppTestCase { func test_ingest_storeS3Readme() async throws { try await withDependencies { $0.date.now = .now + $0.github.fetchLicense = { @Sendable _, _ in nil } } operation: { // setup let app = self.app! @@ -571,6 +579,7 @@ class IngestionTests: AppTestCase { try await withDependencies { $0.date.now = .now + $0.github.fetchLicense = { @Sendable _, _ in nil } } operation: { // MUT try await Ingestion.ingest(client: app.client, database: app.db, mode: .limit(1)) @@ -601,6 +610,7 @@ class IngestionTests: AppTestCase { do { // first ingestion, no readme has been saved try await withDependencies { $0.date.now = .now + $0.github.fetchLicense = { @Sendable _, _ in nil } } operation: { // MUT let app = self.app! @@ -619,22 +629,25 @@ class IngestionTests: AppTestCase { func test_issue_761_no_license() async throws { // https://github.com/SwiftPackageIndex/SwiftPackageIndex-Server/issues/761 - // setup - let pkg = Package(url: "https://github.com/foo/1") - try await pkg.save(on: app.db) - // use mock for metadata request which we're not interested in ... - Current.fetchMetadata = { _, _, _ in Github.Metadata() } - // and live fetch request for fetchLicense, whose behaviour we want to test ... - Current.fetchLicense = { client, owner, repo in await Github.fetchLicense(client: client, owner: owner, repository: repo) } - // 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 } + 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) } + } operation: { + // setup + let pkg = Package(url: "https://github.com/foo/1") + try await pkg.save(on: app.db) + // use mock for metadata request which we're not interested in ... + Current.fetchMetadata = { _, _, _ in Github.Metadata() } + // 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 } - // MUT - let (_, license, _) = try await Ingestion.fetchMetadata(client: client, package: pkg, owner: "foo", repository: "1") + // MUT + let (_, license, _) = try await Ingestion.fetchMetadata(client: client, package: pkg, owner: "foo", repository: "1") - // validate - XCTAssertEqual(license, nil) + // validate + XCTAssertEqual(license, nil) + } } func test_migration076_updateRepositoryResetReadmes() async throws { diff --git a/Tests/AppTests/MastodonTests.swift b/Tests/AppTests/MastodonTests.swift index acf28d8f3..bd8445be1 100644 --- a/Tests/AppTests/MastodonTests.swift +++ b/Tests/AppTests/MastodonTests.swift @@ -25,6 +25,7 @@ final class MastodonTests: AppTestCase { let message = QueueIsolated(nil) try await withDependencies { $0.environment.allowSocialPosts = { true } + $0.github.fetchLicense = { @Sendable _, _ in nil } $0.httpClient.mastodonPost = { @Sendable msg in if message.value == nil { message.setValue(msg) diff --git a/Tests/AppTests/Mocks/AppEnvironment+mock.swift b/Tests/AppTests/Mocks/AppEnvironment+mock.swift index 8bb1183c8..25dc04c94 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( - fetchLicense: { _, _, _ in .init(htmlUrl: "https://github.com/foo/bar/blob/main/LICENSE") }, fetchMetadata: { _, _, _ in .mock }, fetchReadme: { _, _, _ in .init(html: "readme html", htmlUrl: "readme html url", imagesToCache: []) }, fetchS3Readme: { _, _, _ in "" }, diff --git a/Tests/AppTests/PackageTests.swift b/Tests/AppTests/PackageTests.swift index d771c75bd..5cbf66869 100644 --- a/Tests/AppTests/PackageTests.swift +++ b/Tests/AppTests/PackageTests.swift @@ -311,6 +311,7 @@ final class PackageTests: AppTestCase { let url = "1".asGithubUrl try await withDependencies { $0.date.now = .now + $0.github.fetchLicense = { @Sendable _, _ in nil } $0.packageListRepository.fetchPackageList = { @Sendable _ in [url.url] } $0.packageListRepository.fetchPackageDenyList = { @Sendable _ in [] } $0.packageListRepository.fetchCustomCollections = { @Sendable _ in [] } diff --git a/Tests/AppTests/PipelineTests.swift b/Tests/AppTests/PipelineTests.swift index f34940663..020e42308 100644 --- a/Tests/AppTests/PipelineTests.swift +++ b/Tests/AppTests/PipelineTests.swift @@ -161,6 +161,7 @@ class PipelineTests: AppTestCase { let urls = ["1", "2", "3"].asGithubUrls try await withDependencies { $0.date.now = .now + $0.github.fetchLicense = { @Sendable _, _ in nil } $0.packageListRepository.fetchPackageList = { @Sendable _ in urls.asURLs } $0.packageListRepository.fetchPackageDenyList = { @Sendable _ in [] } $0.packageListRepository.fetchCustomCollections = { @Sendable _ in [] }