Skip to content

Fix: do not apply model_defaults statements to external models - #6189

Open
simen-strand wants to merge 2 commits into
SQLMesh:mainfrom
simen-strand:fix/external-model-skip-default-statements
Open

simen-strand wants to merge 2 commits into
SQLMesh:mainfrom
simen-strand:fix/external-model-skip-default-statements

Conversation

@simen-strand

@simen-strand simen-strand commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Description

External models inherit the pre_statements, post_statements and on_virtual_update from model_defaults, but they never run them, and they're built without the project's macros. If a default statement calls a project macro and you edit it, the plan tries to render the external models' copies and fails:

ConfigError: Failed to resolve macros for
@tag_query('v2')
Macro 'tag_query' does not exist.
 at '.../external_models.yaml'

This PR stops external models from inheriting those three defaults, the same way they already skip the default gateway. Statements set explicitly on an external model are kept. The docs now say the defaults don't apply to external models.

Upgrade impact

Projects whose model_defaults define statements will see a one-off change on their first plan after upgrading:

  • Every external model shows as changed (non-breaking), and the models downstream of it as indirectly changed. No backfill is needed.
  • Exception: if SQLMesh can't determine the columns of the external model or of any model downstream of it (e.g. SELECT * from an undeclared source), the default external: full auto-categorization makes the change breaking, and that external model's downstream models are backfilled. A forward-only plan, or external: semi, avoids that.

Projects without default statements are unaffected.

Alternatives considered

  • If rendering fails, compare the unrendered statements of external models: fingerprints stay the same, so there's no upgrade impact. But external models keep statements that never run, and the fix relies on catching render errors.
  • Pass the project's macros to external models: rendering works. But fingerprints still change, so the upgrade impact is the same, and external models still carry statements that never run.

Test Plan

  • New unit test: external models no longer inherit the default statements, explicit ones are kept, and SQL models still inherit them.
  • New context test, for pre- and post-statements: plan with @tag_query('v1'), change it to v2, plan again. It fails on main with the error above and passes here.
  • Upgrade checked by applying state with main and planning with this branch. The results match the impact described above, and a forward-only plan needs no backfill.

Checklist

  • I have run make style and fixed any issues
  • I have added tests for my changes (if applicable)
  • All existing tests pass (make fast-test)
  • My commits are signed off (git commit -s) per the DCO

Summary by CodeRabbit

  • Bug Fixes

    • External models no longer inherit project-level default pre-statements, post-statements, or virtual-update statements. Statements specified directly on an external model are preserved; other models continue to receive the defaults.
    • Changes to default statements are reflected when determining whether affected SQL models have been modified.
  • Documentation

    • Clarified which models receive project-level statement defaults and how defaults combine with model-specific statements.

External models inherited the model_defaults pre_statements, post_statements
and on_virtual_update, but are built without the project's macros and never
execute statements. When a default statement called a project macro and its
text changed, the plan rendered the external model's statements to compare
them and failed with "Macro '<name>' does not exist".

Stop external models from inheriting these defaults, as is already done for
the default gateway. This changes the fingerprint of external models in
projects that use default statements; the empty migration bumps the state
schema version so cached models are rebuilt.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Simen Strand <simen.strand@netcheck.de>
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

External models no longer inherit project-level pre_statements, post_statements, or on_virtual_update defaults. Explicitly supplied statements remain. Documentation and tests cover the behavior and model modification tracking.

Changes: External model default statements

Layer / File(s) Summary
Exclude inherited statement defaults
sqlmesh/core/model/definition.py, docs/reference/model_configuration.md, tests/core/test_model.py
External models exclude inherited pre-statements, post-statements, and virtual-update defaults. Tests check that explicit external-model statements remain and SQL models still inherit the defaults.
Cover model modification tracking
sqlmesh/migrations/v0103_exclude_default_statements_from_external_models.py, tests/core/test_context.py
The migration module adds no-op schema and row migration functions. Context tests check that changing statement defaults marks the SQL model as directly modified and that the external model has no inherited pre- or post-statements.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related PRs

  • SQLMesh/sqlmesh#6179: Adds a migration module with the same migration version, 0103, for a separate fingerprint change.

Suggested reviewers: mday-io

Merge Risk: Low · up to 644e0

Projects with default statements may see downstream backfills on their first plan after upgrading. Document that condition and its stated workarounds before merging, or accept the documentation gap as a bounded follow-up.

Security architecture risk: Low · up to 0fe06

The change narrows which models receive project statements without adding execution privileges. The main risk is the first upgraded plan: affected external models can cause downstream rebuilds when column information is incomplete. Migration recovery behavior is unchanged, but interruption and concurrent-upgrade behavior were not fully established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The supported propagation scope is affected external models and their downstream dependency graphs in a project. The reviewed change does not add credentials or a new privileged sink; its wider effects arise through existing fingerprinting, planning and migration paths.

Trust Boundaries and Controls

  • observed — The routed test functions, including write_config, operate within temporary test fixtures rather than exposing production request handlers. The migration functions accept existing database authority but do not use it in their bodies.

Resilience and Maintainability Implications

  • observed — The existing snapshot migrator catches fingerprint-computation exceptions and skips the affected traversal without propagating failure. Migration completion can consequently advance the recorded version without invoking rollback for that exception. This behavior is unchanged against the PR base; no PR-specific increase in its failure exposure was established.
Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title names the main change: external models no longer apply model_defaults statements. It matches the summarized code changes in definition.py, the docs, and the tests. It uses the repository's "…
Description check Passed The description covers the required template sections. The Description section explains the problem (external models inherit pre_statements, post_statements, and on_virtual_update but never run them, …
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main change: external models no longer apply model_defaults statements. It matches the summarized code changes in definition.py, the docs, and the tests. It uses the repository's "Fix:" prefix and is short and specific.

Full details: Description check

Explanation

The description covers the required template sections. The Description section explains the problem (external models inherit pre_statements, post_statements, and on_virtual_update but never run them, and fail to render project macros) and the fix. The Test Plan section lists the unit test, the context test, and a manual upgrade check. The Checklist section is filled in with all four items checked. The author also adds Upgrade impact and Alternatives considered sections, which go beyond the template but add useful context. The template's HTML comment placeholders are removed, and the content is specific to the change.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 4 files.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: f1371943-da2d-4f78-af1a-0534bbd94672
Commits

Reviewing files that changed from the base of the PR and between 0fe0645 and 644e06b.

Files selected for processing: 3
  • sqlmesh/core/model/definition.py
  • tests/core/test_context.py
  • tests/core/test_model.py
Files skipped from review as they are similar to previous changes: 3
  • sqlmesh/core/model/definition.py
  • tests/core/test_context.py
  • tests/core/test_model.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

@coderabbitai coderabbitai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix comments: Fix CodeRabbit comments on this PR

Comment thread docs/reference/model_configuration.md Resolved
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Simen Strand <simen.strand@netcheck.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant