json-schema-org / JSON-Schema-Test-Suite

A language agnostic test suite for the JSON Schema specifications
MIT License
625 stars 209 forks source link

add tests for $dynamicRef skipping over resources #721

Closed gregsdennis closed 7 months ago

gregsdennis commented 9 months ago

Resolves #719

gregsdennis commented 9 months ago

I've run these against my implementation, and they pass.

Julian commented 9 months ago

I haven't yet made time to understand the discussion in #719 to form an opinion, but is this ready for a PR?

Specifically it sounded like Ben and at one point yourself were saying part of this behavior is undefined in the spec. Is it or is it not? If it is, we should not test it -- and if it is I think Ben should confirm he agrees, but just sending a PR seems to ignore that comment.

That aside, until I actually understand what the discussion is about, is this example really minimized? It seems unlikely that's the case, no? Specifically whatever the subject of discussion is, it doesn't have anything to do with properties does it? Does a schema like:

{
  "$ref": "item",
  "$defs": {
    "bar": {
      "$id": "https://example.com/bar",
      "type": "array",
      "items": { "$ref": "item" },
      "$defs": {
        "item": {
          "$id": "item",
          "type": "object",
          "properties": {
            "content": { "$dynamicRef": "#content" }
          },
          "$defs": {
            "defaultContent": {
              "$dynamicAnchor": "content",
              "type": "integer"
            }
          }
        },
        "content": {
          "$dynamicAnchor": "content",
          "type": "string"
        }
      }
    }
  }
}

not raise whatever same question as that issue? An even better one presumably would remove the "content" property and just end up validating a single value, though whether that's feasible under the question in #719 I don't know yet -- but a nice middle if not would be replacing the types with consts.

I'll try to understand what's happening there, and obviously things aren't blocked on me so if others feel this is specified by the spec and a minimal, good test, feel free to LGTM.

jdesrosiers commented 9 months ago

For the required tests in this PR ($ref: "items"), I don't think there's any ambiguity. I don't see how the spec could be interpreted to include bar in the dynamic scope. However, I don't think the spec is clear for the $ref: "bar#/$defs/items" tests, which is why they were put in "optional".

Let's definitely make sure we're all on the same page first, but this PR is exactly where I think this should end up (although, yes, it could be simplified a little).

gregsdennis commented 9 months ago

Happy to accept simplifications, but this is the smallest I could come up with.

karenetheridge commented 9 months ago

This PR fails in my implementation due to a duplicate resource uri https://example.com/bar.

gregsdennis commented 9 months ago

@karenetheridge try again, please.

karenetheridge commented 9 months ago

much better; thanks!

notEthan commented 9 months ago

I feel like we agreed on the other issue this is the most sensible/intuitive thing to do, but I don't think the specification actually says this or specifies any particular handling of this case.