Skip to content

Simplify process for $dynamicRef/$dynamicAnchor #1033

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

Closed

Conversation

jdesrosiers
Copy link
Member

This is an alternate specification for $dynamicRef/$dynamicAnchor based on my comments from #1030. It includes removing the two step resolve process and not requiring $dynamicAnchors at both ends of the dynamic scope.

I wanted to update the example as well, but I didn't have time. Since these keywords are not just for recursion any more, I think it will be easier to show how they work without recursion.

Copy link
Member

@Relequestual Relequestual left a comment

Choose a reason for hiding this comment

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

I THINK this is all OK.
It looks like there are only changes in language and phrasing, and no functional or requirement changes.
The only thing I notice is it now says the resolution of $dynamicRef cannot be done at load because "the process of evaluating an instance can change how the reference resolves".

@jdesrosiers Can you provide an example of where this would be the case? I thought the purpose of anchor-based referencing was that it would NOT be dependant on the instance.

@jdesrosiers
Copy link
Member Author

If the initially resolved starting point URI includes a fragment that was created by the "$dynamicAnchor" keyword,

That's the part that changed. This says that if the initially resolved starting point has a $dynamicAnchor, then you can resolve against the dynamic scope. The significant change is that I removed the "if". Now, $dynamicRef always resolve against the dynamic scope.

The only thing I notice is it now says the resolution of $dynamicRef cannot be done at load because "the process of evaluating an instance can change how the reference resolves".

Previously, it said that when a single schema is loaded, an implementation could initially resolve $dynamicRefs using the lexical context. Since I removed the concept of initial resolution, that didn't make sense any more. I replaced it with as near to the language used to describe $ref as possible. It's the same words used to describe dynamic scope, but you're right that it's wrong. In my rush to get this in, I reused those words without thinking about it deeply enough because I wasn't changing the concept of dynamic scope. It's not dependent on the instance, it's dependent on the other schemas that reference it. That should be fixed as well.

@Relequestual
Copy link
Member

I'm thinking on this a little longer because I'm pretty sure there are some things we're not considering here. I'll come back with some further thoughts.

@jdesrosiers
Copy link
Member Author

I took a closer look at the links.json schema with respect to this change and realized that this change allows the $dynamicRefs used in this schema to be simplified. A fragment-only URI is sufficient. We don't need the rest of the URI.

With the current spec, we only need the rest of the URI in order to satisfy the requirement that there is $dynamicAnchor at the initially resolved starting point. There's no place that the $dynamicAnchor makes sense in the links.json schema, so it has to use the non-fragment part of the URI to change the schema where it expects to find a $dynamicAnchor. That's a pretty confusing hoop to jump through just to satisfy a requirement that doesn't seem to have any benefit.

I also noticed that the links.json was structured oddly. I inlined unnecessary $defs to clean it up. I did this in a separate commit to make it easier to review.

@Relequestual
Copy link
Member

We won't be releasing a new HyperSchema meta-schema this time round (nor new HyperSchema spec), so I'd rather not include such changes in this PR.

@Relequestual
Copy link
Member

In trying to rebuff your PR and talking with other people, it's clear we're going to need more time than we have to conclude this if this is a good change or not.

The current system works and is not broken. The part of the PR which makes a requirements change is for convenience as opposed to fixes something, as I understand it.

As such, I think we can't proceed to put the change in 2020-11, especially this late in the game.

If we discuss for another few months and decide it's really important, we can publish a new draft, and advise implementers step over 2020-11.

@Relequestual
Copy link
Member

I'll try to extract the fixes you've made from the PR into another PR.

@gregsdennis
Copy link
Member

gregsdennis commented Nov 30, 2020

@jdesrosiers I see where you're coming from in your argument against bookending the dynamic scope with $dynamicAnchor. However if you consider the case where the outer scope doesn't define the anchor, then you run into problems of how to resolve the $dynamicRef.

Requiring the anchor to be a sibling of the ref ensures that there is always an anchor to resolve.

@jdesrosiers
Copy link
Member Author

@Relequestual

The current system works and is not broken.

We actually don't know that for sure. As far as I know, it hasn't been implemented and proven to work. This proposal has been implemented and proven to work where it needs to and more.

@gregsdennis

Requiring the anchor to be a sibling of the ref ensures that there is always an anchor to resolve.

But why is that a desirable property? $refs don't always resolve to something. I can't even think of a time I would want a $ref to fall back to resolving to another schema if the one I referenced isn't available. If a schema I referenced isn't available, I always want that to be an error. The same goes for $dynamicRef. We've now seen two real life examples of where bookending is problematic and I still see no benefits.

@handrews
Copy link
Contributor

Given the age of this, the lack of consensus, and the existence of issue #1064, I'm going to convert this to a draft PR.

@handrews handrews marked this pull request as draft May 12, 2021 16:19
@handrews handrews changed the base branch from master to draft-next May 19, 2021 19:08
@jdesrosiers jdesrosiers closed this Oct 7, 2021
@jdesrosiers jdesrosiers deleted the alternative-dynamic branch July 8, 2022 15:54
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.

4 participants