Merge pull request #3321 from SwiftPackageIndex/cleanup-error-handling

Cleanup error handling

Author: finestructure

Sources/App/Commands/Common.swift

@@ -121,12 +121,8 @@ func recordError(database: Database,
             try await setStatus(id: id, status: .invalidCachePath)
         case let .cacheDirectoryDoesNotExist(id, _):
             try await setStatus(id: id, status: .cacheDirectoryDoesNotExist)
-        case let .invalidPackageUrl(id, _):
-            try await setStatus(id: id, status: .invalidUrl)
         case let .invalidRevision(id, _):
             try await setStatus(id: id, status: .analysisFailed)
-        case let .metadataRequestFailed(id, _, _):
-            try await setStatus(id: id, status: .metadataRequestFailed)
         case let .noValidVersions(id, _):
             try await setStatus(id: id, status: .noValidVersions)
     }

Sources/App/Core/AppError.swift

@@ -18,11 +18,9 @@ import Vapor
 enum AppError: LocalizedError {
     case analysisError(Package.Id?, _ message: String)
     case envVariableNotSet(_ variable: String)
-    case invalidPackageUrl(Package.Id?, _ url: String)
     case invalidPackageCachePath(Package.Id?, _ path: String)
     case cacheDirectoryDoesNotExist(Package.Id?, _ path: String)
     case invalidRevision(Version.Id?, _ revision: String?)
-    case metadataRequestFailed(Package.Id?, HTTPStatus, URI)
     case noValidVersions(Package.Id?, _ url: String)
     case shellCommandFailed(_ command: String, _ path: String, _ message: String)
 
@@ -34,16 +32,12 @@ enum AppError: LocalizedError {
                 return "Analysis failed: \(message) (id: \(id))"
             case let .envVariableNotSet(value):
                 return "Environment variable not set: \(value)"
-            case let .invalidPackageUrl(id, value):
-                return "Invalid packge URL: \(value) (id: \(id))"
             case let .invalidPackageCachePath(id, value):
                 return "Invalid packge cache path: \(value) (id: \(id))"
             case let .cacheDirectoryDoesNotExist(id, value):
                 return "Package cache directory, \(value), does not exist: (id: \(id)"
             case let .invalidRevision(id, value):
                 return "Invalid revision: \(value ?? "nil") (id: \(id))"
-            case let .metadataRequestFailed(id, status, uri):
-                return "Metadata request for URI '\(uri.description)' failed with status '\(status)'  (id: \(id))"
             case let .noValidVersions(id, value):
                 return "No valid version found for package '\(value)' (id: \(id))"
             case let .shellCommandFailed(command, path, message):

Tests/AppTests/ErrorReportingTests.swift

@@ -22,11 +22,11 @@ class ErrorReportingTests: AppTestCase {
     func test_recordError() async throws {
         let pkg = try await savePackageAsync(on: app.db, "1")
         try await recordError(database: app.db,
-                              error: AppError.invalidPackageUrl(pkg.id, "foo"),
+                              error: AppError.cacheDirectoryDoesNotExist(pkg.id, "path"),
                               stage: .ingestion)
         do {
             let pkg = try fetch(id: pkg.id, on: app.db)
-            XCTAssertEqual(pkg.status, .invalidUrl)
+            XCTAssertEqual(pkg.status, .cacheDirectoryDoesNotExist)
             XCTAssertEqual(pkg.processingStage, .ingestion)
         }
     }

Tests/AppTests/IngestorTests.swift

@@ -240,7 +240,7 @@ class IngestorTests: AppTestCase {
                                                             "https://github.com/foo/2"])
             .map(Joined<Package, Repository>.init(model:))
         let results: [Result<Joined<Package, Repository>, Error>] = [
-            .failure(AppError.metadataRequestFailed(try pkgs[0].model.requireID(), .badRequest, "1")),
+            .failure(AppError.genericError(try pkgs[0].model.requireID(), "error 1")),
             .success(pkgs[1])
         ]
 
@@ -253,7 +253,7 @@ class IngestorTests: AppTestCase {
         // validate
         do {
             let pkgs = try await Package.query(on: app.db).sort(\.$url).all()
-            XCTAssertEqual(pkgs.map(\.status), [.metadataRequestFailed, .new])
+            XCTAssertEqual(pkgs.map(\.status), [.ingestionFailed, .new])
             XCTAssertEqual(pkgs.map(\.processingStage), [.ingestion, .ingestion])
         }
     }
@@ -308,7 +308,7 @@ class IngestorTests: AppTestCase {
                                                    processingStage: .reconciliation)
         Current.fetchMetadata = { _, owner, repository in
             if owner == "foo" && repository == "2" {
-                throw AppError.metadataRequestFailed(packages[1].id, .badRequest, URI("2"))
+                throw AppError.genericError(packages[1].id, "error 2")
             }
             return .mock(owner: owner, repository: repository)
         }
@@ -326,7 +326,7 @@ class IngestorTests: AppTestCase {
         (try await Package.query(on: app.db).all()).forEach { pkg in
             switch pkg.url {
                 case "https://github.com/foo/2":
-                    XCTAssertEqual(pkg.status, .metadataRequestFailed)
+                    XCTAssertEqual(pkg.status, .ingestionFailed)
                 default:
                     XCTAssertEqual(pkg.status, .new)
             }

Tests/AppTests/Util+ext.swift

@@ -94,12 +94,8 @@ extension App.AppError: Swift.Equatable {
                 return (id1, v1) == (id2, v2)
             case let (.invalidPackageCachePath(id1, v1), .invalidPackageCachePath(id2, v2)):
                 return (id1, v1) == (id2, v2)
-            case let (.invalidPackageUrl(id1, v1), .invalidPackageUrl(id2, v2)):
-                return (id1, v1) == (id2, v2)
             case let (.invalidRevision(id1, v1), .invalidRevision(id2, v2)):
                 return (id1, v1) == (id2, v2)
-            case let (.metadataRequestFailed(id1, s1, u1), .metadataRequestFailed(id2, s2, u2)):
-                return (id1, s1.code, u1.description) == (id2, s2.code, u2.description)
             default:
                 return false
         }