PR feedback

Fix comments
Implement a TODO around duplicate macro names
 - added unit tests for it
Implement a TODO around ref resolution

core/dbt/clients/jinja.py

@@ -3,8 +3,9 @@
 import os
 import tempfile
 from contextlib import contextmanager
+from dataclasses import dataclass
 from typing import (
- List, Union, Set, Optional, Dict, Any, Callable, Iterator, Type, NoReturn
+ List, Union, Set, Optional, Dict, Any, Iterator, Type, NoReturn
 )
 
 import jinja2
@@ -164,6 +165,18 @@ def call_macro(self, *args, **kwargs):
  return e.value
 
 
+@dataclass
+class MacroProxy:
+ generator: 'MacroGenerator'
+
+ @property
+ def node(self):
+ return self.generator.node
+
+ def __call__(self, *args, **kwargs):
+ return self.generator.call_macro(*args, **kwargs)
+
+
 class MacroGenerator(BaseMacroGenerator):
  def __init__(self, node, context: Optional[Dict[str, Any]] = None) -> None:
  super().__init__(context)
@@ -185,9 +198,9 @@ def exception_handler(self) -> Iterator[None]:
  e.stack.append(self.node)
  raise e
 
- def __call__(self, context: Dict[str, Any]) -> Callable:
+ def __call__(self, context: Dict[str, Any]) -> MacroProxy:
  self.context = context
- return self.call_macro
+ return MacroProxy(self)
 
 
 class QueryStringGenerator(BaseMacroGenerator):

core/dbt/config/profile.py

