Expanding lint rules to parsed migrations

[mfridman]

The lint rule functionality recently added is awesome!

Would you consider expanding this beyond query and config to support something like migration?

Since sqlc already parses various migration files one could also use the sqlc lint rules to vet migrations.

rules:
  - name: no-drop-table
    message: "never drop tables"
    rule: |
      migration.contains("DROP TABLE")

[andrewmbenton]

We could expand the scope of the query variable to include all the statements in the schema and query config paths. Then your rule would just be

rules:
  - name: no-drop-table
    message: "never drop tables"
    rule: |
      query.contains("DROP TABLE")

Would you expect to have a way to easily distinguish whether a rule will apply to a "query" (all sqlc vet does today) vs a "schema" change though?

[mfridman]

I think I'd like to distinguish between schema. vs query. changes since not all bits of the migration may translate into generated code.

I could see the argument where folks might want to apply a rule to both, and it'd be "annoying" to have to define duplicate rules for "query" vs "schema". But this might be an edge case.

For some context, I was recently evaluating migration-specific linters like:

• https://github.com/sbdchd/squawk
• atlas (https://atlasgo.io/versioned/lint)
• heck, I even considered adding this as a goose lint command to pressly/goose

But given sqlc already knows about the schema and has this functionality it seems like a huge win to have these lint rules applied to both the queries and schema changes.

[andrewmbenton]

That makes sense. I think it's a good idea and probably not that hard to add. I haven't looked at the way sqlc handles schema migration parsing though.

Even if we provide access via a single variable like query or statement I think we can provide an easy way to distinguish between queries and schema changes. One thing @kyleconroy suggested was adding a simple filter expression to a rule definition. Something like this:

rules:
- name: no-drop-table
  message: "never drop tables"
  rule: |
      statement.contains("DROP TABLE")
  if: !statement.is_query

In any case I'll try to find a little time to implement a first version this morning to see what roadblocks I run into.

[andrewmbenton]

Coming back to this, there are a few questions due to the way sqlc currently handles DDL statements. We parse them all and then "apply" them to an internal database catalog along with natively-defined objects. Filtering out the user-defined objects isn't hard, but we don't keep track of the original DDL statement strings along the way.

If the most important thing is to just run some simple text matching against DDL statement strings, I think that's pretty easy. sqlc can keep track of the original strings after parsing and hand those over to the CEL environment. I don't know how useful that is though, so I'm hoping to have some folks weigh in.

We could also try to keep track of the AST from the parse step and pass that into the vet CEL context, but we'd need to massage the AST into a proto somehow.