Repository: altair-viz/altair
Pull request #2883 (2023-02-12 15:51:46)

Use most specific class possible in schema validation errors
Addresses https://github.com/altair-viz/altair/pull/2568#discussion_r1096814378

Works already well but I'll still want to

- [x] Extend existing and add new tests to check if correct class name appears in error message
- [x] Test against multiple `jsonschema` versions (3, 4, 4.5, 4.16, 4.17)

binste: @joelostblom This already works if a wrong argument was passed to an encoding channel. Your example https://github.com/altair-viz/altair/pull/2568#issuecomment-1373564808 works anyway even without this PR because `alt.Scale` is immediately validated when it's instantiated. Do you have any other examples from #2568  I should test it against? 
joelostblom: This is looking great! Thanks for getting to it so quickly @binste. I tried a few different examples and they all seem to work fine. One thing that I did notice is that if we use to datum encoding then their message is incorrect:

```py
alt.Chart().mark_point().encode(x=alt.datum(1, wrong_name=1))
```

```
SchemaValidationError: Invalid specification

        altair.vegalite.v5.schema.channels.X, validating 'additionalProperties'

        Additional properties are not allowed ('datum', 'wrong_name' were unexpected)
```

`'datum'` should not be included above as an incorrect property to the X channel, instead `wrong_name` should be mentioned as an incorrect parameter to `datum`. The way to handle this is probably with special error checking inside the datum function, and it is not introduced by this pull request, but I thought I would mention it here anyways. The tests here also look reasonable to me.
binste: Good to go from my side

Patch:
diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py
index 0c8f9b27..de631b62 100644
--- a/altair/utils/schemapi.py
+++ b/altair/utils/schemapi.py
@@ -159,13 +159,27 @@ class SchemaValidationError(jsonschema.ValidationError):
         self._additional_errors = getattr(err, "_additional_errors", [])
 
     def __str__(self):
-        cls = self.obj.__class__
+        # Try to get the lowest class possible in the chart hierarchy so
+        # it can be displayed in the error message. This should lead to more informative
+        # error messages pointing the user closer to the source of the issue.
+        for prop_name in reversed(self.absolute_path):
+            # Check if str as e.g. first item can be a 0
+            if isinstance(prop_name, str):
+                potential_class_name = prop_name[0].upper() + prop_name[1:]
+                cls = getattr(vegalite, potential_class_name, None)
+                if cls is not None:
+                    break
+        else:
+            # Did not find a suitable class based on traversing the path so we fall
+            # back on the class of the top-level object which created
+            # the SchemaValidationError
+            cls = self.obj.__class__
         schema_path = ["{}.{}".format(cls.__module__, cls.__name__)]
         schema_path.extend(self.schema_path)
         schema_path = "->".join(
             str(val)
             for val in schema_path[:-1]
-            if val not in ("properties", "additionalProperties", "patternProperties")
+            if val not in (0, "properties", "additionalProperties", "patternProperties")
         )
         message = self.message
         if self._additional_errors:
@@ -406,6 +420,8 @@ class SchemaBase(object):
             kwds = {
                 k: v for k, v in kwds.items() if k not in list(ignore) + ["shorthand"]
             }
+            if "mark" in kwds and isinstance(kwds["mark"], str):
+                kwds["mark"] = {"type": kwds["mark"]}
             result = _todict(
                 kwds,
                 context=context,
diff --git a/doc/user_guide/internals.rst b/doc/user_guide/internals.rst
index f63479ad..d7187db4 100644
--- a/doc/user_guide/internals.rst
+++ b/doc/user_guide/internals.rst
@@ -136,7 +136,7 @@ from the Vega-Lite documentation, which has the following JSON specification::
           {"a": "G","b": 19}, {"a": "H","b": 87}, {"a": "I","b": 52}
         ]
       },
-      "mark": "bar",
+      "mark": {"type": "bar"},
       "encoding": {
         "x": {"field": "a", "type": "ordinal"},
         "y": {"field": "b", "type": "quantitative"}
@@ -161,7 +161,7 @@ construct an Altair chart object from this string of Vega-Lite JSON:
           {"a": "G","b": 19}, {"a": "H","b": 87}, {"a": "I","b": 52}
         ]
       },
-      "mark": "bar",
+      "mark": {"type": "bar"},
       "encoding": {
         "x": {"field": "a", "type": "ordinal"},
         "y": {"field": "b", "type": "quantitative"}
@@ -186,7 +186,7 @@ you can use the :meth:`~Chart.from_dict` method to construct the chart object:
           {"a": "G","b": 19}, {"a": "H","b": 87}, {"a": "I","b": 52}
         ]
       },
-      "mark": "bar",
+      "mark": {"type": "bar"},
       "encoding": {
         "x": {"field": "a", "type": "ordinal"},
         "y": {"field": "b", "type": "quantitative"}
diff --git a/doc/user_guide/saving_charts.rst b/doc/user_guide/saving_charts.rst
index ec0d5939..6fc18526 100644
--- a/doc/user_guide/saving_charts.rst
+++ b/doc/user_guide/saving_charts.rst
@@ -61,7 +61,7 @@ The contents of the resulting file will look something like this:
           "type": "quantitative"
         }
       },
-      "mark": "point"
+      "mark": {"type": "point"}
     }
 
 This JSON can then be inserted into any web page using the vegaEmbed_ library.
@@ -121,7 +121,7 @@ javascript-enabled web browser:
               "type": "quantitative"
             }
           },
-          "mark": "point"
+          "mark": {"type": "point"}
         };
         var opt = {"renderer": "canvas", "actions": false};
         vegaEmbed("#vis", spec, opt);
diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py
index 190446bb..9fa2dccb 100644
--- a/tools/schemapi/schemapi.py
+++ b/tools/schemapi/schemapi.py
@@ -157,13 +157,27 @@ class SchemaValidationError(jsonschema.ValidationError):
         self._additional_errors = getattr(err, "_additional_errors", [])
 
     def __str__(self):
-        cls = self.obj.__class__
+        # Try to get the lowest class possible in the chart hierarchy so
+        # it can be displayed in the error message. This should lead to more informative
+        # error messages pointing the user closer to the source of the issue.
+        for prop_name in reversed(self.absolute_path):
+            # Check if str as e.g. first item can be a 0
+            if isinstance(prop_name, str):
+                potential_class_name = prop_name[0].upper() + prop_name[1:]
+                cls = getattr(vegalite, potential_class_name, None)
+                if cls is not None:
+                    break
+        else:
+            # Did not find a suitable class based on traversing the path so we fall
+            # back on the class of the top-level object which created
+            # the SchemaValidationError
+            cls = self.obj.__class__
         schema_path = ["{}.{}".format(cls.__module__, cls.__name__)]
         schema_path.extend(self.schema_path)
         schema_path = "->".join(
             str(val)
             for val in schema_path[:-1]
-            if val not in ("properties", "additionalProperties", "patternProperties")
+            if val not in (0, "properties", "additionalProperties", "patternProperties")
         )
         message = self.message
         if self._additional_errors:
@@ -404,6 +418,8 @@ class SchemaBase(object):
             kwds = {
                 k: v for k, v in kwds.items() if k not in list(ignore) + ["shorthand"]
             }
+            if "mark" in kwds and isinstance(kwds["mark"], str):
+                kwds["mark"] = {"type": kwds["mark"]}
             result = _todict(
                 kwds,
                 context=context,


Test patch:
diff --git a/tests/tests/test_examples.py b/tests/tests/test_examples.py
index 9a268ba8..4dcfee36 100644
--- a/tests/tests/test_examples.py
+++ b/tests/tests/test_examples.py
@@ -35,9 +35,17 @@ def test_render_examples_to_chart(syntax_module):
 
         if chart is None:
             raise ValueError(
-                "Example file should define chart in its final " "statement."
+                f"Example file {filename} should define chart in its final "
+                "statement."
+            )
+
+        try:
+            assert isinstance(chart.to_dict(), dict)
+        except Exception as e:
+            raise AssertionError(
+                f"Example file {filename} raised an exception when "
+                f"converting to a dict: {e}"
             )
-        assert isinstance(chart.to_dict(), dict)
 
 
 # We do not apply the save_engine mark to this test. This mark is used in
diff --git a/tests/tests/test_magics.py b/tests/tests/test_magics.py
index d794c733..db24ff48 100644
--- a/tests/tests/test_magics.py
+++ b/tests/tests/test_magics.py
@@ -45,7 +45,7 @@ VEGALITE_SPEC = {
         "x": {"field": "category", "type": "ordinal"},
         "y": {"field": "amount", "type": "quantitative"},
     },
-    "mark": "bar",
+    "mark": {"type": "bar"},
 }
 
 
diff --git a/tests/utils/tests/test_html.py b/tests/utils/tests/test_html.py
index 5f44704b..ccd41745 100644
--- a/tests/utils/tests/test_html.py
+++ b/tests/utils/tests/test_html.py
@@ -7,7 +7,7 @@ from altair.utils.html import spec_to_html
 def spec():
     return {
         "data": {"url": "data.json"},
-        "mark": "point",
+        "mark": {"type": "point"},
         "encoding": {
             "x": {"field": "x", "type": "quantitative"},
             "y": {"field": "y", "type": "quantitative"},
diff --git a/tests/utils/tests/test_mimebundle.py b/tests/utils/tests/test_mimebundle.py
index 6cdb9c37..3aa8734f 100644
--- a/tests/utils/tests/test_mimebundle.py
+++ b/tests/utils/tests/test_mimebundle.py
@@ -32,7 +32,7 @@ def vegalite_spec():
                 {"a": "I", "b": 52},
             ]
         },
-        "mark": "bar",
+        "mark": {"type": "bar"},
         "encoding": {
             "x": {"field": "a", "type": "ordinal"},
             "y": {"field": "b", "type": "quantitative"},
diff --git a/tests/utils/tests/test_schemapi.py b/tests/utils/tests/test_schemapi.py
index d7263404..af48ea74 100644
--- a/tests/utils/tests/test_schemapi.py
+++ b/tests/utils/tests/test_schemapi.py
@@ -467,30 +467,36 @@ def chart_example_invalid_y_option_value_with_condition():
     [
         (
             chart_example_invalid_y_option,
-            r"Additional properties are not allowed \('unknown' was unexpected\)",
+            r"schema.channels.X.*"
+            + r"Additional properties are not allowed \('unknown' was unexpected\)",
         ),
         (
             chart_example_invalid_y_option_value,
-            r"'asdf' is not one of \['zero', 'center', 'normalize'\].*"
+            r"schema.channels.Y.*"
+            + r"'asdf' is not one of \['zero', 'center', 'normalize'\].*"
             + r"'asdf' is not of type 'null'.*'asdf' is not of type 'boolean'",
         ),
         (
             chart_example_layer,
-            r"Additional properties are not allowed \('width' was unexpected\)",
+            r"api.VConcatChart.*"
+            + r"Additional properties are not allowed \('width' was unexpected\)",
         ),
         (
             chart_example_invalid_y_option_value_with_condition,
-            r"'asdf' is not one of \['zero', 'center', 'normalize'\].*"
+            r"schema.channels.Y.*"
+            + r"'asdf' is not one of \['zero', 'center', 'normalize'\].*"
             + r"'asdf' is not of type 'null'.*'asdf' is not of type 'boolean'",
         ),
         (
             chart_example_hconcat,
-            r"\{'text': 'Horsepower', 'align': 'right'\} is not of type 'string'.*"
+            r"schema.core.TitleParams.*"
+            + r"\{'text': 'Horsepower', 'align': 'right'\} is not of type 'string'.*"
             + r"\{'text': 'Horsepower', 'align': 'right'\} is not of type 'array'",
         ),
         (
             chart_example_invalid_channel_and_condition,
-            r"Additional properties are not allowed \('invalidChannel' was unexpected\)",
+            r"schema.core.Encoding->encoding.*"
+            + r"Additional properties are not allowed \('invalidChannel' was unexpected\)",
         ),
     ],
 )
