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

check for node name uniqueness across refable resource types #737

Merged
merged 5 commits into from
Apr 20, 2018
Merged
Show file tree
Hide file tree
Changes from 3 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 dbt/compilation.py
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,22 @@ def get_all_projects(self):

return all_projects

def _check_resource_uniqueness(cls, flat_graph):
nodes = flat_graph['nodes']
names_resources = {}

for resource, attribs in nodes.items():
if attribs.get('resource_type') not in NodeType.refable():
continue

name = attribs['name']
existing_name = names_resources.get(name)
if existing_name is not None:
dbt.exceptions.raise_duplicate_resource_name(resource,
existing_name)

names_resources[name] = resource
Copy link
Member

Choose a reason for hiding this comment

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

do we have logic like this anywhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code previously existed in the loader, but I ripped it out and put it in the compiler class. I liked the idea of the Loader just returning a flat graph and not validating uniqueness or other business logic constraints. It feels like that will be important if we expose the loader through our API

Copy link
Contributor Author

Choose a reason for hiding this comment

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


def compile(self):
linker = Linker()

Expand All @@ -270,6 +286,8 @@ def compile(self):
flat_graph = dbt.loader.GraphLoader.load_all(
root_project, all_projects)

self._check_resource_uniqueness(flat_graph)

flat_graph = dbt.parser.process_refs(flat_graph,
root_project.get('name'))

Expand Down
6 changes: 6 additions & 0 deletions dbt/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,3 +291,9 @@ def raise_dep_not_found(node, node_description, required_pkg):
'Error while parsing {}.\nThe required package "{}" was not found. '
'Is the package installed?\nHint: You may need to run '
'`dbt deps`.'.format(node_description, required_pkg), node=node)


def raise_duplicate_resource_name(resource_1_name, resource_2_name):
raise_compiler_error(
'Found two resources with the same name: \n- {}\n- {}'.format(
resource_1_name, resource_2_name))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmcarthur I accidentally deleted your comment 🙃

You asked if these resources were full paths or just node names.

I think the answer is that it's both, depending where we call this. Will update the code to make it consistent

10 changes: 0 additions & 10 deletions dbt/loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,16 +84,6 @@ def load_all(cls, root_project, all_projects, macros=None):

to_return.update(project_loaded)

# Check for duplicate model names
names_models = {}
for model, attribs in to_return.items():
name = attribs['name']
existing_name = names_models.get(name)
if existing_name is not None:
raise dbt.exceptions.CompilationException(
'Found models with the same name: \n- %s\n- %s' % (
model, existing_name))
names_models[name] = model
return to_return

@classmethod
Expand Down
2 changes: 1 addition & 1 deletion dbt/node_runners.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def raise_on_first_error(self):

@classmethod
def is_refable(cls, node):
return node.get('resource_type') in [NodeType.Model, NodeType.Seed]
return node.get('resource_type') in NodeType.refable()

@classmethod
def is_ephemeral(cls, node):
Expand Down
7 changes: 7 additions & 0 deletions dbt/node_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ def executable(cls):
cls.Seed,
]

@classmethod
def refable(cls):
return [
cls.Model,
cls.Seed,
]


class RunHookType:
Start = 'on-run-start'
Expand Down
5 changes: 2 additions & 3 deletions dbt/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -288,10 +288,9 @@ def parse_sql_nodes(nodes, root_project, projects, tags=None, macros=None):
# Check for duplicate model names
existing_node = to_return.get(node_path)
if existing_node is not None:
raise dbt.exceptions.CompilationException(
'Found models with the same name:\n- %s\n- %s' % (
dbt.exceptions.raise_duplicate_resource_name(
existing_node.get('original_file_path'),
node.get('original_file_path')))
node.get('original_file_path'))

to_return[node_path] = node_parsed

Expand Down
2 changes: 1 addition & 1 deletion dbt/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ def model_immediate_name(model, non_destructive):

def find_refable_by_name(flat_graph, target_name, target_package):
return find_by_name(flat_graph, target_name, target_package,
'nodes', [NodeType.Model, NodeType.Seed])
'nodes', NodeType.refable())


def find_macro_by_name(flat_graph, target_name, target_package):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def profile_config(self):

@attr(type="postgres")
def test_duplicate_model_enabled(self):
message = "Found models with the same name:.*"
message = "Found two resources with the same name:.*"
with self.assertRaisesRegexp(CompilationException, message):
self.run_dbt(["run"])

Expand Down Expand Up @@ -114,7 +114,7 @@ def project_config(self):
@attr(type="postgres")
def test_duplicate_model_enabled_across_packages(self):
self.run_dbt(["deps"])
message = "Found models with the same name:.*"
message = "Found two resources with the same name:.*"
with self.assertRaisesRegexp(CompilationException, message):
self.run_dbt(["run"])

Expand Down