make undefined strict when not capturing macros (#1389)

[beckjake]

Fixes #1389

During the non-macro-capture part of dbt parsing, set the undefined handling to 'strict' - currently we let jinja use its default, which is permissive - many times, overly so.

I expect this will break a good number of macros/models/materializations in end user code.

A fix for a lot of issues is, if x.y now raises an error, try x.get('y', '').

[drewbanin]

@beckjake this does look great, but I am just realizing exactly how many macros out in the wild are going to be totally broken once this is merged.

I think this is in the category of "bandaids we should rip off sooner rather than later". Let me play around with this on some example projects to determine the extent of the damage.

[beckjake]

We could subclass that class to have it go through a WARN_ERROR check, and then users could --strict to turn warnings into errors.

[drewbanin]

Will that approach arrest parsing? I imagine that once the exception is thrown, dbt isn't going to actually parse the rest of the file, right?

This is broadly what I had in mind, but I was picturing either 1) tucking this thing behind --strict mode, or (maybe less desirably) parsing twice, once with StrictUndefined to catch any errors (and throwing a warning) and then with the non-strict Undefined to actually parse the files. I don't think this is necessarily a good idea, but wanted to throw it out there

[beckjake]

I think we're saying the same thing - I'm just saying you'd do something along these lines (untested, but seems reasonable enough):

class WarnUndefined(jinja2.StrictUndefined):
    def _fail_with_undefined_warning(self, *args, **kwargs):
        try:
            self._fail_with_undefined_error(*args, **kwargs)
        except Exception as exc:
            if dbt.flags.WARN_ERROR:
                raise
            else:
                logger.warning('WARNING - Undefined jinja: {}'.format(str(exc)))

    __slots__ = ()
    __iter__ = __str__ = __len__ = __nonzero__ = __eq__ = \
    __ne__ = __bool__ = __hash__ = \
        _fail_with_undefined_warning

args['undefined'] = WarnUndefined

Following this pattern: https://github.com/pallets/jinja/blob/a7ba0b637805c53d442e975e3864d3ea38d8743f/jinja2/runtime.py#L801-L803

[drewbanin]

Oh, ok, I think i might misunderstand how this jinja handles undefined values. Sure, let's try something like this! I will ping you to make sure I have the right idea :)

[clrcrl]

Does this close #1389?

[drewbanin]

@beckjake this is super close! Are we able to add a WARNING in the event that an undefined value is encountered, but --warn-error is not provided? At is stands, there's no indication that dbt encountered an undefined variable unless --warn-error is supplied, which doesn't feel right to me.

I think we can use self._undefined_name to help print the warning message

[beckjake]

I think we can use self._undefined_name to help print the warning message

So, I definitely could rewrite the function that generates the message and hope it doesn't change in future jinja versions. That said, how much would you object to just doing:

try:
    self._fail_with_undefined_error(*args, **kwargs)
except Exception as exc:
    if dbt.flags.WARN_ERROR:
        raise
    else:
        logger.warning('WARNING: {}'.format(str(exc)))

[drewbanin]

I'm ok with that! Is there a reason we wouldn't go through the existing warn_or_error codepath?

The other thing I would love would be to add info about the resource where the undefined variable was found. Is that something that Jinja would make easy for us? I'm picturing:

WARNING: <exception text> in model models/abc.sql

Just looking at the code though, I'm not sure how we'd be able to snag that from the WarnUndefined scope

[beckjake]

I'm ok with that! Is there a reason we wouldn't go through the existing warn_or_error codepath?

Just that I feel super comfortable letting jinja raise the error and generate a great error message in all sorts of weird edge cases (where does hint come from and what kind of stuff can it get up to? I sure don't know!).

[beckjake]

If we let jinja do the reporting, I believe we'll catch it here and display something pleasant that includes the file path, right?

Ah, I see - only if --strict is set. That's very annoying.

[drewbanin]

Ah, I meant specifically for the WARNING case, I assumed we'd decorate the error message for an exception... maybe that's not the case....

Let's get this bad boy merged with just a warning log line. Don't worry about the model path -- we're not doing a good job of handling that anywhere at the moment. Presumably if some var is undefined, it will be pretty easy to cmd+f for :)

[beckjake]

Let's get this bad boy merged with just a warning log line. Don't worry about the model path -- we're not doing a good job of handling that anywhere at the moment. Presumably if some var is undefined, it will be pretty easy to cmd+f for :)

Are you sure? I've come up with a reasonably nice solution for it and it's actually not too hard.

[drewbanin]

I take it all back! Yeah, give it a shot then!

[beckjake]

On warnings it now looks like this:

$ dbt compile
Running with dbt=0.13.0
Found 1 models, 0 tests, 0 archives, 0 analyses, 101 macros, 0 operations, 0 seed files, 0 sources

19:42:39 | Concurrency: 2 threads (target='default')
19:42:39 |
COMPILATION WARNING in model x (models/x.sql)
	'value' is undefined
19:42:40 | Done.

[drewbanin]

One tiny nitpick on the warning message, but otherwise this is ready to go! It's great to slot in UX niceties like this alongside more fundamental updates/features