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] [Spring] [Spring-Boot] Exploded query parameters are not correctly serialized in controller #14860

Open
5 of 6 tasks
GregDThomas opened this issue Mar 2, 2023 · 10 comments

Comments

@GregDThomas
Copy link
Contributor

GregDThomas commented Mar 2, 2023

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

When generating code with an exploded query parameter, the spring generator with the spring-boot library does not correctly resolve the supplied parameters. Some experimentation suggests that the serialization is using the name of the field in the Java object, not the name of the field in the OpenAPI specification.

openapi-generator version

6.4.0

OpenAPI declaration file content or url

https://github.com/GregDThomas/openapi-generator-spring-boot-example/blob/main/specs/explode-query-parameter.yaml

Generation Details

Using the OpenAPI Generator Gradle Plugin with

    generatorName = "spring"
    library = 'spring-boot'
Steps to reproduce

See the sample project at https://github.com/GregDThomas/openapi-generator-spring-boot-example and more specifically the tests in ExampleControllerTest

Specifically -

  • the fields in the exploded object that match the name of the field in the POJO (someCamelString) are correctly serialized.
  • the fields in the exploded object that do no match the name of the field in the POJO are not correctly serialized.
    -- some-kebab-string / someKebabString
    -- some_snake_string / someSnakeString
Related issues/PRs
Suggest a fix
@GregDThomas
Copy link
Contributor Author

One solution is to "explode" the object in the method signature, so instead of

    default ResponseEntity<SomeReturnValue> search(
        @NotNull @Parameter(name = "object-param", description = "", required = true, in = ParameterIn.QUERY) @Valid SearchObjectParamParameter objectParam,
...

becomes

    default ResponseEntity<SomeReturnValue> search(
      @Parameter(name = "kebab-param", description = "", in = ParameterIn.QUERY) @Valid @RequestParam(value = "kebab-param", required = false) String kebabParam
      @Parameter(name = "snake_param", description = "", in = ParameterIn.QUERY) @Valid @RequestParam(value = "snake_param", required = false) String snakeParam
...

But don't know if this is the right approach for Spring Boot, or if there is a better one. Suggestions welcome.

@wing328
Copy link
Member

wing328 commented Mar 14, 2023

thanks for reporting the issue.

cc @cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09)

@borsch
Copy link
Member

borsch commented Mar 17, 2023

Probably make sense to explode object into parameters. There is no may to map Object with @RequestParam with custom names .. Also would make sense to add configuration properties that would control this

@GregDThomas
Copy link
Contributor Author

Yeah, I was coming to the conclusion that there was a sane way to map an Object to @RequestParam for Spring Boot, not sure if we need a config property to control the behaviour, but I guess there may be some edge cases (that I can't think of!) where the current behaviour is desirable. I'll see if I can get some time to look at this.

@borsch
Copy link
Member

borsch commented Mar 20, 2023

I'll see if I can get some time to look at this.

@GregDThomas that's exactly the reason, why I would prefer to go with config option and by default leave as it is. I believe there are already users, which are fine with current behavior. Current behavior works pretty fine, when request parameters name has exactly same names as generate model's fields. Example

/api/search:
    get:
      operationId: Search
      parameters:
        - name: regularParam
          in: query
          required: false
          schema:
            type: string
        - name: objectParam
          in: query
          required: true
          schema:
            type: object
            properties:
              someField1:
                type: string
              someField2:
                type: string
              someCamelString:
                type: string
      responses:
        '200':
          description: Some description.
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/SomeReturnValue'

and generated model would be

public class ObjectParam {
  private String someField1;
  private String someField2;
  private String someCamelString;
}

In above case current generator's behavior is perfectly fine

@GregDThomas
Copy link
Contributor Author

More research over lunch has turned up https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/method/support/HandlerMethodArgumentResolver.html which should allow a customer parameter annotation to be used - I may experiment with that to see if I can get it working ...

@borsch
Copy link
Member

borsch commented Mar 20, 2023

More research over lunch has turned up https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/method/support/HandlerMethodArgumentResolver.html which should allow a customer parameter annotation to be used - I may experiment with that to see if I can get it working ...

I saw that thing. To be honest I would prefer to avoid this. Think about cases, when in single model you are trying to deal with list of custom models

/api/search:
    get:
      operationId: Search
      parameters:
        - name: regularParam
          in: query
          required: false
          schema:
            type: string
        - name: objectParam
          in: query
          required: true
          schema:
            type: object
            properties:
              someField1:
                type: string
              listField:
                type: array
                items:
                  type: object
                  properties:
                    id:
                      type: integer
                    name:
                      type: string
                    statuses:
                      type: string
                      enum: [enum1, enum2, enum3]
      responses:
        '200':
          description: Some description.
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/SomeReturnValue'

/api/search?listField[0].id=1&listField[0].name=asd&listField[0].statuses=enum1,enum2

Handling such thing would be a real pain ..

What your thoughts on this?
@cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09)

@GregDThomas
Copy link
Contributor Author

/api/search?listField[0].id=1&listField[0].name=asd&listField[0].statuses=enum1,enum2

I suspect there's no good way to handle that, tbh.

But I think;

  • leaving default behaviour to use the current @RequestParam
  • Supporting an optional @NamedRequestParam which is a simple HandlerMethodArgumentResolver that provides a mapping of primitives to named fields in a POJO (i.e. essentially a slight improvement over the existing @RequestParam support)

gives 80% of the benefit of exploded query params for 20% of the effort.

@ketankhairnar
Copy link

@GregDThomas did you find a workaround for this? we're also facing this issue. In generated model - it also has annotation with correct case - snake_case here. It still expects queryBy instead of query_by in query parameters

  @JsonProperty("query_by")
  private String queryBy;

@GregDThomas
Copy link
Contributor Author

No; AFAICT you'll either need your field names to be camel case, otherwise requires a change to the generator code which I've yet to get time to take a look at.

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

No branches or pull requests

4 participants