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.

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

Handle undefined variables in jinja better (#935) #1086

Merged
merged 3 commits into from
Oct 23, 2018

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Oct 23, 2018

Fixes #935 (I think!)
There were some issues with ParsedMacroCapture and its inheritance from jinja2.Undefined that were causing issues.
In particular, ParsedMacroCapture's super() call was calling object.__init__ as it had the wrong class as its first argument. This meant that the underlying machinery for jinja2.Undefined wasn't getting set up. Once that was fixed, I added in the hint and name parameters so we can get nice messages

The file in question:

select * from {{ ref(thing+'a') }}

Old (snipped the stacktrace for brevity):

Encountered an error:
exceptions must derive from BaseException
Traceback (most recent call last):
  ...
  File "dbt-0b0c26f75515b90cb4d064c0", line 1, in top-level template code
    from __future__ import division, generator_stop
TypeError: exceptions must derive from BaseException

New:

Encountered an error:
Compilation Error in model x (models/x.sql)
  'thing' is undefined

Another kind of bad file:

{{ config(something=TRUE) }}
select 1 as id

Old: Same bad stuff

New:

Encountered an error:
Compilation Error in model x (models/x.sql)
  'TRUE' is undefined

Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

LGTM when the tests pass!

@beckjake beckjake changed the base branch from fix/more-helpful-invalid-refs to dev/guion-bluford October 23, 2018 15:53
@drewbanin
Copy link
Contributor

@beckjake I think this can also happen in config blocks, so maybe it makes sense to revise our error message? For instance, this class of error will also be caught by the new code:

{{ config(something=TRUE) }}

Here, TRUE is an undefined variable, and we see:

dbt has detected at least one invalid reference in my_project.idk. Check logs for more information

which is probably not ideal

@beckjake
Copy link
Contributor Author

beckjake commented Oct 23, 2018

I changed the error in __deepcopy__ to exactly replicate the message jinja itself emits, so we always get the same (reasonably useful) output and updated the PR description accordingly.

@drewbanin
Copy link
Contributor

ship it!

@beckjake beckjake merged commit 92670cb into dev/guion-bluford Oct 23, 2018
@beckjake beckjake deleted the fix/undefined-error-cleanup branch October 23, 2018 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants