diff --git a/CHANGELOG.md b/CHANGELOG.md index f835be05f18..3e3919bd708 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ ## dbt 0.17.1 (Release TBD) +### Fixes +- dbt native rendering now requires an opt-in with the `as_native` filter. Added `as_bool` and `as_number` filters, which are like `as_native` but also type-check. ([#2612](https://github.com/fishtown-analytics/dbt/issues/2612), [#2618](https://github.com/fishtown-analytics/dbt/pull/2618)) + + ## dbt 0.17.1rc3 (July 01, 2020) diff --git a/core/dbt/clients/jinja.py b/core/dbt/clients/jinja.py index 5ebb206042f..37586fdda6e 100644 --- a/core/dbt/clients/jinja.py +++ b/core/dbt/clients/jinja.py @@ -8,7 +8,8 @@ from contextlib import contextmanager from itertools import chain, islice from typing import ( - List, Union, Set, Optional, Dict, Any, Iterator, Type, NoReturn, Tuple + List, Union, Set, Optional, Dict, Any, Iterator, Type, NoReturn, Tuple, + Callable ) import jinja2 @@ -28,7 +29,7 @@ from dbt.contracts.graph.parsed import ParsedSchemaTestNode from dbt.exceptions import ( InternalException, raise_compiler_error, CompilationException, - invalid_materialization_argument, MacroReturn + invalid_materialization_argument, MacroReturn, JinjaRenderingException ) from dbt.flags import MACRO_DEBUGGING from dbt.logger import GLOBAL_LOGGER as logger # noqa @@ -111,6 +112,24 @@ class TextMarker(str): """ +class NativeMarker(str): + """A special native-env marker that indicates the field should be passed to + literal_eval. + """ + + +class BoolMarker(NativeMarker): + pass + + +class NumberMarker(NativeMarker): + pass + + +def _is_number(value) -> bool: + return isinstance(value, (int, float)) and not isinstance(value, bool) + + def quoted_native_concat(nodes): """This is almost native_concat from the NativeTemplate, except in the special case of a single argument that is a quoted string and returns a @@ -119,23 +138,31 @@ def quoted_native_concat(nodes): head = list(islice(nodes, 2)) if not head: - return None + return '' if len(head) == 1: raw = head[0] if isinstance(raw, TextMarker): return str(raw) + elif not isinstance(raw, NativeMarker): + # return non-strings as-is + return raw else: - raw = "".join([str(v) for v in chain(head, nodes)]) + # multiple nodes become a string. + return "".join([str(v) for v in chain(head, nodes)]) try: result = literal_eval(raw) except (ValueError, SyntaxError, MemoryError): - return raw - - # if it was a str and it still is a str, return it as-is. - if isinstance(result, str): result = raw + if isinstance(raw, BoolMarker) and not isinstance(result, bool): + raise JinjaRenderingException( + f"Could not convert value '{raw!s}' into type 'bool'" + ) + if isinstance(raw, NumberMarker) and not _is_number(result): + raise JinjaRenderingException( + f"Could not convert value '{raw!s}' into type 'number'" + ) return result @@ -417,6 +444,22 @@ def __reduce__(self): return Undefined +NATIVE_FILTERS: Dict[str, Callable[[Any], Any]] = { + 'as_text': TextMarker, + 'as_bool': BoolMarker, + 'as_native': NativeMarker, + 'as_number': NumberMarker, +} + + +TEXT_FILTERS: Dict[str, Callable[[Any], Any]] = { + 'as_text': lambda x: x, + 'as_bool': lambda x: x, + 'as_native': lambda x: x, + 'as_number': lambda x: x, +} + + def get_environment( node=None, capture_macros: bool = False, @@ -436,13 +479,13 @@ def get_environment( text_filter: Type if native: env_cls = NativeSandboxEnvironment - text_filter = TextMarker + filters = NATIVE_FILTERS else: env_cls = MacroFuzzEnvironment - text_filter = str + filters = TEXT_FILTERS env = env_cls(**args) - env.filters['as_text'] = text_filter + env.filters.update(filters) return env diff --git a/core/dbt/config/profile.py b/core/dbt/config/profile.py index b3c145348ca..f5de7a0d323 100644 --- a/core/dbt/config/profile.py +++ b/core/dbt/config/profile.py @@ -8,6 +8,7 @@ from dbt.clients.yaml_helper import load_yaml_text from dbt.contracts.connection import Credentials, HasCredentials from dbt.contracts.project import ProfileConfig, UserConfig +from dbt.exceptions import CompilationException from dbt.exceptions import DbtProfileError from dbt.exceptions import DbtProjectError from dbt.exceptions import ValidationException @@ -268,7 +269,10 @@ def render_profile( raw_profile, profile_name, target_name ) - profile_data = renderer.render_data(raw_profile_data) + try: + profile_data = renderer.render_data(raw_profile_data) + except CompilationException as exc: + raise DbtProfileError(str(exc)) from exc return target_name, profile_data @classmethod diff --git a/core/dbt/exceptions.py b/core/dbt/exceptions.py index f5d539c8f00..e56642cf23c 100644 --- a/core/dbt/exceptions.py +++ b/core/dbt/exceptions.py @@ -256,6 +256,10 @@ def __reduce__(self): return (JSONValidationException, (self.typename, self.errors)) +class JinjaRenderingException(CompilationException): + pass + + class UnknownAsyncIDException(Exception): CODE = 10012 MESSAGE = 'RPC server got an unknown async ID' diff --git a/test/integration/039_config_test/test_configs.py b/test/integration/039_config_test/test_configs.py index 1246d72be71..db9823ed3bc 100644 --- a/test/integration/039_config_test/test_configs.py +++ b/test/integration/039_config_test/test_configs.py @@ -110,9 +110,9 @@ def postgres_profile(self): 'default2': { 'type': 'postgres', # make sure you can do this and get an int out - 'threads': "{{ 1 + 3 }}", + 'threads': "{{ (1 + 3) | as_number }}", 'host': self.database_host, - 'port': "{{ 5400 + 32 }}", + 'port': "{{ (5400 + 32) | as_number }}", 'user': 'root', 'pass': 'password', 'dbname': 'dbt', @@ -121,9 +121,9 @@ def postgres_profile(self): 'disabled': { 'type': 'postgres', # make sure you can do this and get an int out - 'threads': "{{ 1 + 3 }}", + 'threads': "{{ (1 + 3) | as_number }}", 'host': self.database_host, - 'port': "{{ 5400 + 32 }}", + 'port': "{{ (5400 + 32) | as_number }}", 'user': 'root', 'pass': 'password', 'dbname': 'dbt', @@ -141,7 +141,7 @@ def project_config(self): 'data-paths': ['data'], 'models': { 'test': { - 'enabled': "{{ target.name == 'default2' }}", + 'enabled': "{{ (target.name == 'default2' | as_bool) }}", }, }, # set the `var` result in schema.yml to be 'seed', so that the @@ -155,7 +155,7 @@ def project_config(self): 'quote_columns': False, 'test': { 'seed': { - 'enabled': "{{ target.name == 'default2' }}", + 'enabled': "{{ (target.name == 'default2') | as_bool }}", }, }, }, diff --git a/test/integration/base.py b/test/integration/base.py index 6ae5a62fe2e..d6c181d9cf8 100644 --- a/test/integration/base.py +++ b/test/integration/base.py @@ -184,7 +184,7 @@ def redshift_profile(self): 'type': 'redshift', 'threads': 1, 'host': os.getenv('REDSHIFT_TEST_HOST'), - 'port': os.getenv('REDSHIFT_TEST_PORT'), + 'port': int(os.getenv('REDSHIFT_TEST_PORT')), 'user': os.getenv('REDSHIFT_TEST_USER'), 'pass': os.getenv('REDSHIFT_TEST_PASS'), 'dbname': os.getenv('REDSHIFT_TEST_DBNAME'), diff --git a/test/unit/test_config.py b/test/unit/test_config.py index 11f8889e133..058f2cf233d 100644 --- a/test/unit/test_config.py +++ b/test/unit/test_config.py @@ -131,7 +131,7 @@ def setUp(self): 'with-vars': { 'type': "{{ env_var('env_value_type') }}", 'host': "{{ env_var('env_value_host') }}", - 'port': "{{ env_var('env_value_port') }}", + 'port': "{{ env_var('env_value_port') | as_number }}", 'user': "{{ env_var('env_value_user') }}", 'pass': "{{ env_var('env_value_pass') }}", 'dbname': "{{ env_var('env_value_dbname') }}", @@ -140,7 +140,7 @@ def setUp(self): 'cli-and-env-vars': { 'type': "{{ env_var('env_value_type') }}", 'host': "{{ var('cli_value_host') }}", - 'port': "{{ env_var('env_value_port') }}", + 'port': "{{ env_var('env_value_port') | as_number }}", 'user': "{{ env_var('env_value_user') }}", 'pass': "{{ env_var('env_value_pass') }}", 'dbname': "{{ env_var('env_value_dbname') }}", @@ -367,7 +367,7 @@ def test_invalid_env_vars(self): renderer, target_override='with-vars' ) - self.assertIn("not of type 'integer'", str(exc.exception)) + self.assertIn("Could not convert value 'hello' into type 'number'", str(exc.exception)) class TestProfileFile(BaseFileTest): @@ -511,7 +511,7 @@ def test_invalid_env_vars(self): with self.assertRaises(dbt.exceptions.DbtProfileError) as exc: self.from_args() - self.assertIn("not of type 'integer'", str(exc.exception)) + self.assertIn("Could not convert value 'hello' into type 'number'", str(exc.exception)) def test_cli_and_env_vars(self): self.args.target = 'cli-and-env-vars' diff --git a/test/unit/test_jinja.py b/test/unit/test_jinja.py index b3d273f4b65..d0d780dcb52 100644 --- a/test/unit/test_jinja.py +++ b/test/unit/test_jinja.py @@ -1,3 +1,4 @@ +from contextlib import contextmanager import pytest import unittest import yaml @@ -5,7 +6,373 @@ from dbt.clients.jinja import get_rendered from dbt.clients.jinja import get_template from dbt.clients.jinja import extract_toplevel_blocks -from dbt.exceptions import CompilationException +from dbt.exceptions import CompilationException, JinjaRenderingException + + +@contextmanager +def returns(value): + yield value + + +@contextmanager +def raises(value): + with pytest.raises(value) as exc: + yield exc + + +def expected_id(arg): + if isinstance(arg, list): + return '_'.join(arg) + + +jinja_tests = [ + # strings + ( + '''foo: bar''', + returns('bar'), + returns('bar'), + ), + ( + '''foo: "bar"''', + returns('bar'), + returns('bar'), + ), + ( + '''foo: "'bar'"''', + returns("'bar'"), + returns("'bar'"), + ), + ( + """foo: '"bar"'""", + returns('"bar"'), + returns('"bar"'), + ), + ( + '''foo: "{{ 'bar' | as_text }}"''', + returns('bar'), + returns('bar'), + ), + ( + '''foo: "{{ 'bar' | as_bool }}"''', + returns('bar'), + raises(JinjaRenderingException), + ), + ( + '''foo: "{{ 'bar' | as_number }}"''', + returns('bar'), + raises(JinjaRenderingException), + ), + ( + '''foo: "{{ 'bar' | as_native }}"''', + returns('bar'), + returns('bar'), + ), + # ints + ( + '''foo: 1''', + returns('1'), + returns('1'), + ), + ( + '''foo: "1"''', + returns('1'), + returns('1'), + ), + ( + '''foo: "'1'"''', + returns("'1'"), + returns("'1'"), + ), + ( + """foo: '"1"'""", + returns('"1"'), + returns('"1"'), + ), + ( + '''foo: "{{ 1 }}"''', + returns('1'), + returns('1'), + ), + ( + '''foo: "{{ '1' }}"''', + returns('1'), + returns('1'), + ), + ( + '''foo: "'{{ 1 }}'"''', + returns("'1'"), + returns("'1'"), + ), + ( + '''foo: "'{{ '1' }}'"''', + returns("'1'"), + returns("'1'"), + ), + ( + '''foo: "{{ 1 | as_text }}"''', + returns('1'), + returns('1'), + ), + ( + '''foo: "{{ 1 | as_bool }}"''', + returns('1'), + raises(JinjaRenderingException), + ), + ( + '''foo: "{{ 1 | as_number }}"''', + returns('1'), + returns(1), + ), + ( + '''foo: "{{ 1 | as_native }}"''', + returns('1'), + returns(1), + ), + ( + '''foo: "{{ '1' | as_text }}"''', + returns('1'), + returns('1'), + ), + ( + '''foo: "{{ '1' | as_bool }}"''', + returns('1'), + raises(JinjaRenderingException), + ), + ( + '''foo: "{{ '1' | as_number }}"''', + returns('1'), + returns(1), + ), + ( + '''foo: "{{ '1' | as_native }}"''', + returns('1'), + returns(1), + ), + # booleans. + # Note the discrepancy with true vs True: `true` is recognized by jinja but + # not literal_eval, but `True` is recognized by ast.literal_eval. + # For extra fun, yaml recognizes both. + # unquoted true + ( + '''foo: "{{ True }}"''', + returns('True'), + returns('True'), + ), + ( + '''foo: "{{ True | as_text }}"''', + returns('True'), + returns('True'), + ), + ( + '''foo: "{{ True | as_bool }}"''', + returns('True'), + returns(True), + ), + ( + '''foo: "{{ True | as_number }}"''', + returns('True'), + raises(JinjaRenderingException), + ), + ( + '''foo: "{{ True | as_native }}"''', + returns('True'), + returns(True), + ), + # unquoted true + ( + '''foo: "{{ true }}"''', + returns("True"), + returns("True"), + ), + ( + '''foo: "{{ true | as_text }}"''', + returns("True"), + returns("True"), + ), + ( + '''foo: "{{ true | as_bool }}"''', + returns("True"), + returns(True), + ), + ( + '''foo: "{{ true | as_number }}"''', + returns("True"), + raises(JinjaRenderingException), + ), + ( + '''foo: "{{ true | as_native }}"''', + returns("True"), + returns(True), + ), + ( + '''foo: "{{ 'true' | as_text }}"''', + returns("true"), + returns("true"), + ), + # quoted 'true' + ( + '''foo: "'{{ true }}'"''', + returns("'True'"), + returns("'True'"), + ), # jinja true -> python True -> str(True) -> "True" -> quoted + ( + '''foo: "'{{ true | as_text }}'"''', + returns("'True'"), + returns("'True'"), + ), + ( + '''foo: "'{{ true | as_bool }}'"''', + returns("'True'"), + returns("'True'"), + ), + ( + '''foo: "'{{ true | as_number }}'"''', + returns("'True'"), + returns("'True'"), + ), + ( + '''foo: "'{{ true | as_native }}'"''', + returns("'True'"), + returns("'True'"), + ), + # unquoted True + ( + '''foo: "{{ True }}"''', + returns('True'), + returns('True'), + ), + ( + '''foo: "{{ True | as_text }}"''', + returns("True"), + returns("True"), + ), # True -> string 'True' -> text -> str('True') -> 'True' + ( + '''foo: "{{ True | as_bool }}"''', + returns("True"), + returns(True), + ), + ( + '''foo: "{{ True | as_number }}"''', + returns("True"), + raises(JinjaRenderingException), + ), + ( + '''foo: "{{ True | as_native }}"''', + returns("True"), + returns(True), + ), + # quoted 'True' within rendering + ( + '''foo: "{{ 'True' | as_text }}"''', + returns("True"), + returns("True"), + ), + # 'True' -> string 'True' -> text -> str('True') -> 'True' + ( + '''foo: "{{ 'True' | as_bool }}"''', + returns('True'), + returns(True), + ), + # quoted 'True' outside rendering + ( + '''foo: "'{{ True }}'"''', + returns("'True'"), + returns("'True'"), + ), + ( + '''foo: "'{{ True | as_bool }}'"''', + returns("'True'"), + returns("'True'"), + ), + # yaml turns 'yes' into a boolean true + ( + '''foo: yes''', + returns('True'), + returns('True'), + ), + ( + '''foo: "yes"''', + returns('yes'), + returns('yes'), + ), + # concatenation + ( + '''foo: "{{ (a_int + 100) | as_native }}"''', + returns('200'), + returns(200), + ), + ( + '''foo: "{{ (a_str ~ 100) | as_native }}"''', + returns('100100'), + returns(100100), + ), + ( + '''foo: "{{( a_int ~ 100) | as_native }}"''', + returns('100100'), + returns(100100), + ), + # multiple nodes -> always str + ( + '''foo: "{{ a_str | as_native }}{{ a_str | as_native }}"''', + returns('100100'), + returns('100100'), + ), + ( + '''foo: "{{ a_int | as_native }}{{ a_int | as_native }}"''', + returns('100100'), + returns('100100'), + ), + ( + '''foo: "'{{ a_int | as_native }}{{ a_int | as_native }}'"''', + returns("'100100'"), + returns("'100100'"), + ), + ( + '''foo:''', + returns('None'), + returns('None'), + ), + ( + '''foo: null''', + returns('None'), + returns('None'), + ), + ( + '''foo: ""''', + returns(''), + returns(''), + ), + ( + '''foo: "{{ '' | as_native }}"''', + returns(''), + returns(''), + ), + # very annoying, but jinja 'none' is yaml 'null'. + ( + '''foo: "{{ none | as_native }}"''', + returns('None'), + returns(None), + ), +] + + +@pytest.mark.parametrize( + 'value,text_expectation,native_expectation', + jinja_tests, + ids=expected_id +) +def test_jinja_rendering(value, text_expectation, native_expectation): + foo_value = yaml.safe_load(value)['foo'] + ctx = { + 'a_str': '100', + 'a_int': 100, + 'b_str': 'hello' + } + with text_expectation as text_result: + assert text_result == get_rendered(foo_value, ctx, native=False) + + with native_expectation as native_result: + assert native_result == get_rendered(foo_value, ctx, native=True) class TestJinja(unittest.TestCase): @@ -17,25 +384,25 @@ def test_do(self): self.assertEqual(mod.my_dict, {'a': 1}) def test_regular_render(self): - s = '{{ "some_value" }}' + s = '{{ "some_value" | as_native }}' value = get_rendered(s, {}, native=False) assert value == 'some_value' - s = '{{ 1991 }}' + s = '{{ 1991 | as_native }}' value = get_rendered(s, {}, native=False) assert value == '1991' s = '{{ "some_value" | as_text }}' - value = get_rendered(s, {}, native=True) + value = get_rendered(s, {}, native=False) assert value == 'some_value' s = '{{ 1991 | as_text }}' - value = get_rendered(s, {}, native=True) + value = get_rendered(s, {}, native=False) assert value == '1991' def test_native_render(self): - s = '{{ "some_value" }}' + s = '{{ "some_value" | as_native }}' value = get_rendered(s, {}, native=True) assert value == 'some_value' - s = '{{ 1991 }}' + s = '{{ 1991 | as_native }}' value = get_rendered(s, {}, native=True) assert value == 1991 @@ -413,65 +780,3 @@ def test_if_endfor_newlines(self): hi {% endmaterialization %} ''' - - -native_expected_behaviors = [ - # strings - ('''foo: bar''', 'bar'), - ('''foo: "bar"''', 'bar'), - ('''foo: "'bar'"''', "'bar'"), - ("""foo: '"bar"'""", '"bar"'), - # ints - ('''foo: 1''', 1), - ('''foo: "1"''', 1), - ('''foo: "'1'"''', "'1'"), - ('''foo: "{{ 1 }}"''', 1), - ('''foo: "{{ '1' }}"''', 1), - ('''foo: "'{{ 1 }}'"''', "'1'"), - ('''foo: "'{{ '1' }}'"''', "'1'"), - ('''foo: "{{ 1 | as_text }}"''', '1'), - ('''foo: "{{ '1' | as_text }}"''', '1'), - # booleans. - # Note the discrepancy with true vs True: `true` is recognized by jinja but - # not literal_eval, but `True` is recognized by ast.literal_eval. - # For extra fun, yaml recognizes both. - ('''foo: "{{ true }}"''', True), - ('''foo: "{{ 'true' }}"''', 'true'), - ('''foo: "'{{ true }}'"''', "'True'"), - ('''foo: "{{ true | as_text }}"''', "True"), # true -> boolean True -> text -> str(True) -> 'True' - ('''foo: "{{ 'true' | as_text }}"''', "true"), # 'true' -> string 'true' -> text -> str('true') -> 'true' - ('''foo: "{{ True }}"''', True), - ('''foo: "{{ 'True' }}"''', True), - ('''foo: "'{{ True }}'"''', "'True'"), - ('''foo: "{{ True | as_text }}"''', "True"), # True -> string 'True' -> text -> str('True') -> 'True' - ('''foo: "{{ 'True' | as_text }}"''', "True"), # 'True' -> string 'True' -> text -> str('True') -> 'True' - ('''foo: yes''', True), # yaml turns 'yes' into a boolean true - ('''foo: "yes"''', "yes"), - # concatenation - ('''foo: "{{ a_int + 100 }}"''', 200), - ('''foo: "{{ a_str ~ 100 }}"''', 100100), - ('''foo: "{{ a_int ~ 100 }}"''', 100100), - ('''foo: "{{ a_str }}{{ a_str }}"''', 100100), - ('''foo: "{{ a_int }}{{ a_int }}"''', 100100), - ('''foo: "'{{ a_int }}{{ a_int }}'"''', "'100100'"), - -] - - -def expected_id(arg): - if isinstance(arg, list): - return '_'.join(arg) - - -@pytest.mark.parametrize( - 'inputvalue,expected', native_expected_behaviors, ids=expected_id -) -def test_native_rendering(inputvalue, expected): - # this test is pretty useless without preprocessing things in yaml. - value = yaml.safe_load(inputvalue)['foo'] - ctx = { - 'a_str': '100', - 'a_int': 100, - 'b_str': 'hello' - } - assert get_rendered(value, ctx, native=True) == expected