Constrain types so that only valid inputs match the schema

[javagl]

Note: For now, this refers to a specific case in the EXT_mesh_features extension, but I think it is appropriate to bring it up here in the 3D Tiles repo, as all this is part of 3D Tiles Next, and we also might want to review other schemas for similar patterns. I noticed this recently during another schema cleanup PR. it might not be crucially important, and the question of whether it will be tackled depends on whether it could be solved with reasonable effort, but ... this issue is just to keep track of it:

---

The primitive.EXT_mesh_features.schema.json defines an array of feature IDs, which may be oneOf featureIdAttribute or featureIdTexture. Thes types already constrain the matching objects, so that only valid objects validate against the schema. For example, featureIdAttribute already makes sure that the object either contains an attribute, or a offset/repeat, but not both (via this clause)

But there are further constraints when these types are combined in the oneOf. Currently, an object like

{
  "featureIds": [
    {
      "index": 0,
      "propertyTable": 0,
      "channel": 0,
      "offset": 0,
    }
  ]
  ...
}

validates against the relevant part of the schema, even though it is both, a featureIdAttribute and a featureIdTexture.

(The tl;dr version is: oneOf means "any one of", and not "exactly one of"....)

---

It might be possible to solve this, by defining a valid feature ID as something like

"oneOf": [ 
    allOf [ { "$ref": "featureIdAttribute.schema.json" }, not { "$ref": "featureIdTexture.schema.sjon"   } ],
    allOf [ { "$ref": "featureIdTexture.schema.json" },   not { "$ref": "featureIdAttribute.schema.sjon" } ]
]

but that looks a bit clumsy...

---

