Skip to content

Conversation

@Zomzog
Copy link
Contributor

@Zomzog Zomzog commented Apr 21, 2019

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Must fix #2655

I have changed the code in defaultCodgen. Maybe it's too low level and must be done in abstractJava?

Another solution is to create another fromProperty for fromParameter with more parameters and force complexType with the $ref. But I'm afraid of losing all language-specific overriding.

@wing328 (2015/07)
@jimschubert (2016/05)
@cbornet (2016/05)
@ackintosh (2018/02)
@jmini (2018/04)

@jimschubert
Copy link
Member

I've evaluated this locally, and it appears to almost fix the linked issue.

Generating with the following from this branch:

openapi-generator generate -g spring \
  -i modules/openapi-generator/src/test/resources/3_0/objectQueryParam.yaml \
  -o .bak/2655 \
  --additional-properties=library=spring-boot,beanValidations=true,reactive=true \
  --skip-validate-spec

And modifying the output in .bak/2655 for PonyApi.java to remove the @RequestParam as requested in the linked issue:

    @ApiOperation(value = "", nickname = "list", notes = "", tags={  })
    @ApiResponses(value = {  })
    @RequestMapping(value = "/pony",
        method = RequestMethod.GET)
    default Mono<ResponseEntity<PageQuery>> list(@ApiParam(value = "", defaultValue = "null") @Valid PageQuery pageQuery, ServerWebExchange exchange) {
        exchange.getResponse().setStatusCode(HttpStatus.OK);
        return Mono.just(new ResponseEntity(pageQuery, HttpStatus.OK));
    }

starting via mvn spring-boot:run and curling:

curl -X GET \
  'http://localhost:8080/pony?offset=1&limit=2' \
  -H 'Content-Type: application/json' \
  -H 'cache-control: no-cache'

I have the input parameters echoed as seems to be expected by the linked issue: {"offset":1,"limit":2}.

Should the @RequestParam modification be done as part of this PR or as a separate one?

@Zomzog
Copy link
Contributor Author

Zomzog commented May 10, 2019

Thx for the review, I will try to fix the @RequestParam in this PR.

@Zomzog
Copy link
Contributor Author

Zomzog commented May 12, 2019

@jimschubert
I've fixed the @RequestParam, but there are still issues with typescript generator. I'm not sure if I must fix them on typescript generator or limit my modification to java generator.

@wing328
Copy link
Member

wing328 commented May 20, 2019

@Zomzog Can you please resolve the merge conflicts when you've time?

@wing328
Copy link
Member

wing328 commented May 20, 2019

Looks like the change results in additional incorrect import in TS generators:

--- a/samples/client/petstore/typescript-angular-v2/npm/api/pet.service.ts
+++ b/samples/client/petstore/typescript-angular-v2/npm/api/pet.service.ts
@@ -21,6 +21,8 @@ import { Observable }                                        from 'rxjs/Observab
 import '../rxjs-operators';
 
 import { ApiResponse } from '../model/apiResponse';
