Add meta schema: a JSON Schema which matches recap schemas #346

mjperrone · 2023-08-02T01:36:12Z

What

This adds a JSON Schema describing valid recap schemas.

Why

There is a robust ecosystem around JSON Schema that provides tools like validation, example generation, code generation, etc out of the schema definitions. Defining a JSON Schema definition for Recap will allow Recap and its users to more easily hook into that ecosystem.

Closes #342

How

I manually created the JSON Schema file by reading closely the Recap Spec.

Known limitations

As mentioned in the issue, I don't know of a way to require that the alias strings are referencing defined types elsewhere in the document, so some invalid documents will pass. No documents that are valid should fail.

Validation

In order to help iterate on the JSON Schema definition (which I expect isn't perfect), I test up some unit tests. I define a few valid recap schema documents and use jsonschema to test the documents against the schema definition.

Potential future work

Define a meta schema in recap (maybe using recap converter from the json schema)
flesh out more positive or negative examples
use the JSON Schema definition for something like editor integrated validation

…ng test

mjperrone · 2023-08-02T01:37:50Z

tests/spec/invalid_field_type.json

+    {
+    "name": "string_field",
+    "doc": "doc",
+    "type": "invalidtype",


I included this in the initial draft PR to show what the failing test UX is like:

E jsonschema.exceptions.ValidationError: {'name': 'string_field', 'doc': 'doc', 'type': 'invalidtype', 'bytes': 255} is not valid under any of the given schemas E E Failed validating 'oneOf' in schema['items']: E {'oneOf': [{'allOf': [{'$ref': '#/$defs/RecapNull'}, E {'$ref': '#/$defs/RecapCommonFields'}]}, E {'allOf': [{'$ref': '#/$defs/RecapBool'}, E {'$ref': '#/$defs/RecapCommonFields'}]}, E {'allOf': [{'$ref': '#/$defs/RecapInt'}, E {'$ref': '#/$defs/RecapCommonFields'}]}, E {'allOf': [{'$ref': '#/$defs/RecapFloat'}, E {'$ref': '#/$defs/RecapCommonFields'}]}, E {'allOf': [{'$ref': '#/$defs/RecapString'}, E {'$ref': '#/$defs/RecapCommonFields'}]}, E {'allOf': [{'$ref': '#/$defs/RecapBytes'}, E {'$ref': '#/$defs/RecapCommonFields'}]}, E {'allOf': [{'$ref': '#/$defs/RecapList'}, E {'$ref': '#/$defs/RecapCommonFields'}]}, E {'allOf': [{'$ref': '#/$defs/RecapMap'}, E {'$ref': '#/$defs/RecapCommonFields'}]}, E {'allOf': [{'$ref': '#/$defs/RecapStruct'}, E {'$ref': '#/$defs/RecapCommonFields'}]}, E {'allOf': [{'$ref': '#/$defs/RecapEnum'}, E {'$ref': '#/$defs/RecapCommonFields'}]}, E {'allOf': [{'$ref': '#/$defs/RecapUnion'}, E {'$ref': '#/$defs/RecapCommonFields'}]}]} E E On instance[0]: E {'bytes': 255, E 'doc': 'doc', E 'name': 'string_field', E 'type': 'invalidtype'} .venv/lib/python3.10/site-packages/jsonschema/validators.py:1302: ValidationError ==================================================================== short test summary info ===================================================================== FAILED tests/unit/meta_schema/test_json_schema.py::test_valid_schemas[tests/spec/invalid_field_type.json] - jsonschema.exceptions.ValidationError: {'name': 'string_field', 'doc': 'doc', 'type': 'invalidtype', 'bytes': 255} is not valid under any of the given schemas

Nice! Can you add functionality to let us ignore expected failed tests (i.e. xfail)? I think it complicates the logic in test_json_schema.py, but it'll be useful as we build out and validate the harness, I think. If it's a pain, though, we can hack it (e.g. .json.failing filename)

Updated the harness to find .unimplemented.invalid.json files and expect that they will throw as invalid schemas, but mark that test as expected to fail while unimplemented.

mjperrone · 2023-08-02T01:42:06Z

tests/0.2.0.json

@@ -0,0 +1,377 @@
+{


After this JSON Schema is reviewed and approved in this PR, I will remove this from this PR and add it to recap-website/specs/type/0.2.0.json, then switch from a file reference to a network call.

I'm including it in this PR draft to make it easier to iterate on the spec and tests in parallel.

mjperrone · 2023-08-02T01:46:45Z

tests/0.2.0.json

+                "$ref": "#/$defs/RecapNull"
+              },
+              {
+                "$ref": "#/$defs/RecapCommonFields"


I chose to put the common fields alias and doc in its own def instead of repeating them in each of the base types (like RecapNull here).

I also chose to put the allOf in this RecapType oneOf union instead of inside each of the base types. I did this both to mirror the DRY way the existing spec introduces these fields, and to emphasize the unique fields when looking at the base types.

I ended up repeating myself so that the error messages are easier to read when validation fails.

mjperrone · 2023-08-02T04:15:01Z

tests/0.2.0.json

+            "minimum": 1,
+            "maximum": 64


I'm not sure about the correct ranges for some of these integer values

Integer bits field is a 32-bit signed int, so min is 1 and max is 2_147_483_647.

Same for bits in float.

bytes for string and bytes is 64-bit signed. The attributes section in the spec has some info there.

updated max for these fields

mjperrone · 2023-08-02T04:15:38Z

tests/0.2.0.json

+          }
+        }
+      }
+    }


I don't think I have represented Logical types in this

No prob.

I think there are two things to do.

Allow "logical" to be defined as a string attribute in RecapCommonFields

Add defs for built-in logical types (the ones defined in the spec)

I think (1) is worth doing in this PR. I think (2) should be put in a follow-on GH issue. I think it requires a bit more thought, since other non-builtin logical types might exist, and those wouldn't get validated (beyond just checking that logical is set).

I've done 1 and made a note to make an issue for 2 after this PR is finished

criccomini

This looks really good. I left a couple comments here and there, but I agree with your decisions.

One thing I didn't catch that I think is a common gotcha: disallowing alias to be set when type reference an alias. See the "But alias inheritance is not allowed" section of the spec for an example.

criccomini · 2023-08-02T15:57:25Z

tests/spec/invalid_field_type.json

+    {
+    "name": "string_field",
+    "doc": "doc",
+    "type": "invalidtype",


Nice! Can you add functionality to let us ignore expected failed tests (i.e. xfail)? I think it complicates the logic in test_json_schema.py, but it'll be useful as we build out and validate the harness, I think. If it's a pain, though, we can hack it (e.g. .json.failing filename)

criccomini · 2023-08-02T16:04:57Z

One other thought:

I could see an argument for being more explicit about the recap types (removing allOf/oneOf), even if it means repeating yourself. The argument for this would be that it makes the JSON schema way more readable to humans.

I'm indifferent on that argument at this point, but I could see the spec being a bit confusing to humans that are trying to read (vs. validators like python-jsonschema). I was a bit disoriented when I looked at it; I didn't actually understand until you mentioned the allOf/oneOf in the comment.

criccomini · 2023-08-02T16:05:16Z

Re: the black failure, try running pdm style to get things cleaned up.

…nd end range ints

…d of factored out for better error messages, fix the definition based on examples

mjperrone · 2023-08-02T18:40:00Z

One thing I didn't catch that I think is a common gotcha: disallowing alias to be set when type reference an alias.

Did this by not repeating the alias definition for RecapAliasReference

I could see an argument for being more explicit about the recap types (removing allOf/oneOf), even if it means repeating yourself. The argument for this would be that it makes the JSON schema way more readable to humans.

I ended up doing as I was iterating on this because the validation errors were harder to understand.

adrianisk · 2023-08-02T19:21:19Z

This is awesome 🚀

criccomini

🔥 This looks great.

Only request:

Can you move unit/meta_schema/test_json_schema.py to tests/spec
And then move spec/*.json to a subdir like schemas or something?
And then update the .github/workflows/ci.yaml to have an extra job called spec-tests that runs pdm run pytest tests/spec -vv

I want to keep the spec enforcement separate from the standard unit tests.

…he same as jsonschema; make aliases FQNs

mjperrone · 2023-08-02T22:59:05Z

Can you move unit/meta_schema/test_json_schema.py to tests/spec

And then move spec/*.json to a subdir like schemas or something?

And then update the .github/workflows/ci.yaml to have an extra job called spec-tests that runs pdm run pytest tests/spec -vv

done
moved them to spec/{in,}valid, so removed the .invalid. file sub-extension
done. I just followed the pattern of the existing jobs, but it's doing checkout+python+pdm installs for each step. That could be cached so it only has to happen once per build, or really once per new dependencies

I also realized that these tests should test that recap is validating the schemas the same way as the JSON Schema, so I added the recap validation to each.

This made me realize that there's no reason we shouldn't take every positive and negative validation example in the recap test suites and evaluate that against the jsonschema validation as well. Given the current structure of the tests, how feasible does that sound?

criccomini · 2023-08-03T17:22:21Z

This made me realize that there's no reason we shouldn't take every positive and negative validation example in the recap test suites and evaluate that against the jsonschema validation as well. Given the current structure of the tests, how feasible does that sound?

Ahh, yes I've been thinking about this.

What I was thinking was to replace the proto/avro/json schema/recap CST tests with this suite. You've already begun to define Recap CSTs (in JSON). We should be able to define matching .proto, .avsc, and .schema.json files that we expect to get when we call types.py :: .from_dict followed by *Converter.from_recap().

We could then go the other way as well, from proto/avsc/json schema back to Recap's CST to verify bidirectional conversion is as expected.

This doesn't let us verify every detail we might want to verify at the AST level, so I think we'll still want to have some unit tests for that part. It should get us VERY comfortable with Recap's behavior, though.

WDYT? Not for this PR, but more GH issues to open if you're into it.

criccomini · 2023-08-03T17:48:33Z

💥 Merged! Thanks!!

Follow-on things I found:

Migrating .json spec to recap-website
Update to pull spec from the recap-website
GH issue for logical type spec tests
(Maybe?) GH issue to do validation when translating to/from recap CST for proto/avro/JSON schema.

mjperrone · 2023-08-03T18:36:28Z

💥 Merged! Thanks!!

Follow-on things I found:

Migrating .json spec to recap-website

Update to pull spec from the recap-website

GH issue for logical type spec tests

(Maybe?) GH issue to do validation when translating to/from recap CST for proto/avro/JSON schema.

Add spec test for built-in logical types #348

Another thought: this test suite could be extended to read specs defined in YAML or TOML. There ¿might? be some situations where those languages handle values differently like null

criccomini · 2023-08-03T19:06:57Z

Yup, totally. I think it's worth defining in YAML, TOML, and JSON, and validating the CST using the python-jsonschema lib.

One gotcha with TOML, in particular, is that it doesn't have a null AFAIK. Makes null defaults tricky. At least we added optional for the common case, which will work with TOML.

criccomini · 2023-08-04T18:10:13Z

Opened these follow-on issues:

#349
#350
#351

mjperrone added 3 commits August 1, 2023 16:24

add jsonschema to dev deps

e7b5623

add test harness for meta schema correctness - with passing and faili…

43ae1d3

…ng test

add json schema to this repo temporarily for review iteration

296a1fb

mjperrone commented Aug 2, 2023

View reviewed changes

mjperrone mentioned this pull request Aug 2, 2023

Add meta schema describing valid recap schemas #342

Closed

mjperrone commented Aug 2, 2023

View reviewed changes

criccomini marked this pull request as ready for review August 2, 2023 16:02

criccomini reviewed Aug 2, 2023

View reviewed changes

mjperrone added 7 commits August 2, 2023 09:18

set maxium for int, float, string, and bytes to 32bit max and 64bit max

a07d76f

pdm black

eef6bb0

add recap alias reference to json schema definition

7656c17

add test specs showing basic types with optionals included/excluded a…

5285946

…nd end range ints

update test harness to also define unimplemented invalid recap schemas

0de4f0c

update JSON Schema: format, repeat common fields in base types instea…

872f684

…d of factored out for better error messages, fix the definition based on examples

update examples based on the recap spec, format them

da32c85

mjperrone requested a review from criccomini August 2, 2023 18:42

criccomini suggested changes Aug 2, 2023

View reviewed changes

mjperrone added 2 commits August 2, 2023 15:47

move meta spec tests into spec/; test that recap validation behaves t…

b55aa64

…he same as jsonschema; make aliases FQNs

add CI step to run spec tests

c7aaf3b

mjperrone requested a review from criccomini August 2, 2023 22:59

criccomini merged commit d53150e into gabledata:main Aug 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add meta schema: a JSON Schema which matches recap schemas #346

Add meta schema: a JSON Schema which matches recap schemas #346

mjperrone commented Aug 2, 2023 •

edited

Loading

mjperrone Aug 2, 2023

criccomini Aug 2, 2023

mjperrone Aug 2, 2023

mjperrone Aug 2, 2023

mjperrone Aug 2, 2023

mjperrone Aug 2, 2023

mjperrone Aug 2, 2023

criccomini Aug 2, 2023

mjperrone Aug 2, 2023

mjperrone Aug 2, 2023

criccomini Aug 2, 2023

mjperrone Aug 2, 2023

criccomini left a comment

criccomini Aug 2, 2023

criccomini commented Aug 2, 2023

criccomini commented Aug 2, 2023

mjperrone commented Aug 2, 2023

adrianisk commented Aug 2, 2023

criccomini left a comment •

edited

Loading

mjperrone commented Aug 2, 2023

criccomini commented Aug 3, 2023

criccomini commented Aug 3, 2023

mjperrone commented Aug 3, 2023

criccomini commented Aug 3, 2023

criccomini commented Aug 4, 2023

Add meta schema: a JSON Schema which matches recap schemas #346

Add meta schema: a JSON Schema which matches recap schemas #346

Conversation

mjperrone commented Aug 2, 2023 • edited Loading

What

Why

How

Known limitations

Validation

Potential future work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

criccomini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

criccomini commented Aug 2, 2023

criccomini commented Aug 2, 2023

mjperrone commented Aug 2, 2023

adrianisk commented Aug 2, 2023

criccomini left a comment • edited Loading

Choose a reason for hiding this comment

mjperrone commented Aug 2, 2023

criccomini commented Aug 3, 2023

criccomini commented Aug 3, 2023

mjperrone commented Aug 3, 2023

criccomini commented Aug 3, 2023

criccomini commented Aug 4, 2023

mjperrone commented Aug 2, 2023 •

edited

Loading

criccomini left a comment •

edited

Loading