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

Error instead of segfault on recursive references in json schema #98

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

torymur
Copy link
Contributor

@torymur torymur commented Nov 26, 2024

Closes #96

@torymur torymur added the bug Something isn't working label Nov 26, 2024
Comment on lines +242 to +244
if ref_path == "#" {
return Err(anyhow!("Recursive references are not supported for now"));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this cover all cases of recursion (e.g. when the ref. path is not just #)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you're right, this is just the most obvious one, which doesn't take away support of internal referencing, which still could be used to create a recursion, like:

        {
            "type": "object",
            "properties": {
                "node": { "$ref": "#/definitions/node" }
            },
            "definitions": {
                "node": {
                    "type": "object",
                    "properties": {
                        "value": { "type": "integer" },
                        "next": { "$ref": "#/definitions/node" }
                    }
                }
            }
        }

I see these ways to proceed here:

  1. this partial fix
  2. turning off all referencing completely (well internal/local, external isn't supported right now), until we implement a recursive support as a feature

Because in order to keep internal referencing we'd need to do a full walk through to detect recursion, which is 99% of the recursive feature, I suppose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in order to keep internal referencing we'd need to do a full walk through to detect recursion, which is 99% of the recursive feature, I suppose.

This is the only approach that will actually address the problem, and I think we should add the recursive feature, so this would be my recommendation.

@rlouf?

Copy link
Member

@rlouf rlouf Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fully address the problem, I don't see any other viable choice here. Let's add the recursive feature while we're at it.

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 this pull request may close these issues.

Recursive JSON seg faults
3 participants