Fix $ref with siblings in pre-2019-09 tests

[jdesrosiers]

This is something that has been bothering me for a while. I have some time right now, so let's get this fixed. Before the nature of $ref was changed in draft 2019-09, any properties with an object with a $ref in it were members of the reference where they had no meaning rather than being part of the schema and interpreted as keywords. We have this test ...

{
  "definitions": {
    "a": {"type": "integer"},
    "b": {"$ref": "#/definitions/a"},
    "c": {"$ref": "#/definitions/b"}
  },
  "$ref": "#/definitions/c"
}

In this case, definitions and everything it contains should be considered part of the reference and ignored. It should not be something the $ref can reference, because it's not part of the schema.

This works in a lot of implementations. I think that's wrong, but the behavior in this case is, in the most generous interpretation, is undefined. If a test like this should be kept around at all, it should be with the "optional" tests. I wouldn't even include it in "optional" because it's at best a highly discouraged pattern and I don't think the test suite should encourage it by including an example.

[Julian]

I disagree with your categorization (I think this interpretation is more correct and obvious than the other), but I agree it's undefined, so lgtm!

(I also disagree with the test suite not including bad examples -- the test suite isn't a style guide, if something is meant to be implemented it should be here even if we discourage it, because implementations are not schema authors, and they should do what the spec says even if someone shouldn't be doing what they're doing. Lots of disagree but yes I suppose today :D -- regardless thanks, this is helpful!)

[jdesrosiers]

I know not everyone is going to agree with my interpretation. I'm ok with that as long as we can agree that it's undefined.

I agree that if it's valid behavior, it should have a test no matter how bad the style. But, it's not valid behavior, it's undefined behavior.

[Julian]

Yep all good, agree!

[notEthan]

previously:
#197
#198

#129

[jdesrosiers]

@notEthan, thanks for linking those issues. I knew this wasn't the first time this has come up, but I didn't know it had come to PR, let alone two PRs.

From my reading of those issues, it was agreed that the behavior is not well enough defined. However, the decision was to do nothing and fix it in the next draft. That conclusion doesn't make sense to me because fixing it in the next draft doesn't address the problem with the previous draft. The behavior is still undefined in draft-04/06/-07 no matter what comes after.

As it turned out, the next draft (2019-09) didn't even end up deciding on a behavior. Instead the nature of $ref was changed in a way that this problem no longer existed. Two drafts later, there still is no decision on what this behavior should be and there won't be because $ref went in a different direction.

[Julian]

That conclusion doesn't make sense to me because fixing it in the next draft doesn't address the problem with the previous draft. The behavior is still undefined in draft-04/06/-07 no matter what comes after.

That conclusion meant simply that the existing tests were sufficiently likely, and in the test suite sufficiently long, that implementations likely have simply followed them for years.

You're making your interpretation seem overly likely :)

My agreement above was mostly "I'm tired of talking about this, and all you're proposing is moving these away or removing them or whatever" -- but the conclusion we came to to me made sense -- that yes fine this is less clear than it should be, but the tests have been here for 6 years+ or whatever -- given that they are not obviously wrong and that the spec can be read that they're correct, that they should have stayed.

[jdesrosiers]

That conclusion meant simply that the existing tests were sufficiently likely
You're making your interpretation seem overly likely :)

Ahh, now I understand what that meant. There is an assumption that there is a correct answer and each interpretation has a likelihood of being correct. That's not the way I see it. If a behavior is not sufficiently defined, any interpretation that doesn't contradict the spec is not wrong. Likelihood has nothing to do with anything when there is no correct answer.

[Julian]

To be clear here I'm good with whatever resolution here but let's not just leave it undecided again, so probably if someone else wants to weigh in we should go with whatever consensus.

[jdesrosiers]

I don't think it's necessary to decide on an interpretation. I think it's fine to just say it wasn't sufficiently defined in the spec, so use this pattern at your own risk. If you want to use this pattern and be sure it will work, you can upgrade to a more current draft that doesn't have this problem.

[karenetheridge]

I agree that if it's valid behavior, it should have a test no matter how bad the style. But, it's not valid behavior, it's undefined behavior.

I agree with this and the changes made in this PR.

[Julian]

Sounds good... Merge away!

…

On Mon, Mar 8, 2021, 16:17 Karen Etheridge ***@***.***> wrote: I agree that if it's valid behavior, it should have a test no matter how bad the style. But, it's not valid behavior, it's undefined behavior. I agree with this and the changes made in this PR. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#458 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACQQXXJ5WJFZSWZLORJD23TCU5GTANCNFSM4YT7ICUQ> .

[ssilverman]

👍