test for confusing not-identifiers in enums

[karenetheridge]

A naive implementation would add all $id or $anchors to its list of known
resources, without checking if they are actually in subschemas or the value of
a keyword like enum, const or examples.

[jdesrosiers]

Looking at the $anchor tests, I don't think the second test has any chance of catching bugs. It doesn't match the enum, so it will always fail validation (pass the test) even if the implementation interprets the $anchor as a real identifier.

My implementation works by removing $anchor keywords from the schema when it per-processes the schema and storing them separately. So, a test that has a chance of tripping up an implementation like mine would be { "type": "null" }. It would match the enum and then where #faux_anchor resolves would determine whether the test passes or not.

Another thing to consider is that my implementation processes $anchors in the order they appear in the schema, so the real anchor would overwrite the not-an-anchor and the test would pass. Although the ordering of properties in an array is undefined, I think we will be more likely to catch bugs if we order the $defs in a way that is most likely to make the not-an-anchor be chosen over the real anchor. By a large margin, the most common ordering to expect from parsers is to preserve the ordering from the schema. Therefore, I suggest reordering to $defs. It won't catch all problems, but I think that configuration would be the most likely to catch bugs.

[jdesrosiers]

I think the same applies to the $id tests, but I haven't thought about those deeply enough yet.

[karenetheridge]

I don't think the second test has any chance of catching bugs.

I'll take a look at rejigging it to make it more likely to be trippable.

Another thing to consider is that my implementation processes $anchors in the order they appear in the schema, so the real anchor would overwrite the not-an-anchor and the test would pass

I was thinking of implementations that worked the other way -- of recording the first anchor it saw, rather than the last. Perhaps there should be a second test with the reverse order, so both implementation choices have a chance of being caught?

[karenetheridge]

I made the following changes:

• renamed identifiers for accuracy
• changed one of the definitions so that a match would occur if $anchor or $id was stripped
• added a third definition using const, to allow for implementations that would incorrectly use either the first or the last definition as the location of the identifier (the middle definition is the only location of a real identifier)
• copied draft7 tests to draft6, draft4
• fixed $defs -> definitions for draft7, 6, 4

[jdesrosiers]

You can't just strip out $ids like you can with $anchors, so I don't see this catching bugs the same way it would with $anchor. What I do is replace the entire schema with an object that represents a reference and is not expressible in JSON. Because it's not expressible in JSON, we can't create a test that would catch that. I think we can drop this test for the "id.json" tests and just keep them for "anchor.json" tests.

Suggested change

	{
	"description": "in implementations that strip $id, this may match either of the $defs",
	"data": {
	"type": "null"
	},
	"valid": false
	},

[karenetheridge]

okay, I removed those tests for id/$id.

[jdesrosiers]

lgtm. 👍

[karenetheridge]

rebased and merged!

[gregsdennis]

Found running against my lib.

[gregsdennis]

Are these suppose to be $id? id was replaced by $id in draft 6.

[gregsdennis]

Here, too.