Remove deep validation and instead use error hierarchy to improve error messages

altair/utils/schemapi.py

@@ -5,7 +5,7 @@
 import inspect
 import json
 import textwrap
-from typing import Any
+from typing import Any, Sequence, List
 
 import jsonschema
 import jsonschema.exceptions
@@ -66,9 +66,50 @@ def validate_jsonschema(spec, schema, rootschema=None):
     if hasattr(validator_cls, "FORMAT_CHECKER"):
         validator_kwargs["format_checker"] = validator_cls.FORMAT_CHECKER
     validator = validator_cls(schema, **validator_kwargs)
-    error = jsonschema.exceptions.best_match(validator.iter_errors(spec))
-    if error is not None:
-        raise error
+    errors = list(validator.iter_errors(spec))
+    if errors:
+        errors = _get_most_relevant_errors(errors)
+        main_error = errors[0]
+        # They can be used to craft more helpful error messages when this error
+        # is being converted to a SchemaValidationError
+        main_error._additional_errors = errors[1:]
+        raise main_error
+
+
+def _get_most_relevant_errors(
+    errors: Sequence[jsonschema.exceptions.ValidationError],
+) -> List[jsonschema.exceptions.ValidationError]:
+    if len(errors) == 0:
+        return []
+
+    # Go to lowest level in schema where an error happened as these give
+    # the most relevant error messages
+    lowest_level = errors[0]
+    while lowest_level.context:
+        lowest_level = lowest_level.context[0]
+
+    most_relevant_errors = []
+    if lowest_level.validator == "additionalProperties":
+        # For these errors, the message is already informative enough and the other
+        # errors on the lowest level are not helpful for a user but instead contain
+        # the same information in a more verbose way
+        most_relevant_errors = [lowest_level]
+    else:
+        parent = lowest_level.parent
+        if parent is None:
+            # In this case we are still at the top level and can return all errors
+            most_relevant_errors = errors
+        else:
+            # Return all errors of the lowest level out of which
+            # we can construct more informative error messages
+            most_relevant_errors = lowest_level.parent.context
+
+    # This should never happen but might still be good to test for it as else
+    # the original error would just slip through without being raised
+    if len(most_relevant_errors) == 0:
+        raise Exception("Could not determine the most relevant errors") from errors[0]
+
+    return most_relevant_errors
 
 
 def _subclasses(cls):
@@ -117,24 +158,9 @@ class SchemaValidationError(jsonschema.ValidationError):
     """A wrapper for jsonschema.ValidationError with friendlier traceback"""
 
     def __init__(self, obj, err):
-        super(SchemaValidationError, self).__init__(**self._get_contents(err))
+        super(SchemaValidationError, self).__init__(**err._contents())
         self.obj = obj
-
-    @staticmethod
-    def _get_contents(err):
-        """Get a dictionary with the contents of a ValidationError"""
-        try:
-            # works in jsonschema 2.3 or later
-            contents = err._contents()
-        except AttributeError:
-            try:
-                # works in Python >=3.4
-                spec = inspect.getfullargspec(err.__init__)
-            except AttributeError:
-                # works in Python <3.4
-                spec = inspect.getargspec(err.__init__)
-            contents = {key: getattr(err, key) for key in spec.args[1:]}
-        return contents
+        self._additional_errors = getattr(err, "_additional_errors", [])
 
     def __str__(self):
         cls = self.obj.__class__
@@ -145,13 +171,18 @@ def __str__(self):
             for val in schema_path[:-1]
             if val not in ("properties", "additionalProperties", "patternProperties")
         )
+        message = self.message
+        if self._additional_errors:
+            message += "\n        " + "\n        ".join(
+                [e.message for e in self._additional_errors]
+            )
         return """Invalid specification
 
         {}, validating {!r}
 
         {}
         """.format(
-            schema_path, self.validator, self.message
+            schema_path, self.validator, message
         )
 
 
@@ -327,11 +358,9 @@ def to_dict(self, validate=True, ignore=None, context=None):
 
         Parameters
         ----------
-        validate : boolean or string
+        validate : boolean
             If True (default), then validate the output dictionary
-            against the schema. If "deep" then recursively validate
-            all objects in the spec. This takes much more time, but
-            it results in friendlier tracebacks for large objects.
+            against the schema.
         ignore : list
             A list of keys to ignore. This will *not* passed to child to_dict
             function calls.
@@ -353,10 +382,9 @@ def to_dict(self, validate=True, ignore=None, context=None):
             context = {}
         if ignore is None:
             ignore = []
-        sub_validate = "deep" if validate == "deep" else False
 
         if self._args and not self._kwds:
-            result = _todict(self._args[0], validate=sub_validate, context=context)
+            result = _todict(self._args[0], validate=False, context=context)
         elif not self._args:
             kwds = self._kwds.copy()
             # parsed_shorthand is added by FieldChannelMixin.
@@ -376,7 +404,7 @@ def to_dict(self, validate=True, ignore=None, context=None):
             }
             result = _todict(
                 kwds,
-                validate=sub_validate,
+                validate=False,
                 context=context,
             )
         else:
@@ -398,11 +426,9 @@ def to_json(
 
         Parameters
         ----------
-        validate : boolean or string
+        validate : string
             If True (default), then validate the output dictionary
-            against the schema. If "deep" then recursively validate
-            all objects in the spec. This takes much more time, but
-            it results in friendlier tracebacks for large objects.
+            against the schema.
         ignore : list
             A list of keys to ignore. This will *not* passed to child to_dict
             function calls.

altair/vegalite/v5/api.py

@@ -556,17 +556,7 @@ def to_dict(self, *args, **kwargs):
         context["top_level"] = False
         kwargs["context"] = context
 
-        try:
-            dct = super(TopLevelMixin, copy).to_dict(*args, **kwargs)
-        except jsonschema.ValidationError:
-            dct = None
-
-        # If we hit an error, then re-convert with validate='deep' to get
-        # a more useful traceback. We don't do this by default because it's
-        # much slower in the case that there are no errors.
-        if dct is None:
-            kwargs["validate"] = "deep"
-            dct = super(TopLevelMixin, copy).to_dict(*args, **kwargs)
+        dct = super(TopLevelMixin, copy).to_dict(*args, **kwargs)
 
         # TODO: following entries are added after validation. Should they be validated?
         if is_top_level:

tests/utils/tests/test_schemapi.py

@@ -4,11 +4,14 @@
 import io
 import json
 import jsonschema
+import re
 import pickle
-import pytest
+import warnings
 
 import numpy as np
 import pandas as pd
+import pytest
+from vega_datasets import data
 
 import altair as alt
 from altair import load_schema
@@ -380,6 +383,113 @@ def test_schema_validation_error():
     assert the_err.message in message
 
 
+def chart_example_layer():
+    points = (
+        alt.Chart(data.cars.url)
+        .mark_point()
+        .encode(
+            x="Horsepower:Q",
+            y="Miles_per_Gallon:Q",
+        )
+    )
+    return (points & points).properties(width=400)
+
+
+def chart_example_hconcat():
+    source = data.cars()
+    points = (
+        alt.Chart(source)
+        .mark_point()
+        .encode(
+            x="Horsepower",
+            y="Miles_per_Gallon",
+        )
+    )
+
+    text = (
+        alt.Chart(source)
+        .mark_text(align="right")
+        .encode(alt.Text("Horsepower:N", title=dict(text="Horsepower", align="right")))
+    )
+
+    return points | text
+
+
+def chart_example_invalid_channel_and_condition():
+    selection = alt.selection_point()
+    return (
+        alt.Chart(data.barley())
+        .mark_circle()
+        .add_params(selection)
+        .encode(
+            color=alt.condition(selection, alt.value("red"), alt.value("green")),
+            invalidChannel=None,
+        )
+    )
+
+
+@pytest.mark.parametrize(
+    "chart_func,expected_error_message",
+    [
+        (
+            lambda: alt.Chart(data.barley())
+            .mark_bar()
+            .encode(
+                x=alt.X("variety", unknown=2),
+                y=alt.Y("sum(yield)", stack="asdf"),
+            ),
+            r"Additional properties are not allowed \('unknown' was unexpected\)",
+        ),
+        (
+            lambda: alt.Chart(data.barley())
+            .mark_bar()
+            .encode(
+                x=alt.X("variety"),
+                y=alt.Y("sum(yield)", stack="asdf"),
+            ),
+            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\)",
+        ),
+        (
+            lambda: alt.Chart(data.barley())
+            .mark_bar()
+            .encode(
+                x="variety",
+                y=alt.Y("sum(yield)", stack="asdf"),
+                opacity=alt.condition("datum.yield > 0", alt.value(1), alt.value(0.2)),
+            ),
+            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"\{'text': 'Horsepower', 'align': 'right'\} is not of type 'array'",
+        ),
+        (
+            chart_example_invalid_channel_and_condition,
+            r"Additional properties are not allowed \('invalidChannel' was unexpected\)",
+        ),
+    ],
+)
+def test_chart_validation_errors(chart_func, expected_error_message):
+    # DOTALL flag makes that a dot (.) also matches new lines
+    pattern = re.compile(expected_error_message, re.DOTALL)
+    # For some wrong chart specifications such as an unknown encoding channel,
+    # Altair already raises a warning before the chart specifications are validated.
+    # We can ignore these warnings as we are interested in the errors being raised
+    # during validation which is triggered by to_dict
+    with warnings.catch_warnings():
+        warnings.filterwarnings("ignore", category=UserWarning)
+        chart = chart_func()
+    with pytest.raises(SchemaValidationError, match=pattern):
+        chart.to_dict()
+
+
 def test_serialize_numpy_types():
     m = MySchema(
         a={"date": np.datetime64("2019-01-01")},