CT 1990 add access property to parsed nodes

[gshank]

resolves #6824

Description

Add access property to parsed nodes. (Did not add to metrics.) Update tests and bump schema version.

Checklist

• I have read the contributing guide and understand what's expected of me
• I have signed the CLA
• I have run this code in development and it appears to resolve the stated issue
• This PR includes tests, or tests are not required/relevant for this PR
• I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
• I have run changie new to create a changelog entry

[MichelleArk]

Would adding the access property to parsed nodes would enable configuring access on all ManifestNodes?

ManifestSQLNode = Union[
    AnalysisNode,
    SingularTestNode,
    HookNode,
    ModelNode,
    RPCNode,
    SqlNode,
    GenericTestNode,
    SnapshotNode,
]

# All SQL nodes plus SeedNode (csv files)
ManifestNode = Union[
    ManifestSQLNode,
    SeedNode,
]

(From: https://github.com/dbt-labs/dbt-core/blob/main/core/dbt/contracts/graph/nodes.py#L1153-L1168)

I'm not sure it's desirable to provide this configuration beyond ModelNodes (and perhaps SnapshotNode) given we don't yet have plans to act on the setting of the access property beyond restricting references to ref-able nodes, which only applies to some of the ManifestNodes.

[gshank]

The code and the tests assume that all of those nodes have the same set of attributes. It's certainly possible to limit it to just ModelNode and SnapshotNode, but it's actually rather more work, which is why I did it this way for commenting. Are we going to limit "group" to the same set of nodes?

[gshank]

Also the way that "patches" are processed, all of the manifest nodes are handled with the same code, so unless we split that up we still have to add "access" to ParsedNodePatch. Would we want to throw an error if somebody tried to set "access" (or group?) for non-supported node types?

[MichelleArk]

Are we going to limit "group" to the same set of nodes?

Nodes that should be 'groupable' are nodes that have the ability to ref other nodes. On the other hand, nodes that can have the access property are nodes that are 'refable'. So no - they should be different sets of nodes.

The configured group value will be used to determine whether the ref is permitted or not for private models. If there is no configured group on a model accessing a private model, the ref should be restricted.

Would we want to throw an error if somebody tried to set "access" (or group?) for non-supported node types?

I'd say an error or at least a warning, given that the user may be expecting dbt to actually something given the set access property and we have no plans to act on this property value for other nodes in Phase 1 of Multi-project deployments

It's certainly possible to limit it to just ModelNode and SnapshotNode...

I'd actually advocate to limit this to just ModelNode, since that is the most concrete product story we have around multi-project deployments. Ultimately, a snapshot or seed could be wrapped in a model to use the access property. Would definitely be open to relaxing this given community / user feedback though.

[gshank]

Note: we didn't move to version 9 of the manifest schema when we should have, so I updated all the v8 manifests that shouldn't have the new fields in them, and switched to v9.

[MichelleArk]

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']

[MichelleArk]

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!

[gshank]

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.

[gshank]

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.

[MichelleArk]

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!

[MichelleArk]

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.