Skip to content

Conversation

@4brunu
Copy link
Contributor

@4brunu 4brunu commented Mar 20, 2025

#20906 introduced two issues related to the parameters encoding.

The first problem is the following.

The default overload is disfavored and this is correct, this overload should be the last option to execute, if no other overload is compatible.

extension QueryStringEncodable {
@_disfavoredOverload
func encodeToQueryString(codableHelper: CodableHelper) -> String { String(describing: self) }
}

Those protocol conformance should execute the previous default implementation, but actually those are codable, so it will execute the overload of the codable.

extension Bool: QueryStringEncodable {}
extension Float: QueryStringEncodable {}
extension Int: QueryStringEncodable {}
extension Int32: QueryStringEncodable {}
extension Int64: QueryStringEncodable {}
extension Double: QueryStringEncodable {}
extension Decimal: QueryStringEncodable {}
extension String: QueryStringEncodable {}
extension URL: QueryStringEncodable {}
extension UUID: QueryStringEncodable {}

extension QueryStringEncodable where Self: Encodable {
func encodeToQueryString(codableHelper: CodableHelper) -> String {
guard let data = try? codableHelper.jsonEncoder.encode(self) else {
fatalError("Could not encode to json: \(self)")
}
return data.encodeToQueryString(codableHelper: codableHelper)
}
}{{/useVapor}}{{#generateModelAdditionalProperties}}

To fix this, I created an implementation to the basic types.

The second problem is that the Any parameter should be any Sendable

{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static func mapValuesToQueryItems(_ source: [String: (wrappedValue: Any?, isExplode: Bool)]) -> [URLQueryItem]? {

Otherwise it will always prefer this overload.

{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} static func mapValuesToQueryItems(_ source: [String: (any Sendable)?]) -> [URLQueryItem]? {

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package || exit
    ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
    ./bin/utils/export_docs_generators.sh || exit
    
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@jgavris (2017/07) @ehyche (2017/08) @Edubits (2017/09) @jaz-ah (2017/09) @4brunu (2019/11) @dydus0x14 (2023/06)

@4brunu
Copy link
Contributor Author

4brunu commented Mar 20, 2025

@x-sheep if you could help to review this PR, since it's related to a previous one you opened, I would appreciate. Thanks 👍

@x-sheep
Copy link
Contributor

x-sheep commented Mar 20, 2025

My bad, I think this is what happens when the unit tests don't work 🙈

I'm not sure we need the Codable override for QueryStringEncodable at all, but otherwise the changes look good to me 👍

@4brunu 4brunu merged commit 22b6787 into OpenAPITools:master Mar 20, 2025
15 of 16 checks passed
@4brunu 4brunu deleted the fix/swift-generator branch March 20, 2025 12:30
@4brunu
Copy link
Contributor Author

4brunu commented Mar 20, 2025

No problem, your PR was a big improvement, that made the Alamofire integration work again.

@4brunu
Copy link
Contributor Author

4brunu commented Mar 20, 2025

@x-sheep I just noticed that Array, Set and Dictionary no longer conform to the protocol, I'm not sure if that's a problem or not. Maybe not because the method is still there?

@4brunu
Copy link
Contributor Author

4brunu commented Mar 20, 2025

Could we return any Sendable instead of a String?

@x-sheep
Copy link
Contributor

x-sheep commented Mar 20, 2025

While you could change it back to any Sendable, this will still not allow Array and Set to conform to the protocol. It's forbidden in Swift to add conditional protocol conformance based on if it's generic subtype is Sendable or not (with the sole exception being conditional Sendable corformance itself)

@4brunu
Copy link
Contributor Author

4brunu commented Mar 20, 2025

Thanks for the help. The thing that is making me uncomfortable is that the tests are flaky, sometimes they pass, sometimes they fail, and I'm not sure if everything it now fixed, because of the big changes that were made.

@wing328 wing328 added this to the 7.13.0 milestone Apr 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants