Skip to content

add tests for $dynamicRef skipping over resources #721

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
merged 5 commits into from
Apr 15, 2024

Conversation

gregsdennis
Copy link
Member

Resolves #719

@gregsdennis gregsdennis requested a review from a team as a code owner February 12, 2024 21:21
@gregsdennis
Copy link
Member Author

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

@Julian
Copy link
Member

Julian commented Feb 12, 2024

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
Copy link
Member

jdesrosiers commented Feb 13, 2024

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
Copy link
Member Author

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

@karenetheridge
Copy link
Member

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

@gregsdennis
Copy link
Member Author

@karenetheridge try again, please.

@karenetheridge
Copy link
Member

much better; thanks!

@notEthan
Copy link
Contributor

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.

@gregsdennis gregsdennis merged commit 64a3e7b into main Apr 15, 2024
@gregsdennis gregsdennis deleted the gregsdennis/dynamicref-skips-resources branch April 15, 2024 04:44
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.

Test for jumping over a resource
5 participants