webcomponents / webcomponents/custom-elements-manifest

Sum type ambiguities lead to incorrect or unclear validations

Open
#138 2 comments 1 reaction 0 assignees View on GitHub
Dominant language
TypeScript
Stars
502
Forks
27
PR merge metrics
No merged PRs in 30d

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:
- `ClassDeclaration` validates with invalid `CustomElementDeclaration` members
- an invalid `ClassDeclaration` will be validated against `FunctionDeclaration` in a `Module`'s `declarations` field.

given

```json
{
"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"
}
]
}
]
}
]
}
```
```bash
ajv --strict=false \
-s ~/.cache/cem/schemas/2.1.0.json \
-d cmd/fixture/invalid-attribute/custom-elements.json
```
**Expected**: 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

1. a `"kind": "class"` can validate as a ClassDeclaration, even if it contains forbidden CustomElementDeclaration subkinds. This is the essential problem
2. a `"kind": "class"` that has `"customElement": true` is 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.

```patch
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"
},
```

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.