Skip to content

[swift][client] make QueryStringEncodable return any Sendable #21142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

4brunu
Copy link
Contributor

@4brunu 4brunu commented Apr 24, 2025

Currently returns a string, but this breaks the FormDataEncoding that is expecting each different type and not all types converted to strings.

This for example break the file upload, because instead of sending the file content to the server, it sends the local file path, because the URL was converted to a String.

This PR fixes #20997 (comment), because it changes the return type from String to any Sendable.

With this change, the QueryStringEncodable name seems a bit off, because:

  • it doesn't return a String any more, it returns any Sendable
  • it encodes more than just query parameters, it also encodes form parameters and header parameters
    Maybe we should rename QueryStringEncodable to ParameterEncodable.
    EDIT: Looks like ParameterEncoding already exists which is similar to ParameterEncodable and can create some confusion. Maybe ParameterValueEncodable?
    @x-sheep what do you think of this?

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)

@x-sheep
Copy link
Contributor

x-sheep commented Apr 25, 2025

This looks like a good fix for the time being, but I do have some ideas for larger changes I want to implement:

  1. Removing the conformance on all model objects, along with the jsonEncodable config option. I'll create a new PR that only contains this change.
  2. The project contains multiple places where this interface should be used, but instead has its own completely separate serialization method. This includes custom cookies and headers.

So while I agree that the name QueryStringEncodable isn't accurate, I think we can keep it as-is until we have these larger changes. At any rate, JSON is never involved so it's still a better name than the old JSONEncodable.

@4brunu
Copy link
Contributor Author

4brunu commented Apr 25, 2025

Thanks, I would appreciate if you can create smaller PRs, because it's easier to review and I really want to prioritise stabilize the Swift Generator, so I would like not to merge big changes now.

1 . Removing the conformance on all model objects, along with the jsonEncodable config option. I'll create a new PR that only contains this change.

I agreed with that change, but yesterday I noticed that the QueryStringEncodable isn't just used for query parameters, but also for form parameters and header parameters.
Does it make sense to send a modelObject in a form parameter?

2 . The project contains multiple places where this interface should be used, but instead has its own completely separate serialization method. This includes custom cookies and headers.

JSONEncodable was not the best name, we have two separated processes here.
The JSON encoding is done by the class JSONEncodingHelper.
The QueryStringEncodable is not really any encodable at all, that's why I think we need better naming for this, to avoid confusion.
This is just a type eraser on parameters, to put all the values in a dictionary to later be encoded.
Maybe a better name would be ParameterConvertible or ParameterValueConvertible or ParameterValueEraser.

I honestly think that we really need to change the naming to avoid this type of confusion.
I think I like ParameterConvertible what do you think?

@x-sheep
Copy link
Contributor

x-sheep commented Apr 25, 2025

Does it make sense to send a modelObject in a form parameter?

The OpenAPI spec allows it, but currently no version of the generator does this correctly (i.e. in a compliant manner). The current dictionary of [String: any Sendable] is generated according to the top-level request model, but nested models are currently not supported. It should actually add a new key-value pair to the dictionary for each parameter of each nested model, but instead sends the objects as Base64-encoded JSON.

My idea is to add a new implementation of Swift's Encoder protocol that can handle any Encodable item and creates a flat list of Parameter objects (i.e. only String, Data and URL for files). This could also encode objects or arrays in a different manner if the style or explode is set in the OpenAPI schema. See https://swagger.io/specification/v3/#parameter-object. I'll create a new Issue for this at a later time.

This would definitely not be required for the swift6 generator to go stable, but could be a later major change (possibly using a config flag).

I think I like ParameterConvertible what do you think?

Sounds like a good way forward to me.

@4brunu
Copy link
Contributor Author

4brunu commented Apr 25, 2025

In this commit I renamed a few things:

  • QueryStringEncodable to ParameterConvertible
  • useJsonEncodable to useParameterConvertible
  • func encodeToQueryString(codableHelper: CodableHelper) -> any Sendable to func asParameter(codableHelper: CodableHelper) -> any Sendable

@4brunu
Copy link
Contributor Author

4brunu commented Apr 25, 2025

Failure doesn't seem to be related to this PR.

@4brunu 4brunu merged commit f950ac9 into OpenAPITools:master Apr 25, 2025
15 of 16 checks passed
@4brunu 4brunu deleted the feature/swift-fix-query-string-encodable branch April 25, 2025 13:41
@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.

[Question] [Swift6] All Codable classes into querystring
3 participants