Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions examples/sushi/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
"noselectstar",
"nomissingaudits",
"nomissingowner",
"nomissingexternalmodels",
],
),
)
Expand Down
29 changes: 28 additions & 1 deletion sqlmesh/core/linter/rules/builtin.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
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):
Expand Down Expand Up @@ -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 file. This can be done by running 'sqlmesh create_external_models'.",
)


BUILTIN_RULES = RuleSet(subclasses(__name__, Rule, (Rule,)))
50 changes: 50 additions & 0 deletions tests/core/linter/test_builtin.py
Original file line number Diff line number Diff line change
@@ -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")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we even need the sushi project here? I think we could get the same violation but simply creating an empty project with a model such as SELECT * FROM external_model.

This way we don't have to:

  • Delete the external_models.yaml file, it doesn't exist in the first place
  • We don't have to copy the whole sushi dir
  • We don't have to string replace the config, we can create one from scratch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am curious to understand the objection to this test.

I don't particularly understand the rationale of the things you mention we avoid. While they are slightly inconvenient, the test is 50 lines, but because it's testing a fully fledged project, it gives me a lot of confidence. The only argument I can get is slow tests, but if we're optimising for that, we should be using other tools.

Another way, I kind of read this is maybe about a philosophical difference in testing: The test you describe is the most isolated, unit-y test you can build, it's as minimal a test as possible to prove the thing works whereas mine feels like I start with the integration tests and test the function way more thoroughly because I exercise through a lot more scenarios. A minimal unit test wouldn't reveal, for example, any false positive, but running through sushi, which exercises a lot of variations, just might.

Someone once described this testing to me as "happy path" testing. You build a robust happy e2e test (e.g. our sushi project) and then you exercise paths around it by altering the inputs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not an outright objection but it also doesn't feel like the right tool for this. My reasoning being:

  • There's a lot more noise compared to a test that'd simply use a dummy model and a Config object. This makes understanding & updating the test harder and some parts of it may be considered "hardcoded" such as the YAML overwrite
  • There's obviously some performance overhead, and although we are not optimising for that per se, it shouldn't be neglected
  • Imo you don't "win" anything in terms of integration testing by using sushi instead of a simpler project for this case

It makes sense to use the pre-existing projects such as sushi or multi when it comes to testing the entirety of the linter end to end, but otherwise this feels like it uses more real estate than is required for such a test case

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
)
16 changes: 12 additions & 4 deletions vscode/extension/tests/quickfix.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,28 @@ 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",
"invalidselectstarexpansion",
"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')
Expand Down