Add project name to default search packages

[JCZuurmond]

We prefer macros in the project over the ones in the namespace (package)

resolves #4106

Description

This changes will choose macros in the root project over the ones in the namespace.

Checklist

• I have signed the CLA
• I have run this code in development and it appears to resolve the stated issue
• This PR includes tests, or tests are not required/relevant for this PR
• I have updated the CHANGELOG.md and added information about my change

[jtcohen6]

Just going to close + re-open to re-trigger adapter integration tests, now that I've added the ok to test label

[JCZuurmond]

@jtcohen6 , I have troubles adding a test for this. I tried to add one to the test/unit/test_contexts.py module, but get stuck in what is mocked and what isn't.

[jtcohen6]

There's a unit test for dispatch over in dbt-redshift here, I think we could borrow some of that logic and pull it here. I'll edit this comment with a bit of code in a moment

This is tricky, but let's get the current tests passing first

[jtcohen6]

Ok, the new issue in the unit tests: In dispatch: Could not find package 'X'

This error is raised by dispatch if we supply to search_order a package that does not have any macros defined. Which is not uncommon in unit tests, or in the real world. Let's think about a way to avoid raising that error.

[JCZuurmond]

Is 't because no macros are found or because the package is not found?

            raise_compiler_error(
               f"Could not find package '{package_name}'"
            )

[jtcohen6]

If a package has no macros defined, the package doesn't appear in MacroResolver.packages:

dbt-core/core/dbt/context/macro_resolver.py

Lines 69 to 72 in eace5b7

	# non-internal packages
	for fnamespace in self.packages.values():
	for macro in fnamespace.values():
	macros_by_name[macro.name] = macro

So per that error message, the package appears to be "missing." I think that's reasonable behavior for the macro resolver, although the error message could be clearer: No macros found in a package named '{package_name}'

Options for how dispatch should handle this:

• Before searching search_packages here, we check to see if the package is the GLOBAL_PROJECT_NAME or is present in self._namespace.macro_resolver.packages
• We catch and smother the CompilationException here, rather than raising it. If dispatch fails to find any valid macros after searching everywhere, it will end up raising a different CompilationException later on.

I'm leaning toward the latter. What do you think?

[JCZuurmond]

Before searching search_packages here, we check to see if the package is the GLOBAL_PROJECT_NAME or is present in self._namespace.macro_resolver.packages

I am wrapping my ahead around all inner dbt parts, so I can not judge well. What my gut feeling about this is, is that it is too tightly coupled: should the dispatch do this check? It is kind of the same logic as self._namespace.get_from_package does, so why add it here too?

We catch and smother the CompilationException here, rather than raising it. If dispatch fails to find any valid macros after searching everywhere, it will end up raising a different CompilationException later on.

It is raised there with the from exc

[jtcohen6]

I agree with your reasoning there!

My proposal would be, replace these lines with:

                except CompilationException as exc:
                    # we don't expect to find every macro we search for
                    pass

That is to say, while normally the macro resolver should raise an error if {{ fake_package.fake_macro() }} is called explicitly, I think it's reasonable for dispatch to step through the entire search order and report back with its more thorough error message:

In dispatch: No macro named 'fake_macro' found
    Searched for: fake_package.postgres__fake_macro, fake_package.default__fake_macro

[JCZuurmond]

I tested it locally and it does the trick! Now, we should add a test for this.

[JCZuurmond]

I have been looking at adding a test, I think the problem with adding it to the unit test is that the unit test assume no plugin/package (postgres, spark, redshift, etc.) is installed, but we pretend like it is because were getting a macro from that package (namespace).

I get the following error if I try this with the postgres plugin.

E               dbt.exceptions.InternalException: No plugin found for <MagicMock name='get_adapter().type()' id='4516255672'>

Should and can we mock this? Or should this test be moved to a package?

[jtcohen6]

@JCZuurmond Ah ok, I don't think we're going to be able to figure that one very quickly. Let's add an integration test instead. There's a good starting point in test/integration/016_macro_tests/test_macros.py, that overrides get_columns_in_relation and makes sure it uses the new version. You could do everything the same, except reimplement postgres__get_columns_in_relation instead of get_columns_in_relation, and make sure it works by default (with no project-level dispatch config).

[JCZuurmond]

@jtcohen6 : ready to merge (if all tests pass)

[jtcohen6]

Thank you for the contribution @JCZuurmond!

[chapmanjacobd]

should the second time be expect pass True ?

[jtcohen6]

There's a check in the model that raises an explicit error if the macro is overridden, so I believe it fails both times:

dbt-core/test/integration/016_macro_tests/override-get-columns-models/model.sql

Lines 1 to 5 in f20e83a

	{% set result = adapter.get_columns_in_relation(this) %}
	{% if execute and result != 'a string' %}
	{% do exceptions.raise_compiler_error('overriding get_columns_in_relation failed') %}
	{% endif %}
	select 1 as id

[JCZuurmond]

As explained in the class docstring, the run in this test should fail because the override does not happen. We need the override to overrule the raise compile error that @jtcohen6 showed.

    The macro in `override-postgres-get-columns-macros` does NOT override the
    `get_columns_in_relation` macro because we tell dispatch to not look at the
    postgres macros.