+import { Array&lt;&#39;available&#39; | &#39;pending&#39; | &#39;sold&#39;&gt; } from '../model/arrayavailablePendingSold';
+import { Array&lt;string&gt; } from '../model/arraystring';
 import { Pet } from '../model/pet';

My guess is that it's due to https://github.com/OpenAPITools/openapi-generator/pull/2710/files#diff-7cb46fa53f89a458a7b7cb201d2214a8R3059

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the import below (line 3059) already do the job?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed my solution for using line 3059. It looks better.

@jimschubert
Copy link
Member

I think this looks great, thank you!

@jimschubert jimschubert merged commit 146c1fb into OpenAPITools:master Jun 5, 2019
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Jun 6, 2019
* master:
  [Java][okhttp-gson] Update dependencies (OpenAPITools#3103)
  Link query parameter to model object (OpenAPITools#2710)
  scala-play-server: fix enum names for reserved words (OpenAPITools#3080)
  Add @Sunn to openapi generator core team (OpenAPITools#3105)
  fix NPE in go generator (OpenAPITools#3104)
  scala-play-server: fix API doc url (OpenAPITools#3096)
  [maven-plugin] fix strictSpec parameter without alias (OpenAPITools#3095)
  Ruby: Avoid double escaping path items (OpenAPITools#3093)
  [Golang][client] Allow generating go client code as a submodule. (OpenAPITools#3012)
  [CI] Test maven plugin in Travis, move jobs from Shippable to Circle CI (OpenAPITools#3087)
  general support to add scopes for bearer auth too (OpenAPITools#1984)
  feat(java-jersey2): Making response headers case-insensitive (OpenAPITools#3072)
  [KOTLIN Spring] fix generation with modelNamePrefix/Suffix (OpenAPITools#3038)
  Mark nodejs-server as deprecated (OpenAPITools#3083)
  Use 4.0.2-SNAPSHOT version in gradle samples (OpenAPITools#3085)
fantavlik added a commit to fantavlik/openapi-generator that referenced this pull request Jun 17, 2019
…to inline-resolver

* 'master' of github.com:OpenAPITools/openapi-generator: (213 commits)
  Idiomatic Rust returns for Error conversions (OpenAPITools#2812)
  Add API timeout handling (OpenAPITools#3078)
  Import inner items for map (OpenAPITools#3123)
  update core team in pom.xml (OpenAPITools#3126)
  [gradle] Document consuming via gradle plugin portal (OpenAPITools#3125)
  Bump up babel-cli version to fix security alert (OpenAPITools#3121)
  [C++] [cpprestsdk] Add examples and test for cpprestsdk (OpenAPITools#3109)
  Add enum support to `rust` and skip none option serialization in clients (OpenAPITools#2244)
  Add/update new core team member: etherealjoy (OpenAPITools#3116)
  Gradle sample on travis (OpenAPITools#3114)
  [typescript-fetch] add bearer token support (OpenAPITools#3097)
  Add Q_DECLARE_METATYPE to the generated models and remove ref in signals (OpenAPITools#3091)
  [Java][okhttp-gson] Update dependencies (OpenAPITools#3103)
  Link query parameter to model object (OpenAPITools#2710)
  scala-play-server: fix enum names for reserved words (OpenAPITools#3080)
  Add @Sunn to openapi generator core team (OpenAPITools#3105)
  fix NPE in go generator (OpenAPITools#3104)
  scala-play-server: fix API doc url (OpenAPITools#3096)
  [maven-plugin] fix strictSpec parameter without alias (OpenAPITools#3095)
  Ruby: Avoid double escaping path items (OpenAPITools#3093)
  ...

# Conflicts:
#	modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java
#	modules/openapi-generator/src/test/java/org/openapitools/codegen/InlineModelResolverTest.java
@mikhail-putilov
Copy link

mikhail-putilov commented Jun 21, 2019

Hey guys, looks like this PR broke code generation of my api.yml:
Previously I had:

parameters:
  - name: after_last_modified_date
    in: query
    description: >
      timestamp in ISO...
    schema:
      type: string
      format: date-time
      example: "2019-06-17T15:53:00+01:00"

And it led to the generation of the following:

@ApiParam(value = "timestamp in ISO ...") @Valid @RequestParam(value = "after_last_modified_date", required = false) OffsetDateTime afterLastModifiedDate

And after this PR now the generated code lacks @RequestParam:

@ApiParam(value = "timestamp in ISO ...") @Valid OffsetDateTime afterLastModifiedDate

@mikhail-putilov
Copy link

I believe SpringCodeGen have no notion of HandlerMethodArgumentResolver. That creates all problems with adding such arguments like Pageable pageable and UriComponentsBuilder uriComponentsBuilder. As well as there is no way to generate throws URISyntaxException which occurs quite naturally when one wants to return ResponseEntity.created(...) from a controller.

Ofc there is always a way to bypass all small problems. But gosh, that's probably the main problem why I can't just introduce API-first approach with generation of code out of OpenAPI spec for my current company. This is a big blocker for API-first approach in current spring boot development.

@mikhail-putilov
Copy link

Sorry for being cloudy. That's just devastating.

@wing328
Copy link
Member

wing328 commented Jun 21, 2019

@mikhail-putilov please use an older stable version (e.g. 4.0.0) for the time being.

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.

[BUG] Query Parameter Serialization (Spring)

4 participants