Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow vars to be set to null and differentiate them from unset vars (#608) #1426

Merged
merged 1 commit into from
Apr 30, 2019
Merged

Allow vars to be set to null and differentiate them from unset vars (#608) #1426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 3 additions & 17 deletions core/dbt/context/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,8 +206,7 @@ def log(msg, info=False):
class Var(object):
UndefinedVarError = "Required var '{}' not found in config:\nVars "\
"supplied to {} = {}"
NoneVarError = "Supplied var '{}' is undefined in config:\nVars supplied "\
"to {} = {}"
_VAR_NOTSET = object()

def __init__(self, model, context, overrides):
self.model = model
Expand Down Expand Up @@ -241,7 +240,7 @@ def pretty_dict(self, data):
return json.dumps(data, sort_keys=True, indent=4)

def assert_var_defined(self, var_name, default):
if var_name not in self.local_vars and default is None:
if var_name not in self.local_vars and default is self._VAR_NOTSET:
pretty_vars = self.pretty_dict(self.local_vars)
dbt.exceptions.raise_compiler_error(
self.UndefinedVarError.format(
Expand All @@ -250,25 +249,12 @@ def assert_var_defined(self, var_name, default):
self.model
)

def assert_var_not_none(self, var_name):
raw = self.local_vars[var_name]
if raw is None:
pretty_vars = self.pretty_dict(self.local_vars)
dbt.exceptions.raise_compiler_error(
self.NoneVarError.format(
var_name, self.model_name, pretty_vars
),
self.model
)

def __call__(self, var_name, default=None):
def __call__(self, var_name, default=_VAR_NOTSET):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, supplying an object (or non-primitive) as a default argument means that Python uses the pointer to that object as the default value for all invocations of a method. This is the thing you're taking advantage of here, right?

This might be way off the mark, but... is this... thread-safe? Is _VAR_NOTSET initialized when the common.py file is imported? This is a neat construct!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly! Python actually guarantees that supplying any value as a default argument means that Python uses that object as the default value. The tricky part that confuses people is that there is in fact only one each of None, True, and False. The id() of an object is guaranteed to be unique, and a is b basically checks id(a) == id(b).

This is a somewhat common technique for handling this irritating case of wanting to distinguish between None and "not set" on an optional arg. The other way I've seen is doing **kwargs and checking what's in there, which I'm not really fond of.

I'm not sure where I first stumbled across it, but I have been using it for a long time and I sure didn't come up with it on my own. Because it's the default argument, the user can never intentionally pass it, so it can only arrive on a function signature. I don't think it's possible to go wrong here from a thread/process safety perspective. You probably shouldn't subclass and override this method or the value though!

If you need even more comfort: Because the variable is defined at the class level, and the class is defined at the module level, the identity of the object is defined at module import time. Python guarantees that module loading is thread-safe by having a lock around imports, so you know that this value will only be set once and be visible to all threads.

self.assert_var_defined(var_name, default)

if var_name not in self.local_vars:
return default

self.assert_var_not_none(var_name)

raw = self.local_vars[var_name]

# if bool/int/float/etc are passed in, don't compile anything
Expand Down
19 changes: 12 additions & 7 deletions test/unit/test_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,19 @@ def setUp(self):
)
self.context = mock.MagicMock()

def test_var_not_none_is_none(self):
def test_var_default_something(self):
var = Var(self.model, self.context, overrides={'foo': 'baz'})
self.assertEqual(var('foo'), 'baz')
self.assertEqual(var('foo', 'bar'), 'baz')

def test_var_default_none(self):
var = Var(self.model, self.context, overrides={'foo': None})
var.assert_var_defined('foo', None)
with self.assertRaises(dbt.exceptions.CompilationException):
var.assert_var_not_none('foo')
self.assertEqual(var('foo'), None)
self.assertEqual(var('foo', 'bar'), None)

def test_var_defined_is_missing(self):
def test_var_not_defined(self):
var = Var(self.model, self.context, overrides={})
var.assert_var_defined('foo', 'bar')

self.assertEqual(var('foo', 'bar'), 'bar')
with self.assertRaises(dbt.exceptions.CompilationException):
var.assert_var_defined('foo', None)
var('foo')