Rules that operate on multiple resource types

[ross-whatnot]

As more types of evaluable are covered by dbt-score, I'm increasingly finding that I'm writing somewhat repetitive tests to cover different evaluable types; for a simple example, imagine:

@rule
def model_is_not_beta(model: Model) -> RuleViolation | None:
    if 'beta' in model.tags:
        return RuleViolation(f"{model.name} is beta, which isn't allowed")
    return None

@rule
def snapshot_is_not_beta(snapshot: Snapshot) -> RuleViolation | None:
    if 'beta' in snapshot.tags:
        return RuleViolation(f"{snapshot.name} is beta, which isn't allowed")
    return None

There remain a lot of cases where I do want type specific tests, but there are also cases where I'm validating properties that are common across multiple types of evaluable. I find myself wondering about being able to define a rule like:

@rule
def resource_is_not_beta(resource: Model | Snapshot) -> RuleViolation | None:
    if 'beta' in resource.tags:
        return RuleViolation(f"{resource.name} is beta, which isn't allowed")
    return None

That would feel like it still allows the rule writer control over what types of evaluable a rule is executed on, but also allow "common" rules to run on multiple evaluable types. I don't think this works like this today, but curious if this feels like a productive direction?

[jochemvandooren]

Hey @ross-whatnot! I totally agree, the way typing is done currently is a little bit cumbersome. At first we just supported models, and all other entities were added in rapid succession without taking a step back/refactoring. It's definitely a topic that needs some work; there's a lot of redundancy because we support multiple entities now.

However, I am not sure if we want to have rules that can take different types of input. 🤔 The creator of the rule needs to be very careful when writing such a rule, because the attributes of the different entities are not 100% the same. So these rules are only interesting for the 'standard' dbt properties like: description, meta, config etc.

I would be curious to hear other opinions on this, in our own dbt project we don't have many rules that should apply to different entities

[StrahilPeykov]

I think with minimal changes we could implement this with union type annotations. Then we could have rules like:

@rule
def resource_has_owner(resource: Model | Snapshot | Source) -> RuleViolation | None:
    """All resources should have an owner."""
    if not resource.meta.get("owner"):
        return RuleViolation(f"{resource.name} lacks an owner.")

It would be by probably modifying _introspect_resource_type to detect union types and store multiple types and then updating should_evaluate to check if the evaluable's type is in the rule's supported types.
In that case, rules would just need to be careful to only access properties that exist on all target types; and document the properties that are safely shared, and that every further resource type should be compatible with these shared properties. But the benefits I think are worth.

[ross-whatnot]

To give a bit more context on our use cases - we use some of these common properties (meta, and in some cases, tags) for build orchestration (think, like, dbt build --select config.meta.build_cadence:1h), and so we have some checks around making sure that everything has a valid build_cadence (like, we don't have a 2h build cadence, so we want to give engineers feedback that they have misconfigured something if we see that in their meta). So ideally, I'd want to be able to write a rule like:

@rule(severity=Severity.CRITICAL)
def build_cadences_must_be_valid(resource: Model | Seed | Snapshot) -> RuleViolation | None:
    """Build cadences must be valid"""
    if resource.meta.get("build_cadence") not in ["15m", "1h", "12h", "24h"]:
        return RuleViolation(
            message=f"Resource {resource.name} has an invalid build cadence: {resource.meta.get('build_cadence')}, must be one of: 15m, 1h, 12h, 24h"
        )
    return None

Whereas right now, I have a single function (build_cadence_must_be_valid) that gets called in separate rules for models, seeds, and snapshots.

Agreed that it wouldn't be reasonable for a lot of other kinds of tests, and it definitely requires a bit more developer attention to using common properties. And it's definitely a quality of life improvement more than anything - I can do all of this today with a common function and separate rules.

I'm happy to take a crack at it (along the lines of what @StrahilPeykov suggests), if it feels like it's a desirable direction and the right time!

[matthieucan]

Unioned types sounds pretty smart 💡 But let's be careful before implementing this - as @jochemvandooren mentioned it might be a good time to refactor to better handle the various supported dbt entities.

In the meantime however, you can easily avoid duplicate code by doing

def entity_is_not_beta(entity: Model | Snapshot) -> RuleViolation | None:
    if 'beta' in entity.tags:
        return RuleViolation(f"{model.name} is beta, which isn't allowed")
    return None

@rule
def model_is_not_beta(model: Model) -> RuleViolation | None:
    return entity_is_not_beta(model)

@rule
def snapshot_is_not_beta(snapshot: Snapshot) -> RuleViolation | None:
    return entity_is_not_beta(snapshot)

right?

I agree we can brainstorm something more concise, but that should hopefully help!

[ross-whatnot]

Yeah, that's exactly what I'm doing right now, and it works just fine; this would be a convenience feature rather than actually unlocking anything, so definitely makes sense to focus on the fundamentals first.