ferdikoomen / openapi-typescript-codegen

NodeJS library that generates Typescript or Javascript clients based on the OpenAPI specification
MIT License
2.91k stars 519 forks source link

Object is not transformed into the URL variable correctly #917

Open altarrok opened 2 years ago

altarrok commented 2 years ago

We are using SpringDoc to generate the openapi specs of our backend, and we are using a feature of SpringDoc where commonly used Pageable object parameter is mapped automatically to openapi specs. The result of the mapping looks like this:

ss1

As you can see the Pageable parameter is defined as object. The generated request URL looks like this:

http://localhost:8080/hackathon/search?page=0&size=1&sort=location

So we can see that swagger-ui generates the correct request URL for the given Pageable object.

However -problem begins here-, when the generated client using openapi-typescript-codegen is used to make a backend call, the request URL looks like this:

http://localhost:8080/hackathon/search?pageable=%5Bobject%20Object%5D

Tracking the code, I can see that in request.ts::getQueryString arrays are accounted for but the objects aren't, and therefore the Pageable object is casted to a string, resulting in the given bug.

Is there a possible fix on our side (although I doubt that)?

A possible fix on the library side (I haven't tested) may be to call the getQueryString method recursively for each entry in the object, and concatenating the result (qs).

ferdikoomen commented 2 years ago

@altarrok There is a fix for that in the current development branch (complex query params support). This should fix your issue. Once i have a few more things fixed i will push a new version to NPM

altarrok commented 2 years ago

Thanks a lot @ferdikoomen !! Let me know if we can contribute in some way, thanks for keeping the library updated

ferdikoomen commented 2 years ago

@altarrok New version is pushed to NPM 0.13.0 Could you check if that resolves your issue?

ferdikoomen commented 2 years ago

@altarrok New version is pushed 0.15.0, that should include all options (nested props, arrays, etc.)

altarrok commented 2 years ago

@ferdikoomen unfortunately the fix doesn't help our case (from my original issue): As you can see the Pageable parameter is defined as object

The fix only works for lists, not objects, objects are still translated as pageable=%5Bobject%20Object%5D which is the issue. I believe there is no case where this 'feature' is useful, so it is a bug. Please let me know what we can do about this

ferdikoomen commented 2 years ago

@altarrok do you by change have a link to the spec or a copy of the spec that i can check? Im mostly interested in the definition of that parameter. That would make it easier to reproduce the scenario

altarrok commented 2 years ago

@ferdikoomen this is the openapi spec for pageable we are using:

{
    "Pageable": {
        "type": "object",
        "properties": {
            "page": {
                "minimum": 0,
                "type": "integer",
                "format": "int32"
            },
            "size": {
                "minimum": 1,
                "type": "integer",
                "format": "int32"
            },
            "sort": {
                "type": "array",
                "items": {
                    "type": "string"
                }
            }
        }
    }
}

I believe in request.ts::getQueryString line#45, the value is being checked to be an array, but there is no other check that if the value is a string or not, but it is being casted to a string downstream. My current best solution would be to check if the type of the value is a string, if not, check if it is an array, if not accept it as an object and call recursively using the objects key value pairs. That way, an object containing other objects, strings, or arrays can be flattened to a query string. Let me know what you think

ferdikoomen commented 2 years ago

@altarrok I assume you then have something like this in the operation parameters right?:

"parameters": [
    {
        "description": "This is a pageable parameter",
        "name": "pageable",
        "in": "query",
        "schema": {
            "$ref": "#/components/schemas/Pageable"
        }
    }
],

I'm testing this now with a client, will come back with results in a bit

ferdikoomen commented 2 years ago

@altarrok When im using the latest version (0.18.1), then i see the following behaviour

When im sending the following parameter:

parameter: {
   page: '0',
   size: '1',
   sort: ['location']
}

Then this is converted to:

parameter%5Bpage%5D=0&parameter%5Bsize%5D=1&parameter%5Bsort%5D%5B0%5D=location

If we decode this, then it looks like:

parameter[page]=0&parameter[size]=1&parameter[sort][0]=location

Our backend (NodeJS Express) just parses this and parses this as:

parameter: {
   page: '0',
   size: '1',
   sort: ['location']
}

That seems exactly right... Are you using the latest version of the generator?

altarrok commented 2 years ago

@ferdikoomen we were using npx. Specifying the version now, we got the same behavior as you do. However this behavior: pageable[sort]=LOCATION is very much different from the intended and swagger behavior: sort=LOCATION

If you disagree with this, I respect your opinion, and please let me know so that I can fork and fix the issue. Thanks!

ferdikoomen commented 2 years ago

@altarrok Do you have the full spec for me (if you cannot send it via github, then please send me an email: info@madebyferdi.com). Then i'll have a look to see why this generated differently.

altarrok commented 2 years ago

@ferdikoomen The generation is identical, however I think that nested query objects should be flattened to a key:value pair list, instead of having the nested structure stringified into parentkey[childkey]:value list.

The swagger behavior is matching what I believe the behavior should be.

I can not share the full spec as it would breach company policies, sorry :(

ferdikoomen commented 2 years ago

Can you share the operation spec itself? For the tests i am using the following: Is there a big difference between this and what you are using?

"post": {
    "tags": [
        "Parameters"
    ],
    "operationId": "PageableParam",
    "parameters": [
        {
            "description": "This is a required parameter",
            "name": "parameter",
            "in": "query",
            "required": true,
            "schema": {
                "$ref": "#/components/schemas/Pageable"
            }
        }
    ],
}
altarrok commented 2 years ago

No it is practically same:

{
    "get": {
        "tags": [
            "foo-controller"
        ],
        "operationId": "bar",
        "parameters": [
            {
                "name": "pageable",
                "in": "query",
                "required": true,
                "schema": {
                    "$ref": "#/components/schemas/Pageable"
                }
            }
        ]
    }
}
ferdikoomen commented 2 years ago

@altarrok That is weird, since the specs basically describes your server is expecting a property in the query called pag pageable. So if the server would receive http://localhost:8080/hackathon/search?page=0&size=1&sort=location then this property is not present... So this would be an invalid URL... No idea why Swagger UI generates this url... weird

altarrok commented 2 years ago

@ferdikoomen Yeah, I will do some testing around it, let's see what is the actual behavior of swagger.

May I ask how is the current behavior useful? If you could give some cases where this is used.

altarrok commented 2 years ago

@ferdikoomen Immediately I tested an object as a request parameter, and the swagger generated using the same behavior, and the request is a bad request (as the response confirms). So this tells me that Swagger and Spring are using the same behavior, where if there is a request parameter specified, it should be a simple type of a variable like a string or a UUID. If the controller input parameter is not specified as a request parameter (in Spring), then it accepts the fields of that variable type as request parameters.

For example: Let's assume CompositeKey is a type consisting of 2 UUIDs: fooId and barId If controller method parameter doesn't specify it as a request parameter such as:

public void controllerMethod(CompositeKey key)

Then the request parameters would be the fields of CompositeKey: fooId and barId

If controller method parameter DOES specify it as a request parameter such as:

public void controllerMethod(@RequestParameter("myKey") CompositeKey key)

Then the request parameter is myKey

The weird thing is that, swagger always uses the first behavior no matter if it is specified or not. That tells me that having objects in the request parameter is not good since it is not supported by swagger.

ferdikoomen commented 2 years ago

Yeah there is indeed some logic in the Java based generator that im trying to figure out:

    /**
     * Formats the specified query parameter to a list containing a single {@code Pair} object.
     *
     * Note that {@code value} must not be a collection.
     *
     * @param name The name of the parameter.
     * @param value The value of the parameter.
     * @return A list containing a single {@code Pair} object.
     */
    public List<Pair> parameterToPair(String name, Object value) {
        List<Pair> params = new ArrayList<Pair>();

        // preconditions
        if (name == null || name.isEmpty() || value == null || value instanceof Collection) return params;

        params.add(new Pair(name, parameterToString(value)));
        return params;
    }

It seems that in some cases (not sure when) they 'explode' the query param and indeed generate a url that looks like

http://localhost:8080/hackathon/search?page=0&size=1&sort=location

In general a beter solution would indeed be to specify separate query params for each of the props inside this object.

altarrok commented 2 years ago

@ferdikoomen Also, Tomcat throws an error when URL has "[" or "]":

Invalid character found in the request target [/foo/boo?pageable[sort]=LOCATION]. The valid characters are defined in RFC 7230 and RFC 3986
altarrok commented 2 years ago

Is that Java based generator publicly available? Maybe I can also take a look and help

ferdikoomen commented 2 years ago

@altarrok https://github.com/swagger-api/swagger-codegen

ferdikoomen commented 2 years ago

You can download and run it like this:

curl https://repo1.maven.org/maven2/io/swagger/codegen/v3/swagger-codegen-cli/3.0.21/swagger-codegen-cli-3.0.21.jar -o swagger-codegen-cli-v3.jar
java -jar ./swagger-codegen-cli-v3.jar generate -i myspec.json -l java -o generated
altarrok commented 2 years ago

So I have been looking into different libraries and examples, it seems like this is really a grey area, so I believe it is your decision as how this library should behave in case we have objects passed as request parameters

That being said, there is a library that has a configuration available for stringifying the request parameters, if no config provided fallback is to cast the object to a String.

ferdikoomen commented 2 years ago

Ill need to have a look here: https://swagger.io/docs/specification/describing-parameters/#query-parameters

And just pick a direction 😃 anyway it seems some work is needed here. Will keep the ticket open.

altarrok commented 2 years ago

Hey @ferdikoomen, just wanted to see if there is an update on this issue?

altarrok commented 2 years ago

I wanted to share a doc I found:

https://spec.openapis.org/oas/v3.1.0 4.8.12.2 Fixed Fields

Note that the default "style" is "form" when parameter is in "query":

Default values (based on value of in): for query - form

And the default "explode" is "true" when "style" is "form":

When style is form, the default value is true.

And when "explode" is "true", the parameters are seperated, look at the table in: https://swagger.io/docs/specification/serialization/ under Query Parameters

altarrok commented 2 years ago

Also, we found an easy solution on spring-doc side, so if anyone is down in this rabbit hole (I doubt), hit me up and I can explain the solution

ferdikoomen commented 2 years ago

@altarrok Sure, send me a message (info@madebyferdi.com) and let's chat. Would be nice to implement this

ruiaraujolink commented 2 years ago

I have the same problem and I fix it by referencing the param as to a schema:

components:
  schemas:
    Sort:
      type: array
      items:
        type: string

  parameters:
    sort:
      name: sort
      in: query
      description: 'Sorted request'
      example: ['pessoa.nome:asc']
      schema:
        # This fixes the parameter not being an array
        $ref: '#/components/schemas/Sort'

the generated code is

image image
Arnagos commented 2 years ago

Also, we found an easy solution on the spring-doc side, so if anyone is down in this rabbit hole (I doubt), hit me up and I can explain the solution

@altarrok I have the exact same problem. What did you change on the spring-doc site?

altarrok commented 2 years ago

@Arnagos I believe we used

@PageableAsQueryParam

for the endpoints on controllers

more info: @PageableAsQueryParam

RobbieFernandez commented 2 years ago

Hi @ferdikoomen. Do you have any updates on this behaviour?

I'm also having this issue and I disagree that this a grey area. OpenAPI allows you to specify object parameters that should explicitly be exploded in this way. See "Query Parameters" here: https://swagger.io/docs/specification/serialization/. What I'm trying to achieve here is an endpoint that allows for dynamic query parameters that cannot be neatly documented, so I'm generating the API with object query parameters with style=form and explode=true in order to indicate that the endpoint accepts arbitrary query parameters. Is there currently any way to get this behaviour with this library?