(If someone wants to do first, quick tests: I assembled the relevant parts of the schema in CesiumGS/glTF#28 (comment) so that it can be copied+pasted into https://www.jsonschemavalidator.net/ )

[ptrgags]

@javagl at least as of JSON Schema 2020-12, oneOf is an XOR, not an OR.

I'll have to check in a schema validator like you mentioned to see how it behaves in practice

[javagl]

As far as it is easily possibe to trace back history, oneOf should always have been an XOR. I tried out the linked snippet with three online validators, and they all reported the input to be valid (which should not be the case if it was truly an XOR). It might well be that the validators are broken, though. It might also be that this whole issue is not crucially important, depending on how much of the validation should be covered by plain schema checks, and how much should be done explicitly with a validator.

[ptrgags]

Hm, I tried this in yet another validator (the coc-json lang server for coc.nvim) and I get different results:

Line 4 (the XOR works?): it considers the mixed-schema invalid as I would expect

Line 15 (the surprise):

Somehow the "oneOf" doesn't like the fact that "channel" is required... I'm going to experiment a bit more and check the docs for "required" again to try to make sense of this.

[ptrgags]

Oh I see what's the problem in my case,

{"index": 0, "texCoord": 0, "channel": 0} is a valid featureIdTexture, yes, but it's also technically a valid featureIdAttribute because it's defined as oneOf.not.required: ["attribute"] which means anything that doesn't have an attribute field

I'm wondering if it would help to split out an implicitFeatureId.schema.json, then each specific sub-schema has additionalProperties: false. I'm going to try this locally (and then with an online schema validator to make sure the idea works)

[ptrgags]

Okay this works. I'm using additionalProperties: false on each sub-schema. (which may be debatable, though given that we have an extensions property, I think this can be justified).

Combined schema for use with https://www.jsonschemavalidator.net/ (the three schemas in the oneOf clause would be separate files). That site supports only up to draft 2020-09.

(Note, I didn't include the textureInfo.schema.json here, but it's not relevant to the problem here.

{
  "$schema": "https://json-schema.org/draft/2020-09/schema",
  "properties": {
    "featureIds": {
      "type": "array",
      "items": {
        "oneOf": [
          {
            "$schema": "https://json-schema.org/draft/2020-12/schema",
            "$id": "featureIdAttribute.schema.json",
            "title": "Feature ID Attribute",
            "type": "object",
            "description": "Feature IDs to be used as indices to property arrays in the property table.",
            "properties": {
              "attribute": {
                "type": "integer",
                "minimum": 0
              },
              "extensions": {},
              "extras": {}
            },
            "required": ["attribute"],
            "additionalProperties": false
          },
          {
            "$id": "implicitFeatureIdAttribute.schema.json",
            "title": "Implicit Feature ID Attribute",
            "type": "object",
            "properties": {
              "offset": {
                "type": "integer",
                "minimum": 0,
                "default": 0
              },
              "repeat": {
                "type": "integer",
                "minimum": 1
              },
              "extensions": {},
              "extras": {}
            },
            "additionalProperties": false
          },
          {
            "$schema": "https://json-schema.org/draft/2020-12/schema",
            "$id": "featureIdTexture.schema.json",
            "title": "Feature ID Texture",
            "type": "object",
            "properties": {
              "index": {},
              "texCoord": {},
              "channel": {
                "type": "integer",
                "minimum": 0
              },
              "extensions": {},
              "extras": {}
            },
            "required": [
              "channel"
            ],
            "additionalProperties": false
          }
        ]
      }
    }
  }
}

Test JSON to validate (first 3 objects are valid feature IDs, the last two are invalid)

{
  "featureIds": [
    {
      "offset": 0,
      "repeat": 1
    },
    {
      "attribute": 3
    },
    {
      "index": 0,
      "texCoord": 0,
      "channel": 0
    },
    {
      "index": 0,
      "propertyTable": 0,
      "channel": 0,
      "offset": 0,
      "repeat": 1
    },
    {
      "attribute": 3,
      "offset": 4
    }
  ]
}

@javagl @donmccurdy any thoughts on this approach? additionalProperties: false can be rather restrictive, but I think here it helps since you have 3 objects that need to be distinguished.

[javagl]

There are a few technicalities to go through, but I think that additionalProperties: false may prevent us from using the "pesudo-(but-not-really)-inheritance" of JSON-Schema to replace the extensions with a reference to the extensions.schema.json in the future (c.f. #495 ).

[donmccurdy]

I'm not sure I understand the details related to extensions.schema.json, but if we think that schema validation might fail on current or future 'valid' schemas, unfortunately that would probably be a blocker. If that's not the case, then I do like @ptrgags's suggestion here!

[ptrgags]

We talked about this on a call, the additionalProperties: false is too restrictive to be future-proof.

However, I also have another idea to fix this. If we change the implicit feature ID schema from:

"not": {
  "required": ["attribute"]
}

to:

"anyOf": [
  {"required": ["offset"]},
  {"required": ["repeat"]}
],
// maybe with the not.required = ["attribute"]? 

I'll experiment with this a bit and see what works.

[ptrgags]

I tried that, but to ensure no mixed cases, you need to list all the fields across all three types of feature ID. I don't like this approach:

{
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "$id": "featureIdAttribute.schema.json",
  "title": "Feature ID Attribute",
  "type": "object",
  "description": "Feature IDs to be used as indices to property arrays in the property table.",
  "properties": {
    "attribute": {
      "type": "integer",
      "minimum": 0
    },
    "offset": {
      "type": "integer",
      "minimum": 0,
      "default": 0
    },
    "repeat": {
      "type": "integer",
      "minimum": 1
    },
    "extensions": {},
    "extras": {}
  },
  "oneOf": [
    {
      "required": ["attribute"],
      "not": {
        "anyOf": [
          {
            "required": ["offset"]
          },
          {
            "required": ["repeat"]
          },
          {
            "required": ["channel"]
          },
          {
            "required": ["index"]
          },
          {
            "required": ["texCoord"]
          }
        ]
      }
    },
    {
      "not": {
        "anyOf": [
          {
            "required": ["attribute"]
          },
          {
            "required": ["channel"]
          },
          {
            "required": ["index"]
          },
          {
            "required": ["texCoord"]
          }
        ]
      },
      "anyOf": [
        {
          "required": ["offset"]
        },
        {
          "required": ["repeat"]
        }
      ]
    }
  ]
}

Another thought I had, what if it's like boundingVolume where each variety has a distinct property name?

featureIds: [
  {
    attribute: 0
  },
  {
    // or perhaps "range" instead of implicit?
    implicit: { constant: 3, repeat: 4 }
  },
  {
    texture: { channel: 0, texCoord: 0 }
  }
]

[donmccurdy]

but to ensure no mixed cases, you need to list all the fields ...

I don't think we need to guarantee that all mixed cases fail validation — adding more indirection to the spec to make the schema validation easier doesn't feel like the right solution to me, unless it's necessary for things like code generation from the schema. There are lots of disallowed combinations of JSON properties that can't be caught by validating against the schema alone, such as:

"primitives": [{
  "mode": 0, // POINTS
  "indices": 0, // disallowed for POINTS
  "attributes": { ... }
}]

If your earlier suggestion is enough to be sure that no valid files will fail validation, let's go with that?

"anyOf": [
  {"required": ["offset"]},
  {"required": ["repeat"]}
],

[ptrgags]

@donmccurdy okay, I'll trim down the schema a bit then I'll put in a PR

[donmccurdy]

That said – putting something like {"type": "implicit", "offset": 0} into the feature ID object would be more idiomatic for glTF, and could also solve the validation issues here... 🤔

[ptrgags]

@donmccurdy you mention solving the validation issues, by that do you mean the schema just describes "type" as required and the other fields as optional? Then it's up to the application to determine which fields are required. Which I've certainly seen done before, but it feels perhaps messy here?

Also how would you handle the difference between feature IDs on a primitive and feature IDs on a node for instancing? (which don't use the textures) Disallow the type: "texture" value in that circumstance but it's still part of the schema?

[javagl]

(I haven't caught up with this one yet, but just stumbled over the point that the EXT_mesh_features spec currently says

Empty feature IDs (e.g. {}) are disallowed

but as I said during the call: Some things will only be caught by a validator. Iff the attempt to squeeze "to much knowledge" into the schema causes the schema to become overly complicated, then this may be something to be covered in a different way...)

[donmccurdy]

@ptrgags in your first example from #508 (comment), suppose we omitted "additionalProperties": false. Then we were concerned that the schema was ambiguous about mixed cases. Adding"type": {"const": "X"} might make them unambiguous (see example below).

{
  "$schema": "https://json-schema.org/draft/2020-09/schema",
  "properties": {
    "featureIds": {
      "type": "array",
      "items": {
        "oneOf": [
          {
            "$schema": "https://json-schema.org/draft/2020-12/schema",
            "$id": "featureIdAttribute.schema.json",
            "title": "Feature ID Attribute",
            "type": "object",
            "description": "Feature IDs to be used as indices to property arrays in the property table.",
            "properties": {
              "type": {"const": "attribute"}, // ⚠️
              "attribute": {
                "type": "integer",
                "minimum": 0
              },
              "extensions": {},
              "extras": {}
            },
            "required": ["type", "attribute"]
          },
          {
            "$id": "implicitFeatureIdAttribute.schema.json",
            "title": "Implicit Feature ID Attribute",
            "type": "object",
            "properties": {
              "type": {"const": "implicit"}, // ⚠️
              "offset": {
                "type": "integer",
                "minimum": 0,
                "default": 0
              },
              "repeat": {
                "type": "integer",
                "minimum": 1
              },
              "extensions": {},
              "extras": {}
            },
            "required": ["type"]
          },
          {
            "$schema": "https://json-schema.org/draft/2020-12/schema",
            "$id": "featureIdTexture.schema.json",
            "title": "Feature ID Texture",
            "type": "object",
            "properties": {
              "type": {"const": "texture"}, // ⚠️
              "index": {},
              "texCoord": {},
              "channel": {
                "type": "integer",
                "minimum": 0
              },
              "extensions": {},
              "extras": {}
            },
            "required": ["type", "channel"]
          }
        ]
      }
    }
  }
}

But as @javagl says, some issues can only be caught by a validator, and that's OK. Unless we think a type property actually benefits implementations (not just schema validation), let's leave it out.

[lilleyse]

I believe this is OBE after the recent schema updates in CesiumGS/glTF#56