Repository navigation
Conversation
…t failed to encode The alamofire and urlsession request builders called fatalError for an unsupported HTTP method, Content-Type, multipart/octet-stream parameter value or response type, and the alamofire builder force-unwrapped parameters and responses. Fail the request instead with the new RequestBuilderError (status 415, as urlsession already uses for requests that can't be created) or DecodableRequestBuilderError.nilHTTPResponse (-2). JSONEncodingHelper only printed a JSON body encoding error, so the request went out without a body. Hand the error to the request builder, which fails the request with RequestBuilderError.bodyEncodingFailed. Update swift6 samples.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 43 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="samples/client/petstore/swift6/alamofireLibrary/Sources/PetstoreClient/Infrastructure/AlamofireImplementations.swift">
<violation number="1" location="samples/client/petstore/swift6/alamofireLibrary/Sources/PetstoreClient/Infrastructure/AlamofireImplementations.swift:205">
P2: This response-type check runs only after `makeRequest` starts the Alamofire request, so `cancel()` can race and send a mutating request before reporting 415. Reject unsupported `T` before constructing the request.</violation>
</file>
<file name="samples/client/petstore/swift6/default/Sources/PetstoreClient/Infrastructure/URLSessionImplementations.swift">
<violation number="1" location="samples/client/petstore/swift6/default/Sources/PetstoreClient/Infrastructure/URLSessionImplementations.swift:282">
P3: For `unsupportedResponseType` the URLSession builder reports the server's actual status code (usually 200), while the Alamofire builder reports a hardcoded 415. A 200 alongside a failure result can mislead callers that gate on `response.statusCode`; align both libraries on one convention (415, matching the other construction failures), or at least document why they differ.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| }) | ||
| default: | ||
| fatalError("Unsupported Response Body Type - \(String(describing: T.self))") | ||
| request.cancel() |
There was a problem hiding this comment.
P2: This response-type check runs only after makeRequest starts the Alamofire request, so cancel() can race and send a mutating request before reporting 415. Reject unsupported T before constructing the request.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At samples/client/petstore/swift6/alamofireLibrary/Sources/PetstoreClient/Infrastructure/AlamofireImplementations.swift, line 205:
<comment>This response-type check runs only after `makeRequest` starts the Alamofire request, so `cancel()` can race and send a mutating request before reporting 415. Reject unsupported `T` before constructing the request.</comment>
<file context>
@@ -183,15 +191,46 @@ open class AlamofireRequestBuilder<T: Sendable>: RequestBuilder<T>, @unchecked S
})
default:
- fatalError("Unsupported Response Body Type - \(String(describing: T.self))")
+ request.cancel()
+ cleanupRequest()
+ let error = RequestBuilderError.unsupportedResponseType(String(describing: T.self))
</file context>
| fatalError("Unsupported Response Body Type - \(String(describing: T.self))") | ||
| let error = RequestBuilderError.unsupportedResponseType(String(describing: T.self)) | ||
| apiConfiguration.interceptor.didComplete(urlRequest: urlRequest, urlSession: urlSession, requestBuilder: self, data: data, response: httpResponse, result: .failure(error)) | ||
| completion(.failure(ErrorResponse.error(httpResponse.statusCode, data, httpResponse, error))) |
There was a problem hiding this comment.
P3: For unsupportedResponseType the URLSession builder reports the server's actual status code (usually 200), while the Alamofire builder reports a hardcoded 415. A 200 alongside a failure result can mislead callers that gate on response.statusCode; align both libraries on one convention (415, matching the other construction failures), or at least document why they differ.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At samples/client/petstore/swift6/default/Sources/PetstoreClient/Infrastructure/URLSessionImplementations.swift, line 282:
<comment>For `unsupportedResponseType` the URLSession builder reports the server's actual status code (usually 200), while the Alamofire builder reports a hardcoded 415. A 200 alongside a failure result can mislead callers that gate on `response.statusCode`; align both libraries on one convention (415, matching the other construction failures), or at least document why they differ.</comment>
<file context>
@@ -267,7 +277,9 @@ open class URLSessionRequestBuilder<T: Sendable>: RequestBuilder<T>, @unchecked
- fatalError("Unsupported Response Body Type - \(String(describing: T.self))")
+ let error = RequestBuilderError.unsupportedResponseType(String(describing: T.self))
+ apiConfiguration.interceptor.didComplete(urlRequest: urlRequest, urlSession: urlSession, requestBuilder: self, data: data, response: httpResponse, result: .failure(error))
+ completion(.failure(ErrorResponse.error(httpResponse.statusCode, data, httpResponse, error)))
}
</file context>
Fail with nilHTTPResponse before writing the file, so a download without an HTTP response no longer leaves an unreturned file in the caches directory.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
A download without an HTTP response now reports nilHTTPResponse (-2) instead of responseDataMissing (400).
|
|
||
| default: | ||
| fatalError("Unsupported HTTPMethod - \(xMethod.rawValue)") | ||
| return nil |
There was a problem hiding this comment.
@spigo shouldn't we do something similar do the URLSessionImplementations instead of returning nil?
return failBeforeSending(RequestBuilderError.unsupportedHTTPMethod(method), urlSession: urlSession, completion: completion)
There was a problem hiding this comment.
createURLRequest() is a separate open helper that returns URLRequest? and has no completion to report to. It already signals failure with nil (the try? URLRequest(...) right below). execute() doesn't use it: on the real request path an unsupported method already fails through failBeforeSending(.unsupportedHTTPMethod), as in URLSessionImplementations. I kept nil because making it throws would change the signature of an open method and break subclasses that override it. Happy to switch it to throws if you prefer.
| mpForm.append(uuid.uuidString.data(using: String.Encoding.utf8)!, withName: k) | ||
| default: | ||
| fatalError("Unprocessable value \(v) with key \(k)") | ||
| break |
There was a problem hiding this comment.
@spigo here we are breaking but this if a silent fail, shouldn't we do the same thing as in URLSessionImplementations?
throw RequestBuilderError.unsupportedParameterValue(key: key)
There was a problem hiding this comment.
You're right. In 59126b6 the form is built up front with MultipartFormData() instead of inside Alamofire's non-throwing closure, so this default case can now fail the request directly, the same way URLSessionImplementations throws:
default:
return failBeforeSending(.unsupportedParameterValue(key: k), managerId: managerId, completion: completion)The request fails with RequestBuilderError.unsupportedParameterValue and nothing is sent. The separate pre-check before the upload is gone too.
…and-body-encoding
…ng the form Build the MultipartFormData up front instead of in Alamofire's non-throwing closure, so an unsupported value fails the request right where it is found (RequestBuilderError.unsupportedParameterValue), as the urlsession library's encoder does, instead of being skipped by a default case. This replaces the separate pre-check.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/resources/swift6/libraries/alamofire/AlamofireImplementations.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/swift6/libraries/alamofire/AlamofireImplementations.mustache:125">
P2: These rewritten multipart parameter lines still force-unwrap `data(using: .utf8)!`, which can crash the request instead of failing it. `String.data(using: .utf8)` returns nil when the string can't be represented in UTF-8 (e.g., `"\u{D800}"` — pathological but user-reachable), and `NSNumber.stringValue.data(...)!` / `uuid.uuidString.data(...)!` on the next cases follow the same pattern. This is exactly the crash class the PR removes elsewhere in this builder, so the String case (the only realistically-nil one) should fail with `RequestBuilderError.unsupportedParameterValue` like the `default:` case does.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| case let string as String: | ||
| mpForm.append(string.data(using: String.Encoding.utf8)!, withName: k) |
There was a problem hiding this comment.
P2: These rewritten multipart parameter lines still force-unwrap data(using: .utf8)!, which can crash the request instead of failing it. String.data(using: .utf8) returns nil when the string can't be represented in UTF-8 (e.g., "\u{D800}" — pathological but user-reachable), and NSNumber.stringValue.data(...)! / uuid.uuidString.data(...)! on the next cases follow the same pattern. This is exactly the crash class the PR removes elsewhere in this builder, so the String case (the only realistically-nil one) should fail with RequestBuilderError.unsupportedParameterValue like the default: case does.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At modules/openapi-generator/src/main/resources/swift6/libraries/alamofire/AlamofireImplementations.mustache, line 125:
<comment>These rewritten multipart parameter lines still force-unwrap `data(using: .utf8)!`, which can crash the request instead of failing it. `String.data(using: .utf8)` returns nil when the string can't be represented in UTF-8 (e.g., `"\u{D800}"` — pathological but user-reachable), and `NSNumber.stringValue.data(...)!` / `uuid.uuidString.data(...)!` on the next cases follow the same pattern. This is exactly the crash class the PR removes elsewhere in this builder, so the String case (the only realistically-nil one) should fail with `RequestBuilderError.unsupportedParameterValue` like the `default:` case does.</comment>
<file context>
@@ -112,34 +112,31 @@ fileprivate class AlamofireRequestBuilderConfiguration: @unchecked Sendable {
+ } else {
+ mpForm.append(fileURL, withName: k)
}
+ case let string as String:
+ mpForm.append(string.data(using: String.Encoding.utf8)!, withName: k)
+ case let number as NSNumber:
</file context>
| case let string as String: | |
| mpForm.append(string.data(using: String.Encoding.utf8)!, withName: k) | |
| case let string as String: | |
| guard let stringData = string.data(using: .utf8) else { | |
| return failBeforeSending(.unsupportedParameterValue(key: k), managerId: managerId, completion: completion) | |
| } | |
| mpForm.append(stringData, withName: k) |
This branch has not been deployed
2 participants
Fixes #25183.
RequestBuilderError(next toDecodableRequestBuilderErrorin Models):bodyEncodingFailed,unsupportedHTTPMethod,unsupportedMediaType,unsupportedParameterValue(key:),unsupportedResponseType.fatalErrors become a failed request. Request-creation failures go through the existing path (interceptordidComplete+ErrorResponse.error(415, ...)), now shared by afailBeforeSendinghelper; the encoders throw instead of crashing.response!becomesDecodableRequestBuilderError.nilHTTPResponse(-2, as the decodable branch already does).parameters!becomesparameters ?? [:], anddataResponse.data as! Tgets a fallback.RequestBuilderError.bodyEncodingFailedto the builder via the parameters (its signature can't throw without changing every generated API method). Both builders then fail the request instead of sending it without a body.Verified against the generated default (urlsession) sample: unsupported media type →
415 unsupportedMediaType("text/plain"), aDatemultipart value →415 unsupportedParameterValue(key: "when"), aDouble.nanbody →415 bodyEncodingFailed(EncodingError.invalidValue ...). The same changes have been running in a downstream alamofire client with tests. The default, alamofireLibrary, urlsessionLibrary and asyncAwaitLibrary samples build.PR checklist
Summary by cubic
Fixes #25183. Replaces
fatalErrorcrashes in the Swift 6 request builders with failed requests (RequestBuilderError, status 415), and stops sending requests whose JSON body failed to encode.nilHTTPResponsewith -2,parameters ?? [:]).nilHTTPResponsebefore writing the file, so a download without a response leaves no unreturned file in the caches directory.JSONEncodingHelperhands a body encoding error to the builder, so the request fails instead of going out without a body.Written for commit 59126b6. Summary will update on new commits.