diff --git a/.changes/unreleased/Features-20230224-182214.yaml b/.changes/unreleased/Features-20230224-182214.yaml new file mode 100644 index 00000000000..ca1c63612c8 --- /dev/null +++ b/.changes/unreleased/Features-20230224-182214.yaml @@ -0,0 +1,6 @@ +kind: Features +body: Disallow refing private model across groups +time: 2023-02-24T18:22:14.965136-05:00 +custom: + Author: gshank + Issue: "6826" diff --git a/core/dbt/contracts/graph/manifest.py b/core/dbt/contracts/graph/manifest.py index 095dd4ce5c8..71b413d6123 100644 --- a/core/dbt/contracts/graph/manifest.py +++ b/core/dbt/contracts/graph/manifest.py @@ -802,8 +802,8 @@ def build_group_map(self): ) group_map = {group.name: [] for group in self.groups.values()} for node in groupable_nodes: - if node.config.group is not None: - group_map[node.config.group].append(node.unique_id) + if node.group is not None: + group_map[node.group].append(node.unique_id) self.group_map = group_map def writable_manifest(self): diff --git a/core/dbt/contracts/graph/nodes.py b/core/dbt/contracts/graph/nodes.py index f302bf1fdd6..368636c44d1 100644 --- a/core/dbt/contracts/graph/nodes.py +++ b/core/dbt/contracts/graph/nodes.py @@ -1063,6 +1063,7 @@ class Metric(GraphNode): refs: List[List[str]] = field(default_factory=list) metrics: List[List[str]] = field(default_factory=list) created_at: float = field(default_factory=lambda: time.time()) + group: Optional[str] = None @property def depends_on_nodes(self): diff --git a/core/dbt/exceptions.py b/core/dbt/exceptions.py index e2db02f6d1b..297aa155a5e 100644 --- a/core/dbt/exceptions.py +++ b/core/dbt/exceptions.py @@ -1114,6 +1114,20 @@ def __init__(self, exc: ValidationError, node): super().__init__(msg=self.msg) +class DbtReferenceError(ParsingError): + def __init__(self, unique_id: str, ref_unique_id: str, group: str): + self.unique_id = unique_id + self.ref_unique_id = ref_unique_id + self.group = group + super().__init__(msg=self.get_message()) + + def get_message(self) -> str: + return ( + f"Node {self.unique_id} attempted to reference node {self.ref_unique_id}, " + f"which is not allowed because the referenced node is private to the {self.group} group." + ) + + class InvalidAccessTypeError(ParsingError): def __init__(self, unique_id: str, field_value: str): self.unique_id = unique_id diff --git a/core/dbt/parser/manifest.py b/core/dbt/parser/manifest.py index 8d6f49e6cfc..112200cdccc 100644 --- a/core/dbt/parser/manifest.py +++ b/core/dbt/parser/manifest.py @@ -37,7 +37,7 @@ Note, ) from dbt.logger import DbtProcessState -from dbt.node_types import NodeType +from dbt.node_types import NodeType, AccessType from dbt.clients.jinja import get_rendered, MacroStack from dbt.clients.jinja_static import statically_extract_macro_calls from dbt.clients.system import make_directory, write_file @@ -1076,7 +1076,7 @@ def _check_valid_group_config(manifest: Manifest): def _check_valid_group_config_node( groupable_node: Union[Metric, ManifestNode], valid_group_names: Set[str] ): - groupable_node_group = groupable_node.config.group + groupable_node_group = groupable_node.group if groupable_node_group and groupable_node_group not in valid_group_names: raise dbt.exceptions.ParsingError( f"Invalid group '{groupable_node_group}', expected one of {sorted(list(valid_group_names))}", @@ -1196,6 +1196,16 @@ def _process_refs_for_exposure(manifest: Manifest, current_project: str, exposur ) continue + elif ( + target_model.resource_type == NodeType.Model + and target_model.access == AccessType.Private + ): + # Exposures do not have a group and so can never reference private models + raise dbt.exceptions.DbtReferenceError( + unique_id=exposure.unique_id, + ref_unique_id=target_model.unique_id, + group=dbt.utils.cast_to_str(target_model.group), + ) target_model_id = target_model.unique_id @@ -1239,6 +1249,16 @@ def _process_refs_for_metric(manifest: Manifest, current_project: str, metric: M should_warn_if_disabled=False, ) continue + elif ( + target_model.resource_type == NodeType.Model + and target_model.access == AccessType.Private + ): + if not metric.group or metric.group != target_model.group: + raise dbt.exceptions.DbtReferenceError( + unique_id=metric.unique_id, + ref_unique_id=target_model.unique_id, + group=dbt.utils.cast_to_str(target_model.group), + ) target_model_id = target_model.unique_id @@ -1336,6 +1356,18 @@ def _process_refs_for_node(manifest: Manifest, current_project: str, node: Manif ) continue + # Handle references to models that are private + elif ( + target_model.resource_type == NodeType.Model + and target_model.access == AccessType.Private + ): + if not node.group or node.group != target_model.group: + raise dbt.exceptions.DbtReferenceError( + unique_id=node.unique_id, + ref_unique_id=target_model.unique_id, + group=dbt.utils.cast_to_str(target_model.group), + ) + target_model_id = target_model.unique_id node.depends_on.nodes.append(target_model_id) diff --git a/core/dbt/parser/schemas.py b/core/dbt/parser/schemas.py index cfeb2bc8aef..dac4895ccac 100644 --- a/core/dbt/parser/schemas.py +++ b/core/dbt/parser/schemas.py @@ -432,7 +432,7 @@ def render_test_update(self, node, config, builder, schema_file_id): attached_node = self._lookup_attached_node(builder.target) if attached_node: node.attached_node = attached_node.unique_id - node.group, node.config.group = attached_node.config.group, attached_node.config.group + node.group, node.group = attached_node.group, attached_node.group def parse_node(self, block: GenericTestBlock) -> GenericTestNode: """In schema parsing, we rewrite most of the part of parse_node that @@ -1235,6 +1235,7 @@ def parse_metric(self, unparsed: UnparsedMetric): tags=unparsed.tags, config=config, unrendered_config=unrendered_config, + group=config.group, ) ctx = generate_parse_metrics( @@ -1274,7 +1275,7 @@ def _generate_metric_config( # first apply metric configs precedence_configs.update(target.config) - return generator.calculate_node_config( + config = generator.calculate_node_config( config_call_dict={}, fqn=fqn, resource_type=NodeType.Metric, @@ -1282,6 +1283,7 @@ def _generate_metric_config( base=False, patch_config_dict=precedence_configs, ) + return config def parse(self): for data in self.get_key_dicts(): diff --git a/schemas/dbt/manifest/v9.json b/schemas/dbt/manifest/v9.json index 222f64a2c35..9fbf4763cac 100644 --- a/schemas/dbt/manifest/v9.json +++ b/schemas/dbt/manifest/v9.json @@ -221,7 +221,7 @@ "generated_at": { "type": "string", "format": "date-time", - "default": "2023-02-24T18:16:27.384938Z" + "default": "2023-02-27T20:52:53.570061Z" }, "invocation_id": { "oneOf": [ @@ -232,7 +232,7 @@ "type": "null" } ], - "default": "678dd0f6-ab20-41c8-a3d8-bbc986a26583" + "default": "f578f819-6554-4f6f-8af4-635e35249cb2" }, "env": { "type": "object", @@ -449,7 +449,7 @@ }, "created_at": { "type": "number", - "default": 1677262587.389697 + "default": 1677531173.5741172 }, "config_call_dict": { "type": "object", @@ -1048,7 +1048,7 @@ }, "created_at": { "type": "number", - "default": 1677262587.3926132 + "default": 1677531173.577135 }, "config_call_dict": { "type": "object", @@ -1433,7 +1433,7 @@ }, "created_at": { "type": "number", - "default": 1677262587.394294 + "default": 1677531173.578816 }, "config_call_dict": { "type": "object", @@ -1706,7 +1706,7 @@ }, "created_at": { "type": "number", - "default": 1677262587.39604 + "default": 1677531173.580534 }, "config_call_dict": { "type": "object", @@ -1978,7 +1978,7 @@ }, "created_at": { "type": "number", - "default": 1677262587.397707 + "default": 1677531173.5821729 }, "config_call_dict": { "type": "object", @@ -2241,7 +2241,7 @@ }, "created_at": { "type": "number", - "default": 1677262587.399359 + "default": 1677531173.5838 }, "config_call_dict": { "type": "object", @@ -2499,7 +2499,7 @@ }, "created_at": { "type": "number", - "default": 1677262587.4014988 + "default": 1677531173.5856462 }, "config_call_dict": { "type": "object", @@ -2794,7 +2794,7 @@ }, "created_at": { "type": "number", - "default": 1677262587.404913 + "default": 1677531173.588902 }, "config_call_dict": { "type": "object", @@ -3277,7 +3277,7 @@ }, "created_at": { "type": "number", - "default": 1677262587.40784 + "default": 1677531173.591784 }, "config_call_dict": { "type": "object", @@ -3677,7 +3677,7 @@ }, "created_at": { "type": "number", - "default": 1677262587.410422 + "default": 1677531173.594429 } }, "additionalProperties": false, @@ -3792,7 +3792,7 @@ "generated_at": { "type": "string", "format": "date-time", - "default": "2023-02-24T18:16:27.379048Z" + "default": "2023-02-27T20:52:53.564664Z" }, "invocation_id": { "oneOf": [ @@ -3803,7 +3803,7 @@ "type": "null" } ], - "default": "678dd0f6-ab20-41c8-a3d8-bbc986a26583" + "default": "f578f819-6554-4f6f-8af4-635e35249cb2" }, "env": { "type": "object", @@ -4156,7 +4156,7 @@ }, "created_at": { "type": "number", - "default": 1677262587.411136 + "default": 1677531173.595732 }, "supported_languages": { "oneOf": [ @@ -4399,7 +4399,7 @@ }, "created_at": { "type": "number", - "default": 1677262587.4128408 + "default": 1677531173.597582 } }, "additionalProperties": false, @@ -4622,11 +4622,21 @@ }, "created_at": { "type": "number", - "default": 1677262587.414759 + "default": 1677531173.599355 + }, + "group": { + "oneOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ] } }, "additionalProperties": false, - "description": "Metric(name: str, resource_type: dbt.node_types.NodeType, package_name: str, path: str, original_file_path: str, unique_id: str, fqn: List[str], description: str, label: str, calculation_method: str, expression: str, filters: List[dbt.contracts.graph.unparsed.MetricFilter], time_grains: List[str], dimensions: List[str], timestamp: Optional[str] = None, window: Optional[dbt.contracts.graph.unparsed.MetricTime] = None, model: Optional[str] = None, model_unique_id: Optional[str] = None, meta: Dict[str, Any] = , tags: List[str] = , config: dbt.contracts.graph.model_config.MetricConfig = , unrendered_config: Dict[str, Any] = , sources: List[List[str]] = , depends_on: dbt.contracts.graph.nodes.DependsOn = , refs: List[List[str]] = , metrics: List[List[str]] = , created_at: float = )" + "description": "Metric(name: str, resource_type: dbt.node_types.NodeType, package_name: str, path: str, original_file_path: str, unique_id: str, fqn: List[str], description: str, label: str, calculation_method: str, expression: str, filters: List[dbt.contracts.graph.unparsed.MetricFilter], time_grains: List[str], dimensions: List[str], timestamp: Optional[str] = None, window: Optional[dbt.contracts.graph.unparsed.MetricTime] = None, model: Optional[str] = None, model_unique_id: Optional[str] = None, meta: Dict[str, Any] = , tags: List[str] = , config: dbt.contracts.graph.model_config.MetricConfig = , unrendered_config: Dict[str, Any] = , sources: List[List[str]] = , depends_on: dbt.contracts.graph.nodes.DependsOn = , refs: List[List[str]] = , metrics: List[List[str]] = , created_at: float = , group: Optional[str] = None)" }, "MetricFilter": { "type": "object", diff --git a/tests/functional/artifacts/test_artifacts.py b/tests/functional/artifacts/test_artifacts.py index 1cd9cc1fbc9..d714bcbafdc 100644 --- a/tests/functional/artifacts/test_artifacts.py +++ b/tests/functional/artifacts/test_artifacts.py @@ -207,7 +207,8 @@ models: - name: ephemeral_summary description: "{{ doc('ephemeral_summary') }}" - group: test_group + config: + group: test_group columns: &summary_columns - name: first_name description: "{{ doc('summary_first_name') }}" diff --git a/tests/functional/groups/test_access.py b/tests/functional/groups/test_access.py index c078bd4e9cb..2a8902b3027 100644 --- a/tests/functional/groups/test_access.py +++ b/tests/functional/groups/test_access.py @@ -1,7 +1,7 @@ import pytest -from dbt.tests.util import run_dbt, get_manifest, write_file +from dbt.tests.util import run_dbt, get_manifest, write_file, rm_file from dbt.node_types import AccessType -from dbt.exceptions import InvalidAccessTypeError +from dbt.exceptions import InvalidAccessTypeError, DbtReferenceError my_model_sql = "select 1 as fun" @@ -34,6 +34,140 @@ access: unsupported """ +ref_my_model_sql = """ + select fun from {{ ref('my_model') }} +""" + +groups_yml = """ +version: 2 + +groups: + - name: analytics + owner: + name: analytics_owner + - name: marts + owner: + name: marts_owner +""" + + +v3_schema_yml = """ +version: 2 + +models: + - name: my_model + description: "my model" + access: private + group: analytics + - name: another_model + description: "yet another model" + - name: ref_my_model + description: "a model that refs my_model" + group: analytics +""" + +v4_schema_yml = """ +version: 2 + +models: + - name: my_model + description: "my model" + access: private + group: analytics + - name: another_model + description: "yet another model" + - name: ref_my_model + description: "a model that refs my_model" + group: marts +""" + +simple_exposure_yml = """ +version: 2 + +exposures: + - name: simple_exposure + label: simple exposure label + type: dashboard + depends_on: + - ref('my_model') + owner: + email: something@example.com +""" + +v5_schema_yml = """ +version: 2 + +models: + - name: my_model + description: "my model" + access: private + group: analytics + - name: another_model + description: "yet another model" + - name: ref_my_model + description: "a model that refs my_model" + group: marts + - name: ref_my_model + description: "a model that refs my_model" + group: analytics + - name: people_model + description: "some people" + access: private + group: analytics +""" + +people_model_sql = """ +select 1 as id, 'Drew' as first_name, 'Banin' as last_name, 'yellow' as favorite_color, true as loves_dbt, 5 as tenure, current_timestamp as created_at +union all +select 1 as id, 'Jeremy' as first_name, 'Cohen' as last_name, 'indigo' as favorite_color, true as loves_dbt, 4 as tenure, current_timestamp as created_at +union all +select 1 as id, 'Callum' as first_name, 'McCann' as last_name, 'emerald' as favorite_color, true as loves_dbt, 0 as tenure, current_timestamp as created_at +""" + +people_metric_yml = """ +version: 2 + +metrics: + + - name: number_of_people + label: "Number of people" + description: Total count of people + model: "ref('people_model')" + calculation_method: count + expression: "*" + timestamp: created_at + time_grains: [day, week, month] + dimensions: + - favorite_color + - loves_dbt + meta: + my_meta: 'testing' + config: + group: analytics +""" + +v2_people_metric_yml = """ +version: 2 + +metrics: + + - name: number_of_people + label: "Number of people" + description: Total count of people + model: "ref('people_model')" + calculation_method: count + expression: "*" + timestamp: created_at + time_grains: [day, week, month] + dimensions: + - favorite_color + - loves_dbt + meta: + my_meta: 'testing' + config: + group: marts +""" + class TestAccess: @pytest.fixture(scope="class") @@ -64,3 +198,54 @@ def test_access_attribute(self, project): with pytest.raises(InvalidAccessTypeError): run_dbt(["run"]) + + # Remove invalid access files and write out model that refs my_model + rm_file(project.project_root, "models", "yet_another_model.sql") + write_file(schema_yml, project.project_root, "models", "schema.yml") + write_file(ref_my_model_sql, project.project_root, "models", "ref_my_model.sql") + results = run_dbt(["run"]) + assert len(results) == 3 + + # make my_model private, set same group on my_model and ref_my_model + write_file(groups_yml, project.project_root, "models", "groups.yml") + write_file(v3_schema_yml, project.project_root, "models", "schema.yml") + results = run_dbt(["run"]) + assert len(results) == 3 + manifest = get_manifest(project.project_root) + ref_my_model_id = "model.test.ref_my_model" + assert manifest.nodes[my_model_id].group == "analytics" + assert manifest.nodes[ref_my_model_id].group == "analytics" + + # Change group on ref_my_model and it should raise + write_file(v4_schema_yml, project.project_root, "models", "schema.yml") + with pytest.raises(DbtReferenceError): + run_dbt(["run"]) + + # put back group on ref_my_model, add exposure with ref to private model + write_file(v3_schema_yml, project.project_root, "models", "schema.yml") + # verify it works again + results = run_dbt(["run"]) + assert len(results) == 3 + # Write out exposure refing private my_model + write_file(simple_exposure_yml, project.project_root, "models", "simple_exposure.yml") + # Fails with reference error + with pytest.raises(DbtReferenceError): + run_dbt(["run"]) + + # Remove exposure and add people model and metric file + write_file(v5_schema_yml, project.project_root, "models", "schema.yml") + rm_file(project.project_root, "models", "simple_exposure.yml") + write_file(people_model_sql, "models", "people_model.sql") + write_file(people_metric_yml, "models", "people_metric.yml") + # Should succeed + results = run_dbt(["run"]) + assert len(results) == 4 + manifest = get_manifest(project.project_root) + metric_id = "metric.test.number_of_people" + assert manifest.metrics[metric_id].group == "analytics" + + # Change group of metric + write_file(v2_people_metric_yml, "models", "people_metric.yml") + # Should raise a reference error + with pytest.raises(DbtReferenceError): + run_dbt(["run"])