Query linter design

[kirawi]

(Taken from the proposal)

Design

For lints that require knowledge of the database schema (such as for type errors), the Schema types defined in sea-schema would need to be pulled out into sea-query so it can be used in sea-linter. It is intended that MockDatabase contain a Schema for the respective DatabaseBackend that it was initialized with in MockDatabase::new(). This schema field is updated on each successful transaction.

(Only MockDatabase has this field as there is a high chance of user error with using real databases, such as using a database with an already defined schema.)

Linting will occur in DatabaseBackend::build() using a static instance of a Linter type that contains all lints. Each lint is defined as an Fn(Context) -> Result<(), LintDiagnostic> where Context is a type referencing the database's schema and the executing statement, and where LintDiagnostic is a type containing information about what triggered the lint and potentially how to fix it, like in clippy. The Linter will stop on the first LintDiagnostic and propogate it.

pub struct Context<'db, C> 
where
    C: ConnectionTrait
{
    database: &'db C
    schema: &'db Schema,
    statement: &'db Statement,
}

pub struct Linter<F> 
where
    F: Fn(Context) -> Result<(), LintDiagnostic>
{
    lints: Vec<F>
}

impl<F> Linter<F> 
where
    F: Fn(Context) -> Result<(), LintDiagnostic>
{
    pub fn new() -> Self { ... }

    pub fn register(&mut self, lint: F) {
        lints.push(lint);
    }

    // This function will construct a `Context` instance and sequentially run it against
    // each lint.
    pub fn execute(&self, schema: &Schema, statement: &Statement) -> Result<(), LintDiagnostic>

}

A lint is registered when it is passed to Linter::register(). However, it is possible to turn the lints field of Linter into a const array and thus avoiding the need to register lints to a static instance of Linter. If third-party lints are not intended, then the latter option would be the better choice.

Backend-specific lints can be supported either through feature-gating, or using an enum type that wraps each backend Schema. For general lints, SQLite's Schema could act as a common denominator to convert each backend's Schema into, but I'm not sure if clean conversions are possible.

Lints for types such as Paginator might need extra work to get working, as it might be impossible to deduce what the query is intended to do if it's just a SelectStatement. Therefore, SelectStatement might need to be modified to trace its use through its lifetime. This doesn't sound ideal, so there may be a better solution or it's a non-issue in practice.

[kirawi]

I believe that any linting should actually occur during execution of the query, not construction of a query from a ___Statement, but at the time I wrote the proposal I wasn't sure how to proceed with that. The plain Statement doesn't appear to contain the metadata that's necessary for the linter (short of parsing the raw SQL string, but that's not ideal). I think that a naïve solution would be to add a field to the Statement with the source ___Statement that is feature-gated.

[tyt2y3]

occur during execution of the query

I agree. I think we can hook into SeaORM's executors and perform checking before build. The question is how can we access the schema information there. It probably makes sense to embed it into the Connection and get_schema from there. We can actually discover the schema on startup (pool::new) and cache the schema at that point.

[billy1624]

the Schema types defined in sea-schema would need to be pulled out into sea-query so it can be used in sea-linter

Yes, you can write the schema info into a db agnostic TableCreateStatement, then parse the statement as your own Schema type. For example, https://github.com/SeaQL/sea-orm/blob/86e7e808b37179315a1cc5c6c852764830c04661/sea-orm-cli/src/commands.rs#L103-L154

[kirawi]

Are you suggesting using sea-schema as a dependency rather than pulling out the Schema types? I saw that there was this PR: SeaQL/sea-schema#59 which would allow for that to be possible.

[billy1624]

Sorry, I'm not sure why it's related to SeaQL/sea-schema#59. Could you please explain?

[kirawi]

I believe trying to depend on sea-schema from sea-orm would result in a cyclical dependency?

Edit: Ah, I see now that it's feature-gated.

[billy1624]

All SeaQL libraries are pointing in one direction. So, an application crate depends on both sea-schema and sea-orm won't result in cyclic dependency

[kirawi]

Instead of creating a separate sea-linter crate, I've decided to create a linter module under sea-orm. Additionally, I'm changing my initial task to having a way to fetch the Schema of a database using sea-schema through the connection trait. This is because I felt that it was a low-friction task that would lead to more initial progress than the original first task. The mock database will be unsupported for now.

[tyt2y3]

Um... I guess the problem is we do not want to couple too tightly to sea-orm, it just cannot be managed that way (unless you also maintain sea-orm)
So I would suggest to open some kind of plugin API via a trait in sea-orm and allow the linter to live in another crate.