Merge pull request #2781 from fishtown-analytics/feature/2700-improve…

…-yaml-selector-errors

Add better error messages for yaml selectors

CHANGELOG.md

@@ -8,6 +8,7 @@
 - Added state and defer arguments to the RPC client, matching the CLI ([#2678](https://github.com/fishtown-analytics/dbt/issues/2678), [#2736](https://github.com/fishtown-analytics/dbt/pull/2736))
 - Added schema and dbt versions to JSON artifacts ([#2670](https://github.com/fishtown-analytics/dbt/issues/2670), [#2767](https://github.com/fishtown-analytics/dbt/pull/2767))
 - Added ability to snapshot hard-deleted records (opt-in with `invalidate_hard_deletes` config option). ([#249](https://github.com/fishtown-analytics/dbt/issues/249), [#2749](https://github.com/fishtown-analytics/dbt/pull/2749))
+- Improved error messages for YAML selectors ([#2700](https://github.com/fishtown-analytics/dbt/issues/2700), [#2781](https://github.com/fishtown-analytics/dbt/pull/2781))
 
 Contributors:
 - [@joelluijmes](https://github.com/joelluijmes) ([#2749](https://github.com/fishtown-analytics/dbt/pull/2749))

core/dbt/config/selectors.py

@@ -1,5 +1,6 @@
 from pathlib import Path
 from typing import Dict, Any
+import yaml
 
 from hologram import ValidationError
 
@@ -33,7 +34,17 @@ def from_dict(cls, data: Dict[str, Any]) -> 'SelectorConfig':
         try:
             selector_file = SelectorFile.from_dict(data)
             selectors = parse_from_selectors_definition(selector_file)
-        except (ValidationError, RuntimeException) as exc:
+        except ValidationError as exc:
+            yaml_sel_cfg = yaml.dump(exc.instance)
+            raise DbtSelectorsError(
+                f"Could not parse selector file data: \n{yaml_sel_cfg}\n"
+                f"Valid root-level selector definitions: "
+                f"union, intersection, string, dictionary. No lists. "
+                f"\nhttps://docs.getdbt.com/reference/node-selection/"
+                f"yaml-selectors",
+                result_type='invalid_selector'
+            ) from exc
+        except RuntimeException as exc:
             raise DbtSelectorsError(
                 f'Could not read selector file data: {exc}',
                 result_type='invalid_selector',

core/dbt/graph/cli.py

@@ -1,5 +1,6 @@
 # special support for CLI argument parsing.
 import itertools
+import yaml
 
 from typing import (
     Dict, List, Optional, Tuple, Any, Union
@@ -116,8 +117,7 @@ def _get_list_dicts(
     values = dct[key]
     if not isinstance(values, list):
         raise ValidationException(
-            f'Invalid value type {type(values)} in key "{key}" '
-            f'(value "{values}")'
+            f'Invalid value for key "{key}". Expected a list.'
         )
     for value in values:
         if isinstance(value, dict):
@@ -165,9 +165,10 @@ def _parse_include_exclude_subdefs(
         if isinstance(definition, dict) and 'exclude' in definition:
             # do not allow multiple exclude: defs at the same level
             if diff_arg is not None:
+                yaml_sel_cfg = yaml.dump(definition)
                 raise ValidationException(
-                    f'Got multiple exclusion definitions in definition list '
-                    f'{definitions}'
+                    f"You cannot provide multiple exclude arguments to the "
+                    f"same selector set operator:\n{yaml_sel_cfg}"
                 )
             diff_arg = _parse_exclusions(definition)
         else:
@@ -198,6 +199,7 @@ def parse_intersection_definition(
     intersection_def_parts = _get_list_dicts(definition, 'intersection')
     include, exclude = _parse_include_exclude_subdefs(intersection_def_parts)
     intersection = SelectionIntersection(components=include)
+
     if exclude is None:
         intersection.raw = definition
         return intersection
@@ -210,7 +212,6 @@ def parse_intersection_definition(
 
 def parse_dict_definition(definition: Dict[str, Any]) -> SelectionSpec:
     diff_arg: Optional[SelectionSpec] = None
-
     if len(definition) == 1:
         key = list(definition)[0]
         value = definition[key]
@@ -230,7 +231,7 @@ def parse_dict_definition(definition: Dict[str, Any]) -> SelectionSpec:
             dct = {k: v for k, v in dct.items() if k != 'exclude'}
     else:
         raise ValidationException(
-            f'Expected exactly 1 key in the selection definition or "method" '
+            f'Expected either 1 key or else "method" '
             f'and "value" keys, but got {list(definition)}'
         )
 
@@ -242,7 +243,18 @@ def parse_dict_definition(definition: Dict[str, Any]) -> SelectionSpec:
         return SelectionDifference(components=[base, diff_arg])
 
 
-def parse_from_definition(definition: RawDefinition) -> SelectionSpec:
+def parse_from_definition(
+    definition: RawDefinition, rootlevel=False
+) -> SelectionSpec:
+
+    if (isinstance(definition, dict) and
+            ('union' in definition or 'intersection' in definition) and
+            rootlevel and len(definition) > 1):
+        keys = ",".join(definition.keys())
+        raise ValidationException(
+            f"Only a single 'union' or 'intersection' key is allowed "
+            f"in a root level selector definition; found {keys}."
+        )
     if isinstance(definition, str):
         return SelectionCriteria.from_single_spec(definition)
     elif 'union' in definition:
@@ -253,8 +265,8 @@ def parse_from_definition(definition: RawDefinition) -> SelectionSpec:
         return parse_dict_definition(definition)
     else:
         raise ValidationException(
-            f'Expected to find str or dict, instead found '
-            f'{type(definition)}: {definition}'
+            f'Expected to find union, intersection, str or dict, instead '
+            f'found {type(definition)}: {definition}'
         )
 
 
@@ -264,5 +276,6 @@ def parse_from_selectors_definition(
     result: Dict[str, SelectionSpec] = {}
     selector: SelectorDefinition
     for selector in source.selectors:
-        result[selector.name] = parse_from_definition(selector.definition)
+        result[selector.name] = parse_from_definition(selector.definition,
+                                                      rootlevel=True)
     return result

core/dbt/graph/selector_spec.py

@@ -93,7 +93,9 @@ def parse_method(
         try:
             method_name = MethodName(method_parts[0])
         except ValueError as exc:
-            raise InvalidSelectorException(method_parts[0]) from exc
+            raise InvalidSelectorException(
+                f"'{method_parts[0]}' is not a valid method name"
+            ) from exc
 
         method_arguments: List[str] = method_parts[1:]
 

test/unit/test_selector_errors.py

@@ -0,0 +1,179 @@
+import dbt.exceptions
+import textwrap
+import yaml
+import unittest
+from dbt.config.selectors import (
+    selector_config_from_data
+)
+
+from dbt.config.selectors import SelectorConfig
+
+
+def get_selector_dict(txt: str) -> dict:
+    txt = textwrap.dedent(txt)
+    dct = yaml.safe_load(txt)
+    return dct
+
+
+class SelectorUnitTest(unittest.TestCase):
+
+    def test_parse_multiple_excludes(self):
+        dct = get_selector_dict('''\
+            selectors:
+                - name: mult_excl
+                  definition:
+                    union:
+                      - method: tag
+                        value: nightly
+                      - exclude:
+                         - method: tag
+                           value: hourly
+                      - exclude:
+                         - method: tag
+                           value: daily
+            ''')
+        with self.assertRaisesRegex(
+                dbt.exceptions.DbtSelectorsError,
+                'cannot provide multiple exclude arguments'
+        ):
+            selector_config_from_data(dct)
+
+    def test_parse_set_op_plus(self):
+        dct = get_selector_dict('''\
+            selectors:
+                - name: union_plus
+                  definition:
+                    - union:
+                       - method: tag
+                         value: nightly
+                       - exclude:
+                          - method: tag
+                            value: hourly
+                    - method: tag
+                      value: foo
+            ''')
+        with self.assertRaisesRegex(
+                dbt.exceptions.DbtSelectorsError,
+                'Valid root-level selector definitions'
+        ):
+            selector_config_from_data(dct)
+
+    def test_parse_multiple_methods(self):
+        dct = get_selector_dict('''\
+            selectors:
+                - name: mult_methods
+                  definition:
+                    - tag:hourly
+                    - tag:nightly
+                    - fqn:start
+            ''')
+        with self.assertRaisesRegex(
+                dbt.exceptions.DbtSelectorsError,
+                'Valid root-level selector definitions'
+        ):
+            selector_config_from_data(dct)
+
+    def test_parse_set_with_method(self):
+        dct = get_selector_dict('''\
+                selectors:
+                  - name: mixed_syntaxes
+                    definition:
+                      key: value
+                      method: tag
+                      value: foo
+                      union:
+                        - method: tag
+                          value: m1234
+                        - exclude:
+                          - method: tag
+                            value: m5678
+            ''')
+        with self.assertRaisesRegex(
+                dbt.exceptions.DbtSelectorsError,
+                "Only a single 'union' or 'intersection' key is allowed"
+        ):
+            selector_config_from_data(dct)
+
+    def test_complex_sector(self):
+        dct = get_selector_dict('''\
+                selectors:
+                  - name: nightly_diet_snowplow
+                    definition:
+                      union:
+                        - intersection:
+                            - method: source
+                              value: snowplow
+                              childrens_parents: true
+                            - method: tag
+                              value: nightly
+                        - method: path
+                          value: models/export
+                        - exclude:
+                            - intersection:
+                                - method: package
+                                  value: snowplow
+                                - method: config.materialized
+                                  value: incremental
+                            - method: fqn
+                              value: export_performance_timing
+            ''')
+        selectors = selector_config_from_data(dct)
+        assert(isinstance(selectors, SelectorConfig))
+
+    def test_exclude_not_list(self):
+        dct = get_selector_dict('''\
+                selectors:
+                  - name: summa_exclude
+                    definition:
+                      union:
+                        - method: tag
+                          value: nightly
+                        - exclude:
+                            method: tag
+                            value: daily
+            ''')
+        with self.assertRaisesRegex(
+                dbt.exceptions.DbtSelectorsError,
+                "Expected a list"
+        ):
+            selector_config_from_data(dct)
+
+    def test_invalid_key(self):
+        dct = get_selector_dict('''\
+                selectors:
+                  - name: summa_nothing
+                    definition:
+                      method: tag
+                      key: nightly
+            ''')
+        with self.assertRaisesRegex(
+                dbt.exceptions.DbtSelectorsError,
+                "Expected either 1 key"
+        ):
+            selector_config_from_data(dct)
+
+    def test_invalid_single_def(self):
+        dct = get_selector_dict('''\
+                selectors:
+                  - name: summa_nothing
+                    definition:
+                      fubar: tag
+            ''')
+        with self.assertRaisesRegex(
+                dbt.exceptions.DbtSelectorsError,
+                "not a valid method name"
+        ):
+            selector_config_from_data(dct)
+
+    def test_method_no_value(self):
+        dct = get_selector_dict('''\
+                selectors:
+                  - name: summa_nothing
+                    definition:
+                      method: tag
+            ''')
+        with self.assertRaisesRegex(
+                dbt.exceptions.DbtSelectorsError,
+                "not a valid method name"
+        ):
+            selector_config_from_data(dct)