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

[Bug] cli.make_context() is not idempotent when passing the same args #9787

Closed
2 tasks done
dbeatty10 opened this issue Mar 20, 2024 · 1 comment · Fixed by #9809
Closed
2 tasks done

[Bug] cli.make_context() is not idempotent when passing the same args #9787

dbeatty10 opened this issue Mar 20, 2024 · 1 comment · Fixed by #9809
Labels
bug Something isn't working

Comments

@dbeatty10
Copy link
Contributor

Is this a new bug in dbt-core?

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

Current Behavior

Calling cli.make_context() twice with the same args leads to a failing test upon the 2nd invocation.

Expected Behavior

I'd expect cli.make_context() to be idempotent like looking at a slice of cake rather than eating a slice of cake. e.g., I should be able to call it with the same args object repeatedly and get the same result.

Steps To Reproduce

Add the following test to tests/unit/test_cli_flags.py:

    @pytest.mark.parametrize(
        "flag_value",
        [
            (None),
            ("my_target"),
        ],
    )
    def test_target_settings(self, flag_value):
        cli_args = ["--target", flag_value]
        subcommand = ["run"]

        # CLI parameters _before_ subcommand
        parent_context_a = self.make_dbt_context("parent_context_a", cli_args)
        child_context_a = self.make_dbt_context("child_context_a", subcommand, parent_context_a)
        flags_a = Flags(child_context_a)

        # CLI parameters _after_ subcommand
        parent_context_b = self.make_dbt_context("parent_context_b", subcommand)
        child_context_b = self.make_dbt_context(
            "child_context_b", cli_args, parent_context_b
        )
        flags_b = Flags(child_context_b)

        assert flags_a.TARGET == flags_b.TARGET
        assert flags_a.TARGET == flag_value
        assert flags_b.TARGET == flag_value

Then run it like this:

python3 -m pytest tests/unit/test_cli_flags.py::TestFlags::test_target_settings

Relevant log output

Specify one of these sub-commands and you can find more help from there.
========================================== short test summary info ==========================================
FAILED tests/unit/test_cli_flags.py::TestFlags::test_target_settings[None] - click.exceptions.Exit: 0
FAILED tests/unit/test_cli_flags.py::TestFlags::test_target_settings[my_target] - click.exceptions.Exit: 0
============================================= 2 failed in 0.42s =============================================

Environment

- OS:
- Python:
- dbt:

Which database adapter are you using with dbt?

No response

Additional Context

If the following edit is made to the code example above, then it will work:

        parent_context_a = self.make_dbt_context("parent_context_a", cli_args.copy())

Alternatively, an equivalent change can be made here instead:

    def make_dbt_context(
        self, context_name: str, args: List[str], parent: Optional[click.Context] = None
    ) -> click.Context:
        ctx = cli.make_context(context_name, args.copy(), parent)
        return ctx

We can see an example within the click source code here of making a copy of the args when calling cli.make_context:

    ctx = cli.make_context(prog_name, args.copy(), **ctx_args)

So that makes it seem like someone else has bumped into a similar issue before.

My guess is that Click's make_context is mutating the args list which is creates an issue if is re-used a second time in a subsequent call to make_context.

If so, then the best solution would be if make_context didn't mutate the list.

But in the meantime, it would behoove us to make a copy of any args variables being passed into cli.make_context.

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

Successfully merging a pull request may close this issue.

1 participant