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

0.17.0 regression: Make native env rendering explicit/opt-in #2612

Closed
drewbanin opened this issue Jul 7, 2020 · 1 comment · Fixed by #2618
Closed

0.17.0 regression: Make native env rendering explicit/opt-in #2612

drewbanin opened this issue Jul 7, 2020 · 1 comment · Fixed by #2618
Assignees
Labels
bug Something isn't working enhancement New feature or request
Milestone

Comments

@drewbanin
Copy link
Contributor

drewbanin commented Jul 7, 2020

Describe the bug

See the discussion in #2608 for more information.

dbt v0.17.0 adds a native env renderer for Jinja. This has the great benefit of coercing stringy values like "True" or "False" to boolean values like True and False. It's a subtle distinction, and the benefit is that you can use dbt's jinja context to do really clever things in configuration files, eg:

models:
  my_package:
    optional:
       enabled: "{{ env_var('DBT_OPTIONAL_MODELS_ENABLED") }}

In this example, the env_var function returns a string, but with the native environment, dbt coerces the string to a boolean value. This is necessary because the enabled config requires a boolean value as a configuration.

The drawbacks of this approach are equally subtle, but there are many:

  • numeric strings can be errantly converted into integers, losing information (eg. "00" --> 0)
  • confusingly, this behavior is inconsistent with the current implementation for obscure reasons ("01" --> "01")*
  • values like the empty string ('') are evaluated to None without special handling
  • quotes in quoted strings are stripped as a part of literal evaluation (eg. "'abc'" --> "abc")
  • the literal evaluation lib that dbt uses will coerce "container" types (eg. "[1,2,3]" --> [1,2,3] as a list instead of a string)
  • 01 is not a valid Python literal, so a literal evaluation of the string raises a SyntaxError and dbt returns the source string instead

So, this feature is powerful and compelling (which is why we added it in the first place!) but it frequently results in unexpected behavior if you don't know the gritty details about how dbt's native env rendering works under the hood. That's the exact opposite of our design goals as we march towards a 1.0 release of dbt, so we unfortunately need to take some action to revert the native env changes introduced in 0.17.0 and gate them behind a syntax which makes their use explicit.

This is going to be a breaking change to a breaking change, so we'll need to take care to communicate out why we're changing this and how users will need to adjust their code.

Example code that should work once this issue is addressed:

models:
  my_package:
    optional:
       enabled: "{{ env_var('DBT_OPTIONAL_MODELS_ENABLED") | as_bool }}

The path forwards:

Add a NativeMarker as described in this comment (#2608 (comment)) which can be accessed using an as_native. Default to not calling literal_eval on strings unless the as_native marker is provided.

The as_native filter is good and useful in many cases, but we should also add some sugary filters that are more explicit. as_native isn't super descriptive, and we should take care to make sure that this behavior is well defined and well documented. I propose:

  • <expression> | as_text: same behavior as 0.17.0 (and more or less a no-op in dbt > 0.17.0)
  • <expression> | as_bool: coerce the string to a boolean value. Fail if the input cannot be represented as a bool
  • <expression> | as_number: coerce the string to a number value. Fail if the input cannot be represented as a number (or, do we want as_int / as_float here instead?)
  • <expression> | as_native: coerce the string into its native representation according to ast.literal_eval. That could be a set/list/tuple/dict/etc, and it's on the user to make sure that their values inside the container are evaluated correctly. From my understanding, there isn't really a good way to make sure dbt does the right thing there 100% of the time, which is kind of the crux of this issue!

This might require the addition of some additional Marker classes, but I think there will be a ton of benefit to coupling type inference and validation in the same jinja filter.

Next steps

  • Drew to write something up since functionality churn like this is a little unusual and is blocking our dbt patch/bugfix release
  • We need to determine the version number for the release that includes this change. 0.17.1 and we add warning labels to 0.17.0? Or 0.18.0 and we acknowledge the breaking change as such? TBD
  • Drew to write up lots of docs and some sort of discourse post outlining the whats and the whys
@drewbanin drewbanin added bug Something isn't working enhancement New feature or request labels Jul 7, 2020
@drewbanin drewbanin added this to the 0.17.1 milestone Jul 7, 2020
@jtcohen6 jtcohen6 linked a pull request Jul 7, 2020 that will close this issue
4 tasks
@jtcohen6
Copy link
Contributor

jtcohen6 commented Jul 8, 2020

Resolved by #2618!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants