-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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-581 grants as configs #5230
Ct-581 grants as configs #5230
Changes from 6 commits
4b46fb6
9b788a4
6d18915
31c0e9f
649c9c5
7459608
266b469
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
kind: Features | ||
body: Grants as Node Configs | ||
time: 2022-05-10T20:49:49.197999-04:00 | ||
custom: | ||
Author: gshank | ||
Issue: "5189" | ||
PR: "5230" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
from typing import List, Iterator, Dict, Any, TypeVar, Generic | ||
|
||
from dbt.config import RuntimeConfig, Project, IsFQNResource | ||
from dbt.contracts.graph.model_config import BaseConfig, get_config_for | ||
from dbt.contracts.graph.model_config import BaseConfig, get_config_for, _listify | ||
from dbt.exceptions import InternalException | ||
from dbt.node_types import NodeType | ||
from dbt.utils import fqn_search | ||
|
@@ -264,18 +264,49 @@ def add_config_call(self, opts: Dict[str, Any]) -> None: | |
|
||
@classmethod | ||
def _add_config_call(cls, config_call_dict, opts: Dict[str, Any]) -> None: | ||
# config_call_dict is already encountered configs, opts is new | ||
# This mirrors code in _merge_field_value in model_config.py which is similar but | ||
# operates on config objects. | ||
for k, v in opts.items(): | ||
# MergeBehavior for post-hook and pre-hook is to collect all | ||
# values, instead of overwriting | ||
if k in BaseConfig.mergebehavior["append"]: | ||
if not isinstance(v, list): | ||
v = [v] | ||
if k in BaseConfig.mergebehavior["update"] and not isinstance(v, dict): | ||
raise InternalException(f"expected dict, got {v}") | ||
if k in config_call_dict and isinstance(config_call_dict[k], list): | ||
config_call_dict[k].extend(v) | ||
elif k in config_call_dict and isinstance(config_call_dict[k], dict): | ||
config_call_dict[k].update(v) | ||
if k in config_call_dict and isinstance(config_call_dict[k], list): | ||
config_call_dict[k].extend(v) | ||
else: | ||
config_call_dict[k] = v | ||
|
||
elif k in BaseConfig.mergebehavior["update"]: | ||
if not isinstance(v, dict): | ||
raise InternalException(f"expected dict, got {v}") | ||
if k in config_call_dict and isinstance(config_call_dict[k], dict): | ||
config_call_dict[k].update(v) | ||
else: | ||
config_call_dict[k] = v | ||
elif k in BaseConfig.mergebehavior["dict_key_append"]: | ||
if not isinstance(v, dict): | ||
raise InternalException(f"expected dict, got {v}") | ||
if k in config_call_dict and isinstance(config_call_dict[k], dict): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same with this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've removed the isinstance check. It ought to be unnecessary. |
||
for key, value in v.items(): | ||
extend = False | ||
# This might start with a +, to indicate we should extend the list | ||
# instead of just clobbering it | ||
if key.startswith("+"): | ||
extend = True | ||
if key in config_call_dict[k] and extend: | ||
# extend the list | ||
config_call_dict[k][key].extend(_listify(value)) | ||
else: | ||
# clobber the list | ||
config_call_dict[k][key] = _listify(value) | ||
else: | ||
# This is always a dictionary | ||
config_call_dict[k] = v | ||
# listify everything | ||
for key, value in config_call_dict[k].items(): | ||
config_call_dict[k][key] = _listify(value) | ||
else: | ||
config_call_dict[k] = v | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1135,6 +1135,12 @@ class WritableManifest(ArtifactMixin): | |
) | ||
) | ||
|
||
def __post_serialize__(self, dct): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reason we move this logic from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem we were running into with the partial parsing bug was that the config_call_dict wasn't being saved with the partial parsing manifest. So when a new schema file was added the config was being applied to the existing model node, but without the config_call_dict we lose the config from the SQL file. There were two options to fix it, 1) save the config_call_dict or 2) every time we add a new schema file reload all of the nodes that might be affected. The second option would complicate partial parsing substantially and felt more inconsistent. The config_call_dict wasn't being saved to start with only because my imagination did not think about this case. I clean it up in the WritableManifest post_serialize method because we don't really want or need it in the manifest artifact. |
||
for unique_id, node in dct["nodes"].items(): | ||
if "config_call_dict" in node: | ||
del node["config_call_dict"] | ||
return dct | ||
|
||
|
||
def _check_duplicates(value: HasUniqueID, src: Mapping[str, HasUniqueID]): | ||
if value.unique_id in src: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,7 @@ class MergeBehavior(Metadata): | |
Append = 1 | ||
Update = 2 | ||
Clobber = 3 | ||
DictKeyAppend = 4 | ||
|
||
@classmethod | ||
def default_field(cls) -> "MergeBehavior": | ||
|
@@ -124,6 +125,9 @@ def _listify(value: Any) -> List: | |
return [value] | ||
|
||
|
||
# There are two versions of this code. The one here is for config | ||
# objects, the one in _add_config_call in context_config.py is for | ||
# config_call_dict dictionaries. | ||
def _merge_field_value( | ||
merge_behavior: MergeBehavior, | ||
self_value: Any, | ||
|
@@ -141,6 +145,31 @@ def _merge_field_value( | |
value = self_value.copy() | ||
value.update(other_value) | ||
return value | ||
elif merge_behavior == MergeBehavior.DictKeyAppend: | ||
if not isinstance(self_value, dict): | ||
raise InternalException(f"expected dict, got {self_value}") | ||
if not isinstance(other_value, dict): | ||
raise InternalException(f"expected dict, got {other_value}") | ||
new_dict = {} | ||
for key in self_value.keys(): | ||
new_dict[key] = _listify(self_value[key]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think
Shows up as expected: Whereas:
Shows up as: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really wish Python hadn't chosen to treat strings as sequences... So yeah, listify in all cases would be better. |
||
for key in other_value.keys(): | ||
extend = False | ||
new_key = key | ||
# This might start with a +, to indicate we should extend the list | ||
# instead of just clobbering it | ||
if new_key.startswith("+"): | ||
new_key = key.lstrip("+") | ||
extend = True | ||
if new_key in new_dict and extend: | ||
# extend the list | ||
value = other_value[key] | ||
new_dict[new_key].extend(_listify(value)) | ||
else: | ||
# clobber the list | ||
new_dict[new_key] = _listify(other_value[key]) | ||
return new_dict | ||
|
||
else: | ||
raise InternalException(f"Got an invalid merge_behavior: {merge_behavior}") | ||
|
||
|
@@ -257,6 +286,7 @@ def same_contents(cls, unrendered: Dict[str, Any], other: Dict[str, Any]) -> boo | |
mergebehavior = { | ||
"append": ["pre-hook", "pre_hook", "post-hook", "post_hook", "tags"], | ||
"update": ["quoting", "column_types", "meta"], | ||
"dict_key_append": ["grants"], | ||
} | ||
|
||
@classmethod | ||
|
@@ -427,6 +457,9 @@ class NodeConfig(NodeAndTestConfig): | |
# sometimes getting the Union order wrong, causing serialization failures. | ||
unique_key: Union[str, List[str], None] = None | ||
on_schema_change: Optional[str] = "ignore" | ||
grants: Dict[str, Any] = field( | ||
default_factory=dict, metadata=MergeBehavior.DictKeyAppend.meta() | ||
) | ||
Comment on lines
+460
to
+462
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related tech debt that we discussed yesterday, which is out of scope for this PR, opened as a new issue: #5236 |
||
|
||
@classmethod | ||
def __pre_deserialize__(cls, data): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need the second part of the check? looks like the current behavior is going to be if it is not a dictionary we would just overwrite
config_call_dict[k]
. Could this cause misterious behavior in the future?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it would cause any problems, but you're right that it's unnecessary. I've removed the isinstance check.