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

Final review and corrections for $dynamicAnchor/$dynamicRef before release #1030

Closed
jdesrosiers opened this issue Nov 24, 2020 · 5 comments
Closed
Assignees
Milestone

Comments

@jdesrosiers
Copy link
Member

I'm doing a final review of $dynamicAnchor/$dynamicRef before release and I think there are a couple places that need some attention.

https://github.com/json-schema-org/json-schema-spec/blob/master/jsonschema-core.xml#L658-L670

Or should this be the schema object at which processing
begins, even if it is not a root? This has some implications
for the case where "$dynamicAnchor" is only allowed in the
root schema but processing begins in a subschema.

This cref doesn't make sense any more. $dynamicAnchor is not "only allowed in the root schema" as it was with $recursiveRef. I think that makes this cref moot and can just be removed, but I'd like a second opinion.

https://github.com/json-schema-org/json-schema-spec/blob/master/jsonschema-core.xml#L3597
"$dynamicAnchor": node => $dynamicAnchor": "node"

https://github.com/json-schema-org/json-schema-spec/blob/master/jsonschema-core.xml#L1487-L1501

If the initially resolved starting point URI includes a fragment that
was created by the "$dynamicAnchor" keyword, the initial URI MUST be
replaced by the URI (including the fragment) for the outermost schema
resource in the dynamic scope that defines
an identically named fragment with "$dynamicAnchor".

First of all, I apologize profusely for not noticing and bringing this up until only a few days before planed release.

Requiring the "initially resolved starting point" to include a $dynamicAnchor doesn't make sense for non-recursive use-cases. I'll follow up with an example later. There's a cref describing why this requirement was included, but it doesn't make sense to me.

Requiring both the initial and final URI fragment to be defined
by "$dynamicAnchor" ensures that the more common "$anchor"
never unexpectedly changes the dynamic resolution process
due to a naming conflict across resources. Users of
"$dynamicAnchor" are expected to be aware of the possibility
of such name collisions, while users of "$anchor" are not.

I don't see how an $anchor can affect dynamic scope resolution. It certainly can't the way I've implemented this feature, so I'm not sure what I'm missing. If someone can explain the potential problem here, maybe we can compare the pros and cons of dropping this requirement so non-recursive use-cases make sense.

I also wouldn't mind an explanation of why the two step resolution process exists. This all seems more complicated than it needs to be. I don't see why it's not simply: $ref resolves against lexical scope and $dynamicRef resolves against dynamic scope where the dynamic scope is marked by the outermost $dynamicAnchor that matches the $dynamicRef. I thought that's what we had decided on, and again I apologize for not noticing until now that this had gotten written up differently than I thought it had.

@Relequestual Relequestual self-assigned this Nov 24, 2020
@jdesrosiers
Copy link
Member Author

Here's an example of a non-recursive use for $dynamicAnchor/$dynamicRef where it doesn't make sense to require a $dynamicAnchor at the initially resolved starting point.

Hyper-schemers will recognize this pattern. Let's say I have several resources and for each resource, I want two schemas: one for a single instance and another for a list of instances. The list of instances follows a common pattern that looks something like this.

{
  "$id": "https://json-schema.hyperjump.io/schema/list",
  "$schema": "https://json-schema.org/draft/2019-09/schema",

  "type": "object",
  "properties": {
    "list": { "type": "array" },
    "nextPage": { "type": "integer" },
    "previousPage": { "type": "integer" },
    "perPage": { "type": "integer" },
    "page": { "type": "integer" }
  }
}

I can then compose this schema with another to create a list schema for a specific resource.

{
  "$id": "https://json-schema.hyperjump.io/schema/foo-list",
  "$schema": "https://json-schema.org/draft/2019-09/schema",

  "$ref": "/schema/list",

  "properties": {
    "list": {
      "items": { "$ref": "/schema/foo" }
    }
  }
}

That's not too bad, but it does couple this schema with the "list" property name. If I decided I want to change that name to "itemList", I would have to change not only "/schema/list", but also every schema I composed it with. That's where $dynamicAnchor/$dynamicRef can help.

You can put the following example in https://json-schema.hyperjump.io and see it in action. (Note: you have to use draft "future" to specify the new unreleased draft)

{
  "$id": "https://json-schema.hyperjump.io/schema/foo-list",
  "$schema": "https://json-schema.org/draft/future/schema",

  "$ref": "/schema/list",

  "$defs": {
    "foo": {
      "$dynamicAnchor": "list",
      "$ref": "/schema/foo"
    }
  }
}
{
  "$id": "https://json-schema.hyperjump.io/schema/list",
  "$schema": "https://json-schema.org/draft/future/schema",

  "type": "object",
  "properties": {
    "list": {
      "type": "array",
      "items": { "$dynamicRef": "#list" }
    },
    "nextPage": { "type": "integer" },
    "previousPage": { "type": "integer" },
    "perPage": { "type": "integer" },
    "page": { "type": "integer" }
  }
}
{
  "$id": "https://json-schema.hyperjump.io/schema/foo",
  "$schema": "https://json-schema.org/draft/future/schema",

  "type": "object",
  "properties": {
    "foo": { "type": "string" }
  }
}

It's a little awkward, but it's decoupled. I can now change whatever I want about the /schema/list schema and not have to change any of my /schema/*-list schemas.

According to the way the spec is written, /schema/list should have a $dynamicAnchor somewhere if it's $dynamicRef is allowed to resolve against the dynamic scope. But, there's nowhere it makes sense to add a $dynamicAnchor in this case. You could add it in a definition.

"$defs": {
  "list": { "$dynamicAnchor": "list" }
}

But that's just putting the $dynamicAnchor somewhere it won't hurt anything just because it has to be somewhere. It's not needed to get the functionality we wanted. It also makes it impossible to make /schema/list abstract in the sense that it can't be used by itself. It can only be used when combined with a schema that sets "$dynamicAnchor": "list".

@jdesrosiers
Copy link
Member Author

I think I figured out why the two-step resolution stuff exists. If an implementation resolves $dynamicRefs when a schema is loaded and during runtime there is no matching $dynamicAnchor for a $dynamicRef, then the implementation can skip a URI resolution when following that $dynamicRef. I may still be missing something, but that's all the two-step resolution process is for, then IMO it's not worth complicating the spec for. It's a very minor optimization for something that would only have benefit for a misuse of $dynamicRef (they should have just used $ref). Implementations can still make this optimization if they want to without us specifying the extra step. It doesn't change the overall behavior, so it ends up being an implementation detail.

@Relequestual
Copy link
Member

Relequestual commented Nov 27, 2020

I find this part of the spec VERY hard to reason about.

According to the way the spec is written, /schema/list should have a $dynamicAnchor somewhere if it's $dynamicRef is allowed to resolve against the dynamic scope. But, there's nowhere it makes sense to add a $dynamicAnchor in this case. You could add it in a definition.

Ah, yeah, I see that.

I don't see how an $anchor can affect dynamic scope resolution. It certainly can't the way I've implemented this feature, so I'm not sure what I'm missing. If someone can explain the potential problem here, maybe we can compare the pros and cons of dropping this requirement so non-recursive use-cases make sense.

I also wouldn't mind an explanation of why the two step resolution process exists. This all seems more complicated than it needs to be. I don't see why it's not simply: $ref resolves against lexical scope and $dynamicRef resolves against dynamic scope where the dynamic scope is marked by the outermost $dynamicAnchor that matches the $dynamicRef. I thought that's what we had decided on, and again I apologize for not noticing until now that this had gotten written up differently than I thought it had.

I think I'd totally missed this also.
I'd have to dig back through issues and PRs, which I WILL be doing when writing docs anyway!

And in response to your comment just above...

I have a feeling we ended up trying to make it simple and with a specific limited use case, and wait and see if and how people use it and later WANT to use it.

@Relequestual
Copy link
Member

emoving from milestone for reasons as detailed: #1033 (comment)

@jdesrosiers
Copy link
Member Author

I'm going to close this and re-file it as a change proposal for the next draft.

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

No branches or pull requests

2 participants