Skip to content

Conversation

wingsofovnia
Copy link
Contributor

This fixes #6553 (@zkdzegede) + incorrect @return JavaDocs in retrofit interfaces. Tested on a real project.

Formatted operation signature in retrofit2/api.mustache (for easier review):

  {{^doNotUseRx}} // if rxJava
    {{#useRxJava}} // if rxJava.ver == 1
        Observable<{{#isResponseFile}}ResponseBody{{/isResponseFile}}{{^isResponseFile}}{{#returnType}}{{{returnType}}}{{/returnType}}{{^returnType}}Void{{/returnType}}{{/isResponseFile}}>
    {{/useRxJava}}

    {{#useRxJava2}} // if rxJava.ver == 2
        {{#returnType}} // if returnType
            Observable<{{#isResponseFile}}ResponseBody{{/isResponseFile}}{{^isResponseFile}}{{{returnType}}}{{/isResponseFile}}>
        {{/returnType}}

        {{^returnType}} // if !returnType
            Completable
        {{/returnType}}
    {{/useRxJava2}}
  {{/doNotUseRx}}

  {{#doNotUseRx}} // if !rxJava
    Call<{{#isResponseFile}}ResponseBody{{/isResponseFile}}{{^isResponseFile}}{{#returnType}}{{{returnType}}}{{/returnType}}{{^returnType}}Void{{/returnType}}{{/isResponseFile}}>
  {{/doNotUseRx}}

  {{operationId}}({{^allParams}});{{/allParams}}

Java technical committee: @bbdouglas @JFCote @sreeshas @jfiala @lukoyanov @cbornet @wing328

@cbornet
Copy link
Contributor

cbornet commented Jan 13, 2018

I commented on #6553 the correct return type should be Single<Response<Void>>

@wingsofovnia
Copy link
Contributor Author

wingsofovnia commented Jan 13, 2018

I disagree, @cbornet. Currently, all Retrofit2 + RxJava endpoints aren't using Response wrappers. Examples from petstore: Observable<List<Pet>>, Observable<Boolean> etc, not Observable<Response<List<Pet>>>, Observable<Response<Boolean>>. Therefore, I don't see a point to introduce this inconsistency in the generated sources for this exact case.

Indeed, the solution with Completable has a drawback - you can't get HTTP metadata like Location header (and it has an advantage either - you don't need to unwrap responses each time). But either do all current endpoints.

My point is that we should consider this as a fix for #6553 as the generator delivers invalid code, but keep consistency and think about migrating to Response wrapping elsewhere, for example, as a part of #5806, but not #6553 that is a bug report. Generator option can be a one way of implementing that.

@cbornet
Copy link
Contributor

cbornet commented Jan 13, 2018

I was going to say that your change is also a breaking one but I realized that since it breaks something that is not working, I guess it doesn't harm. So for me this PR is OK.
But for the next breaking version, I would rather have the Response wrapper by default and avoid having yet another option (rxJava is already an option...)

@wingsofovnia
Copy link
Contributor Author

Any updates? Could you please stage it for the next release, @wing328 ?

@wing328 wing328 changed the base branch from master to 2.4.0 January 20, 2018 05:51
@wing328 wing328 added this to the v2.4.0 milestone Jan 20, 2018
@wing328
Copy link
Contributor

wing328 commented Jan 20, 2018

@wingsofovnia Thanks for the PR. Can you please rebase on the latest 2.4.0 to resolve the merge conflicts?

@wingsofovnia
Copy link
Contributor Author

@wing328 I rebased on the 2.4.0 (https://github.com/wingsofovnia/swagger-codegen/tree/fix/issue-6553-completable-rebased-2_4_0) branch but the build fails:

[INFO] ------------------------------------------------------------------------
[INFO] Building swagger-generator 2.4.0-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO] 
[INFO] swagger-codegen-project ............................ SUCCESS [  2.565 s]
[INFO] swagger-codegen (core library) ..................... SUCCESS [ 21.061 s]
[INFO] swagger-codegen (executable) ....................... SUCCESS [  4.979 s]
[INFO] swagger-codegen (maven-plugin) ..................... SUCCESS [  6.814 s]
[INFO] swagger-generator .................................. FAILURE [  0.579 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 36.663 s
[INFO] Finished at: 2018-01-20T12:44:08+01:00
[INFO] Final Memory: 55M/805M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal on project swagger-generator: Could not resolve 
dependencies for project io.swagger:swagger-generator:war:2.4.0-SNAPSHOT: 
Failure to find io.swagger:swagger-codegen:jar:tests:2.4.0-SNAPSHOT in https://
oss.sonatype.org/content/repositories/snapshots was cached in the local 
repository, resolution will not be reattempted until the update interval of sonatype-
snapshots has elapsed or updates are forced -> [Help 1]

Is that ok?

@wing328 wing328 changed the base branch from 2.4.0 to master January 21, 2018 15:03
@wing328 wing328 changed the base branch from master to 2.4.0 January 21, 2018 15:04
@wing328 wing328 closed this Jan 22, 2018
@wing328 wing328 changed the base branch from 2.4.0 to master January 22, 2018 07:16
@wing328 wing328 reopened this Jan 22, 2018
@joeboyscout04
Copy link
Contributor

Anybody able to fix the conflicts and merge this one?

@wing328
Copy link
Contributor

wing328 commented Feb 1, 2018

Anybody able to fix the conflicts and merge this one?

Let me give it another try tomorrow. Will let you guys if I need any help.

@wing328 wing328 merged commit 642c056 into swagger-api:master Feb 2, 2018
@wing328
Copy link
Contributor

wing328 commented Feb 2, 2018

@joeboyscout04 I've resolved the merge conflicts and merged the change into master.

Would you please pull the latest master to give it a try to make sure the change looks good?

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.

[JAVA] Retrofit2 Observable<Void> should be Observable<Response<Void>>
4 participants