Skip to content

add tests for $dynamicAnchor in multiple branches of propertyDependencies #617

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

Merged

Conversation

gregsdennis
Copy link
Member

Resolves #615

@gregsdennis gregsdennis requested a review from a team as a code owner November 22, 2022 21:50
Copy link
Contributor

@handrews handrews left a comment

Choose a reason for hiding this comment

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

As noted in the issue, this scenario has undefined behavior. I'd be happier if it did not, but we have not been able to agree on such a change.

Copy link
Contributor

@handrews handrews left a comment

Choose a reason for hiding this comment

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

Never mind, as noted in the issue this was my mistake reading things too quickly.

Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

This should be in draft-next. propertyDependencies doesn't exist in 2020-12.

Also, the schema isn't right. The dynamic anchors in "east" and "west" need to be in a definition or something where it doesn't get evaluated. This should fix it so it does what you intended.

{
    "$schema": "https://json-schema.org/draft/next/schema",
    "$id": "http://localhost:1234/draft2020-12/dynamicanchor-in-propertydependencies.json",
    "$defs": {
        "inner": {
            "$id": "inner",
            "type": "object",
            "additionalProperties": {
                "$dynamicRef": "#foo"
            }
        }
    },
    "propertyDependencies": {
        "expectedTypes": {
            "strings": {
                "$id": "east",
                "$ref": "inner",
                "$defs": {
                    "foo": {
                        "$dynamicAnchor": "foo",
                        "type": "string"
                    }
                }
            },
            "integers": {
                "$id": "west",
                "$ref": "inner",
                "$defs": {
                    "foo": {
                        "$dynamicAnchor": "foo",
                        "type": "integer"
                    }
                }
            }
        }
    }
}

@gregsdennis
Copy link
Member Author

This should be in draft-next.

Yep. My bad. That's what I meant.

The dynamic anchors in "east" and "west" need to be in a definition or something where it doesn't get evaluated.

The goal of this test is to check whether the implementation can resolve to the right anchor. I don't see how burying the anchors inside $defs adds value.

east is only evaluated when expectedTypes is strings (similarly for west when expectedTypes is integers), so the argument that putting the anchor in a def prevents it from being evaluated doesn't hold.

Yes, the anchor and the associated $id are the same subschema, but that's irrelevant for this test.

@gregsdennis
Copy link
Member Author

🤔 @jdesrosiers I do see a problem with what I have that your approach solves: having $ref and $dynamicAnchor together would probably cause the $ref to re-evaluate, resulting in a loop, which should be rejected by the implementation.

I'll update.

@jdesrosiers
Copy link
Member

I'm not sure I see the loop. The problem I saw was much more fundamental than that. In order for the sub-schemas to apply, the instance must be an object, but the propertyDependencies sub-schemas assert that the instance is a string or integer depending on which sub-schema matches. Then the sub-schema that's asserting that the instance is a string or integer references "inner" which asserts that the instance is an object. These contradictions mean that the propertyDependencies sub-schemas are effectively false and the test cases don't correctly describe the schema's expected results, nor do they demonstrate dynamic behavior.

@gregsdennis gregsdennis merged commit c4c490f into main Nov 27, 2022
@gregsdennis gregsdennis deleted the gregsdennis/dynamicAnchor-inside-propertyDependencies branch November 27, 2022 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$dynamicAnchor inside propertyDependencies
3 participants