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

Allow duplicated model names across packages #3053

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions core/dbt/contracts/graph/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,14 @@ def search(self, haystack: Iterable[N]) -> Optional[N]:
return model
return None

def search_many(self, haystack: Iterable[N]) -> List[N]:
"""Find multiple entries in the given iterable by name."""
found = []
for model in haystack:
if self._matches(model):
found.append(model)
return found


D = TypeVar('D')

Expand Down Expand Up @@ -515,6 +523,16 @@ def build_flat_graph(self):
}
}

def find_nodes_by_name(
self, name: str, package: Optional[str] = None
) -> List[ManifestNode]:
searcher: NameSearcher = NameSearcher(
name, package, NodeType.refable()
)

result = searcher.search_many(self.nodes.values())
return result

def find_disabled_by_name(
self, name: str, package: Optional[str] = None
) -> Optional[ManifestNode]:
Expand Down
15 changes: 15 additions & 0 deletions core/dbt/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,21 @@ def raise_duplicate_macro_name(node_1, node_2, namespace) -> NoReturn:
)


def raise_ambiguous_ref(node_1, node_2):
duped_name = node_1.name
get_func = f'{{{{ ref("{duped_name}") }}}}'

raise_compiler_error(
f'The reference {get_func} is ambiguous because there are two nodes '
f'with the name "{duped_name}". To fix this, either change the name '
'of one of these nodes, or use the two-argument version of ref() '
'to specify the package that contains the intended node. Found:\n'
f'- {node_1.unique_id} ({node_1.original_file_path})\n'
f'- {node_2.unique_id} ({node_2.original_file_path})\n\n'
f'Example: {{{{ ref("{node_1.package_name}", "{node_1.name}") }}}}'
)


def raise_duplicate_resource_name(node_1, node_2):
duped_name = node_1.name

Expand Down
21 changes: 16 additions & 5 deletions core/dbt/parser/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -522,11 +522,22 @@ def _check_resource_uniqueness(
relation = relation_cls.create_from(config=config, node=node)
full_node_name = str(relation)

existing_node = names_resources.get(name)
if existing_node is not None:
dbt.exceptions.raise_duplicate_resource_name(
existing_node, node
)
# Check refs - is there an unqualified ref to a
# model that exists in two different packages?
for ref in node.refs:
if len(ref) == 1:
matched = manifest.find_nodes_by_name(ref[0])
else:
# two-arg refs are namespaced, so no need
# to check for duplicates. Nodes must still
# have unique names within a package
continue

if len(matched) > 1:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do wonder if, in the event that there are same-named models in the current project and in an installed package, we should still support single-argument ref and default to the resource in the same namespace. We would only raise this error if all the resources by that name are in other namespaces.

My motivation here: It's possible that a user wants to install a package that has a same-named model as one in their project, and the models in the package only uses single-argument ref. Of course, I think there's a lot of benefit in being more explicit to avoid surprises, so alternatively we'd establish a convention that package authors ought to use two-argument ref everywhere. It wouldn't be a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea!

dbt.exceptions.raise_ambiguous_ref(
matched[0],
matched[1]
)

existing_alias = alias_resources.get(full_node_name)
if existing_alias is not None:
Expand Down