Tagging @4brunu

Honestly I would like to simplify things, because there are old things in the swift generator that doesn't make sense or are not needed anymore, but I'm a bit afraid of breaking things, so we should be careful with that changes.

This was introduced here
#11202
I'm not sure what are the implications of doing what you suggest.
Maybe this is a good starting point to investigate if this change can break other things.

Another thing that we should do is rename useJsonEncodable to useQueryString.
#12664
But maybe we should keep the old useJsonEncodable as deprecated to simplify the transition.

Also, inspired by this issue, I already started so clean up of old things and some fixes.
#21117
#21116

In the current Swift6 generator (and Swift5) with version 7.13.0-snapshot a non-standard class that conforms to Encodable is serialized by first converting it to JSON, and then base64-encoding the result.

Is this being used at all? It would seem much nicer to me if this was done explicitly by a user, and leave only the serialization of Data (byte arrays) to Base64 as a standard. Right now, this behavior is implicit whenever an object is passed into a request that uses it as a query parameter.

I agree that this doesn't look correct.

This change was introduced to fix this issue.
#8925

If we remove this code

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)
    }
}

Then I think the models will use the default implementation

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

which is also not correct.

But sending a model for example a user in a query parameter also feels wrong.

Honestly I'm a bit lost in this.

@x-sheep What do you think of this?

@0x0c could you help with this?

Thanks

I don't think QueryStringEncodable (or in fact the old JSONEncodable) should ever be used for models... Right now the generator will break up a model class into a function with multiple parameters, if the function expects a Query string or a Multipart body. So it will generate a class for a model, but not use it. (unless there's a different call that expects that model in a regular JSON body).

The whole idea of serializing a model to JSON, then converting it to Base64, is not something I can find in any OpenAPI specification. Nor do I expect an API to actually want such a request. So I would be in favor of removing QueryStringEncodable for models, and only keeping it for enums, primitive values and the Foundation Data struct (which can keep the base64 behavior).

If this does break behavior for users (which my gut tells me is unlikely) it's still possible to use JSONEncoder yourself and then pass in the Data value to the generated function.

@x-sheep looks good to me. Could you please create a PR?

Hi, I found another bug related to QueryStringEncodable introduction in this PR #20906.
The QueryStringEncodable always returns a String, but this is a problem, because here

we check for the type of parameter and encode it according to the type.

But with QueryStringEncodable since we always return a String, we will always encode everything as a string.

This created s bug that instead of uploading a file, it was uploading the path of the file.

Instead of returning a String I think we should return any Sendable.

And since the QueryStringEncodable doesn't return a String anymore, maybe we should call it QueryEncodable?

@4brunu You're right. I think it's a better idea to make an enum called FormData, and give it several cases:

1. A String
2. A Data block
3. A URL to a local file

QueryStringEncodable (or whatever it'll be called) can then return one of these, and we can avoid casting to specific values.

That would require a lot of changes in the generator.
I will try to release a quick fix, so that we have generator stable again, and then we can try to improve this.

Sounds good to me.

@4brunu Could you reopen this issue? The issue in the first post isn't actually solved by #21142