@@ -554,3 +560,11 @@ def test_to_dict_no_side_effects():
     assert "shorthand" not in dct["encoding"]["y"]
     assert dct["encoding"]["y"]["field"] == "b"
     assert dct["encoding"]["y"]["type"] == "quantitative"
+
+
+def test_to_dict_expand_mark_spec():
+    # Test that `to_dict` correctly expands marks to a dictionary
+    # without impacting the original spec which remains a string
+    chart = alt.Chart().mark_bar()
+    assert chart.to_dict()["mark"] == {"type": "bar"}
+    assert chart.mark == "bar"
diff --git a/tests/vegalite/tests/test_common.py b/tests/vegalite/tests/test_common.py
index 3a3bc4db..0cc216e6 100644
--- a/tests/vegalite/tests/test_common.py
+++ b/tests/vegalite/tests/test_common.py
@@ -11,7 +11,7 @@ from altair.vegalite import v5
 def basic_spec():
     return {
         "data": {"url": "data.csv"},
-        "mark": "line",
+        "mark": {"type": "line"},
         "encoding": {
             "color": {"type": "nominal", "field": "color"},
             "x": {"type": "quantitative", "field": "xval"},
diff --git a/tools/schemapi/tests/test_schemapi.py b/tools/schemapi/tests/test_schemapi.py
index 3dd93e4e..09667a6e 100644
--- a/tools/schemapi/tests/test_schemapi.py
+++ b/tools/schemapi/tests/test_schemapi.py
@@ -465,30 +465,36 @@ def chart_example_invalid_y_option_value_with_condition():
     [
         (
             chart_example_invalid_y_option,
-            r"Additional properties are not allowed \('unknown' was unexpected\)",
+            r"schema.channels.X.*"
+            + r"Additional properties are not allowed \('unknown' was unexpected\)",
         ),
         (
             chart_example_invalid_y_option_value,
-            r"'asdf' is not one of \['zero', 'center', 'normalize'\].*"
+            r"schema.channels.Y.*"
+            + r"'asdf' is not one of \['zero', 'center', 'normalize'\].*"
             + r"'asdf' is not of type 'null'.*'asdf' is not of type 'boolean'",
         ),
         (
             chart_example_layer,
-            r"Additional properties are not allowed \('width' was unexpected\)",
+            r"api.VConcatChart.*"
+            + r"Additional properties are not allowed \('width' was unexpected\)",
         ),
         (
             chart_example_invalid_y_option_value_with_condition,
-            r"'asdf' is not one of \['zero', 'center', 'normalize'\].*"
+            r"schema.channels.Y.*"
+            + r"'asdf' is not one of \['zero', 'center', 'normalize'\].*"
             + r"'asdf' is not of type 'null'.*'asdf' is not of type 'boolean'",
         ),
         (
             chart_example_hconcat,
-            r"\{'text': 'Horsepower', 'align': 'right'\} is not of type 'string'.*"
+            r"schema.core.TitleParams.*"
+            + r"\{'text': 'Horsepower', 'align': 'right'\} is not of type 'string'.*"
             + r"\{'text': 'Horsepower', 'align': 'right'\} is not of type 'array'",
         ),
         (
             chart_example_invalid_channel_and_condition,
-            r"Additional properties are not allowed \('invalidChannel' was unexpected\)",
+            r"schema.core.Encoding->encoding.*"
+            + r"Additional properties are not allowed \('invalidChannel' was unexpected\)",
         ),
     ],
 )
@@ -552,3 +558,11 @@ def test_to_dict_no_side_effects():
     assert "shorthand" not in dct["encoding"]["y"]
     assert dct["encoding"]["y"]["field"] == "b"
     assert dct["encoding"]["y"]["type"] == "quantitative"
+
+
+def test_to_dict_expand_mark_spec():
+    # Test that `to_dict` correctly expands marks to a dictionary
+    # without impacting the original spec which remains a string
+    chart = alt.Chart().mark_bar()
+    assert chart.to_dict()["mark"] == {"type": "bar"}
+    assert chart.mark == "bar"