From e0af3efe75d85e68edfa138c2975ed4dcfda5549 Mon Sep 17 00:00:00 2001 From: Ben King <9087625+benfdking@users.noreply.github.com> Date: Thu, 24 Jul 2025 10:12:46 +0100 Subject: [PATCH 1/2] feat: linting rule no unregistered external models --- examples/sushi/config.py | 1 + sqlmesh/core/linter/rules/builtin.py | 31 +++++++++++++++-- tests/core/linter/test_builtin.py | 50 ++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 tests/core/linter/test_builtin.py diff --git a/examples/sushi/config.py b/examples/sushi/config.py index bbe9ec7988..2c124421dd 100644 --- a/examples/sushi/config.py +++ b/examples/sushi/config.py @@ -48,6 +48,7 @@ "noselectstar", "nomissingaudits", "nomissingowner", + "nomissingexternalmodels", ], ), ) diff --git a/sqlmesh/core/linter/rules/builtin.py b/sqlmesh/core/linter/rules/builtin.py index 8bf6e33720..e50b5c8822 100644 --- a/sqlmesh/core/linter/rules/builtin.py +++ b/sqlmesh/core/linter/rules/builtin.py @@ -10,11 +10,11 @@ from sqlmesh.core.linter.helpers import TokenPositionDetails, get_range_of_model_block from sqlmesh.core.linter.rule import Rule, RuleViolation, Range, Fix, TextEdit from sqlmesh.core.linter.definition import RuleSet -from sqlmesh.core.model import Model, SqlModel +from sqlmesh.core.model import Model, SqlModel, ExternalModel class NoSelectStar(Rule): - """Query should not contain SELECT * on its outer most projections, even if it can be expanded.""" + """Query should not contain SELECT * on its outermost projections, even if it can be expanded.""" def check_model(self, model: Model) -> t.Optional[RuleViolation]: # Only applies to SQL models, as other model types do not have a query. @@ -110,4 +110,31 @@ def check_model(self, model: Model) -> t.Optional[RuleViolation]: return self.violation() +class NoMissingExternalModels(Rule): + """All external models must be registered in the external_models.yaml file""" + + def check_model(self, model: Model) -> t.Optional[RuleViolation]: + # Ignore external models themselves, because either they are registered, + # and if they are not, they will be caught as referenced in another model. + if isinstance(model, ExternalModel): + return None + + # Handle other models that may refer to the external models. + not_registered_external_models: t.Set[str] = set() + for depends_on_model in model.depends_on: + existing_model = self.context.get_model(depends_on_model) + if existing_model is None: + not_registered_external_models.add(depends_on_model) + + if not not_registered_external_models: + return None + + return RuleViolation( + rule=self, + violation_msg=f"Model '{model.name}' depends on unregistered external models: " + f"{', '.join(m for m in not_registered_external_models)}. " + "Please register them in the external_models.yaml file.", + ) + + BUILTIN_RULES = RuleSet(subclasses(__name__, Rule, (Rule,))) diff --git a/tests/core/linter/test_builtin.py b/tests/core/linter/test_builtin.py new file mode 100644 index 0000000000..208b591a2d --- /dev/null +++ b/tests/core/linter/test_builtin.py @@ -0,0 +1,50 @@ +import os + +from sqlmesh import Context + + +def test_no_missing_external_models(tmp_path, copy_to_temp_path) -> None: + """ + Tests that the linter correctly identifies unregistered external model dependencies. + + This test removes the `external_models.yaml` file from the sushi example project, + enables the linter, and verifies that the linter raises a violation for a model + that depends on unregistered external models. + """ + sushi_paths = copy_to_temp_path("examples/sushi") + sushi_path = sushi_paths[0] + + # Remove the external_models.yaml file + os.remove(sushi_path / "external_models.yaml") + + # Override the config.py to turn on lint + with open(sushi_path / "config.py", "r") as f: + read_file = f.read() + + before = """ linter=LinterConfig( + enabled=False, + rules=[ + "ambiguousorinvalidcolumn", + "invalidselectstarexpansion", + "noselectstar", + "nomissingaudits", + "nomissingowner", + "nomissingexternalmodels", + ], + ),""" + after = """linter=LinterConfig(enabled=True, rules=["nomissingexternalmodels"]),""" + read_file = read_file.replace(before, after) + assert after in read_file + with open(sushi_path / "config.py", "w") as f: + f.writelines(read_file) + + # Load the context with the temporary sushi path + context = Context(paths=[sushi_path]) + + # Lint the models + lints = context.lint_models(raise_on_error=False) + assert len(lints) == 1 + assert ( + "Model 'sushi.customers' depends on unregistered external models: " + in lints[0].violation_msg + ) From c0eb91c5c6c806d89093c742475ef89cad23e78b Mon Sep 17 00:00:00 2001 From: Ben King <9087625+benfdking@users.noreply.github.com> Date: Thu, 24 Jul 2025 10:35:10 +0100 Subject: [PATCH 2/2] added better message --- sqlmesh/core/linter/rules/builtin.py | 4 ++-- vscode/extension/tests/quickfix.spec.ts | 16 ++++++++++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/sqlmesh/core/linter/rules/builtin.py b/sqlmesh/core/linter/rules/builtin.py index e50b5c8822..f16bb5d111 100644 --- a/sqlmesh/core/linter/rules/builtin.py +++ b/sqlmesh/core/linter/rules/builtin.py @@ -14,7 +14,7 @@ class NoSelectStar(Rule): - """Query should not contain SELECT * on its outermost projections, even if it can be expanded.""" + """Query should not contain SELECT * on its outer most projections, even if it can be expanded.""" def check_model(self, model: Model) -> t.Optional[RuleViolation]: # Only applies to SQL models, as other model types do not have a query. @@ -133,7 +133,7 @@ def check_model(self, model: Model) -> t.Optional[RuleViolation]: rule=self, violation_msg=f"Model '{model.name}' depends on unregistered external models: " f"{', '.join(m for m in not_registered_external_models)}. " - "Please register them in the external_models.yaml file.", + "Please register them in the external models file. This can be done by running 'sqlmesh create_external_models'.", ) diff --git a/vscode/extension/tests/quickfix.spec.ts b/vscode/extension/tests/quickfix.spec.ts index 7023257540..c31acaeeb3 100644 --- a/vscode/extension/tests/quickfix.spec.ts +++ b/vscode/extension/tests/quickfix.spec.ts @@ -17,7 +17,15 @@ test('noselectstar quickfix', async ({ page, sharedCodeServer }) => { const configPath = path.join(tempDir, 'config.py') const read = await fs.readFile(configPath, 'utf8') // Replace linter to be on + const target = 'enabled=True' const replaced = read.replace('enabled=False', 'enabled=True') + // Assert replaced correctly + expect(replaced).toContain(target) + + // Replace the rules to only have noselectstar + const targetRules = `rules=[ + "noselectstar", + ],` const replacedTheOtherRules = replaced.replace( `rules=[ "ambiguousorinvalidcolumn", @@ -25,12 +33,12 @@ test('noselectstar quickfix', async ({ page, sharedCodeServer }) => { "noselectstar", "nomissingaudits", "nomissingowner", + "nomissingexternalmodels", ],`, - `rules=[ - "noselectstar", - ], -`, + targetRules, ) + expect(replacedTheOtherRules).toContain(targetRules) + await fs.writeFile(configPath, replacedTheOtherRules) // Replace the file to cause the error const modelPath = path.join(tempDir, 'models', 'latest_order.sql')