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 1990 add access property to parsed nodes #7007

Merged
merged 17 commits into from
Feb 24, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 6 additions & 0 deletions .changes/unreleased/Features-20230218-092816.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Features
body: Add access attribute to parsed nodes
time: 2023-02-18T09:28:16.448175-05:00
custom:
Author: gshank
Issue: "6824"
10 changes: 8 additions & 2 deletions core/dbt/contracts/graph/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -1166,7 +1166,7 @@ def __init__(self, macros):


@dataclass
@schema_version("manifest", 8)
@schema_version("manifest", 9)
class WritableManifest(ArtifactMixin):
nodes: Mapping[UniqueID, ManifestNode] = field(
metadata=dict(description=("The nodes defined in the dbt project and its dependencies"))
Expand Down Expand Up @@ -1220,7 +1220,13 @@ class WritableManifest(ArtifactMixin):

@classmethod
def compatible_previous_versions(self):
return [("manifest", 4), ("manifest", 5), ("manifest", 6), ("manifest", 7)]
return [
("manifest", 4),
("manifest", 5),
("manifest", 6),
("manifest", 7),
("manifest", 8),
]

def __post_serialize__(self, dct):
for unique_id, node in dct["nodes"].items():
Expand Down
27 changes: 25 additions & 2 deletions core/dbt/contracts/graph/nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,17 @@
from dbt.contracts.util import Replaceable, AdditionalPropertiesMixin
from dbt.events.proto_types import NodeInfo
from dbt.events.functions import warn_or_error
from dbt.exceptions import ParsingError
from dbt.exceptions import ParsingError, InvalidAccessTypeError
from dbt.events.types import (
SeedIncreased,
SeedExceedsLimitSamePath,
SeedExceedsLimitAndPathChanged,
SeedExceedsLimitChecksumChanged,
ValidationWarning,
)
from dbt.events.contextvars import set_contextvars
from dbt.flags import get_flags
from dbt.node_types import ModelLanguage, NodeType
from dbt.node_types import ModelLanguage, NodeType, AccessType
from dbt.utils import cast_dict_to_dict_of_strings


Expand Down Expand Up @@ -365,6 +366,26 @@ def patch(self, patch: "ParsedNodePatch"):
self.created_at = time.time()
self.description = patch.description
self.columns = patch.columns
# This might not be the ideal place to validate the "access" field,
# but at this point we have the information we need to properly
# validate and we don't before this.
if patch.access:
if self.resource_type == NodeType.Model:
if AccessType.is_valid(patch.access):
self.access = AccessType(patch.access)
else:
raise InvalidAccessTypeError(
unique_id=self.unique_id,
field_value=patch.access,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of a warning (which a user can promote to an error), Is it possible to raise a ParsingError here instead? I think that's more consistent with existing product expectations for other entities. For example, configuring type: invalid on an exposure gets:

dbt.exceptions.YamlParseDictError: Parsing Error
Invalid exposures config given in FilePath(searched_path='models', relative_path='schema.yml', modification_time=1677270162.964944, project_root='/Users/michelleark/src/jaffle_shop') @ exposures: {'name': 'jaffle_shop_report', 'type': 'invalid', 'owner': {'name': 'testsss'}} - at path ['type']: 'invalid' is not one of ['dashboard', 'notebook', 'analysis', 'ml', 'application']

Copy link
Contributor

@MichelleArk MichelleArk Feb 24, 2023

Choose a reason for hiding this comment

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

Also, can we define access on the dataclass declaration for the ParsedNode, simialr to the other class attributes set by this method (created_at, description, patch_path). Ideally access is a AccessType if possible. nevermind!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. It's an exception, so it aborts everything, which is not my favorite pattern because if you have multiple errors you have to address them one at a time, but we can certainly do that.

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 not sure what you're suggesting with regard to ParsedNode. It's set in the dataclass declaration for ModelNode, which is where you wanted it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you're suggesting with regard to ParsedNode. It's set in the dataclass declaration for ModelNode, which is where you wanted it.

I see, makes sense. Still getting my head around patches. Sorry for the confusion!

Copy link
Contributor

@MichelleArk MichelleArk Feb 24, 2023

Choose a reason for hiding this comment

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

which is not my favorite pattern because if you have multiple errors you have to address them one at a time

Agree - but I'm nervous about gracefully handling this invalid config because, given that the default protected value is not the most restricted access option. For example, a typo to access:priivate would lead to access:protected on the model, which is probably a more open level of access than the user meant to specify. Hopefully they'd catch it in review / via logs but I think it's worth raising an exception for this case.

else:
warn_or_error(
ValidationWarning(
field_name="access",
resource_type=self.resource_type.value,
node_name=patch.name,
)
)

def same_contents(self, old) -> bool:
if old is None:
Expand Down Expand Up @@ -465,6 +486,7 @@ class HookNode(CompiledNode):
@dataclass
class ModelNode(CompiledNode):
resource_type: NodeType = field(metadata={"restrict": [NodeType.Model]})
access: AccessType = AccessType.Protected


# TODO: rm?
Expand Down Expand Up @@ -1140,6 +1162,7 @@ class ParsedPatch(HasYamlMetadata, Replaceable):
@dataclass
class ParsedNodePatch(ParsedPatch):
columns: Dict[str, ColumnInfo]
access: Optional[str]


@dataclass
Expand Down
1 change: 1 addition & 0 deletions core/dbt/contracts/graph/unparsed.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ class HasDocs(AdditionalPropertiesMixin, ExtensibleDbtClassMixin, Replaceable):
constraints: Optional[List[str]] = None
constraints_check: Optional[str] = None
docs: Docs = field(default_factory=Docs)
access: Optional[str] = None
_extra: Dict[str, Any] = field(default_factory=dict)


Expand Down
2 changes: 1 addition & 1 deletion core/dbt/contracts/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ def read_and_check_versions(cls, path: str):
expected=str(cls.dbt_schema_version),
found=previous_schema_version,
)
if get_manifest_schema_version(data) <= 7:
if get_manifest_schema_version(data) <= 8:
data = upgrade_manifest_json(data)
return cls.from_dict(data) # type: ignore

Expand Down
29 changes: 29 additions & 0 deletions core/dbt/events/proto_types.py

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 24 additions & 1 deletion core/dbt/events/types.proto
Original file line number Diff line number Diff line change
Expand Up @@ -762,7 +762,30 @@ message FinishedRunningStatsMsg {

// I - Project parsing

// Skipping I001, I002, I003, I004, I005, I006, I007, I008, I009
// Skipping I001, I002, I003, I004, I005, I006, I007

// I008
message InvalidValueForField {
string field_name = 1;
string field_value = 2;
}

message InvalidValueForFieldMsg {
EventInfo info = 1;
InvalidValueForField data = 2;
}

// I009
message ValidationWarning {
string resource_type = 1;
string field_name = 2;
string node_name = 3;
}

message ValidationWarningMsg {
EventInfo info = 1;
ValidationWarning data = 2;
}

// I010
message ParsePerfInfoPath {
Expand Down
20 changes: 19 additions & 1 deletion core/dbt/events/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -768,7 +768,25 @@ def message(self) -> str:
# =======================================================


# Skipping I001, I002, I003, I004, I005, I006, I007, I008, I009
# Skipping I001, I002, I003, I004, I005, I006, I007


@dataclass
class InvalidValueForField(WarnLevel, pt.InvalidValueForField):
def code(self):
return "I008"

def message(self) -> str:
return f"Invalid value ({self.field_value}) for field {self.field_name}"


@dataclass
class ValidationWarning(WarnLevel, pt.ValidationWarning):
def code(self):
return "I009"

def message(self) -> str:
return f"Field {self.field_name} is not valid for {self.resource_type} ({self.node_name})"


@dataclass
Expand Down
10 changes: 10 additions & 0 deletions core/dbt/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -1114,6 +1114,16 @@ def __init__(self, exc: ValidationError, node):
super().__init__(msg=self.msg)


class InvalidAccessTypeError(ParsingError):
def __init__(self, unique_id: str, field_value: str):
self.unique_id = unique_id
self.field_value = field_value
msg = (
f"Node {self.unique_id} has an invalid value ({self.field_value}) for the access field"
)
super().__init__(msg=msg)


class SameKeyNestedError(CompilationError):
def __init__(self):
msg = "Test cannot have the same key at the top-level and in config"
Expand Down
14 changes: 14 additions & 0 deletions core/dbt/node_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,20 @@
from dbt.dataclass_schema import StrEnum


class AccessType(StrEnum):
Protected = "protected"
Private = "private"
Public = "public"

@classmethod
def is_valid(cls, item):
try:
cls(item)
except ValueError:
return False
return True


class NodeType(StrEnum):
Model = "model"
Analysis = "analysis"
Expand Down
1 change: 1 addition & 0 deletions core/dbt/parser/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -878,6 +878,7 @@ def parse_patch(self, block: TargetBlock[NodeTarget], refs: ParserRef) -> None:
meta=block.target.meta,
docs=block.target.docs,
config=block.target.config,
access=block.target.access,
)
assert isinstance(self.yaml.file, SchemaSourceFile)
source_file: SchemaSourceFile = self.yaml.file
Expand Down
Loading