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

cpp-qt5-client: remove host since it is not well handled #4429

Merged
merged 5 commits into from Nov 12, 2019

Conversation

MartinDelille
Copy link
Contributor

This PR replace part of the job done here: #3688

Now that the base path is well handled, the API class can use the one provided in the API description.

There was previously a mismatch between host and base path as mentionned here: #3399

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@ravinikam @stkrwork @etherealjoy @muttleyxd

@MartinDelille MartinDelille mentioned this pull request Nov 9, 2019
4 tasks
@etherealjoy
Copy link
Contributor

etherealjoy commented Nov 10, 2019

@MartinDelille
May I propose a different approach to solving this?

  1. Leave the base path in the API as it is.
    fullPath.append(this->host).append(this->basePath).append("{{{path}}}");

There are many use cases to have base path as is. e.g.

  • when same client is using multiple hosts at runtime.
  • When using REST over an existing socket. (By subclassing QNetworkManager)
  1. Properly parse the host and update/add constructor if necessary?

@MartinDelille
Copy link
Contributor Author

You're right there is indeed a problem in this code I wrote this summer.

The problem I raised here and that I'm trying to solve is that http://api.example.com is not a host but its not a base path neither.

I need to dig into this

@etherealjoy
Copy link
Contributor

etherealjoy commented Nov 10, 2019

I tried something quick on my side to add parsing of scheme

in AbstractCppCodegen.java add the parsing of the scheme like below

    @Override
    public void preprocessOpenAPI(OpenAPI openAPI) {
        URL url = URLPathUtils.getServerURL(openAPI, serverVariableOverrides());
        String port = URLPathUtils.getPort(url, "");
        String host = url.getHost();
        String scheme = url.getProtocol();
        
        if(!port.isEmpty()) {
            this.additionalProperties.put("serverPort", port);
        }
        if(!host.isEmpty()) {
            this.additionalProperties.put("serverHost", host);
        }
        if(!scheme.isEmpty()) {
            this.additionalProperties.put("scheme", scheme);
        }
    }

Then you can remove all these lines from the three header files

private:
    const QString PetStoreHost = QStringLiteral("http://petstore.swagger.io");

And from all three source files

    api.setHost(PetStoreHost);

Generated constructor looks like this

PFXPetApi::PFXPetApi() : basePath("/v2"),
    host("http://petstore.swagger.io"),
    timeout(0){

I ran the test and it passed with these changes.

@MartinDelille
Copy link
Contributor Author

The only thing is that as I said above, http://api.example.com is more a base url that a host (the host is api.example.com), so maybe we should rename host to baseUrl in the api class?

@etherealjoy
Copy link
Contributor

so maybe we should rename host to baseUrl in the api class?

Ok for me

@MartinDelille
Copy link
Contributor Author

Ok I finally change to the following implementation.

I had to rebase #4435 so maybe we should merge it first if everything is fine with it.

@etherealjoy etherealjoy merged commit 21a291f into OpenAPITools:master Nov 12, 2019
@etherealjoy etherealjoy added this to the 4.2.1 milestone Nov 12, 2019
@MartinDelille MartinDelille deleted the remove-host branch November 12, 2019 10:19
jimschubert added a commit that referenced this pull request Nov 13, 2019
* master:
  [csharp-netcore] Propagate raw content to the ApiException error content. (#4381)
  Remove path relative to my home dir. (#4470)
  Avoid generating uncompilable response body in Spring's API template (#2903)
  cpp-qt5-client: remove host since it is not well handled (#4429)
  [Java] Refactor webClient generator to use URI templates to capture correct metrics (#4314)
  [Java]: Client resttemplate and webclient: array parameters of type integer in path badly generated (#4379)
  Dart generate keywords (#4449)
  [core] Fix system properties being immutable (#4447)
  [Kotlin][Client] minor improvements (#4419)
  [typescript-rxjs]: Add support for nullable (#4438)
  [asciidoc] fix names of parameters (#4440)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants