fix: Avoid injected settings to reach manifest and overselect on state:modified

[29antonioac]

Summary

Fixes a bug where get_model_settings mutated model['config']['settings'] in-place, causing adapter-injected settings (e.g. replicated_deduplication_window='0') to be persisted into manifest.json after dbt run. This produced a permanent asymmetry with manifests generated by dbt parse or dbt ls, which never call get_model_settings and therefore produced a clean manifest. The mismatch caused --select state:modified+ --defer to report config_changed on every affected model on every subsequent run, even when nothing had changed.

Fix: deep-copy the settings dict in get_model_settings before passing it to update_model_settings, so the original model['config'] is never touched.

Related to #372.
Closes #676

Checklist

• Unit and integration tests covering the common scenarios were added
• A human-readable description of the changes was provided to include in CHANGELOG

[koletzilla]

Ey @29antonioac , thanks as always for reporting it and for sharing a PR 🙇

I was wondering that the solution feels like should go in the other direction: Instead of skipping it, any setting the adapter add should be recorded in the manifest in both situations.

Out of curiosity, have you checked it we can do it otherwise? Maybe we need to change when this setting is added to make sure it's always included

[29antonioac]

Hey mate, no problem! Happy to support OSS 😄

Please take my opinion with a pinch of salt: I'm not a dbt expert or power user, just a guy trying to help his colleagues, I feel more comfortable with ClickHouse than dbt in general, so I can be missing details 😄 .

I was wondering the same, maybe the manifest should get all settings of the model, including the injected ones. However, when finding the issue because of replicated_deduplication_window=0, my first thought was adapter-specific settings should not belong to the manifest: if tomorrow this setting disappears from the adapter (ClickHouse default, table engine refined behaviour or any other reason), all users will find their models flagged as modified when they didn't do any explicit change in the model.

The setting only applies to MergeTree tables, not sure if dbt parse could retrieve this without a connection (I assume it can get the engine from the model). And given the code uses conn I guess the injection could depend on the server/instance?

I'm happy to look for a way to get the manifest with whatever setting the adapter injects at dbt parse step, as you are the main maintainers I trust you more than myself 😂 .

[koletzilla]

if tomorrow this setting disappears from the adapter (ClickHouse default, table engine refined behaviour or any other reason), all users will find their models flagged as modified when they didn't do any explicit change in the model.

Agree 100% that it wouldn't be nice at all, but after giving a second thought, maybe it's worth it to flag them 🤔. If some config change, like replicated_deduplication_window, you'd probably need to make sure you're not depending on that setting at all or your data will be impacted in the next run. I guess most of the settings would be probably false positives as you mentioned: things that are now default, settings that are no longer needed... but worth checking in case you're still running on an old system and maybe these settings still apply to you 🤔

Would you check if there's indeed a way to do that? Btw, I thought version checking may get in the middle of it, so assume in your checks that the version checking is removed. I already removed that code here https://github.com/ClickHouse/dbt-clickhouse/pull/678/changes

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit bb857a2. Configure here.}

[cursor]

Missing server version gate

Medium Severity

The get_injected_model_settings function now unconditionally adds replicated_deduplication_window='0' when automatic deduplication is disabled. This change removes a prior server version check (for ClickHouse >= 22.7.1.2484), meaning older ClickHouse clusters will now receive this setting, which was previously skipped.

 

^{Reviewed by Cursor Bugbot for commit bb857a2. Configure here.}

[29antonioac]

@koletzilla It's not the nicest solution, but with Claude and inspecting dbt core it seems there is a point where the manifest is resolved at parse time and we can inject settings there instead: set_macro_resolver. The solution looks clean to me, injecting this in just one place and being able to have the same one after parsing and after running.

It feels a bit hacky though. Also the solution in the new test to read explicitly the manifest without get_manifest makes me feel a bit uncomfortable 😂 . Not sure if we are smashing parts of the dbt core flow.

I removed the version comparison too as you mentioned 😄