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

Recursive schema definitions result in infinite recursion #8

Closed
sw23 opened this issue Feb 28, 2020 · 10 comments · Fixed by #26
Closed

Recursive schema definitions result in infinite recursion #8

sw23 opened this issue Feb 28, 2020 · 10 comments · Fixed by #26
Labels
bug Something isn't working

Comments

@sw23
Copy link
Contributor

sw23 commented Feb 28, 2020

Currently if you have a schema with a recursive definition, the code will spin on the recursion until hitting the max recursion limit. Here's an example of a recursive schema definition:

"person": {
  "type": "object",
  "properties": {
    "children": {
      "type": "array",
      "items": {
        "$ref": "#/definitions/person"
      }
    }
  }
}

I worked around this by hacking the code to stop resolving references after a certain depth, which is a fairly simple solution. I think a better solution would be to catch when schemas reference a parent element, and to create an anchor link to the parent. That would prevent duplicate content as well.

Any thoughts?

@dblanchette
Copy link
Collaborator

I like your suggestion! Actually, we wanted to do anchor links for a while and never got around to it, so that would be very nice.

Let me know if you intend to work on a PR, I may do something on my side otherwise.

@sw23
Copy link
Contributor Author

sw23 commented Mar 17, 2020

Ok cool! I don't have a PR planned as of now. I'll let you know if I circle back to it at some point

@dblanchette dblanchette added the bug Something isn't working label Apr 22, 2020
@mauvilsa
Copy link

mauvilsa commented May 13, 2020

@dblanchette @sw23 this is a very nice project. I am eager to start using it. However almost all the json schemas that I write have recursions, so this issue blocks me from using it. Just wondering if you are working on this or if there is some plan for it.

@dblanchette
Copy link
Collaborator

Hi @mauvilsa,

I was able to fix part of the issue.

chrome_bkMFT4AaQK

From the schema:

{
  "$id": "https://example.com/person.schema.json",
  "$schema": "http://json-schema.org/draft-07/schema#",
  "title": "Person",
  "type": "object",
  "properties": {
    "person": {
      "$ref": "#/definitions/person"
    }
  },
  "definitions": {
    "person": {
      "type": "object",
      "description": "A human being",
      "properties": {
        "children": {
          "type": "array",
          "description": "The children they had",
          "items": {
            "$ref": "#/definitions/person"
          }
        }
      }
    }
  }
}

However, this does not work for all schema, here is what I get for another one:
chrome_Byq5QgHmCl

The other schema:

{
  "$id": "https://example.com/person.schema.json",
  "$schema": "http://json-schema.org/draft-07/schema#",
  "title": "Person",
  "type": "object",
  "properties": {
    "a_list_of_people": {
      "type": "array",
      "description": "A list of people",
      "items": {
        "$ref": "#/definitions/person"
      }
    }
  },
  "definitions": {
    "person": {
      "type": "object",
      "description": "A human being",
      "properties": {
        "children": {
          "type": "array",
          "description": "The children they had",
          "items": {
            "$ref": "#/definitions/person"
          }
        }
      }
    }
  }
}

Not everything has anchor links currently, only the properties and "tabs" (allOf and oneOf). In the example, "person" does not have an anchor link because it is the content of an array.

I realize that mentioning the path of the property in the schema is of little help, so I will keep looking for a solution.

I can release this partial solution if you think it has some value.

@mauvilsa
Copy link

Hi @dblanchette. This partial fix is in master or a branch? No need to release a partial solution, I can try with my schemas cloning the repo. Better to release when there is a more general solution.

@dblanchette
Copy link
Collaborator

@mauvilsa You can see PR #26 for a more complete fix.

Both examples from my last comment now works and clicking on the anchor link to the referenced section will make it flash so that users may see what exactly is being referenced.

I invite you to try it out and I would be glad to have your comments on it.

@mauvilsa
Copy link

mauvilsa commented May 16, 2020

@dblanchette I have tried it with one of the schemas I am currently working on. Running generate-schema-doc works well with the recursion. However, the result does not show the details of the recursion item, but guess that the problem is different than the one discussed in this issue. There are other things that don't seem to be correct. In the next few days I will look at it in detail and create new issues for them.

You can find the schema that I tested on at https://github.com/omni-us/narchi/blob/master/schema/narchi_schema.json and also the generated html at https://omni-us.github.io/narchi/schema.html. The problem is that blocks is an array of block items. But the definition of block is not shown.

@dblanchette
Copy link
Collaborator

@mauvilsa Thanks for the real life example, it really helps me.

I found some issues:

  • The code would only check if the reference is a parent of the current element and not if it was already documented elsewhere. Looking in a global variable for everything already documented fixes that
  • There was a bug when checking for the parent where definitions/block would be "in" definitions/blocks for example. Fixed that
  • Cross-file references did not work, fixed that by keeping the absolute path to the file in the global variable for used references (not in your example)

I fixed those and pushed to the PR branch (#26). However, it is not complete, tests will need to be adjusted.

Also, we do not support the if, then and else keywords yet, so your schema still looks incomplete.

Here is what I have now:
chrome_8g6Ynlwjum

@dblanchette
Copy link
Collaborator

Released in version 0.10!

@dblanchette
Copy link
Collaborator

@mauvilsa Please have a look at #28 to have better support for the schema you provided. I added support for if-then-else keywords and listing required properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants