[PHP] Handle case where resourcePath already contains a querystring

[mikespub]

When the resourcePath of the original YAML already contains a querystring (with some fixed or variable "path" parameters), the generated PHP code fails.

Issue 1. The generated classes all have & instead of & in the $resourcePath, which gets passed as is to ApiClient->callApi and curl. I'm not sure where in the code generation process this url encoding happens.

Issue 2. If the call contains additional queryParams, either directly or by using a "query" apiKey, ApiClient->callApi simply adds them after a ?, without checking if there is already a ? in the url.

Both cause the generated API calls to send invalid URLs and fail, and are easy to fix. Thanks...

P.S.: don't blame me for the messy APIs, I'm just a consumer :-)

[mikespub]

Err, that's & with an amp; for issue 1

[wing328]

@mikespub do you mind sharing the spec with us? Is there a hard-coded query parameter in most or all endpoints?

[mikespub]

You'll find 3 yaml files in https://github.com/mikespub/synology/tree/master/tools

Most of the operations have 3 hard-coded query parameters (api, version, method), and some extra variable parameters (e.g. _sid, or account & passwd, ...). The SYNO.API ones have been tweaked by hand, the rest were generated from API documentation.

As a good example, have a look at the /AudioStation/song.cgi path. It supports 6 different api methods via the same path: [list, search, getinfo, setrating, setsharing, getsharing]

The _query version only specifies the actual path and puts all default parameters as query. The supported api methods for that path are listed as enum. This allows you to test it out via Swagger Editor or Swagger UI, but it doesn't allow you to generate very useful client code with Swagger Codegen because: a. all api methods for that path end up in a single method, and b. the codegen doesn't take into account the default values (PHP/Python)

The _path version specifies the path with the querystring, both the hard-coded ones and the variable ones. This also works with Swagger Editor and Swagger UI, and Swagger Codegen will create a separate method for each of the supported api methods, which is what we want here. The only remaining clean-up is the _sid parameter showing up in every generated method.

The _path_sec version is the one closest to how it should work, i.e. using the _sid parameter as security apiKey and the rest hard-coded. Unfortunately both Swagger Editor and Swagger UI have the same stupid approach of adding query params after a '?', regardless of whether the path already contains one, so this one can't be tested with them out of the box. However, the generated code is closest to what you might actually want to use - if those issues above were fixed :-)

[wing328]

According to spec, it does not say anything about disallowing query parameter hard-coded in the path (ref: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#patterned-fields) so I think technically a path like /query.cgi?api=SYNO.API.Info&version=1&method=query&query={query} is valid.

I don't think it's a good idea mixing query parameter and path parameter. To explain what I mean, take a look at the {query} parameter. Technically it's a URL query parameter but it's a path parameter in OpenAPI/Swagger spec.

Technically it's still possible to workaround all the limitations/problems. A good starting point is here: https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/php/api.mustache#L139

A high-level description of the fix: 1) process path parameters first 2) before processing query parameters, need to sanitize the path to extract the path before ? and put the query parameters into the $queryParams hash

May I know if you've time to work on the fix?

[mikespub]

Thanks for your feedback. I would actually recommend a slightly different approach to resolve this:

For issue 1, figure out why the & characters in {{path}} are HTML/XML escaped. Is this done by the mustache templating and is there some way to avoid doing that here, or does the escaping already happen earlier in the code generation process and do we need to fix it somewhere else? https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/php/api.mustache#L128

For issue 2, I would recommend letting the generated ApiClient->callApi method handle any url & queryParams & whatever combinations properly, in particular by adapting the following line: https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/php/ApiClient.mustache#L175

I only use the codegen online, so I don't have the proper development environment to trace issue 1 to its source, or to create the test cases for issue 2, sorry.

[wing328]

RE: b. the codegen doesn't take into account the default values (PHP/Python)

We've not implemented default parameter value in API client since the default value should be enforced in the server side.

[wing328]

As @fehguy has pointed out, path with URL query string is invalid according to the OpenAPI specification.

Issue closed.