diff --git a/Sources/App/Commands/Ingestion.swift b/Sources/App/Commands/Ingestion.swift index 72dbf3f86..14a25c861 100644 --- a/Sources/App/Commands/Ingestion.swift +++ b/Sources/App/Commands/Ingestion.swift @@ -267,10 +267,11 @@ enum Ingestion { // Sending 'github' into async let risks causing data races between async let uses and local uses @Dependency(\.github.fetchMetadata) var fetchMetadata @Dependency(\.github.fetchLicense) var fetchLicense + @Dependency(\.github.fetchReadme) var fetchReadme async let metadata = try await fetchMetadata(owner, repository) async let license = await fetchLicense(owner, repository) - async let readme = await Current.fetchReadme(client, owner, repository) + async let readme = await fetchReadme(owner, repository) do { return try await (metadata, license, readme) diff --git a/Sources/App/Core/AppEnvironment.swift b/Sources/App/Core/AppEnvironment.swift index 1daa33b08..a8de72aa8 100644 --- a/Sources/App/Core/AppEnvironment.swift +++ b/Sources/App/Core/AppEnvironment.swift @@ -23,7 +23,6 @@ import FoundationNetworking struct AppEnvironment: Sendable { - var fetchReadme: @Sendable (_ client: Client, _ owner: String, _ repository: String) async -> Github.Readme? 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 @@ -64,7 +63,6 @@ extension AppEnvironment { nonisolated(unsafe) static var logger: Logger! static let live = AppEnvironment( - 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) }, fileManager: .live, getStatusCount: { client, status in diff --git a/Sources/App/Core/Dependencies/GithubClient.swift b/Sources/App/Core/Dependencies/GithubClient.swift index 100b944f8..531078a97 100644 --- a/Sources/App/Core/Dependencies/GithubClient.swift +++ b/Sources/App/Core/Dependencies/GithubClient.swift @@ -23,6 +23,7 @@ import IssueReporting 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? } @@ -30,7 +31,8 @@ extension GithubClient: DependencyKey { static var liveValue: Self { .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) } + 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) } ) } } @@ -40,7 +42,8 @@ extension GithubClient: TestDependencyKey { static var testValue: Self { .init( fetchLicense: { _, _ in unimplemented("fetchLicense"); return nil }, - fetchMetadata: { _, _ in unimplemented("fetchMetadata"); return .init() } + fetchMetadata: { _, _ in unimplemented("fetchMetadata"); return .init() }, + fetchReadme: { _, _ in unimplemented("fetchReadme"); return nil } ) } } diff --git a/Sources/App/Core/Github.swift b/Sources/App/Core/Github.swift index 92b140c8d..55b643f8c 100644 --- a/Sources/App/Core/Github.swift +++ b/Sources/App/Core/Github.swift @@ -37,15 +37,6 @@ enum Github { return decoder } - @available(*, deprecated) - static func rateLimit(response: ClientResponse) -> Int? { - guard - let header = response.headers.first(name: "X-RateLimit-Remaining"), - let limit = Int(header) - else { return nil } - return limit - } - static func rateLimit(response: HTTPClient.Response) -> Int? { guard let header = response.headers.first(name: "X-RateLimit-Remaining"), @@ -54,13 +45,6 @@ enum Github { 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) @@ -95,14 +79,6 @@ extension Github { case readme } - @available(*, deprecated) - static func apiUri(owner: String, repository: String, resource: Resource) -> URI { - switch resource { - case .license, .readme: - return URI(string: "https://api.github.com/repos/\(owner)/\(repository)/\(resource.rawValue)") - } - } - static func apiURL(owner: String, repository: String, resource: Resource) -> String { switch resource { case .license, .readme: @@ -110,21 +86,22 @@ extension Github { } } - @available(*, deprecated) - static func fetch(client: Client, uri: URI, headers: [(String, String)] = []) async throws -> (content: String, etag: String?) { + static func fetch(url: String, headers: [(String, String)] = []) async throws -> (content: String, etag: String?) { guard let token = Current.githubToken() else { throw Error.missingToken } - let response = try await client.get(uri, headers: defaultHeaders(with: token).adding(contentsOf: headers)) + @Dependency(\.httpClient) var httpClient + + let response = try await httpClient.get(url: url, headers: defaultHeaders(with: token).adding(contentsOf: headers)) guard !isRateLimited(response) else { - Current.logger().critical("rate limited while fetching uri \(uri)") + Current.logger().critical("rate limited while fetching \(url)") throw Error.requestFailed(.tooManyRequests) } guard response.status == .ok else { - Current.logger().warning("Github.fetch of '\(uri.path)' failed with status \(response.status)") + Current.logger().warning("Github.fetch of '\(url)' failed with status \(response.status)") throw Error.requestFailed(response.status) } @@ -136,26 +113,6 @@ 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 - } - - let response = try await client.get(uri, headers: defaultHeaders(with: token)) - - guard !isRateLimited(response) else { - Current.logger().critical("rate limited while fetching resource \(uri)") - throw Error.requestFailed(.tooManyRequests) - } - - guard response.status == .ok else { - throw Error.requestFailed(response.status) - } - - return try response.content.decode(T.self, using: decoder) - } - static func fetchResource(_ type: T.Type, url: String) async throws -> T { guard let token = Current.githubToken() else { throw Error.missingToken @@ -181,11 +138,11 @@ extension Github { return try? await Github.fetchResource(Github.License.self, url: url) } - static func fetchReadme(client: Client, owner: String, repository: String) async -> Readme? { - let uri = Github.apiUri(owner: owner, repository: repository, resource: .readme) + static func fetchReadme(owner: String, repository: String) async -> Readme? { + let url = Github.apiURL(owner: owner, repository: repository, resource: .readme) // Fetch readme html content - let readme = try? await Github.fetch(client: client, uri: uri, headers: [ + let readme = try? await Github.fetch(url: url, headers: [ ("Accept", "application/vnd.github.html+json") ]) guard var html = readme?.content else { return nil } @@ -195,7 +152,7 @@ extension Github { struct Response: Decodable { var htmlUrl: String } - return try? await Github.fetchResource(Response.self, client: client, uri: uri).htmlUrl + return try? await Github.fetchResource(Response.self, url: url).htmlUrl }() guard let htmlUrl else { return nil } @@ -212,46 +169,12 @@ extension Github { extension Github { - @available(*, deprecated) - static let graphQLApiUri = URI(string: "https://api.github.com/graphql") static let graphQLApiURL = "https://api.github.com/graphql" struct GraphQLQuery: Content { var query: String } - @available(*, deprecated) - static func fetchResource(_ type: T.Type, client: Client, query: GraphQLQuery) async throws(Github.Error) -> T { - guard let token = Current.githubToken() else { - throw Error.missingToken - } - - let response: ClientResponse - do { - response = try await client.post(Self.graphQLApiUri, headers: defaultHeaders(with: token)) { - try $0.content.encode(query) - } - } catch { - throw .postRequestFailed(graphQLApiUri.string, error) - } - - guard !isRateLimited(response) else { - Current.logger().critical("rate limited while fetching resource \(T.self)") - throw Error.requestFailed(.tooManyRequests) - } - - guard response.status == .ok else { - Current.logger().warning("fetchResource<\(T.self)> request failed with status \(response.status)") - throw Error.requestFailed(response.status) - } - - do { - return try response.content.decode(T.self, using: decoder) - } catch { - throw .decodeContentFailed(graphQLApiUri.string, error) - } - } - static func fetchResource(_ type: T.Type, query: GraphQLQuery) async throws(Github.Error) -> T { guard let token = Current.githubToken() else { throw Error.missingToken diff --git a/Tests/AppTests/GithubTests.swift b/Tests/AppTests/GithubTests.swift index c5cc41d07..748229d36 100644 --- a/Tests/AppTests/GithubTests.swift +++ b/Tests/AppTests/GithubTests.swift @@ -349,48 +349,52 @@ class GithubTests: AppTestCase { // setup Current.githubToken = { "secr3t" } let requestCount = QueueIsolated(0) - let client = MockClient { req, resp in - requestCount.increment() - switch req.headers[.accept] { - case ["application/vnd.github.html+json"]: - resp.status = .ok - resp.body = makeBody("readme html") - resp.headers.add(name: .eTag, value: "etag") - case []: - resp.status = .ok - struct Response: Encodable { - var htmlUrl: String - } - resp.body = makeBody(try! JSONEncoder().encode(Response(htmlUrl: "readme url"))) - default: - XCTFail("unexpected accept header") + await withDependencies { + $0.httpClient.get = { @Sendable _, headers in + requestCount.increment() + switch headers[.accept] { + case ["application/vnd.github.html+json"]: + return .ok(body: "readme html", headers: ["ETag": "etag"]) + case []: + struct Response: Encodable { + var htmlUrl: String + } + return try .ok(jsonEncode: Response(htmlUrl: "readme url")) + default: + XCTFail("unexpected accept header") + } + enum Error: Swift.Error { case unexpectedCodePath } + throw Error.unexpectedCodePath } - } + } operation: { + // MUT + let res = await Github.fetchReadme(owner: "foo", repository: "bar") - // MUT - let res = await Github.fetchReadme(client: client, owner: "foo", repository: "bar") - - // validate - XCTAssertEqual(requestCount.value, 2) - XCTAssertEqual( - res, - .init(etag: "etag", - html: "readme html", - htmlUrl: "readme url", - imagesToCache: []) - ) + // validate + XCTAssertEqual(requestCount.value, 2) + XCTAssertEqual( + res, + .init(etag: "etag", + html: "readme html", + htmlUrl: "readme url", + imagesToCache: []) + ) + } } func test_fetchReadme_notFound() async throws { // setup Current.githubToken = { "secr3t" } - let client = MockClient { _, resp in resp.status = .notFound } - // MUT - let res = await Github.fetchReadme(client: client, owner: "foo", repository: "bar") + await withDependencies { + $0.httpClient.get = { @Sendable _, headers in .notFound } + } operation: { + // MUT + let res = await Github.fetchReadme(owner: "foo", repository: "bar") - // validate - XCTAssertEqual(res, nil) + // validate + XCTAssertEqual(res, nil) + } } func test_extractImagesRequiringCaching() async throws { @@ -482,11 +486,16 @@ class GithubTests: AppTestCase { private extension HTTPClient.Response { - static func ok(body: String) -> Self { - .init(status: .ok, body: .init(string: body)) + static func ok(body: String, headers: HTTPHeaders = .init()) -> Self { + .init(status: .ok, headers: headers, body: .init(string: body)) } static func ok(fixture: String) throws -> Self { try .init(status: .ok, body: .init(data: fixtureData(for: fixture))) } + + static func ok(jsonEncode value: T) throws -> Self { + let data = try JSONEncoder().encode(value) + return .init(status: .ok, body: .init(data: data)) + } } diff --git a/Tests/AppTests/IngestionTests.swift b/Tests/AppTests/IngestionTests.swift index 021f668ad..57559d578 100644 --- a/Tests/AppTests/IngestionTests.swift +++ b/Tests/AppTests/IngestionTests.swift @@ -37,6 +37,7 @@ class IngestionTests: AppTestCase { $0.date.now = .now $0.github.fetchLicense = { @Sendable _, _ in nil } $0.github.fetchMetadata = { @Sendable owner, repository in .mock(owner: owner, repository: repository) } + $0.github.fetchReadme = { @Sendable _, _ in nil } } operation: { // MUT try await Ingestion.ingest(client: app.client, database: app.db, mode: .limit(10)) @@ -73,6 +74,7 @@ class IngestionTests: AppTestCase { } return .mock(owner: owner, repository: repository) } + $0.github.fetchReadme = { @Sendable _, _ in nil } } operation: { // setup let packages = try await savePackages(on: app.db, ["https://github.com/foo/1", @@ -313,6 +315,7 @@ class IngestionTests: AppTestCase { $0.date.now = .now $0.github.fetchLicense = { @Sendable _, _ in nil } $0.github.fetchMetadata = { @Sendable owner, repository in .mock(owner: owner, repository: repository) } + $0.github.fetchReadme = { @Sendable _, _ in nil } } operation: { // MUT try await Ingestion.ingest(client: app.client, database: app.db, mode: .limit(testUrls.count)) @@ -341,6 +344,7 @@ class IngestionTests: AppTestCase { } return .mock(owner: owner, repository: repository) } + $0.github.fetchReadme = { @Sendable _, _ in nil } } operation: { // MUT try await Ingestion.ingest(client: app.client, database: app.db, mode: .limit(10)) @@ -394,6 +398,7 @@ class IngestionTests: AppTestCase { stars: 0, summary: "desc") } + $0.github.fetchReadme = { @Sendable _, _ in nil } } operation: { // MUT try await Ingestion.ingest(client: app.client, database: app.db, mode: .limit(10)) @@ -459,17 +464,12 @@ class IngestionTests: AppTestCase { } func test_ingest_storeS3Readme() async throws { + let fetchCalls = QueueIsolated(0) try await withDependencies { $0.date.now = .now $0.github.fetchLicense = { @Sendable _, _ in nil } $0.github.fetchMetadata = { @Sendable owner, repository in .mock(owner: owner, repository: repository) } - } 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 fetchCalls = QueueIsolated(0) - Current.fetchReadme = { _, _, _ in + $0.github.fetchReadme = { @Sendable _, _ in fetchCalls.increment() if fetchCalls.value <= 2 { return .init(etag: "etag1", @@ -483,6 +483,11 @@ 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 storeCalls.increment() @@ -548,27 +553,6 @@ class IngestionTests: AppTestCase { processingStage: .reconciliation) try await pkg.save(on: app.db) Current.storeS3Readme = { _, _, _ in "objectUrl" } - Current.fetchReadme = { _, _, _ in - return .init(etag: "etag", - html: """ - - - - - - - - """, - htmlUrl: "readme url", - imagesToCache: [ - .init(originalUrl: "https://private-user-images.githubusercontent.com/with-jwt-1.jpg?jwt=some-jwt", - s3Key: .init(bucket: "awsReadmeBucket", - path: "/foo/bar/with-jwt-1.jpg")), - .init(originalUrl: "https://private-user-images.githubusercontent.com/with-jwt-2.jpg?jwt=some-jwt", - s3Key: .init(bucket: "awsReadmeBucket", - path: "/foo/bar/with-jwt-2.jpg")) - ]) - } let storeS3ReadmeImagesCalls = QueueIsolated(0) Current.storeS3ReadmeImages = { _, imagesToCache in storeS3ReadmeImagesCalls.increment() @@ -580,6 +564,27 @@ class IngestionTests: AppTestCase { $0.date.now = .now $0.github.fetchLicense = { @Sendable _, _ in nil } $0.github.fetchMetadata = { @Sendable owner, repository in .mock(owner: owner, repository: repository) } + $0.github.fetchReadme = { @Sendable _, _ in + return .init(etag: "etag", + html: """ + + + + + + + + """, + htmlUrl: "readme url", + imagesToCache: [ + .init(originalUrl: "https://private-user-images.githubusercontent.com/with-jwt-1.jpg?jwt=some-jwt", + s3Key: .init(bucket: "awsReadmeBucket", + path: "/foo/bar/with-jwt-1.jpg")), + .init(originalUrl: "https://private-user-images.githubusercontent.com/with-jwt-2.jpg?jwt=some-jwt", + s3Key: .init(bucket: "awsReadmeBucket", + path: "/foo/bar/with-jwt-2.jpg")) + ]) + } } operation: { // MUT try await Ingestion.ingest(client: app.client, database: app.db, mode: .limit(1)) @@ -594,12 +599,6 @@ class IngestionTests: AppTestCase { // setup let pkg = Package(url: "https://github.com/foo/bar".url, processingStage: .reconciliation) try await pkg.save(on: app.db) - Current.fetchReadme = { _, _, _ in - return .init(etag: "etag1", - html: "readme html 1", - htmlUrl: "readme url", - imagesToCache: []) - } let storeCalls = QueueIsolated(0) Current.storeS3Readme = { owner, repo, html throws(S3Readme.Error) in storeCalls.increment() @@ -611,6 +610,12 @@ class IngestionTests: AppTestCase { $0.date.now = .now $0.github.fetchLicense = { @Sendable _, _ in nil } $0.github.fetchMetadata = { @Sendable owner, repository in .mock(owner: owner, repository: repository) } + $0.github.fetchReadme = { @Sendable _, _ in + return .init(etag: "etag1", + html: "readme html 1", + htmlUrl: "readme url", + imagesToCache: []) + } } operation: { // MUT let app = self.app! @@ -634,6 +639,7 @@ class IngestionTests: AppTestCase { $0.github.fetchLicense = { @Sendable owner, repo in await Github.fetchLicense(owner: owner, repository: repo) } // use mock for metadata request which we're not interested in ... $0.github.fetchMetadata = { @Sendable _, _ in .init() } + $0.github.fetchReadme = { @Sendable _, _ in nil } } operation: { // setup let pkg = Package(url: "https://github.com/foo/1") diff --git a/Tests/AppTests/MastodonTests.swift b/Tests/AppTests/MastodonTests.swift index 74f74e7e4..26a646e6c 100644 --- a/Tests/AppTests/MastodonTests.swift +++ b/Tests/AppTests/MastodonTests.swift @@ -27,6 +27,7 @@ final class MastodonTests: AppTestCase { $0.environment.allowSocialPosts = { true } $0.github.fetchLicense = { @Sendable _, _ in nil } $0.github.fetchMetadata = { @Sendable owner, repository in .mock(owner: owner, repository: repository) } + $0.github.fetchReadme = { @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 c9b5e2b4f..11e831979 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( - fetchReadme: { _, _, _ in .init(html: "readme html", htmlUrl: "readme html url", imagesToCache: []) }, fetchS3Readme: { _, _, _ in "" }, fileManager: .mock, getStatusCount: { _, _ in 100 }, diff --git a/Tests/AppTests/PackageTests.swift b/Tests/AppTests/PackageTests.swift index 3ad6539f6..5b624484e 100644 --- a/Tests/AppTests/PackageTests.swift +++ b/Tests/AppTests/PackageTests.swift @@ -313,6 +313,7 @@ final class PackageTests: AppTestCase { $0.date.now = .now $0.github.fetchLicense = { @Sendable _, _ in nil } $0.github.fetchMetadata = { @Sendable owner, repository in .mock(owner: owner, repository: repository) } + $0.github.fetchReadme = { @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 ce27b2d66..8dbbde8d6 100644 --- a/Tests/AppTests/PipelineTests.swift +++ b/Tests/AppTests/PipelineTests.swift @@ -163,6 +163,7 @@ class PipelineTests: AppTestCase { $0.date.now = .now $0.github.fetchLicense = { @Sendable _, _ in nil } $0.github.fetchMetadata = { @Sendable owner, repository in .mock(owner: owner, repository: repository) } + $0.github.fetchReadme = { @Sendable _, _ in nil } $0.packageListRepository.fetchPackageList = { @Sendable _ in urls.asURLs } $0.packageListRepository.fetchPackageDenyList = { @Sendable _ in [] } $0.packageListRepository.fetchCustomCollections = { @Sendable _ in [] }