From 03829084c8b7ac5e4b766f31295e4a7547d2e597 Mon Sep 17 00:00:00 2001 From: Stefan Hausotte Date: Sun, 17 May 2026 21:26:19 +0200 Subject: [PATCH] refactor: small code refactoring --- .../ForgejoKit/Services/ForgejoClient.swift | 16 ++-- .../Services/PullRequestService.swift | 29 ++++--- Tests/ForgejoKitTests/ErrorMappingTests.swift | 87 +++++++++++++++++++ Tests/ForgejoKitTests/NotificationTests.swift | 26 ------ 4 files changed, 116 insertions(+), 42 deletions(-) diff --git a/Sources/ForgejoKit/Services/ForgejoClient.swift b/Sources/ForgejoKit/Services/ForgejoClient.swift index f9fc5a3..d3b345a 100644 --- a/Sources/ForgejoKit/Services/ForgejoClient.swift +++ b/Sources/ForgejoKit/Services/ForgejoClient.swift @@ -456,7 +456,7 @@ public enum AuthenticationError: LocalizedError, Sendable, Equatable { } } -public enum ServiceError: LocalizedError, Sendable { +public enum ServiceError: LocalizedError, Sendable, Equatable { case noActiveInstance case invalidURL case invalidResponse @@ -502,19 +502,25 @@ public enum ServiceError: LocalizedError, Sendable { } } - private static func normalizedHTTPMessage(_ message: String) -> String? { + private struct ForgejoErrorBody: Decodable { + let message: String? + } + + static func normalizedHTTPMessage(_ message: String) -> String? { let trimmed = message.trimmingCharacters(in: .whitespacesAndNewlines) guard !trimmed.isEmpty else { return nil } guard let data = trimmed.data(using: .utf8), - let object = try? JSONSerialization.jsonObject(with: data) as? [String: Any], - let jsonMessage = object["message"] as? String + let body = try? JSONDecoder().decode(ForgejoErrorBody.self, from: data) else { return trimmed } + guard let jsonMessage = body.message else { + return trimmed + } let trimmedJSONMessage = jsonMessage.trimmingCharacters(in: .whitespacesAndNewlines) - return trimmedJSONMessage.isEmpty ? trimmed : trimmedJSONMessage + return trimmedJSONMessage.isEmpty ? nil : trimmedJSONMessage } } diff --git a/Sources/ForgejoKit/Services/PullRequestService.swift b/Sources/ForgejoKit/Services/PullRequestService.swift index 933a477..a7f7242 100644 --- a/Sources/ForgejoKit/Services/PullRequestService.swift +++ b/Sources/ForgejoKit/Services/PullRequestService.swift @@ -173,17 +173,24 @@ public final class PullRequestService: Sendable { do { _ = try await client.performRequestNoContent(url: url, method: "POST", body: jsonData, validateStatus: true) } catch let error as ServiceError { - if case let .httpError(statusCode, message) = error { - switch statusCode { - case 405 where message.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty: - throw ServiceError.notMergeable - case 409 where message.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty: - throw ServiceError.mergeConflict - default: - throw error - } - } - throw error + throw Self.collapseMergeError(error) + } + } + + static func collapseMergeError(_ error: ServiceError) -> ServiceError { + guard case let .httpError(statusCode, message) = error else { + return error + } + guard message.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { + return error + } + switch statusCode { + case 405: + return .notMergeable + case 409: + return .mergeConflict + default: + return error } } diff --git a/Tests/ForgejoKitTests/ErrorMappingTests.swift b/Tests/ForgejoKitTests/ErrorMappingTests.swift index 9af12e4..31e593a 100644 --- a/Tests/ForgejoKitTests/ErrorMappingTests.swift +++ b/Tests/ForgejoKitTests/ErrorMappingTests.swift @@ -45,4 +45,91 @@ struct ErrorMappingTests { #expect(AuthenticationError.certificateError.httpStatusCode == nil) #expect(AuthenticationError.certificateError.httpErrorCategory == nil) } + + // MARK: - ServiceError descriptions + + @Test func serviceErrorDescriptions() { + #expect(ServiceError.noActiveInstance.errorDescription == "No active Forgejo instance") + #expect(ServiceError.invalidURL.errorDescription == "Invalid URL") + #expect(ServiceError.invalidResponse.errorDescription == "Invalid response from server") + #expect(ServiceError.httpError(statusCode: 404).errorDescription == "HTTP error: 404") + #expect(ServiceError.httpError(statusCode: 500).errorDescription == "HTTP error: 500") + #expect(ServiceError.decodingFailed(detail: "missing key").errorDescription == "Decoding failed: missing key") + } + + @Test func httpErrorDescriptionUsesForgejoMessageBody() { + let error = ServiceError.httpError( + statusCode: 405, + message: #"{"message":"This pull request has failing status checks"}"#, + ) + + #expect(error.errorDescription == "HTTP 405: This pull request has failing status checks") + } + + @Test func httpErrorDescriptionFallsBackToRawBody() { + let error = ServiceError.httpError(statusCode: 500, message: "database unavailable") + + #expect(error.errorDescription == "HTTP 500: database unavailable") + } + + @Test func httpErrorDescriptionIgnoresWhitespaceOnlyMessage() { + let error = ServiceError.httpError(statusCode: 503, message: " \n ") + + #expect(error.errorDescription == "HTTP error: 503") + } + + @Test func httpErrorDescriptionFallsBackWhenJSONMessageFieldIsEmpty() { + let error = ServiceError.httpError( + statusCode: 422, + message: #"{"message":" "}"#, + ) + + #expect(error.errorDescription == "HTTP error: 422") + } + + @Test func httpErrorDescriptionReturnsRawJSONWhenMessageFieldMissing() { + let raw = #"{"errors":["bad request"]}"# + let error = ServiceError.httpError(statusCode: 400, message: raw) + + #expect(error.errorDescription == "HTTP 400: \(raw)") + } + + // MARK: - Merge error collapsing + + @Test func collapseMergeErrorMaps405WithEmptyBodyToNotMergeable() { + let collapsed = PullRequestService.collapseMergeError( + .httpError(statusCode: 405, message: ""), + ) + #expect(collapsed == .notMergeable) + } + + @Test func collapseMergeErrorMaps409WithEmptyBodyToMergeConflict() { + let collapsed = PullRequestService.collapseMergeError( + .httpError(statusCode: 409, message: " \n "), + ) + #expect(collapsed == .mergeConflict) + } + + @Test func collapseMergeErrorPreserves405WithBody() { + let original = ServiceError.httpError( + statusCode: 405, + message: #"{"message":"failing status checks"}"#, + ) + #expect(PullRequestService.collapseMergeError(original) == original) + } + + @Test func collapseMergeErrorPreserves409WithBody() { + let original = ServiceError.httpError(statusCode: 409, message: "conflict in README.md") + #expect(PullRequestService.collapseMergeError(original) == original) + } + + @Test func collapseMergeErrorPassesOtherStatusCodesThrough() { + let original = ServiceError.httpError(statusCode: 500, message: "") + #expect(PullRequestService.collapseMergeError(original) == original) + } + + @Test func collapseMergeErrorPassesNonHTTPErrorsThrough() { + #expect(PullRequestService.collapseMergeError(.invalidURL) == .invalidURL) + #expect(PullRequestService.collapseMergeError(.noActiveInstance) == .noActiveInstance) + } } diff --git a/Tests/ForgejoKitTests/NotificationTests.swift b/Tests/ForgejoKitTests/NotificationTests.swift index 24c6919..73afbda 100644 --- a/Tests/ForgejoKitTests/NotificationTests.swift +++ b/Tests/ForgejoKitTests/NotificationTests.swift @@ -64,32 +64,6 @@ struct NotificationTests { #expect(notificationSubjectNumber(from: url) == 15) } - // MARK: - ServiceError descriptions - - @Test func serviceErrorDescriptions() { - #expect(ServiceError.noActiveInstance.errorDescription == "No active Forgejo instance") - #expect(ServiceError.invalidURL.errorDescription == "Invalid URL") - #expect(ServiceError.invalidResponse.errorDescription == "Invalid response from server") - #expect(ServiceError.httpError(statusCode: 404).errorDescription == "HTTP error: 404") - #expect(ServiceError.httpError(statusCode: 500).errorDescription == "HTTP error: 500") - #expect(ServiceError.decodingFailed(detail: "missing key").errorDescription == "Decoding failed: missing key") - } - - @Test func httpErrorDescriptionUsesForgejoMessageBody() { - let error = ServiceError.httpError( - statusCode: 405, - message: #"{"message":"This pull request has failing status checks"}"#, - ) - - #expect(error.errorDescription == "HTTP 405: This pull request has failing status checks") - } - - @Test func httpErrorDescriptionFallsBackToRawBody() { - let error = ServiceError.httpError(statusCode: 500, message: "database unavailable") - - #expect(error.errorDescription == "HTTP 500: database unavailable") - } - // MARK: - Full API response decoding @Test func decodesNotificationArray() throws {