Sum type ambiguities lead to incorrect or unclear validations #138
Description
The json schema contains many anyOfs, which cause failures in a subkind of a type to continue up to the next sibling kind. Concrete examples:
ClassDeclarationvalidates with invalidCustomElementDeclarationmembers- an invalid
ClassDeclarationwill be validated againstFunctionDeclarationin aModule'sdeclarationsfield.
given
{
"schemaVersion": "2.1.0",
"modules": [
{
"kind": "javascript-module",
"path": "invalid-attribute.js",
"declarations": [
{
"kind": "class",
"name": "InvalidAttribute",
"tagName": "invalid-attribute",
"customElement": true,
"attributes": [
{
"summary": "nameless attribute"
}
]
}
]
}
]
}ajv --strict=false \
-s ~/.cache/cem/schemas/2.1.0.json \
-d cmd/fixture/invalid-attribute/custom-elements.jsonExpected: fails because Attribute (only possible on a CustomElementDeclaration) must have a name field.
Actual:
cmd/fixture/invalid-attribute/custom-elements.json valid
The reason this validates is because declarations[0] is first tried as a custom element, but because the attribute doesn't have a name, the validator continues to try it as a class declaration, and that passes. So there are two problems here
- a
"kind": "class"can validate as a ClassDeclaration, even if it contains forbidden CustomElementDeclaration subkinds. This is the essential problem - a
"kind": "class"that has"customElement": trueis sometimes not validated as a CustomElementDeclaration. it should always be validated as such
Related to #134, but if this can be solved without changing the schema implementation, it should be. I'm sure this isn't the only such example.
The solution I found was to replace "anyOf" with "if/then" for our sum types. This patch should hopefully fix all such ambiguities. In my testing at least, it does the trick. If this proposal is accepted, I'll work to update the build tooling to prefer if/then to anyOf for typescript discriminated unions.
diff --git a/2.1.0.json b/2.2.0.json
index d9e45b7..4823a82 100644
--- a/2.1.0.json
+++ b/2.2.0.json
@@ -63,14 +63,34 @@
},
"members": {
"items": {
- "anyOf": [
- {
- "$ref": "#/definitions/ClassField"
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "field"
+ }
},
- {
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
+ "$ref": "#/definitions/ClassField"
+ },
+ "else": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "method"
+ }
+ },
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
"$ref": "#/definitions/ClassMethod"
}
- ]
+ }
},
"type": "array"
},
@@ -750,26 +770,106 @@
"declarations": {
"description": "The declarations of a module.\n\nFor documentation purposes, all declarations that are reachable from\nexports should be described here. Ie, functions and objects that may be\nproperties of exported objects, or passed as arguments to functions.",
"items": {
- "anyOf": [
- {
- "$ref": "#/definitions/ClassDeclaration"
+ "if": {
+ "properties": {
+ "customElement": {
+ "const": true
+ }
},
- {
- "$ref": "#/definitions/FunctionDeclaration"
+ "required": [
+ "customElement"
+ ]
+ },
+ "then": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "class"
+ }
+ },
+ "required": [
+ "kind"
+ ]
},
- {
- "$ref": "#/definitions/MixinDeclaration"
+ "then": {
+ "$ref": "#/definitions/CustomElementDeclaration"
},
- {
- "$ref": "#/definitions/VariableDeclaration"
+ "else": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "mixin"
+ }
+ },
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
+ "$ref": "#/definitions/CustomElementMixinDeclaration"
+ }
+ }
+ },
+ "else": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "class"
+ }
+ },
+ "required": [
+ "kind"
+ ]
},
- {
- "$ref": "#/definitions/CustomElementDeclaration"
+ "then": {
+ "$ref": "#/definitions/ClassDeclaration"
},
- {
- "$ref": "#/definitions/CustomElementMixinDeclaration"
+ "else": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "function"
+ }
+ },
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
+ "$ref": "#/definitions/FunctionDeclaration"
+ },
+ "else": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "mixin"
+ }
+ },
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
+ "$ref": "#/definitions/MixinDeclaration"
+ },
+ "else": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "variable"
+ }
+ },
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
+ "$ref": "#/definitions/VariableDeclaration"
+ }
+ }
+ }
}
- ]
+ }
},
"type": "array"
},
@@ -787,14 +887,34 @@
"exports": {
"description": "The exports of a module. This includes JavaScript exports and\ncustom element definitions.",
"items": {
- "anyOf": [
- {
- "$ref": "#/definitions/JavaScriptExport"
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "js"
+ }
},
- {
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
+ "$ref": "#/definitions/JavaScriptExport"
+ },
+ "else": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "custom-element-definition"
+ }
+ },
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
"$ref": "#/definitions/CustomElementExport"
}
- ]
+ }
},
"type": "array"
},