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

Fix subResourcePath when using tags in java-jersey #215

Merged
merged 4 commits into from
Jun 30, 2018

Conversation

tht13
Copy link
Contributor

@tht13 tht13 commented Jun 4, 2018

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 and ./bin/security/{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: Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

I am porting this PR over from swagger-codegen

The java jersey useTags option added in #6278 breaks functionality for subResourcePaths (@path is only added to the class route, not each method), this causes conflicts on startup as swagger-core assumes every method in the class is using the same path, and flags "conflicting methods" when all other annotations are identical (ie http type and @produces and @consumes).

View bc26d73#diff-774426b5ae03223547c129bb2cc6bed4R38 for an example of the missing @path annotations

@wing328

@jmini
Copy link
Member

jmini commented Jun 4, 2018

Java technical committee:
@bbdouglas (2017/07) @JFCote (2017/08) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01)

@jmini jmini added this to the 3.0.1 milestone Jun 4, 2018
Copy link
Member

@jmini jmini left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@jmini
Copy link
Member

jmini commented Jun 4, 2018

Thank you a lot for this contribution.

I leave it open for other to have a chance to review it. If nobody is against this change I will merge it.

@jmini
Copy link
Member

jmini commented Jun 5, 2018

I did some test with:

openapi: 3.0.1
info:
  title: OpenAPI Test API
  description: Test for PR 215
  version: 1.0.0
servers:
  - url: 'http://api.company.xyz/v2'
paths:
  /group1/op1:
    get:
      tags:
        - tag1
      operationId: op1
      responses:
        '200':
          description: Ok
  /group1/op2:
    get:
      tags:
        - tag2
      operationId: op2
      responses:
        '200':
          description: Ok
  /group2/op3:
    get:
      tags:
        - tag2
      operationId: op3
      responses:
        '200':
          description: Ok
  /group3/op4:
    get:
      operationId: op4
      responses:
        '200':
          description: Ok
  /group4/op5:
    get:
      tags:
        - group4
      operationId: op5
      responses:
        '200':
          description: Ok
  /group4/op6:
    get:
      tags:
        - group4
      operationId: op6
      responses:
        '200':
          description: Ok
components:
  schemas:
    SomeObj:
      type: object
      properties:
        someProp:
          type: string

Using the JavaJerseyServerCodegen generator (generator name jaxrs-jersey) with the jersey2 library (default value)

Good improvement, the warning is gone:

Caused by: org.glassfish.jersey.server.model.ModelValidationException: Validation of the application resource model has failed during application initialization.
[[FATAL] A resource model has ambiguous (sub-)resource method for HTTP method GET and input mime-types as defined by"@Consumes" and "@Produces" annotations at Java methods public javax.ws.rs.core.Response org.openapitools.api.Group4Api.op5(javax.ws.rs.core.SecurityContext) throws org.openapitools.api.NotFoundException and public javax.ws.rs.core.Response org.openapitools.api.Group4Api.op6(javax.ws.rs.core.SecurityContext) throws org.openapitools.api.NotFoundException at matching regular expression /Group4. These two methods produces and consumes exactly the same mime-types and therefore their invocation as a resource methods will always fail.; source='org.glassfish.jersey.server.model.RuntimeResource@353b0719', [FATAL] A resource model has ambiguous (sub-)resource method for HTTP method GET and input mime-types as defined by"@Consumes" and "@Produces" annotations at Java methods public javax.ws.rs.core.Response org.openapitools.api.Tag2Api.op2(javax.ws.rs.core.SecurityContext) throws org.openapitools.api.NotFoundException and public javax.ws.rs.core.Response org.openapitools.api.Tag2Api.op3(javax.ws.rs.core.SecurityContext) throws org.openapitools.api.NotFoundException at matching regular expression /Tag2. These two methods produces and consumes exactly the same mime-types and therefore their invocation as a resource methods will always fail.; source='org.glassfish.jersey.server.model.RuntimeResource@2a4e939a']

This is already an improvement and therefore in my opinion we can merge this.


But, I then did additional tests. I started the jetty server:

mvn clean package jetty:run

None of the defined endpoints in my spec are responding:

http://localhost:8080/v2/group1/op1
http://localhost:8080/v2/group1/op2
http://localhost:8080/v2/group2/op3
http://localhost:8080/v2/group3/op4
http://localhost:8080/v2/group4/op5
http://localhost:8080/v2/group4/op6

Then I have opened the auto-generated specification (that corresponds to the JaxRS code):

http://localhost:8080/v2/swagger.yaml

(It is still swagger.yaml and not openapi.yaml because #27 is not implemented yet)

I got this:

swagger: "2.0"
info:
  description: "Test for PR 215"
  version: "1.0.0"
  title: "OpenAPI Server"
  termsOfService: ""
  contact:
    email: ""
  license:
    name: ""
    url: "http://unlicense.org"
host: "api.company.xyz"
basePath: "/v2"
tags:
- name: "group4"
- name: "tag1"
- name: "tag2"
schemes:
- "http"
paths:
  /Default/op4:
    get:
      summary: ""
      description: ""
      operationId: "op4"
      parameters: []
      responses:
        200:
          description: "Ok"
  /Group4/op5:
    get:
      tags:
      - "group4"
      summary: ""
      description: ""
      operationId: "op5"
      parameters: []
      responses:
        200:
          description: "Ok"
  /Group4/op6:
    get:
      tags:
      - "group4"
      summary: ""
      description: ""
      operationId: "op6"
      parameters: []
      responses:
        200:
          description: "Ok"
  /Tag1/op1:
    get:
      tags:
      - "tag1"
      summary: ""
      description: ""
      operationId: "op1"
      parameters: []
      responses:
        200:
          description: "Ok"
  /Tag2/op2:
    get:
      tags:
      - "tag2"
      summary: ""
      description: ""
      operationId: "op2"
      parameters: []
      responses:
        200:
          description: "Ok"
  /Tag2/op3:
    get:
      tags:
      - "tag2"
      summary: ""
      description: ""
      operationId: "op3"
      parameters: []
      responses:
        200:
          description: "Ok"

As you can see, the specification I got do not define the same endpoints.

Of course if you navigate to one of the endpoints like:

http://localhost:8080/v2/Group4/op5

You get the expected result:

<apiResponseMessage>
  <message>magic!</message>
  <type>ok</type>
</apiResponseMessage>

@tht13 Do you want to work on this inside this PR? Or do you want me to merge this and file an other issue for that?

@wing328 wing328 removed this from the 3.0.1 milestone Jun 11, 2018
Conflicts:
	samples/server/petstore/jaxrs/jersey1-useTags/.openapi-generator/VERSION
	samples/server/petstore/jaxrs/jersey2-useTags/.openapi-generator/VERSION
@jmini
Copy link
Member

jmini commented Jun 28, 2018

I have merged master in this PR. I plan to merge this PR soon.

@jmini
Copy link
Member

jmini commented Jun 30, 2018

This PR improves the current situation.
I consider that this can be merged with 3.1.0.

I will try to work on the follow-up PR to fix the path issue I have discussed here.

@jmini jmini added this to the 3.1.0 milestone Jun 30, 2018
@jmini jmini merged commit 79856ab into OpenAPITools:master Jun 30, 2018
@jmini
Copy link
Member

jmini commented Jul 2, 2018

The issue discussed in this PR is fixed by #437

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
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.

None yet

3 participants