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 1993 handle invalid access to private models #7069

Merged
merged 15 commits into from
Feb 27, 2023

Conversation

gshank
Copy link
Contributor

@gshank gshank commented Feb 27, 2023

resolves #6826

Description

Throw a DbtReferenceError if a private model is accessed from a non-group model, or from an exposure or metric.

Checklist

@gshank gshank requested review from a team as code owners February 27, 2023 17:00
@gshank gshank requested review from emmyoop and ChenyuLInx February 27, 2023 17:00
@cla-bot cla-bot bot added the cla:yes label Feb 27, 2023
@gshank gshank requested review from MichelleArk and removed request for ChenyuLInx February 27, 2023 17:00
target_model.resource_type == NodeType.Model
and target_model.access == AccessType.Private
):
# Metrics do not have a group and so can never reference private models
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe metrics should actually be groupable as per #6823 and the implementation in #6965. Exposures should not be groupable though (#6826). cc @jtcohen6 to confirm 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no "group" field on a Metric node, but I see that there is on MetricConfig, so if it were set in config on a metric it would blow up in "update_parsed_node_config". Should I add a "group" field on Metric?

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 looks like group doesn't work at all for metrics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually metrics doesn't even use update_parsed_node_config, so it wouldn't have blown up, but there wasn't a "group" field on metric. I added the group field and copied it from config to node level. I used the node level field in 'process_refs..." calls. If we're going to copy that field to node level we should probably use that consistently. Or not bother with the copying.

core/dbt/exceptions.py Outdated Show resolved Hide resolved
@MichelleArk MichelleArk mentioned this pull request Feb 27, 2023
6 tasks
def get_message(self) -> str:
return (
f"Node {self.unique_id} attempted to reference node {self.ref_unique_id}, "
"which is not allowed because the referenced node is private to the {self.group} group."
Copy link
Contributor

Choose a reason for hiding this comment

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

Error message looks good, but this is missing the f-string + initializion self.group

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's just so very Monday today.

@gshank gshank force-pushed the ct-1993-handle_invalid_access branch from 90c6129 to 946fa01 Compare February 27, 2023 21:18
@gshank gshank merged commit ec5d31d into main Feb 27, 2023
@gshank gshank deleted the ct-1993-handle_invalid_access branch February 27, 2023 22:33
acurtis-evi pushed a commit to acurtis-evi/dbt-core that referenced this pull request Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-1993] Raise an exception from ref if referencing a private model across groups
2 participants