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

Conversation

drewbanin
Copy link
Contributor

fixes: #736

This came up because a view model and a seed file had the same name. Previously, dbt's check for name uniqueness only pertained to models.

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.

@dbt-labs dbt-labs deleted a comment from cmcarthur Apr 20, 2018
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

@drewbanin
Copy link
Contributor Author

@cmcarthur I cleaned up the code and improved the error message. Now the exception function just takes a couple of nodes. Seems like a better approach. New error:

Compilation Error
  dbt found two resources with the name "idk". Since these resources have the same name,
  dbt will be unable to find the correct resource when ref("idk") is used. To fix this,
  change the name of one of these resources:
  - model.my_project.idk (models/idk.sql)
  - model.my_project.idk (models/tmp/idk.sql)

We do need to check this twice, since the flat map is a dict of unique_id --> node. If we don't check this in the parser, then nodes will be overwritten by a secondary node with a duplicated name. Crucially, two models with the same name in the same package will have equivalent unique_ids.

We need to check a second time to assert uniqueness across projects and resources types.

@drewbanin drewbanin merged commit e20796e into development Apr 20, 2018
@drewbanin drewbanin deleted the check-for-dupe-seed-names branch April 20, 2018 20:23
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
* check for node name uniqueness across refable resource types

* change test for new error msg

* consistent error message for dupes

* small refactor, improve error msg

* fix tests


automatic commit by git-black, original commits:
  e20796e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants