dbt-labs / dbt-labs/dbt

[1.x Bug] `ModelNode.same_ref_representation` compares `access`/`latest_version`/`deprecation_date` rendered and unconditionally — inconsistent with `state_modified_compare_more_unrendered_values`?

Open
#15,972 1 comment 0 reactions 0 assignees View on GitHub
area:engine engine:v1 triage
Dominant language
Rust
Stars
13.8k
Forks
2.6k
Avg merge
21h 31m
Merged PRs (30d)
56

Description

### Is this a new bug in dbt-core?

- [x] I believe this **may** be an inconsistency worth confirming — see "Expected Behavior" below,
this is as much a question as a bug report.
- [x] I have searched the existing issues, and I could not find an existing issue for this bug

### Current Behavior

`ModelNode.same_contents` ANDs in a third conjunct beyond the base `same_contents`:

```python
def same_contents(self, old: Optional["ModelNode"], adapter_type: Optional[str]) -> bool:
return (
old is not None
and super().same_contents(old, adapter_type)
and self.same_ref_representation(old)
)

def same_ref_representation(self, old: "ModelNode") -> bool:
return (
# Changing the latest_version may break downstream unpinned refs
self.latest_version == old.latest_version
# Changes to access or deprecation_date may lead to ref-related parsing errors
and self.access == old.access
and self.deprecation_date == old.deprecation_date
)
```

This compares three attributes read off the **rendered**, in-memory node — not the raw/unrendered
`schema.yml` config — and it runs **unconditionally**, regardless of the
`state_modified_compare_more_unrendered_values` flag.

By contrast, essentially every other `config:`-sourced field that `state:modified` compares (via
`BaseConfig.same_contents` / `ModelConfig`) moved to **unrendered** comparison behind that flag,
specifically to suppress false positives from environment-aware Jinja — a value that renders
differently between the "old" run's environment and the current one, even though the *authored*
config didn't change. `access` is itself one of those `ModelConfig` fields, compared a second time (by
design — it's a node attribute *and* a config key, and both comparisons are meant to happen). So
`access` gets the unrendered/flag-gated treatment once, via `check_configs_modified`, and the
rendered/unconditional treatment a second time, via `same_ref_representation` — two code paths that
can disagree with each other about the same authored config.

### Expected Behavior

We're not certain which of the following is intended, and would appreciate a maintainer's read:

1. **Deliberate.** `latest_version`, `access`, and `deprecation_date` are rarely (if ever) expected to
be templated with environment-aware Jinja in practice, so rendered/unconditional comparison here is
considered acceptably safe, and the omission from
`state_modified_compare_more_unrendered_values` is intentional. If so, a one-line comment at
`same_ref_representation`'s definition saying so would save the next person from re-raising this.
2. **Oversight.** These three should also respect the flag / move to unrendered comparison for
consistency with the rest of `state:modified`, since a project that *does* template one of them
(e.g. a `deprecation_date` gated on `target.name`) can hit exactly the false-positive class of bug
the flag exists to prevent — and would do so regardless of the flag's setting, since this conjunct
isn't gated by it at all.

### Steps To Reproduce

(Only demonstrates a problem under interpretation 2 above — i.e., this reproduces the false positive
that would exist if the answer is "oversight".)

1. Author a versioned model whose `deprecation_date` depends on environment-aware Jinja, e.g.:
```yaml
models:
- name: my_model
latest_version: 1
versions:
- v: 1
defined_in: my_model_v1
deprecation_date: "{{ '2099-01-01' if target.name == 'prod' else none }}"
```
2. `dbt parse --target dev` (renders to `deprecation_date: null`) and save the manifest as the prior
state.
3. `dbt list -s state:modified --target prod --state `, without editing the project.
4. The model is selected as modified, purely because `target.name` differs between the two runs — not
because the author changed anything. This is the same shape of false positive that
`state_modified_compare_more_unrendered_values` was introduced to fix for other fields, but it isn't
covered by that flag.

### Relevant log output

```shell
(not captured — this is a design question raised from source review, not an observed incident; see
"Steps To Reproduce" for how to produce a concrete example if wanted)
```

### Environment

```markdown
- dbt: `1.latest` (1.14.0a1) as of this issue; mechanism present since `same_ref_representation` was
introduced
- OS / Python: not relevant
```

### Which database adapter are you using with dbt?

N/A — parser/state-comparison logic, not adapter-specific.

### Additional Context

Source: [`core/dbt/contracts/graph/nodes.py:671-684`](https://github.com/dbt-labs/dbt-core/blob/f54a42c2e47dd5ff4bd9375851fa238cdcd4eb22/core/dbt/contracts/graph/nodes.py#L671-L684).
Contrast with the flag-gated unrendered-config merge for the config-key comparison path at
[`core/dbt/parser/base.py:413-426`](https://github.com/dbt-labs/dbt-core/blob/f54a42c2e47dd5ff4bd9375851fa238cdcd4eb22/core/dbt/parser/base.py#L413-L426).

**Why we're asking:** dbt Labs' Fusion engine is adding a `same_ref_representation`-equivalent check to
match dbt Core for `state:modified` parity (in [dbt-labs/fs#13181](https://github.com/dbt-labs/fs/pull/13181)),
and has for now transcribed the exact rendered/ungated
semantics above, since matching current dbt Core behavior is the immediate goal. If dbt Core's answer
here is "oversight" and this later changes, Fusion would need a matching follow-up change — flagging
now so that dependency is visible rather than discovered again independently later.

This was raised alongside #15971 — `versions[].access:` sibling handling —
found during the same audit, but is a distinct question (different code path, different kind of
question: intent vs. discard-bug) so it's filed separately rather than folded into that thread.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.