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

Refs not fixed up #51

Closed
1 task done
RomanHotsiy opened this issue Jan 24, 2018 · 9 comments
Closed
1 task done

Refs not fixed up #51

RomanHotsiy opened this issue Jan 24, 2018 · 9 comments
Assignees
Labels

Comments

@RomanHotsiy
Copy link

Not all the refs are fixed up after conversion. The internal ones (not to #/definitions) are not.
I checked your code and I'm not sure whether we can fix it efficiently but maybe you know how to do it.

Checklist

  • Conversion: I have checked my source definition is valid OpenAPI 2.0

Detailed Description

Input:

swagger: '2.0'
info:
  version: '1.0'
  title: Demo API
paths:
  "/test":
    get:
      responses:
        '200':
          description: OK
          schema:
            type: object
            properties:
              default:
                title: Check
                type: object
                properties:
                  message:
                    type: string
                    description: bla
              database:
                "$ref": "#/paths/~1test/get/responses/200/schema/properties/default"

Output with broken ref:

openapi: 3.0.0
servers: []
info:
  version: '1.0'
  title: Demo API
paths:
  "/test":
    get:
      responses:
        '200':
          description: OK
          content:
            "*/*":
              schema:
                type: object
                properties:
                  default:
                    title: Check
                    type: object
                    properties:
                      message:
                        type: string
                        description: bla
                  database:
                     # Broken ref below
                    "$ref": "#/paths/~1test/get/responses/200/schema/properties/default" 
@MikeRalphson
Copy link
Contributor

🤢

@MikeRalphson
Copy link
Contributor

That's one reason my current resolver outputs

openapi: 3.0.0
info:
  version: '1.0'
  title: Demo API
paths:
  /health/all.json:
    get:
      responses:
        '200':
          description: OK
          content:
            '*/*':
              schema:
                type: object
                properties:
                  default:
                    title: Check
                    type: object
                    properties:
                      message:
                        type: string
                        description: bla
                  database:
                    title: Check
                    type: object
                    properties:
                      message:
                        type: string
                        description: bla

However, I don't think I can currently recommend using the resolver in master (it's being rewritten as we speak).

cc @philsturgeon :

I agree with @RomanGotsiy, this is unlikely to be trivial. Workaround (everything can be solved by another level of indirection):

swagger: 2.0
info:
  version: '1.0'
  title: Demo API
paths:
  /health/all.json:
    get:
      responses:
        200:
          description: OK
          schema:
            $ref: '#/definitions/health'
definitions:
  health:
    $ref: definitions/health.yml#/Result

Ie. extract schemas to #/definitions and $ref from there.

I see two possible approaches:

  1. Track all schema objects with a source from the input and destination position in the converted output
  2. Automatically extract directly-referenced schemas into #/components/schemas first

Problems (respectively):

  1. Sounds a nightmare (see also requestBodies)
  2. Have to check the source or structure of what is being referenced to tell what kind of thing it is (I assume the same problem can happen with response headers). Plus "naming things is hard".

@RomanGotsiy does any other approach leap out at you?

@RomanHotsiy
Copy link
Author

@MikeRalphson thanks for looking into this.
The resolver I use (json-schema-ref-parser) has the ability to output the similar output as your resolver (it is called dereference while I'm using bundle).
Why I am not using it is because this is it dereferences all the refs (even internal ones) so I'm losing valuable info (e.g. discriminator won't work with full deref).

Nevertheless, this issue is not limited to resolver output. I think someone can write similar spec by hand (it's completely valid according to the spec I believe). So we have to fix it anyway.

The approach 1 looks simpler to me TBH. Let me stew on it a bit and I will try to send a PR on weekends.

@MikeRalphson
Copy link
Contributor

Nevertheless, this issue is not limited to resolver output. I think someone can write similar spec by hand (it's completely valid according to the spec I believe). So we have to fix it anyway.

Absolutely. I'm looking at approach 2 at the moment (its only around a dozen lines of code so far). I can always revert it if you come up with a better fix.

When do you next plan to cut a release of ReDoc, so we can coordinate?

@RomanHotsiy
Copy link
Author

RomanHotsiy commented Jan 24, 2018

I am going cut the next alpha today without this issue fixed. I will cut one more once this issue is fixed. I'm flexible with alpha releases 😄

@MikeRalphson
Copy link
Contributor

@philsturgeon is it possible to share (privately?) your full OAS v2 document so I have another testcase?

@MikeRalphson MikeRalphson self-assigned this Jan 24, 2018
MikeRalphson added a commit that referenced this issue Jan 24, 2018
MikeRalphson added a commit that referenced this issue Jan 24, 2018
@philsturgeon
Copy link

Done! Suggested the workaround for now.

@RomanHotsiy
Copy link
Author

@MikeRalphson well done 👍 😮!
Let me know once you publish a new release.

@MikeRalphson
Copy link
Contributor

Fixed using a $ref-to-type heuristic in v2.11.11 - it passes the test cases given, seems to work with the complex Azure definitions I'm trying to resolve, and doesn't appear to break anything else. Please chase if the heuristic needs tweaking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants