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

CT 2510 Throw error for duplicate versioned and non versioned model names #7577

Merged
merged 5 commits into from
May 10, 2023

Conversation

gshank
Copy link
Contributor

@gshank gshank commented May 9, 2023

resolves #7487

Description

The _check_resource_uniqueness method in core/dbt/parser/manifest.py has been modified to check for non-versioned models with the same "name" as versioned models.

Checklist

@gshank gshank requested review from a team as code owners May 9, 2023 21:09
@gshank gshank requested review from QMalcolm, emmyoop and VersusFacit and removed request for a team May 9, 2023 21:10
@cla-bot cla-bot bot added the cla:yes label May 9, 2023
core/dbt/exceptions.py Outdated Show resolved Hide resolved
intersection_versioned = set(versioned_resources.keys()).intersection(
set(unversioned_resources.keys())
)
if intersection_versioned:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think raising this sets us back in terms of being able to handle ref-able resource with duplicate names across packages. For example - if I define project_a.model (unversioned) and project_b.model.v1 (versioned) - those work when run in their respective projects as root, but when installed as packages would raise DuplicateVersionedUnversionedError

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'm thinking that we need to store the versioned_resources and unversioned_resources by package. Maybe we'd only need to check the root package?

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've updated the code to check versioned/unversioned by packages and tweaked the error message to also suggest adding the unversioned model to the versions.

@gshank gshank requested a review from MichelleArk May 10, 2023 13:47
@gshank gshank closed this May 10, 2023
@gshank gshank reopened this May 10, 2023

To fix this, change the name of the unversioned resource
{self.unversioned_node.unique_id} ({self.unversioned_node.original_file_path})
or add the unversioned model to the versions in {self.versioned_node.patch_path}
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -1280,6 +1288,18 @@ def _check_resource_uniqueness(

alias_resources[full_node_name] = node

for ver_unver_dict in name_resources.values():
versioned_names = ver_unver_dict["ver"].keys()
unversioned_names = ver_unver_dict["unver"].keys()
Copy link
Contributor

@MichelleArk MichelleArk May 10, 2023

Choose a reason for hiding this comment

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

Non blocking at all, just a suggestion:

I think it'd be more straightforward to read this method if name_resources just have had level of nesting (packages) and omit "ver" and "unver". and then construct the verioned_names and un_versioned names at this point by:

for name_resources in name_resources_by_package.values():
  versioned_names = {resource.name for resource in name_resources if resource.is_versioned}
  versioned_names = {resource.name for resource in name_resources if not resource.is_versioned}

just a little bit less state/nesting to keep in mind while reading - your call though!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It kind of bothers me that this way of doing it means looping over an n-sized array 3 times instead of 1 though.

Copy link
Contributor

@MichelleArk MichelleArk left a comment

Choose a reason for hiding this comment

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

Looks good, although I've found an unrelated bug in version parsing across packages: #7593

@gshank gshank merged commit 29f2cfc into main May 10, 2023
@gshank gshank deleted the ct-2510-pp_versioned_and_non-versioned branch May 10, 2023 20:16
@github-actions
Copy link
Contributor

The backport to 1.5.latest failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.5.latest 1.5.latest
# Navigate to the new working tree
cd .worktrees/backport-1.5.latest
# Create a new branch
git switch --create backport-7577-to-1.5.latest
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 29f2cfc48d19c0500d71ed282b04acd735b9f87e
# Push it to GitHub
git push --set-upstream origin backport-7577-to-1.5.latest
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.5.latest

Then, create a pull request where the base branch is 1.5.latest and the compare/head branch is backport-7577-to-1.5.latest.

gshank added a commit that referenced this pull request May 11, 2023
…ames (#7577)

* Check for versioned/unversioned duplicates

* Add new exception DuplicateVersionedUnversionedError

* Changie

* Handle packages when finding versioned and unversioned duplicates

(cherry picked from commit 29f2cfc)
gshank added a commit that referenced this pull request May 12, 2023
#7605)

* CT 2510 Throw error for duplicate versioned and non versioned model names (#7577)

* Check for versioned/unversioned duplicates

* Add new exception DuplicateVersionedUnversionedError

* Changie

* Handle packages when finding versioned and unversioned duplicates

(cherry picked from commit 29f2cfc)

* Issue AmbiguousAlias error after DuplicateResourceName
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-2510] [Bug] Raise an error if non-versioned model is defined with same name as existing versioned model
2 participants