@@ -147,7 +147,8 @@ def _credentials_from_profile(
 
  @staticmethod
  def pick_profile_name(
- args_profile_name: str, project_profile_name: Optional[str] = None,
+ args_profile_name: Optional[str],
+ project_profile_name: Optional[str] = None,
  ) -> str:
  profile_name = project_profile_name
  if args_profile_name is not None:
@@ -370,7 +371,7 @@ def render_from_args(
  threads_override = getattr(args, 'threads', None)
  target_override = getattr(args, 'target', None)
  raw_profiles = read_profile(args.profiles_dir)
- profile_name = cls.pick_profile_name(args.profile,
+ profile_name = cls.pick_profile_name(getattr(args, 'profile', None),
  project_profile_name)
 
  return cls.from_raw_profiles(

core/dbt/context/configured.py

@@ -1,5 +1,6 @@
-from typing import Callable, Any, Dict, Iterable, Union
+from typing import Any, Dict, Iterable, Union
 
+from dbt.clients.jinja import MacroProxy
 from dbt.contracts.connection import AdapterRequiredConfig
 from dbt.contracts.graph.manifest import Manifest
 from dbt.contracts.graph.parsed import ParsedMacro
@@ -8,6 +9,7 @@
 
 from dbt.context.base import contextproperty
 from dbt.context.target import TargetContext
+from dbt.exceptions import raise_duplicate_macro_name
 
 
 class ConfiguredContext(TargetContext):
@@ -23,31 +25,33 @@ def project_name(self) -> str:
  return self.config.project_name
 
 
+Namespace = Union[Dict[str, MacroProxy], MacroProxy]
+
+
 class _MacroNamespace:
  def __init__(self, root_package, search_package):
  self.root_package = root_package
  self.search_package = search_package
- self.globals: Dict[str, Callable] = {}
- self.locals: Dict[str, Callable] = {}
- self.packages: Dict[str, Dict[str, Callable]] = {}
+ self.globals: Dict[str, MacroProxy] = {}
+ self.locals: Dict[str, MacroProxy] = {}
+ self.packages: Dict[str, Dict[str, MacroProxy]] = {}
 
  def add_macro(self, macro: ParsedMacro, ctx: Dict[str, Any]):
  macro_name: str = macro.name
- macro_func: Callable = macro.generator(ctx)
+ macro_func: MacroProxy = macro.generator(ctx)
 
- # put plugin macros into the nam
+ # put plugin macros into the root namespace
  if macro.package_name in PACKAGES:
  namespace: str = GLOBAL_PROJECT_NAME
  else:
  namespace = macro.package_name
 
  if namespace not in self.packages:
- value: Dict[str, Callable] = {}
+ value: Dict[str, MacroProxy] = {}
  self.packages[namespace] = value
 
- # TODO: if macro_name exists already, that means you had duplicate
- # names in the same namespace (probably a plugin vs core/multiple
- # plugins issue). That should be an error, right?
+ if macro_name in self.packages[namespace]:
+ raise_duplicate_macro_name(macro_func.node, macro, namespace)
  self.packages[namespace][macro_name] = macro_func
 
  if namespace == self.search_package:
@@ -60,7 +64,7 @@ def add_macros(self, macros: Iterable[ParsedMacro], ctx: Dict[str, Any]):
  self.add_macro(macro, ctx)
 
  def get_macro_dict(self) -> Dict[str, Any]:
- root_namespace: Dict[str, Union[Dict[str, Callable], Callable]] = {}
+ root_namespace: Dict[str, Namespace] = {}
 
  root_namespace.update(self.packages)
  root_namespace.update(self.globals)

core/dbt/contracts/graph/manifest.py

@@ -753,9 +753,10 @@ def resolve_ref(
  # first pass: look for models in the current_project
  # second pass: look for models in the node's package
  # final pass: look for models in any package
- # todo: exclude the packages we have already searched. overriding
- # a package model in another package doesn't necessarily work atm
- candidates = [current_project, node_package, None]
+ if current_project == node_package:
+ candidates = [current_project, None]
+ else:
+ candidates = [current_project, node_package, None]
  for candidate in candidates:
  target_model = self.find_refable_by_name(
  target_model_name,

core/dbt/contracts/graph/parsed.py

@@ -10,7 +10,6 @@
  Tuple,
  NewType,
  MutableMapping,
- Callable,
 )
 
 from hologram import JsonSchemaMixin
@@ -507,7 +506,7 @@ def local_vars(self):
  return {}
 
  @property
- def generator(self) -> Callable[[Dict[str, Any]], Callable]:
+ def generator(self) -> MacroGenerator:
  """
  Returns a function that can be called to render the macro results.
  """

core/dbt/exceptions.py

@@ -643,6 +643,29 @@ def approximate_relation_match(target, relation):
  relation=relation))
 
 
+def raise_duplicate_macro_name(node_1, node_2, namespace) -> NoReturn:
+ duped_name = node_1.namespace
+ if node_1.package_name != node_2.package_name:
+ extra = (
+ ' ({} and {} are both in the {} namespace)'
+ .format(node_1.package_name, node_2.package_name, namespace)
+ )
+ else:
+ extra = ''
+
+ raise_compiler_error(
+ 'dbt found two macros with the name "{}" in the namespace "{}"{}. '
+ 'Since these macros have the same name and exist in the same '
+ 'namespace, dbt will be unable to decide which to call. To fix this, '
+ 'change the name of one of these macros:\n- {} ({})\n- {} ({})'
+ .format(
+ duped_name, namespace, extra,
+ node_1.unique_id, node_1.original_file_path,
+ node_2.unique_id, node_2.original_file_path
+ )
+ )
+
+
 def raise_duplicate_resource_name(node_1, node_2):
  duped_name = node_1.name
 

core/dbt/task/debug.py

@@ -226,10 +226,10 @@ def _choose_target_name(self, profile_name: str):
  renderer = ConfigRenderer(generate_base_context(self.cli_vars))
 
  target_name, _ = Profile.render_profile(
- raw_profile,
- self.profile_name,
- getattr(self.args, 'target', None),
- renderer
+ raw_profile=raw_profile,
+ profile_name=profile_name,
+ target_override=getattr(self.args, 'target', None),
+ renderer=renderer
  )
  return target_name
 
@@ -256,7 +256,7 @@ def _load_profile(self):
  renderer = ConfigRenderer(generate_base_context(self.cli_vars))
  for profile_name in profile_names:
  try:
- profile: Profile = QueryCommentedProfile.from_args(
+ profile: Profile = QueryCommentedProfile.render_from_args(
  self.args, renderer, profile_name
  )
  except dbt.exceptions.DbtConfigError as exc:

test/unit/test_context.py

@@ -3,7 +3,13 @@
 from typing import Set, Dict, Any
 from unittest import mock
 
-from dbt.contracts.graph.parsed import ParsedModelNode, NodeConfig, DependsOn, ParsedMacro
+import pytest
+
+# make sure 'postgres' is in PACKAGES
+from dbt.adapters import postgres # noqa
+from dbt.contracts.graph.parsed import (
+ ParsedModelNode, NodeConfig, DependsOn, ParsedMacro
+)
 from dbt.context import base, target, configured, providers, docs
 from dbt.node_types import NodeType
 import dbt.exceptions
@@ -317,28 +323,31 @@ def mock_model():
  )
 
 
-import pytest
-
-
 @pytest.fixture
 def get_adapter():
  with mock.patch.object(providers, 'get_adapter') as patch:
  yield patch
 
 
-def test_query_header_context():
- config = config_from_parts_or_dicts(PROJECT_DATA, PROFILE_DATA)
+@pytest.fixture
+def config():
+ return config_from_parts_or_dicts(PROJECT_DATA, PROFILE_DATA)
+
+
+@pytest.fixture
+def manifest(config):
+ return mock_manifest(config)
+
 
+def test_query_header_context(config, manifest):
  ctx = configured.generate_query_header_context(
  config=config,
- manifest=mock_manifest(config),
+ manifest=manifest,
  )
  assert_has_keys(REQUIRED_QUERY_HEADER_KEYS, MAYBE_KEYS, ctx)
 
 
-def test_macro_parse_context(get_adapter):
- config = config_from_parts_or_dicts(PROJECT_DATA, PROFILE_DATA)
- manifest = mock_manifest(config)
+def test_macro_parse_context(config, manifest, get_adapter):
  ctx = providers.generate_parser_macro(
  macro=manifest.macros['macro.root.macro_a'],
  config=config,
@@ -348,9 +357,7 @@ def test_macro_parse_context(get_adapter):
  assert_has_keys(REQUIRED_MACRO_KEYS, MAYBE_KEYS, ctx)
 
 
-def test_macro_runtime_context(get_adapter):
- config = config_from_parts_or_dicts(PROJECT_DATA, PROFILE_DATA)
- manifest = mock_manifest(config)
+def test_macro_runtime_context(config, manifest, get_adapter):
  ctx = providers.generate_runtime_macro(
  macro=manifest.macros['macro.root.macro_a'],
  config=config,
@@ -360,34 +367,45 @@ def test_macro_runtime_context(get_adapter):
  assert_has_keys(REQUIRED_MACRO_KEYS, MAYBE_KEYS, ctx)
 
 
-def test_model_parse_context(get_adapter):
- config = config_from_parts_or_dicts(PROJECT_DATA, PROFILE_DATA)
+def test_model_parse_context(config, manifest, get_adapter):
  ctx = providers.generate_parser_model(
  model=mock_model(),
  config=config,
- manifest=mock_manifest(config),
+ manifest=manifest,
  source_config=mock.MagicMock(),
  )
  assert_has_keys(REQUIRED_MODEL_KEYS, MAYBE_KEYS, ctx)
 
 
-def test_model_runtime_context(get_adapter):
- config = config_from_parts_or_dicts(PROJECT_DATA, PROFILE_DATA)
+def test_model_runtime_context(config, manifest, get_adapter):
  ctx = providers.generate_runtime_model(
  model=mock_model(),
  config=config,
- manifest=mock_manifest(config),
+ manifest=manifest,
  )
  assert_has_keys(REQUIRED_MODEL_KEYS, MAYBE_KEYS, ctx)
 
 
-def test_docs_parse_context():
- config = config_from_parts_or_dicts(PROJECT_DATA, PROFILE_DATA)
+def test_docs_parse_context(config):
  ctx = docs.generate_parser_docs(config, mock_model(), [])
  assert_has_keys(REQUIRED_DOCS_KEYS, MAYBE_KEYS, ctx)
 
 
-def test_docs_runtime_context():
- config = config_from_parts_or_dicts(PROJECT_DATA, PROFILE_DATA)
+def test_docs_runtime_context(config):
  ctx = docs.generate_runtime_docs(config, mock_model(), [], 'root')
  assert_has_keys(REQUIRED_DOCS_KEYS, MAYBE_KEYS, ctx)
+
+
+def test_macro_namespace(config, manifest):
+ mn = configured._MacroNamespace('root', 'search')
+ mn.add_macros(manifest.macros.values(), {})
+
+ # same pkg, same name
+ with pytest.raises(dbt.exceptions.CompilationException):
+ mn.add_macros(manifest.macros.values(), {})
+
+ mn.add_macro(mock_macro('some_macro', 'dbt'), {})
+
+ # same namespace, same name (different pkg!)
+ with pytest.raises(dbt.exceptions.CompilationException):
+ mn.add_macro(mock_macro('some_macro', 'dbt_postgres'), {})