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

[Regression] all --vars of run_results.json artifact are casted to str but they were not before #10421

Closed
2 tasks done
fabientra opened this issue Jul 9, 2024 · 3 comments · Fixed by dbt-labs/dbt-common#165
Labels
bug Something isn't working regression retry

Comments

@fabientra
Copy link

Is this a regression in a recent version of dbt-core?

  • I believe this is a regression in dbt-core functionality
  • I have searched the existing issues, and I could not find an existing issue for this regression

Current Behavior

Our dbt retry using a variable from CLI of type INT are failing because during the retry dbt sees it as STR.

I compared the run_results.json from dbt 1.8 to run_results.json from dbt 1.7 and I can see that the one from 1.8 have all the vars as STR whereas from 1.7 they kept their original type.

Expected/Previous Behavior

vars in run_results.json should keep the same type as the one give through the CLI

Steps To Reproduce

  1. with dbt 1.8, run a dbt run command with a variable of type int
  2. look at the run_results.json artifact

Relevant log output

No response

Environment

- OS:MacOS Sonoma 14.5
- Python: Python 3.10.11
- dbt (working version): 1.7
- dbt (regression version): 1.8

Which database adapter are you using with dbt?

redshift

Additional Context

I believe the regression is caused by this PR that applies the scrubbing function to ALL cli variables whereas this function cast its input to str systematically

def scrub_secrets(msg: str, secrets: List[str]) -> str:
    scrubbed = str(msg)

    for secret in secrets:
        scrubbed = scrubbed.replace(secret, "*****")

    return scrubbed
@fabientra fabientra added bug Something isn't working regression triage labels Jul 9, 2024
@fabientra fabientra changed the title [Regression] all --vars of run_resulsts artifact are casted to str but they were not before [Regression] all --vars of run_results artifact are casted to str but they were not before Jul 9, 2024
@dbeatty10 dbeatty10 added the retry label Jul 9, 2024
@dbeatty10
Copy link
Contributor

Thanks for reporting this @fabientra and identifying the originating PR 👑

I was able to reproduce this (see details in "Reprex" below).

Possible fix

A possible fix might be leaving msg as-if here unless there were any secrets scrubbed:

def scrub_secrets(msg: str, secrets: List[str]) -> str:
    scrubbed = str(msg)

    for secret in secrets:
        scrubbed = scrubbed.replace(secret, "*****")

    return msg if str(msg) == scrubbed else scrubbed

i.e. just replace the return statement here with this:

    return msg if str(msg) == scrubbed else scrubbed

Of course you'll probably still get an error during dbt retry if you included a secret within your --vars, but that's acceptable because:

  1. we'd hope folks don't include secrets in --vars, and
  2. we explicitly don't want to write out secrets in plain text (which would be required for retry).

Reprex

Start with a models/my_model.sql like this:

{% if execute %}
  {{ exceptions.raise_compiler_error("Force a failure") }}
{% endif %}

{% if not var("id") is number %}
  {{ exceptions.raise_compiler_error("var('id') is not a number") }}
{% endif %}

select 1 as id

And try to run it like this (which will yield an error):

dbt run -s my_model --vars '{"id": 2}'

Then remove these first 3 lines from models/my_model.sql so that it no longer will error this specific way:

{% if execute %}
  {{ exceptions.raise_compiler_error("Force a failure") }}
{% endif %}

And retry it:

dbt retry

@dbeatty10 dbeatty10 removed the triage label Jul 10, 2024
@dbeatty10
Copy link
Contributor

For completeness, we'd probably want to update the type hints as well:

def scrub_secrets(msg: Any, secrets: List[str]) -> Any:

@fabientra
Copy link
Author

Thank you for the fix @dbeatty10 :)

@dbeatty10 dbeatty10 changed the title [Regression] all --vars of run_results artifact are casted to str but they were not before [Regression] all --vars of run_results.json artifact are casted to str but they were not before Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working regression retry
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants