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

Detect unresolvable cirular $refs during prism startup #891

Closed
patrickp-at-work opened this issue Dec 10, 2019 · 29 comments · Fixed by #1271
Closed

Detect unresolvable cirular $refs during prism startup #891

patrickp-at-work opened this issue Dec 10, 2019 · 29 comments · Fixed by #1271
Assignees
Labels
t/bug team/platinum-falcons Team Platinum Falcons
Milestone

Comments

@patrickp-at-work
Copy link

patrickp-at-work commented Dec 10, 2019

User story.
As an API mock tester, when I startup PRISM and my OpenAPI spec contains an unresolvable $ref, then I want to receive a notification, so that I can try to fix the problem without having to try to fire that request against the mock by myself.

Is your feature request related to a problem?
i have tried to get started with PRISM and I have a spec with DTO models externalized into a separate file. My spec is affected by bug #390.
If you experiment with the $refs, you need to try some calls before you know if it works or not.
[15:43:16] » [CLI] ► start Prism is listening on http://127.0.0.1:4010
(...) I'm firing a call manually with Postman (...)
[15:43:38] » [NEGOTIATOR] √ success Found a compatible content for */* (node:10040) UnhandledPromiseRejectionWarning: Error: Your schema contains $ref. You must provide specification in the third parameter.

Describe the solution you'd like
During startup, Prism tries to resolve the $refs and outputs a warning if calls use unresolvable $refs.

Additional context
Go with a "Fail fast" approach

@XVincentX
Copy link
Contributor

@patrickp-bosch Thanks for writing us. Would it be possible to get the specification file that's causing the problem or a minimal repro? Prism should indeed fail fast on the ref resolution and that particular error is caused by something else

@patrickp-at-work
Copy link
Author

patrickp-at-work commented Dec 13, 2019

Thanks for responding, @XVincentX . I am not allowed to disclose the whole API specification files. I have tried to create a minimal reproducible example. So far, I was not able to reproduce the problem with a minimized variant of the specification. Unfortunately, the resource that I try to fetch has plenty of $refs to other DTOs (especially because we must consider nested DTO references), so it would take a lot of time to try it out one by one. Now the next question is: Is it possible to reflect in the error output which particular $ref prism could not resolve?

@XVincentX
Copy link
Contributor

@patrickp-bosch — the thing is that such error is coming from an internal library that we do not control and it's not printing the unresolved reference; moreover, that library should never see references because we're solving them ahead of the time and we do report errors when some reference goes undefined for some reason (see getHttpOperations.ts, line 32).

Unfortunately I do not see any other way to help you unless you can share a reproducible example.

Any chance you've got a circular reference?

@albertpastrana
Copy link

Hey both. I'm having the same issue and I've seen my api docs generator is generating a weird circular reference.

@XVincentX you were asking about it because prism doesn't support recursive data types?

@XVincentX
Copy link
Contributor

I think it's related to some issues with our resolver for circular references, but I'd need a reference document or something to tackle down the issue :)

If you can share the offending document, that'd be just great!

@albertpastrana
Copy link

albertpastrana commented Dec 16, 2019

Sure! I've seen different flavours of the error, depending on the exact schema. This is the minimum schema I've seen to fail:

{
  "openapi" : "3.0.1",
  "info" : {
    "title" : "The API",
    "version" : "0.1"
  },
  "paths" : {
    "/endpoint" : {
      "get" : {
        "operationId" : "getEndpoint",
        "responses" : {
          "200" : {
            "description" : "",
            "content" : {
              "application/json" : {
                "schema" : {
                  "$ref" : "#/components/schemas/Recursive"
                }
              }
            }
          }
        }
      }
    }
  },
  "components" : {
    "schemas" : {
      "Recursive" : {
        "oneOf" : [
          {
            "$ref" : "#/components/schemas/Recursive1"
          },
          {
            "$ref" : "#/components/schemas/Recursive2"
          }
        ]
      },
      "Recursive1" : {
        "allOf" : [
          {
            "$ref" : "#/components/schemas/Recursive"
          },
          {
            "required" : [
              "prop1"
            ],
            "type" : "object",
            "properties" : {
              "prop1" : {
                "type" : "string"
              }
            }
          }
        ]
      },
      "Recursive2" : {
        "allOf" : [
          {
            "$ref" : "#/components/schemas/Recursive"
          },
          {
            "required" : [
              "prop2"
            ],
            "type" : "object",
            "properties" : {
              "prop2" : {
                "type" : "string"
              },
              "index" : {
                "type" : "string"
              }
            }
          }
        ]
      }
    }
  }
}

Also, during my tests I've seen (or at least I think I've seen ;-)) it working when generating it dynamically but failing without the -d.

Thanks for your help.

Btw, the schema is autogenerated using https://github.com/softwaremill/tapir

Sharing the scala case classes that generate this schema too:

  sealed trait Recursive
  case class Recursive2(prop2: String) extends Recursive
  case class Recursive1(prop1: String) extends Recursive

Oh! Btw, you've already seen the case classes are not recursive, but I kept the name as I was trying to figure out the minimum test case that would generate the error :-D

@XVincentX
Copy link
Contributor

Ok in case of circular references — our ref resolver will effectively "give up" after a certain amount of iterations since it would otherwise end up in a infinite document and we do not "lazily" evaluate the document (which is a way to handle circular references).

I'm going to keep this issue open but I would consider this kind of an edge case; but I'll bring this up internally in Stoplight and see if we're going to come up with something else. Thanks!

@XVincentX XVincentX changed the title Detect unresolvable $refs during prism startup Detect unresolvable cirular $refs during prism startup Dec 16, 2019
@albertpastrana
Copy link

Thanks for looking into it @XVincentX.
Just wanted to add that in our codebase this is not an edge case but something pretty common when we deal with ADTs, so it'd be great if this can be fixed.
In case you don't have plans to fix it, I'd really appreciate if you can tell us, as this may be a blocker in our adoption to prism and we may need to look for an alternative.
In any case, thanks a lot for open sourcing it, it's great and it works really well in the other scenarios :)

@wackykid

This comment has been minimized.

@XVincentX
Copy link
Contributor

XVincentX commented Dec 17, 2019

Hey y'all,

let me give you some context here!

First of all, I understand that this feature is annoying to a lot people (way more than I initially thought, to be honest) — and we clearly want to fix that.

The major problem we have is that — Prism has been designed with the foreword of receiving a resolved document to operate with.

With circular references, this is clearly impossible, since the resulting document would be essentially infinite. In this regard, our ref-resolver is correctly stopping when it detects a circular references — but Prism is not prepared to handle these. This is a shortcoming in the design phase. We didn't see it coming.

Now — at the moment we're not carrying any reference information alongside the whole Prism pipeline, and changing this is going to require some efforts that I cannot quantify at the moment. I am going to try to escalate this and see if I can prioritise this, but I can't really give you any guarantees at the moment.

// @philsturgeon

@philsturgeon
Copy link
Contributor

@wackykid we don't appreciate that kind of tone around here. You can speak to us professionally and respectfully, or you can leave. We are working with others to resolve this issue, and @XVincentX has already said he is asking around internally to see what we can do about this, so let's keep the conversation constructive and on track.

@philsturgeon
Copy link
Contributor

@albertpastrana thank you for the recreatable example! @patrickp-bosch does this seem representative of your situation also?

I remember we fixed one recursive situation, where User would have an "owner" and that owner would be a User. The person who designed that schema did not put nullable in there, so we had an issue that it would just reference forever until some memory limit or recursion limit in node was hit. User > User > User > User > User > User > User > User > User > User > User > User > User > User > User > User > User > User > User > User > User > User > User > User > User > User > ....

We fixed that by suggesting they added nullable because what they were doing was pretty much abusing the spec, and I would not expect any tooling to handle that.

This situation seems a little different though, and it might be something our resolver needs help figuring out. @MikeRalphson mind if I pull you into this? I know you have faced all sorts of recursive nightmares and won, have you faced something like this with oas-kit?

@philsturgeon
Copy link
Contributor

@XVincentX I think we can do something with our resolver to handle these more advanced ref situations for the entire ecosystem, but in the meantime can we do something with prism to detect a "remaining $ref"? Like, a workaround to say: oh we noticed a $ref is floating around here, better throw that away because it will cause trouble?

@patrickp-at-work
Copy link
Author

patrickp-at-work commented Dec 18, 2019

@patrickp-bosch does this seem representative of your situation also?

@philsturgeon So far, I did not find any obvious recursion. I could only check again in 2020, after the seasonal holidays.

The major problem we have is that — Prism has been designed with the foreword of receiving a resolved document to operate with.

@XVincentX I got an advice from a colleague: In my stoplight studio desktop, I have right-clicked the original, unchanged oas2.yaml file and selected "export OpenAPI". As per our understanding, this creates what you call a resolved document because the result combines the path declarations and the DTO model declarations. The result is some 29 000 lines, which is a lot, but not infinite. I feed this into prism mock. Again, it fails with the same error message. This tells me that my issue is not caused (at least not exclusively caused) by our externalized DTO file.

Addendum: Very happy holidays to all of you. I appreciate your support.

@albertpastrana
Copy link

Hi all. Thanks a lot for looking into this.
For now, I've been able to remove the allOf reference from the children and this has allowed us to use prism.
In the future, I'm not sure if we'll face a similar scenario but for now the workaround is good enough.
Again, thanks for open sourcing it, we've already saved so many time using it :-D

@philsturgeon
Copy link
Contributor

@patrickp-bosch the same resolver is used for both "Export OpenAPI" and for reading files into Prism. It should create roughly the same thing, but the outcome could be different. The resolver could give up and leave a $ref in place, and your OpenAPI document would still appear fine, but Prism might choke on that $ref.

Could you Ctrl + F through that 29k line file and see if there are any $ref's left in place?

@patrickp-at-work
Copy link
Author

I did, and I found that in fact, the generated output has one DTO $ref which is left unresolved due to a recursive structure that nobody had noticed so far. Thanks for helping to identify this! Best wishes for 2020.

@philsturgeon
Copy link
Contributor

@patrickp-bosch no worries! We'll keep digging in on the prism side, to see if we can detect these and bail out gracefully.

To summarize:

  • Hard circular references (e.g: not nullable and therefore A > A > A > A > A > A > A > A > infinity) should simply be avoided
  • We need to make sure we don't punish users who make a mistake
  • Nice clear error message if possible

@patrickp-at-work
Copy link
Author

patrickp-at-work commented Jan 7, 2020

Yes, your summary is correct from my point of view, and the improvement would be very helpful for many users.

@ghost
Copy link

ghost commented Feb 4, 2020

We have run into a similiar issue except of a child ref that points to self as describe above we have a array of child refs that point to itself. I tried making the array nullable but i dont think thats valid swagger.

I assume its the same problem since its a hard circular references just multiple of them each time.

Is there any update on this?

@XVincentX
Copy link
Contributor

Unfortunately I haven't been able to double down on this yet.

@XVincentX
Copy link
Contributor

Just a quick update — we have been able to identify a very promising library to resolve the references and apparently it works just great with the circular ones. I'll keep you posted.

@philsturgeon
Copy link
Contributor

An update for folks, we switched to json-schema-ref-parser, which is functionally better at resolving various situations.

We are sending them a few more PRs to bring it up to feature parity with the old json-ref-resolver, then we can migrate Prism over to using it.

@vintprox
Copy link

vintprox commented Apr 30, 2020

Would like to bump, because of inability to simultaneously retain recursive $ref to same model and run "Try It" in Stoplight Studio.
image

How do I work around this?

@philsturgeon
Copy link
Contributor

Aww man, bumping me on an issue I updated yesterday? 😂

If you can gimme your OpenAPI I bet we can sort it out. As you can see from above most folks have discovered a wonky aspect to their OpenAPI through this issue and have been able to resolve it, I'm sure we can do the same for you.

@vintprox
Copy link

vintprox commented Apr 30, 2020

If you can gimme your OpenAPI I bet we can sort it out. As you can see from above most folks have discovered a wonky aspect to their OpenAPI through this issue and have been able to resolve it, I'm sure we can do the same for you.

I found rather simplistic workaround: just don't use self-$ref as array item, leaving empty object. So, preview seems to be fine with it (not that I am 100% satisfied). The point is: if I provide good description for array, like "Child tasks with this same model", that should be enough for the client admin. =)

Aww man, bumping me on an issue I updated yesterday? 😂

Ahha, indeed! Half-year no commit project - "I'll... probably look into it... later"; 1 day no comment on open issue - "Workers of the world, unite!" 😂

Well, anyway, Stoplight Studio is the best documentation suite for REST API I've seen so far 👍 I even look into possibility to move one from obscuring code with a lot of javadocs and examples to starting their APIs via Stoplight.

@XVincentX XVincentX added the team/platinum-falcons Team Platinum Falcons label Jun 17, 2020
@XVincentX XVincentX self-assigned this Jun 17, 2020
@chohmann chohmann added this to the 2020-06-24 milestone Jun 23, 2020
@XVincentX
Copy link
Contributor

XVincentX commented Jul 1, 2020

After switching to the new resolver, we are now able to detect circular references that we can't handle, and refuse to start Prism. #1271 closes this.

image

We could still do a better job (there are some situations where we can use a circular reference such as validating the input payload) but this is something we will hopefully deal with in the future.

@black-snow
Copy link

black-snow commented Sep 22, 2020

I cross-read the issue and I'm wondering what to do now. I have circular refs in my spec and I need a mock server. My spec has an example for the case - why doesn't the server just use this? :|
Can someone please point me into the right direction? @XVincentX @philsturgeon

Neither the example on the component is used nor the full example for the application/json response. Just says Circular $ref pointer found at ... and exits.

/edit: My components:

    Field:
      title: A leaf-field containing a value
      type: object
      properties:
        id:
          type: string
        type:
          type: string
          enum:
            - PANEL
            - TEXT_AREA
            - SUBFORM
        label:
          $ref: '#/components/schemas/Label'
        showInMultiplePublicationDoc:
          type: boolean
        value:
          $ref: '#/components/schemas/Label'
    CircularField:
      title: A field with subfields
      type: object
      properties:
        id:
          type: string
        type:
          type: string
          enum:
            - PANEL
            - TEXT_AREA
            - SUBFORM
        label:
          $ref: '#/components/schemas/Label'
        showInMultiplePublicationDoc:
          type: boolean
        fields:
          type: array
          items:
            anyOf:
              - $ref: '#/components/schemas/CircularField'
              - $ref: '#/components/schemas/Field'
          example:
            - id: id
              type: PANEL
              label:
                defaultValue: def
                de: de def
              showInMultiplePublicationDoc: false
              value:
                defaultValue: def
                de: de def
      additionalProperties: false

Already had to copypasta id, type, label, showIn... due to some other tools having issues with extending a component.

@XVincentX
Copy link
Contributor

We haven't been able to really tame this down properly yet — the problem is that as much as we can support circular references internally, we will always ultimately fail to serialise them back in a payload.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t/bug team/platinum-falcons Team Platinum Falcons
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants