Refactor REST makeCall logic

[tkausch]

User description

Refactored makeCall into smaller helpers so MIME validation, success decoding, and error construction each live in focused private methods. The main flow now early-exits for bodyless responses, validates the content type, decodes success payloads, or builds structured errors.

---

PR Type

Enhancement

---

Description

• Refactored makeCall into focused private helper methods

• Extracted shouldBypassDeserialization for early-exit logic

• Extracted validatedMimeType for MIME type validation

• Extracted decodeSuccessfulResponse for success payload deserialization

• Extracted buildErrorResponse for structured error construction

• Improved code readability and testability with clearer separation of concerns

---

Diagram Walkthrough

flowchart LR
  makeCall["makeCall<br/>Main orchestrator"] --> bypass["shouldBypassDeserialization<br/>Early-exit check"]
  makeCall --> validate["validatedMimeType<br/>MIME validation"]
  makeCall --> decode["decodeSuccessfulResponse<br/>Success deserialization"]
  makeCall --> error["buildErrorResponse<br/>Error construction"]
  bypass --> result1["Return nil response"]
  decode --> result2["Return deserialized data"]
  error --> result3["Throw RestError"]

Loading

File Walkthrough

Relevant files

Enhancement

RestApiCaller.swift Extract makeCall logic into focused helper methods              Sources/SwiftRestRequests/RestApiCaller.swift Refactored makeCall method by extracting four private helper methods shouldBypassDeserialization checks if deserialization should be skipped validatedMimeType validates and returns the MIME type from response headers decodeSuccessfulResponse handles successful response deserialization with error handling buildErrorResponse constructs structured error responses from error data Simplified main flow with early exits and clearer success/error branching +36/-39

---

[qodo-code-review]

PR Compliance Guide 🔍

(Compliance updated until commit 511aa16)

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No auditing: The new helper methods for REST calling do not add any audit logging for critical actions or outcomes, and the diff does not show user/timestamp/action context being recorded. Referred Code _ = try validatedMimeType(from: httpResponse) if httpStatus.type == .success { let transformedResponse = try decodeSuccessfulResponse(data: data, response: httpResponse, deserializer: responseDeserializer) return (transformedResponse, httpStatus) } throw try buildErrorResponse(data: data, response: httpResponse, status: httpStatus) } private func shouldBypassDeserialization<T: Deserializer>(_ deserializer: T, status: HTTPStatusCode) -> Bool { (deserializer is VoidDeserializer) || status == .noContent } private func validatedMimeType(from response: HTTPURLResponse) throws -> MimeType { let contentType = response.value(forHTTPHeaderField: HTTPHeaderKeys.ContentType.rawValue) let firstContentMimeType = contentType?.components(separatedBy: ";").first guard let firstContentMimeType, let mimeType = MimeType(rawValue: firstContentMimeType) else { throw RestError.invalidMimeType(contentType) ... (clipped 23 lines)
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Error detail exposure: The code propagates detailed error payloads via RestError which may be surfaced to callers; it is unclear whether these are user-facing or sanitized, requiring verification. Referred Code private func buildErrorResponse(data: Data, response: HTTPURLResponse, status: HTTPStatusCode) throws -> RestError { do { let errorPayload = try errorDeserializer?.deserialize(data) return RestError.failedRestCall(response, status, error: errorPayload) } catch { throw RestError.malformedResponse(response, data, error) }
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: No logging context: The refactor adds no structured logging around request/response handling, making it unclear whether sensitive data is logged elsewhere or whether logs are structured. Referred Code // For requests without deserialization and no error just return the status if shouldBypassDeserialization(responseDeserializer, status: httpStatus), httpStatus.type == .success { return (nil, httpStatus) } guard !data.isEmpty else { throw RestError.failedRestCall(httpResponse, httpStatus, error: nil) } // Postcondition: We have a response object or error that needs to be parsed! _ = try validatedMimeType(from: httpResponse) if httpStatus.type == .success { let transformedResponse = try decodeSuccessfulResponse(data: data, response: httpResponse, deserializer: responseDeserializer) return (transformedResponse, httpStatus) } throw try buildErrorResponse(data: data, response: httpResponse, status: httpStatus) } ... (clipped 35 lines)

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

---

Previous compliance checks

Compliance check up to commit 511aa16

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No auditing: The new logic adds/deserializes responses and builds errors without emitting any audit logs, but it is unclear whether auditing is handled elsewhere in the call stack. Referred Code // For requests without deserialization and no error just return the status if shouldBypassDeserialization(responseDeserializer, status: httpStatus), httpStatus.type == .success { return (nil, httpStatus) } guard !data.isEmpty else { throw RestError.failedRestCall(httpResponse, httpStatus, error: nil) } // Postcondition: We have a response object or error that needs to be parsed! _ = try validatedMimeType(from: httpResponse) if httpStatus.type == .success { let transformedResponse = try decodeSuccessfulResponse(data: data, response: httpResponse, deserializer: responseDeserializer) return (transformedResponse, httpStatus) } throw try buildErrorResponse(data: data, response: httpResponse, status: httpStatus) } ... (clipped 35 lines)
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Error exposure: The code forwards deserialized error payloads via RestError which might be user-facing, but visibility of these messages is not shown in the diff. Referred Code private func buildErrorResponse(data: Data, response: HTTPURLResponse, status: HTTPStatusCode) throws -> RestError { do { let errorPayload = try errorDeserializer?.deserialize(data) return RestError.failedRestCall(response, status, error: errorPayload) } catch { throw RestError.malformedResponse(response, data, error) }

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Consider a dedicated response handler To further improve separation of concerns, encapsulate the new response-handling helper methods within a dedicated ResponseHandler struct or class, leaving RestApiCaller to manage only the network request. Examples: Sources/SwiftRestRequests/RestApiCaller.swift [303-336] private func shouldBypassDeserialization<T: Deserializer>(_ deserializer: T, status: HTTPStatusCode) -> Bool { (deserializer is VoidDeserializer) || status == .noContent } private func validatedMimeType(from response: HTTPURLResponse) throws -> MimeType { let contentType = response.value(forHTTPHeaderField: HTTPHeaderKeys.ContentType.rawValue) let firstContentMimeType = contentType?.components(separatedBy: ";").first guard let firstContentMimeType, let mimeType = MimeType(rawValue: firstContentMimeType) else { ... (clipped 24 lines) Solution Walkthrough: Before: class RestApiCaller { func makeCall(...) throws -> (ResponseType?, HTTPStatusCode) { let (data, httpResponse) = try await dataTask(...) if shouldBypassDeserialization(...) { return (nil, httpStatus) } _ = try validatedMimeType(from: httpResponse) if httpStatus.type == .success { return try decodeSuccessfulResponse(...) } throw try buildErrorResponse(...) } private func shouldBypassDeserialization(...) -> Bool { ... } private func validatedMimeType(...) throws -> MimeType { ... } private func decodeSuccessfulResponse(...) throws -> ResponseType? { ... } private func buildErrorResponse(...) throws -> RestError { ... } } After: struct ResponseHandler { func handleResponse(data: Data, response: HTTPURLResponse, ...) throws -> (ResponseType?, HTTPStatusCode) { if shouldBypassDeserialization(...) { ... } _ = try validatedMimeType(...) if response.status.type == .success { return try decodeSuccessfulResponse(...) } throw try buildErrorResponse(...) } // private helper methods are now part of this struct } class RestApiCaller { private let responseHandler = ResponseHandler() func makeCall(...) throws -> (ResponseType?, HTTPStatusCode) { let (data, httpResponse) = try await dataTask(...) return try responseHandler.handleResponse(data: data, response: httpResponse, ...) } } Suggestion importance[1-10]: 7 __ Why: This is a strong architectural suggestion that logically extends the PR's goal of separating concerns, proposing to move the new helper methods into a dedicated ResponseHandler type.	Medium
Possible issue	Deserialize body for all success statuses Modify decodeSuccessfulResponse to attempt deserialization for any successful 2xx status code, not just for 200 OK, to correctly process response bodies from statuses like 201 Created. Sources/SwiftRestRequests/RestApiCaller.swift [318-327] private func decodeSuccessfulResponse<T: Deserializer>(data: Data, response: HTTPURLResponse, deserializer: T) throws -> T.ResponseType? { - if response.status == .ok { - do { - return try deserializer.deserialize(data) - } catch { - throw RestError.malformedResponse(response, data, error) - } + // Any 2xx status code can have a response body that needs to be deserialized. + // If no body is expected, the caller should use VoidDeserializer. + do { + return try deserializer.deserialize(data) + } catch { + throw RestError.malformedResponse(response, data, error) } - return nil } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a functional limitation where response bodies for successful statuses other than 200 OK are ignored, which is a valid issue to address for more robust API handling.	Medium
No more code suggestions