Skip to content
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

[BUG] Compilation error in swift-combine generation with query param named url or path #18940

Closed
5 of 6 tasks
andyland opened this issue Jun 16, 2024 · 1 comment · Fixed by #18969
Closed
5 of 6 tasks

Comments

@andyland
Copy link
Contributor

andyland commented Jun 16, 2024

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

APIs that have a parameter value of url or path generate the error Cannot convert value of type 'URL' to expected argument type 'String?' when generating a swift-combine client. This is due to duplicate local variables with the name of url and path

openapi-generator version

7.6.0

OpenAPI declaration file content or url
    "/path": {
      "get": {
        "operationId": "Service_Method",
        "parameters": [
          {
            "type": "string",
            "name": "url",
            "in": "query"
          },
          {
            "type": "string",
            "name": "path",
            "in": "query"
          },
        ],
        "responses": {
          "200": {
            "description": "A successful response.",
            "schema": {
              "$ref": "#/definitions/response"
            }
          },
          "default": {
            "description": "An unexpected error response.",
            "schema": {
              "$ref": "#/definitions/rpcStatus"
            }
          }
        }
      }
    }
Generation Details
    /// - GET /path
    /// - parameter url: (query)  (optional)
    /// - returns: AnyPublisher<Response, Error> 
    open func serviceMethod(url: String? = nil, path: String? = nil) -> AnyPublisher<Response, Error> {
        Deferred {
            Result<URLRequest, Error> {
                guard let baseURL = self.transport.baseURL ?? self.baseURL else {
                    throw OpenAPITransportError.badURLError()
                }
                let path = "/path"
                let url = baseURL.appendingPathComponent(path)
                var components = URLComponents(url: url, resolvingAgainstBaseURL: false)
                var queryItems: [URLQueryItem] = []
                if let url = url { queryItems.append(URLQueryItem(name: "url", value: url)) } // Error is here, duplicate local variable
                components?.queryItems = queryItems
                guard let requestURL = components?.url else {
                    throw OpenAPITransportError.badURLError()
                }
                var request = URLRequest(url: requestURL)
                request.httpMethod = "GET"
                return request
            }.publisher
        }.flatMap { request -> AnyPublisher<Response, Error> in 
            return self.transport.send(request: request)
                .tryMap { response in
                    try self.decoder.decode(Response.self, from: response.data)
                }
                .eraseToAnyPublisher()
        }.eraseToAnyPublisher()
    }
Steps to reproduce
  1. Create a request with url or path as a parameter
  2. Generate a swift-combine client
  3. Use that client in an XCode project
Related issues/PRs

None found

Suggest a fix

So I have two proposals for a fix, either will work for me, and I can create the PR myself

  1. Rename the variable here to localVarURL. This seems to have some precedent in other clients to prefix localVar on local variables to reduce the likelihood of a collision. This however does introduce the possibility of regressing this for someone who has a parameter named localVarURL or localVarPath
- let path = "/path"
- let url = baseURL.appendingPathComponent(path)
- {{#hasQueryParams}}var{{/hasQueryParams}}{{^hasQueryParams}}let{{/hasQueryParams}} components = URLComponents(url: url, resolvingAgainstBaseURL: false)
+ let localVarPath = "/path"
+ let localVarURL = baseURL.appendingPathComponent(localVarPath)
+ {{#hasQueryParams}}var{{/hasQueryParams}}{{^hasQueryParams}}let{{/hasQueryParams}} components = URLComponents(url: localVarURL, resolvingAgainstBaseURL: false)
  1. Eliminate the local variables by inlining it into the values below
- let path = "/path"
- let url = baseURL.appendingPathComponent(path)
- {{#hasQueryParams}}var{{/hasQueryParams}}{{^hasQueryParams}}let{{/hasQueryParams}} components = URLComponents(url: url, resolvingAgainstBaseURL: false)
+ {{#hasQueryParams}}var{{/hasQueryParams}}{{^hasQueryParams}}let{{/hasQueryParams}} components = URLComponents(url: baseURL.appendingPathComponent("/path"), resolvingAgainstBaseURL: false)
@andyland andyland changed the title [BUG] Combination error in swift-combine generation with parameter named url [BUG] Combination error in swift-combine generation with query param named url Jun 16, 2024
@andyland andyland changed the title [BUG] Combination error in swift-combine generation with query param named url [BUG] Compilation error in swift-combine generation with query param named url Jun 16, 2024
@andyland andyland changed the title [BUG] Compilation error in swift-combine generation with query param named url [BUG] Compilation error in swift-combine generation with query param named url or path Jun 16, 2024
@wing328
Copy link
Member

wing328 commented Jun 17, 2024

we prefer 1 as that's what we've doing in other clients

please open a PR when you've time

thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants