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

Crash with recursive definition in referenced OpenAPI file #132

Closed
kf6kjg opened this issue Mar 2, 2020 · 5 comments
Closed

Crash with recursive definition in referenced OpenAPI file #132

kf6kjg opened this issue Mar 2, 2020 · 5 comments
Labels

Comments

@kf6kjg
Copy link

kf6kjg commented Mar 2, 2020

The following is marked as valid by swagger-cli validate openapi.yaml using swagger-cli 3.0.1.
Tested using "exegesis-express": "^2.0.0", which is bringing in "exegesis": { "version": "2.5.1", on both Node.js 10 and 13 under Ubuntu 18.04.

# openapi.yaml
openapi: 3.0.1
info:
  title: Demo API
  version: 1.0.0
paths:
  /entry:
    $ref: "entry.yaml#/paths/Entry"
# entry.yaml
paths:
  Entry:
    x-exegesis-controller: ExampleService
    get:
      operationId: fetch
      responses:
        default:
          description: Recursive entry.
          content:
            application/json:
              schema:
                type: object
                properties:
                  entry:
                    $ref: "#/components/schemas/Recurse"
components:
  schemas:
    Recurse:
      type: object
      properties:
        recurse:
          $ref: "#/components/schemas/Recurse"

Of course my real spec files are much larger and more complex, I've pared them down drastically into a minimal set.

Expected

Assuming the declared service file exists, exports a thefetch method requested, etc. that he service should ready itself and launch.

What happens

An exception is thrown with the following message:

{"message":"can't resolve reference #/properties/value/roperties/entry from id #","missingRef":"#/properties/value/roperties/entry","missingSchema":""}

Setting a a breakpoint for uncaught exceptions lands me in ajs.js line 350:

 var v;
  try { v = compileSchema.call(this, schemaObj.schema, root, schemaObj.localRefs); }
  catch(e) {
    delete schemaObj.validate;
    throw e; // this is line 350.
  }

Notes

  1. If I inline the paths entry into the root file the crash is still reproduced. Example:
# openapi.yaml
openapi: 3.0.1
info:
  title: Demo API
  version: 1.0.0
paths:
  /entry:
    x-exegesis-controller: ExampleService
    get:
      operationId: fetch
      responses:
        default:
          description: Recursive entry.
          content:
            application/json:
              schema:
                type: object
                properties:
                  entry:
                    $ref: "entry.yaml#/components/schemas/Recurse"
# entry.yaml
components:
  schemas:
    Recurse:
      type: object
      properties:
        recurse:
          $ref: "#/components/schemas/Recurse"
  1. If I inline the second file into the first the crash does not happen, but makes maintenance on a large API like I'm working on a royal pain by comparison. Example:
# openapi.yaml
openapi: 3.0.1
info:
  title: Demo API
  version: 1.0.0
paths:
  /entry:
    x-exegesis-controller: ExampleService
    get:
      operationId: fetch
      responses:
        default:
          description: Recursive entry.
          content:
            application/json:
              schema:
                type: object
                properties:
                  entry:
                    $ref: "#/components/schemas/Recurse"
components:
  schemas:
    Recurse:
      type: object
      properties:
        recurse:
          $ref: "#/components/schemas/Recurse"
  1. If I simply break the recursion, say by replacing
        recurse:
          $ref: "#/components/schemas/Recurse"

with

        recurse:
          type: string

there is likewise no error.

From doing some digging in AJS's repo and issue tracker it looks like they support recursive definitions just fine, but that they often find that people aren't doing things right when feeding the library. Dunno if that's the case here, but from what I've read this doesn't appear to be an AJS problem, which leads me back here.

@kf6kjg
Copy link
Author

kf6kjg commented Mar 4, 2020

I've also validated the files with Redocly's openapi-cli, which did catch some other stuff in my real API definition that swagger-cli did not, but was totally fine with the architecture represented in the above minimal case that causes such issues in Exegesis.

@jwalton
Copy link
Contributor

jwalton commented Mar 4, 2020

I am very surprised this is breaking; I certainly have recursive defs similar to this in my OpenAPI def. I'm looking into this now.

I am extremely suspicious of:

reference #/properties/value/roperties/entry

Note the "roperties" in that path. We do this thing in exegesis where, when we construct a validator, we extract the schema out of OpenAPI and turn it into a plain JSON schema, but we also transform that schema so there's a root object that looks like {value: ...}. This is very handy in cases where, e.g., we're creating a validator for a parameter, and we want it to be a number. AJV will transform {value: '7'} into {value: 7} for us, but won't do the same if we just pass in the string '7'.

This line is obviously very suspicious here. -_-

@kf6kjg
Copy link
Author

kf6kjg commented Mar 4, 2020

As I noted if everything is in a single file then it works fine, see note 2. It's only when the recursive component is in a secondary file that this happens, e.g. note 1. So it's likely your similar OpenAPI def is all in one big file.

@jwalton
Copy link
Contributor

jwalton commented Mar 4, 2020

No, ours is in 139 files. :P

But this problem looks to basically be a case where I'm assuming my json-ptr is going to be in URI fragment format, when it isn't. Should be a straightforward fix.

@jwalton jwalton closed this as completed in 4992f95 Mar 4, 2020
@jwalton
Copy link
Contributor

jwalton commented Mar 4, 2020

🎉 This issue has been resolved in version 2.5.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

2 participants