APIDevTools / swagger-parser

Swagger 2.0 and OpenAPI 3.0 parser/validator
https://apitools.dev/swagger-parser
MIT License
1.1k stars 155 forks source link

Bundle behavior feels illogical #127

Closed marcelstoer closed 1 year ago

marcelstoer commented 5 years ago

I only recently learned about the bundle command and it promised to be exactly what I was looking for. After taking it for a test ride I'm left with mixed feeling. There's some behavior that feels illogical to me. However, there may be perfectly sensible explanations for what I'm seeing.

API file

swagger: '2.0'
info:
  version: "1.0"
  title: test API
paths:
  /foo:
    get:
      summary: Find foos
      parameters:
        - $ref: 'shared.yaml#/parameters/Page'
        - $ref: 'shared.yaml#/parameters/PageSize'
        - $ref: 'shared.yaml#/parameters/Sort'
        - name: topic
          in: query
          description: topic
          required: false
          type: string
      responses:
        200:
          description: OK
          schema:
            type: array
            items:
              $ref: '#/definitions/Foo'
        400:
          $ref: 'shared.yaml#/responses/clientErrorMessage'
        500:
          $ref: 'shared.yaml#/responses/internalErrorMessage'
  /bar:
    get:
      summary: Returns all bars
      responses:
        200:
          description: OK
          schema:
            $ref: "#/definitions/Bar"
        400:
          $ref: 'shared.yaml#/responses/clientErrorMessage'
        500:
          $ref: 'shared.yaml#/responses/internalErrorMessage'
definitions:
  Bar:
    type: object
    required:
      - channel
    properties:
      channel:
        type: string
  Foo:
    type: object
    required:
      - id
    properties:
      id:
        type: integer
        format: int64

shared.yaml The content of the file shared across multiple APIs contains the expected stuff. Snippet:

parameters:
  Page:
    name: page
    in: query
    minimum: 1
    type: integer
    default: 1
    required: false

...
responses:
  internalErrorMessage:
    description: yadayada
    schema:
      $ref: '#/definitions/InternalError'
...

Bundled result I run swagger-cli bundle --outfile bundle.yaml --type yaml api.yaml && swagger-cli validate bundle.yaml and verify the output. bundle.yaml is reported to be valid, that's cool.

JamesMessinger commented 5 years ago

The bundle command replaces one $ref to a particular value with the value itself, and then replaces all other $refs to that same value with a pointer to the first value. So it sounds like it's behaving as intended.

You can structure your $ref pointers in such a way that the bundle command produces exactly the results you want. For example, many people prefer to bundle external refs into the definitions or parameters sections of their Swagger file, and then point to those definitions everywhere else.

marcelstoer commented 5 years ago

I appreciate your feedback.

The bundle command replaces one $ref to a particular value with the value itself, and then replaces all other $refs to that same value with a pointer to the first value. So it sounds like it's behaving as intended.

Yep, that's the point. I'm not surprised it's intentional to you - the author 😜. It doesn't feel clean or logical to me because it's not how one would normally structure a Swagger file if you were to write it yourself (you would use the definitions, parameters sections). I have never seen Swagger files with such /~1 $refs in the wild and some OpenAPI tools struggle with that.

But never mind, I will get used to them.

For example, many people prefer to bundle external refs into the definitions or parameters sections of their Swagger file, and then point to those definitions everywhere else.

You mean an extra level of indirection? Like so?

paths:
  /foo:
    get:
      summary: Find foos
      parameters:
        - $ref: '#/parameters/Page'

parameters:
  Page:
# how to $ref: 'shared.yaml#/parameters/Page' here?
marcelstoer commented 4 years ago

Some more feedback after experimenting with bundle and other Swagger/OpenAPI tools more seriously.

The bundler's approach can produce $refs with lots of indirections such as #/definitions/CustomerMasterDataPermissionRead/allOf/1/properties/commonObject/allOf/0. Not all Swagger/OpenAPI tools are able to handle this correctly.

Example

A simplified example.

Original OpenAPI

  PersonData:
    description: A generic person structure...
    type: object
    properties:
      metadata:
        type: array
        items:
          $ref: '../../../../technical-definitions/common-types/v1/common-types-model.yaml#/definitions/Metadata'
  AddressData:
    description: A postal address.
    type: object
    properties:
      metadata:
        type: array
        items:
          $ref: '../../../../technical-definitions/common-types/v1/common-types-model.yaml#/definitions/Metadata'

Bundled OpenAPI

  PersonData:
    description: A generic person structure...
    type: object
    properties:
      metadata:
        type: array
        items:
          description: 'long description here'
          type: object
          properties:
            ...
  AddressData:
    description: A postal address.
    type: object
    properties:
      metadata:
        type: array
        items:
          $ref: '#/definitions/PersonData/properties/metadata/items'

Code The code generator (the "original" one from https://github.com/swagger-api/swagger-codegen) produces Java code like this

public class AddressData {
  @JsonProperty("metadata")
  private List<PersonDatapropertiesmetadataitems> metadata = null;

Yet, it doesn't generate a class PersonDatapropertiesmetadataitems but PersonDataMetadata. However, even if it got that right it would be undesirable to have a class PersonDataMetadata just because PersonData happens to be the first definition that references the generic Metadata definition. If you re-order the definitions in the OpenAPI file you would end up with different class names.

My conclusion: even if the bundle-output in its current form may be intended for machines rather than humans it cannot be used for code generation.

Update Update Update I now realized this actually belongs to https://github.com/APIDevTools/json-schema-ref-parser. I was able to "redefine" $RefParser.prototype.bundle using pretty much all existing code but providing my own remap() function.

lehphyro commented 4 years ago

@marcelstoer could you please share your solution? The current behavior is indeed not good for generating code.

gnongsiej commented 4 years ago

The bundle command replaces one $ref to a particular value with the value itself, and then replaces all other $refs to that same value with a pointer to the first value. So it sounds like it's behaving as intended.

@JamesMessinger : This is confusing. It should replace the first $ref to a particular value with the value itself.

Currently, I am facing an issue where the first $ref is referring to the second $ref which has itself been replaced. Highly confusing why it skipped the first $ref. [A]

Also, what happens to allOf, oneOf, anyOf ?

I have the following structure:

request1:
     $ref : path1
. . .
request2:
     allOf:
          - $ref : path1
          - schema for another structure
. . .
request3:
     $ref : path1/some_field_inside_this_schema

Because of the issue explained in [A],

request1:
     $ref : #/request2/allOf/0
. . .
request2:
     allOf:
          - full replaced schema
          - schema for another structure
. . .
request3:
     $ref : #/request1/field_inside_schema

In the end, I cannot dereference the $ref in request3.

How is this going to work?

juliangp commented 4 years ago

We have the same issues here, the first occurence of a reference get's embedded whereas I would like it to be created as a separate model schema and have all references point to this schema. The current approach breaks our code generation tools and means that we can not use the bundle command.

rdccosmo commented 4 years ago

Maybe I am a bit late, but I would also appreciate if you could share your solution to this, @marcelstoer. Specifically when you said you were able to "redefine" $RefParser.prototype.bundle using pretty much all existing code but providing my own remap() function.

marcelstoer commented 4 years ago

@lehphyro @rdccosmo here's a gist: https://gist.github.com/marcelstoer/750739c6e3b357872f953469ac7dd7ad Take note of the caveats in its comments section. As stated in earlier comments above I needed all external definitions, parameters, responses, etc. to be pulled into the bundled file as internal objects of the same type and to point $refs to the new internal object.

rdccosmo commented 4 years ago

thank you @marcelstoer. I'm still reading and experimenting with and trying to understand your function. If I am getting it right, if I want to stop getting $refs with indirections (~1) I should put the model I want to reference in a shared.yaml and reference the model throught it, eg. $ref: './shared.yaml#/definitions/MyModel'?

gnongsiej commented 4 years ago

I have also written my own code to fix this. It is huge and cumbersome but it does work for my requirements. Now, it dereferences the first $ref to the full structure. And every subsequent $ref to the same object is replaced by a $ref to the first $ref which has been expanded.

francismijares commented 4 years ago

can you please share your solution @gnongsiej

marcelstoer commented 3 years ago

To anyone commenting here: seems Stoplight/@philsturgeon recently joined forces with this project and contributed changes to json-schema-ref-parser. I didn't test whether the behavior discussed here is still the same.

philsturgeon commented 3 years ago

@marcelstoer we have written functionality that creates the sort of bundle you want, it's built into the whole Stoplight ecosystem: Platform, Explorer, Studio, etc.

Whilst we do help maintain a few repos for APIDevTools, most of our focus has been on json-schema-ref-parser as we use it, but swagger-parser not so much. I'm doing what I can, but not actively developing complex functionality.

Anyway, the functionality for tidy bundling is hidden in a fork we quickly put together as we were struggling to get changes upstream at the time, and I've asked the developers to either a) document the functionality in the fork, or b) merge the changes upstream. You can keep track that over here. https://github.com/stoplightio/json-schema-ref-parser/issues/27

pauldraper commented 3 years ago

@philsturgeon I couldn't figure out how to get the fork to work. I added to yarn resolutions, but it seems that swagger-cli bundle has the same behavior?

pauldraper commented 3 years ago

The obvious -- if annoying -- workaround is to put components first, and reference everything there.

Example input **example.yml** ```yml openapi: "3.0.3" components: # reference everything here schemas: a: { $ref: "./schema/a.yml" } b: { $ref: "./schema/b.yml" } info: title: Example version: 0.0.0 paths: /a: get: responses: "200": content: application/json: schema: { $ref: "./schema/a.yml" } description: Success /b: get: responses: "200": content: application/json: schema: { $ref: "./schema/b.yml" } description: Success ``` **schema/a.yml** ```yml properties: aName: { type: string } b: { $ref: "./b.yml" } ``` **schema/b.yml** ```yml properties: bName: { type: string } ```
Example output ```sh swagger-cli bundle example.yml ``` yields ```yml openapi: 3.0.3 components: schemas: a: properties: aName: type: string b: $ref: '#/components/schemas/b' b: properties: bName: type: string info: title: Example version: 0.0.0 paths: /a: get: responses: '200': content: application/json: schema: $ref: '#/components/schemas/a' description: Success /b: get: responses: '200': content: application/json: schema: $ref: '#/components/schemas/b' description: Success ```
gnongsiej commented 2 years ago

can you please share your solution @gnongsiej

@francismijares : Apologies. I do not check this profile as often as I would like.

Also, since I did write this solution in a professional capacity, I am not at liberty to share the exact solution. But if you give me some time, I can write a blog post about the algorithm I used.

ChuckJonas commented 2 years ago

I think you could do some preprocessing before bundling to setup @pauldraper' workaround so you don't have to do it manually...

It would be amazing if the bundle method had an option to do this for you.

philsturgeon commented 1 year ago

Hey, the bundle method in Redocly looks like this, which I'm pretty sure is what you're looking for?

swagger: '2.0'
info:
  version: '1.0'
  title: test API
paths:
  /foo:
    get:
      summary: Find foos
      parameters:
        - $ref: '#/parameters/Page'
        - $ref: '#/parameters/PageSize'
        - $ref: '#/parameters/Sort'
        - name: topic
          in: query
          description: topic
          required: false
          type: string
      responses:
        '200':
          description: OK
          schema:
            type: array
            items:
              $ref: '#/definitions/Foo'
        '400':
          $ref: '#/responses/clientErrorMessage'
        '500':
          $ref: '#/responses/internalErrorMessage'
  /bar:
    get:
      summary: Returns all bars
      responses:
        '200':
          description: OK
          schema:
            $ref: '#/definitions/Bar'
        '400':
          $ref: '#/responses/clientErrorMessage'
        '500':
          $ref: '#/responses/internalErrorMessage'
definitions:
  Bar:
    type: object
    required:
      - channel
    properties:
      channel:
        type: string
  Foo:
    type: object
    required:
      - id
    properties:
      id:
        type: integer
        format: int64
  ClientError:
    type: object
    properties:
      message:
        type: string
        example: Client error
      code:
        type: integer
        example: 400
  InternalError:
    type: object
    properties:
      message:
        type: string
        example: Internal error
      code:
        type: integer
        example: 500
parameters:
  Page:
    name: page
    in: query
    minimum: 1
    type: integer
    default: 1
    required: false
  PageSize:
    name: pageSize
    in: query
    minimum: 1
    maximum: 100
    type: integer
    default: 10
    required: false
  Sort:
    name: sort
    in: query
    type: string
    enum:
      - id
      - name
    default: id
    required: false
responses:
  clientErrorMessage:
    description: yadayada
    schema:
      $ref: '#/definitions/ClientError'
  internalErrorMessage:
    description: yadayada
    schema:
      $ref: '#/definitions/InternalError'

If so, go grab redocly-cli and we'll let swagger-parser die in peace.

pauldraper commented 1 year ago

Agreed, I recently switched to redocly-cli and it works beautifully.