Should identifier declarations be respected in non-schema locations?

[jdesrosiers]

The following quote from the spec was brought up in a slack discussion.

From JSON Schema Core - Section 9.4.2

Similarly, a reference target under a known keyword, for which the value is known not to be a schema, results in undefined behavior in order to avoid burdening implementations with the need to detect such targets.

This brings into question a few of the tests in the suite. (There may be others. These are the ones I'm aware of.)

• https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/main/tests/draft2020-12/id.json#L211-L261
• https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/main/tests/draft2020-12/anchor.json#L84-L141
• https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/main/tests/draft2020-12/unknownKeyword.json

These tests require that identifier declarations ($id or $anchor) in locations not known to be schemas are ignored. However, the quoted passage above would seem to indicate that the behavior is undefined in this situation and therefore we should not have tests that require a behavior in this case.

Full Disclosure: My implementation doesn't pass these tests and I don't intended to fix it. Although the removal of these tests is in my best interest, I'm only interesting in getting to the bottom of what is required by the spec. I've been content to skip those tests and if we decide they actually do belong in the suite, I'll just continue skipping them.

[Julian]

Your reading sounds right to me seeing that line quoted. "Reference target" in this case is "the thing with $id in it" (not the schema with $ref in it, which is probably how I've read that paragraph un-carefully before) -- and the spec is saying "referencing that thing is undefined behavior", so it indeed would sound like to me they should go in optional unless someone points out what we're missing about that paragraph.

[gregsdennis]

I'm happy to put them in optional.

For the record, my implementation does pass these. (Not a boast, just a consequence of how I process schemas.)

[jdesrosiers]

@karenetheridge said in Slack,

I think the only change we could make is to remove those ref-to-nonschema tests entirely — leaving them in optional/ may be confusing, if it’s intepreted as “this is something you could implement but you don’t have to”.

I'm inclined to agree with Karen on this. Unless someone can point to something in the spec that indicates that this is expected-but-not-required behavior, rather than undefined behavior, then it probably doesn't belong in "optional" either.

[Julian]

Why would it appear in the spec? That isn't what optional means (c.f. of course the PR which clarifies what it means explicitly). Optional is any behavior that isn't required by the spec that an implementer may implement, and allows any implementers doing so to share the tests for such a thing rather than rewriting them themselves. These kinds of tests have been in the suite since literally day 1 when I wrote it, what possible benefit is there in removing them? There's no evidence anyone has been confused in the way indicated there, can you recall any case ever of someone showing up and saying "hey should I implement this, I see it in the folder"?

[gregsdennis]

This calls back to the idea that we need to split the tests according to requirement level. It sounds like a single "optional" is insufficient.

[jdesrosiers]

Fair enough. I've always ignored the optional tests, so I haven't kept up on what they're for. Sorry, I misunderstood the purpose.

[karenetheridge]

I don't think this is optional behaviour - I think we should (and we do, iirc) have non-optional tests that check that such identifiers are NOT recognized. Properties that look like identifiers in non-schemas are absolutely not keywords, they are just plain data.

[gregsdennis]

For reference, the conversation in Slack stemmed from #697.

Also from 9.4.2 (linked above):

Therefore, having a reference target in such an unrecognized structure cannot be reliably implemented, and the resulting behavior is undefined.

This is the sentence before the one that @jdesrosiers quoted. His pertains to known locations, and this pertains to unknown locations. Both indicate that the behavior is undefined.

Under @Julian's perspective that any undefined behavior that's actually implemented should be an optional test, I think this test specifically should be an optional test.

Other tests that only include direct references into unknown locations should be non-optional.

I think we should (and we do, iirc) have non-optional tests that check that such identifiers are NOT recognized.

I can't find any language in the spec that says implementations MUST NOT support this.

[mwadams]

This kind of optional test essentially defines an undefined behaviour for a particular implementation, for specific input of a kind we (sadly but inevitably) meet in the real world.

Is there any reason for not having the orthogonal optional tests that verify whether it does work in that particular implementation (in those specific circumstances)?

It feels like a set of tests that could live in their own folder in the "optional" hierarchy, and it allows consumers to track if "undefined" behaviour that they depend on for their real-world use case has changed in their implementation.

It also encourages implementers to understand when they are straying into the realms of the Undefined, and to understand the implications.

[Julian]

This kind of optional test essentially defines an undefined behaviour for a particular implementation, for specific input of a kind we (sadly but inevitably) meet in the real world.

(I'm not sure if by "particular implementation" you mean one specific concrete implementation of JSON Schema, i.e. mine, but I definitely don't think I'm the only one who has this behavior. Perhaps you mean one way of implementing JSON Schema, in which case I agree).

Is there any reason for not having the orthogonal optional tests that verify whether it does work in that particular implementation (in those specific circumstances)? It feels like a set of tests that could live in their own folder in the "optional" hierarchy, and it allows consumers to track if "undefined" behaviour that they depend on for their real-world use case has changed in their implementation.

That's essentially what #590 does, or sets us up for, which simply needs more people saying they do or don't understand where files are ending up.

[mwadams]

(I'm not sure if by "particular implementation" you mean one specific concrete implementation of JSON Schema, i.e. mine, but I definitely don't think I'm the only one who has this behavior. Perhaps you mean one way of implementing JSON Schema, in which case I agree).

That's exactly what I mean, yes.

That's essentially what #590 does, or sets us up for, which simply needs more people saying they do or don't understand where files are ending up.

And yes - that's exactly the "more granular" situation I was thinking of. I hadn't spotted #590.

[jdesrosiers]

Properties that look like identifiers in non-schemas are absolutely not keywords, they are just plain data.

Unfortunately, I think the spec is a little more complicated than that. I think Section 9.4.2 is acknowledging that any reference target could be interpreted as a schema, but the behavior is undefined for locations not known to be a schema.

{
  "anyOf": [
    { "$ref": "#/$defs/foo/const" },
    { "$ref": "/$defs/foo" }
  ],
  "$defs": {
    "foo": {
      "const": { "type": "string" }
    }
  }
}

This is a valid schema, but it contains some undefined behavior. When /$defs/foo is evaluated, the value of const is just data, but when /$defs/foo/const is evaluated, it could be interpreted as a schema. The "type" property in const is sometimes just data and sometimes a keyword depending on the context in which it's being evaluated. The behavior here is technically specified as undefined, but I think it's expected that this would work. I think the intention of section 9.4.2 is to recognize that things get messy when identifiers are introduced and that's why the behavior is left undefined.

{
  "$id": "https://example.com/schema",
  "$ref": "/foo",
  "$defs": {
    "foo": {
      "const": {
        "$id": "/foo",
        "type": "string"
      }
    }
  }
}

In the previous example, there was no ambiguity about the target of the reference, but in this case, it could be argued that the reference target doesn't exist because the $id in const isn't a keyword until a reference resolves to that location and the reference can't resolve to that location until $id is a keyword. It's a chicken and egg problem.

What do you all think? According the spec (not any particular implementation), does this case fall under the "undefined" behavior specified in 9.4.2, or must this be interpreted as a bad reference? My take is that this should be considered undefined behavior. I think it's reasonable for an implementation to consider this a bad reference, but it's also reasonable for an implementation to identify /$defs/foo/const/$id as a possible reference target (even though it's in const) and allow $ref to target that location as a schema.

[Julian]

I agree with all of your above analysis.

[jdesrosiers]

@gregsdennis

@karenetheridge would you then expect only the tests which have pointer $refs that go into non-schema locations be moved to optional? That would leave any tests which use $id-based $refs in required.

I'm not following the distinction you're making here. Can you give an example?

[gregsdennis]

The difference:

• A schema with only a pointer $ref to an unknown location (the first example in your previous comment) would be able to follow the pointer and interpret the value there as a schema. It could be argued that any implementation can follow the pointer, but whether it interprets the value as a schema is undefined.
• A schema with $id-based $refs to unknown locations (the second example in your previous comment) is indeterminable because the implementation isn't expected to look in those locations for identifiers.

The question now is what's optional vs forbidden.

Rereading the spec, I don't think it makes the above distinction (although they are two different cases). The spec simply mentions "reference targets."

Multi-level structures of unknown keywords are capable of introducing nested subschemas, which would be subject to the processing rules for "$id". Therefore, having a reference target in such an unrecognized structure cannot be reliably implemented, and the resulting behavior is undefined.

It could be argued that "reference target" here means "objects in unknown locations that have $id/$anchor," but that only serves to leave pointer-$refs unspecified for unknown locations.

To me, this text means that both of these cases are undefined and optional.

That said, the tests in #707 don't actually test any of this.

The test $anchor inside an enum is not a real identifier and its $id counterpart also define a valid $anchor/$id to resolve. The test would need to only have an $anchor/$id in an unknown location in order to test this behavior. But then the implementation could (correctly) error due to a failed resolution, which the test suite isn't set up to cover. Alternatively, with the tests as they are, an implementation could (correctly) error because multiple $anchors/$ids are registered with the same value. Again, the suite doesn't handle errors.

So as far as these tests are concerned, I think they just need to be removed.

[jdesrosiers]

So as far as these tests are concerned, I think they just need to be removed.

Given the way Julian has defined the purpose of "optional" tests, I think it's reasonable that these tests could be in "optional". However, I agree that the tests should be more focused and in their current state there's so many different outcomes that could be considered spec compliant that it's awkward to include tests just for one of those outcomes. I think the best course of action is to remove them for now and more focused tests can be added to "optional" later, but I'm ok with leaving them how they are as well as long as they are optional.

[jdesrosiers]

@karenetheridge, do you have any comments before we move back to the PR stage?

[karenetheridge]

According the spec (not any particular implementation), does this case fall under the "undefined" behavior specified in 9.4.2, or must this be interpreted as a bad reference? My take is that this should be considered undefined behavior. I think it's reasonable for an implementation to consider this a bad reference, but it's also reasonable for an implementation to identify /$defs/foo/const/$id as a possible reference target (even though it's in const) and allow $ref to target that location as a schema.

I disagree with this conclusion. It's not reasonable for an implementation to proactively examine non-schema locations for identifiers (as it must do for schema locations) and treat them as if they are actual schema locations. It could even be considered a security risk, if some tooling reads in data from external sources e.g. for populating "description" properties with markdown, in allowing what should be opaque data to hijack a namespace.

I think what we have now in the tests is mostly correct:

• things that look like identifiers, in opaque data e.g. "const", "enum", "examples" are not recognized as identifiers and cannot be addressed (therefore, a test can "redefine" an identifier inside such data and there be no errors during evaluation)
• attempting to $ref inside such a non-schema "might" work, or it might not, depending on what's there (I agree with the spec wording that says expecting implementations to detect and prevent such scenarios could pose a high burden)

We could benefit from another test alongside the existing optional tests:

• evaluate a $ref to a non-schema location, that contains an $id in it (that duplicates an existing known-good identifier)
• evaluate another $ref to the identifier in question, and confirm that it still goes to the original location and hasn't been overwritten to point to the new non-schema location

[gregsdennis]

things that look like identifiers, in opaque data e.g. "const", "enum", "examples" are not recognized as identifiers and cannot be addressed (therefore, a test can "redefine" an identifier inside such data and there be no errors during evaluation) - @karenetheridge

I think this is the sticking point for me and the tests as they are. Currently they're using a pointer $ref to get inside of those opaque values. I 100% agree that this is undefined behavior and should be optional.

However, placing an $id inside one of those opaque values and attempting to $ref directly to the $id MUST error, and the test suite isn't set up to check for errors, so this can't be tested currently. I think this scenario, specifically, is what 9.4.2 is talking about, not a pointer into a non-schema location.

[karenetheridge]

It sounds like we agree then.

I described a scenario where we could catch some errors, depending on how tooling is implemented -- after referencing a non-schema location that contains an $id, then evaluate a $ref to that location and confirm it still goes to the previously-defined location and not the new non-schema location (an implementation that treated such constructs as identifiers might throw a "duplicate identifier" error, or it might even overwrite the good location with a bad one and therefore the $ref will go to the wrong place).

[Relequestual]

To just confirm the consensus, it sounds like we're agreeing that the tests specifically for identifiers in non-schema locations MUST NOT be respected, and such must be a regular and non-optional test?

[jdesrosiers]

it's also reasonable for an implementation to identify /$defs/foo/const/$id as a possible reference target (even though it's in const) and allow $ref to target that location as a schema.

It's not reasonable for an implementation to proactively examine non-schema locations for identifiers (as it must do for schema locations) and treat them as if they are actual schema locations. It could even be considered a security risk, if some tooling reads in data from external sources e.g. for populating "description" properties with markdown, in allowing what should be opaque data to hijack a namespace.

There might be some confusion from my use of the word "reasonable". By "reasonable", I meant that the spec seems to allow it. Your response gives reason why it might be difficult to implement or not be a good idea, but we're not trying to decide what we think the behavior should be, we're trying to determine what the spec says about this behavior. What wording in the spec says that a reference to a $id in a non-schema MUST NOT be respected? 9.4.2 says,

Multi-level structures of unknown keywords are capable of introducing nested subschemas, which would be subject to the processing rules for "$id".

It goes on to say that such behavior is a burden to implement and therefore not required. I don't see anything that says it's not allowed. It seems pretty clear that 9.4.2 thinks that the spec expects $ids in non-schema locations to be respected if 9.4.2 didn't exist and 9.4.2 was included to relieve implementers of that perceived complexity.

Is there a way to interpret 9.4.2 that excludes references to $ids in non-schema locations? If not, is there something else in the spec that contradicts 9.4.2?

[gregsdennis]

I'm not sure why we're arguing about this.

The current tests don't need the bad $ids/$anchors since they're not referencing them. The tests aren't checking the thing they were created to check. Further, I don't think the test suite can handle such tests because it requires that evaluation results in an error.

{
  "$schema": "https://json-schema.org/draft/next/schema",
  "$defs": {
    "const_not_anchor": {
      "const": {
        "$anchor": "not_a_real_anchor"
      }
    }
  },
  "if": {
    "const": "skip not_a_real_anchor"
  },
  "then": true,
  "else": {
    "$ref": "#/$defs/const_not_anchor"
  }
}

This is testing that "$ref": "#/$defs/const_not_anchor" can be resolved and that the value there {"$anchor": "not_a_real_anchor"} is interpreted as a schema. The interpretation of this value as a schema is the undefined behavior. Thus this is an optional test.

That said "$anchor": "not_a_real_anchor" doesn't impact this test at all (because it's not being referenced directly), and only serves to confuse what the test is checking. A better test would be

{
  "$schema": "https://json-schema.org/draft/next/schema",
  "$defs": {
    "const_not_anchor": {
      "const": false
    }
  },
  "if": {
    "const": "skip ref"
  },
  "then": true,
  "else": {
    "$ref": "#/$defs/const_not_anchor"
  }
}

For an implementation that interprets the $ref target as a schema, this will pass for "skip ref" and fail for anything else.

For an implementation that doesn't interpret the $ref target as a schema because it's a known non-schema location, the $ref resolution will fail, producing an error.

---

If you want to test whether a $id/$anchor located in a non-schema location is a recognized identifier, you need to actively reference it. Something like

{
  "$schema": "https://json-schema.org/draft/next/schema",
  "$defs": {
    "const_not_anchor": {
      "const": {
        "$anchor": "not_a_real_anchor"
      }
    }
  },
  "if": {
    "const": "skip not_a_real_anchor"
  },
  "then": true,
  "else": {
    "$ref": "#not_a_real_anchor"
  }
}

For an implementation that recognizes "$anchor": "not_a_real_anchor" as an identifier, this will pass for all values.

For an implementation that doesn't recognize "$anchor": "not_a_real_anchor" as an identifier, this will error.

[jdesrosiers]

I'm not sure why we're arguing about this.

I'm not trying to argue. I'm trying refocus the discussion on what I think is the most important outcome of this discussion. To me the important outcome is to come to a consensus about what the spec says. What we do with the tests is just a consequence of that outcome.

I totally agree that that test doesn't make sense as it is and should be removed or improved, but I think this test does make sense and should be a required test if we decide identifiers in non-schemas MUST NOT be treated as valid reference targets. I believe this test is an example of the scenario Karen is talking about in this discussion. If we decide the behavior of identifiers in non-schemas is undefined, then several different results are possible and it belongs in "optional" or should be removed.

[gregsdennis]

if we decide identifiers in non-schemas MUST NOT be treated as valid reference targets.

The spec is very clear that this is undefined behavior.

As such, that's an optional test. I agree that implementations should code so that that test passes, but the spec doesn't require it. As the spec is worded, an implementation could

• process the schema as we expect it should, not being confused by the $id in the enum
• error due to a duplicate $id
• register one of the $ids, then overwrite it with the other one, and continue processing the schema.

I'd argue that an implementation should only do the first one, but I don't read that the spec requires it to.

[jdesrosiers]

That sounds like we're very much on the same page. I'm just confused because that seems to contradict what you and Karen were just arguing. I feel like I'm missing some nuance here and it's making hard for me to understand where people stand.

@karenetheridge, do you agree with Greg's assessment in the previous comment? If so, I think we have consensus to move the tests to optional.

[gregsdennis]

I may have said a thing and then performed actual research allowing me to form an informed opinion.

[karenetheridge]

we're not trying to decide what we think the behavior should be, we're trying to determine what the spec says about this behavior

That's not what I was doing. I was arguing for what we should allow. If the spec is vague and open to interpretation, I don't want the tests suggesting a specific behaviour for it, especially one opposite to how we think it should be.

I think the spec is clear enough about this, but that wasn't my main point in replying here. Quoting 9.4.2, emphasis mine:

Multi-level structures of unknown keywords are capable of introducing nested subschemas, which would be subject to the processing rules for "$id". Therefore, having a reference target in such an unrecognized structure cannot be reliably implemented, and the resulting behavior is undefined.

• I agree that the not_a_real_anchor test is no good, and should go. It doesn't test anything useful about the anchor, unlike the test added earlier for $id ("description": "$id inside an enum is not a real identifier", which checks that an $id property inside a non-schema doesn't interfere with a real identifier using that name). We could rewrite the test to DTRT, though.

FWIW: I think fixing this section of the spec to be less ambiguous should be on the shortlist of fixes for the next interim draft release (there is still one happening before another "major" release, right?)

[jdesrosiers]

If the spec is vague and open to interpretation, I don't want the tests suggesting a specific behaviour for it

Great. Sounds like we all agree with moving the tests to "optional".

I agree that the not_a_real_anchor test is no good, and should go.

Agreed. I'll update the PR to remove this one instead of moving it to "optional".

FWIW: I think fixing this section of the spec to be less ambiguous should be on the shortlist of fixes for the next interim draft release (there is still one happening before another "major" release, right?)

Unfortunately, spec work is on hold for the time being.