swagger-api / swagger-parser

Swagger Spec to Java POJOs
http://swagger.io
Apache License 2.0
788 stars 531 forks source link

Duplicated schemas in parsed result #1081

Closed ackintosh closed 4 years ago

ackintosh commented 5 years ago

Related issue: https://github.com/OpenAPITools/openapi-generator/issues/2701


Specs that reproduces the issue

openapi: 3.0.0
info:
  title: Common Data Types
  version: "1.0"
paths:
  /subscriptions:
    get:
      summary: read all of the active subscriptions for the app.
      operationId: getSubscriptionsById
      parameters:
        - name: appId
          in: path
          description: App ID
          required: true
          schema:
            type: integer
            format: int64
      responses:
        '200':
          description: OK (Successful)
          content:
            application/json:
              schema:
                type: array
                items:
                  $ref: '#/components/schemas/Subscription'
        '500':
          $ref: './common.yaml#/components/responses/E500'  
components:
  schemas:
      ProblemDetails:
        type: object
        properties:
          title:
            type: string
            description: A short, human-readable summary of the problem
          status:
            type: integer
            description: The HTTP status code for this occurrence of the problem.
      Subscription:
        type: string
openapi: 3.0.0
info:
  title: Common Data Types
  version: "1.0"
paths: {}
components:
  responses:
    E500:
      description: Server Error
      content:
        application/json:
          schema:
            $ref: './openapi.yaml#/components/schemas/ProblemDetails'

Test codes which shows the issue

    @Test
    public void fooBar() {
        String location = "src/test/resources/xxx/openapi.yaml";
        OpenAPIV3Parser parser = new OpenAPIV3Parser();

        OpenAPI result = parser.read(location);
    }

The result has a duplicated schema ProblemDetails_2.

image

r-sreesaran commented 5 years ago

@ackintosh can you add a failing test case.

ackintosh commented 5 years ago

@r-sreesaran Thanks for your reply. I've filed the PR to add a failing test case. https://github.com/swagger-api/swagger-parser/pull/1102

r-sreesaran commented 5 years ago

@ackintosh This may be valid as "Problem Details" is referenced from external open api document. It needs to be duplicated and added in the object.

Consider a case where there are two schema having same name but of different type. Then in that case if we remove duplicate, then one schema will be lost.

Here the Problems Details are present in both documents and have different definitions. If we remove the duplicate entries then only one schema will be present. But we need both.

info:
  title: Common Data Types
  version: "1.0"
paths:
  /subscriptions:
    get:
      summary: read all of the active subscriptions for the app.
      operationId: getSubscriptionsById
      parameters:
        - name: appId
          in: path
          description: App ID
          required: true
          schema:
            type: integer
            format: int64
      responses:
        '200':
          description: OK (Successful)
          content:
            application/json:
              schema:
                type: array
                items:
                  $ref: '#/components/schemas/Subscription'
        '500':
          $ref: './issue-1108-1.yaml#/components/responses/E500'
        '501':
          $ref: '#/components/schemas/ProblemDetails' 
components:
  schemas:
    ProblemDetails:
      type: object
      properties:
        title:
          type: string
          description: A short, human-readable summary of the problem
        status:
          type: integer
          description: The HTTP status code for this occurrence of the problem.
    Subscription:
      type: string

issue-1108-.yaml

openapi: 3.0.0
info:
  title: Common Data Types
  version: "1.0"
paths: {}
components:
  responses:
    E500:
      description: Server Error
      content:
        application/json:
          schema:
            $ref: '#/components/schemas/ProblemDetails'
  schemas:
   ProblemDetails:
      type: object         

@gracekarina @ymohdriz please do share your comments on this issue.

gracekarina commented 5 years ago

Hi @r-sreesaran thanks for the reply, the case @ackintosh presents it's the same schema, so it should not be duplicating it. The case you present above it's ok for "duplicating" because there are different schemas. Sorry it's taking long for me to check this, I hope to have a fix for this week.

ymohdriz commented 5 years ago

@r-sreesaran Issue here is a new schema (ProblemDetails_2) is introduced in the components which is incorrect. We need to fix it

r-sreesaran commented 5 years ago

@gracekarina Few clarifications about the duplication part from the example i have mentioned. As per Open API specification no two schema definitions can have the same name. Since these references are added from the same file this does qualify them to be treated as schemas from same document and in such case the open api document becomes invalid as there two schemas with name name.

As of now we are not handling the references like wise. Should this be done this way or in existing way where a different name is created and then the schema is added using the created name.

jphorx commented 5 years ago

We've seen this issue as well using version 2.0.12. These two swagger files can be used to reproduce it if needed and will produce two models for flowDirection.

spec1.yaml

openapi: "3.0.0"
info:
  version: 1.0.0
  title: spec1
paths:
  /flow:
    get:
      responses:
        '200':
          description: Expected response to a valid request
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/FlowInformation'
components:            
  schemas:
    FlowDirection:
      enum:
        - DOWNLINK
        - UPLINK
        - BIDIRECTIONAL
        - UNSPECIFIED
    FlowInformation:
      type: object
      properties:
        flowDescription:
          $ref: 'spec2.yaml#/components/schemas/FlowDescription'
        ethFlowDescription:
          $ref: 'spec2.yaml#/components/schemas/EthFlowDescription'
        flowDirection:
          $ref: '#/components/schemas/FlowDirection'

spec2.yaml

openapi: "3.0.0"
info:
  version: 1.0.0
  title: spec2
components:            
  schemas:
    FlowDescription:
      type: string
    EthFlowDescription:
      type: object
      properties:
        fDesc:
          $ref: '#/components/schemas/FlowDescription'
        fDir:
          $ref: 'spec1.yaml#/components/schemas/FlowDirection'
binaryunary commented 4 years ago

I'm cross-posting my comment from an issue in openapi-generator as the issue is related to swagger-parser. Original link: https://github.com/OpenAPITools/openapi-generator/issues/2701#issuecomment-582320928.

After playing around with external refs for a bit I discovered that swagger-parser will do some normalization on them, but it seems to happen lazily.

If you have a ref like

somefile.yml#/components/schemas/SomeSchema

then it will become

./somefile.yml#/components/schemas/SomeSchema

inside the parser after the first time this ref is resolved. Internally SomeSchema is cached when it is first read, when accessed the second time its source is different (somefile.yml != ./somefile.yml) so a number is appended to its name to avoid conflicts.

Prefixing relative paths with ./ solves this problem, but it seems the parser still has problems normalizing the paths relative to the root spec file as having complex relative references (e.g. different levels, from different subdirectories) still break the parser.

schlagi123 commented 4 years ago

The Issue #1292 resolves this issue. The current master branch works.

gracekarina commented 4 years ago

yes that is true!, we solve the duplication issue, thanks for pointing this here! @ackintosh please test and in case the issue is no longer happening please close. fixed by #1343

schlagi123 commented 4 years ago

I tested the two examples from @ackintosh and @jphorx and they work. Please close the issue.