Read-only connection

[AlisCode]

Motivation

It can be useful when implementing a business application to differentiate between actions that are read-only and actions that are read+write. This gives us more type-safety when working with databases, which is IMO what Diesel is all about. It transforms a fair amount of potential security bugs and smelly design into compile-time errors.

This suggestion aims to experiment with a design that makes it a compile-error when a read-only connection tries to launch an insert / update / delete SQL statement.

Basic implementation here.

Example

For example with the suggested design, this compiles:

use crate::schema::users;
use diesel::prelude::*;

#[macro_use]
extern crate diesel;

mod schema; // omitted in this file for readability

fn main() {
    let conn = SqliteConnection::establish("test.db").expect("Failed to connect");
    create_user(&mut conn, "MyName");

    let mut ro_conn = conn.read_only();
    let user = read_first_user(&mut ro_conn);
    println!("{user:?}");
}

/// Compiles fine, an &mut SqliteConnection should be able to insert
fn create_user(conn: &mut SqliteConnection, name: &str) {
    diesel::insert_into(users::table)
        .values(User {
            id: 1,
            name: name.to_string(),
        })
        .execute(conn)
        .expect("Failed to insert");
}

/// Compiles fine, a read-only connection should be able to read from database
fn read_first_user(conn: &mut ReadOnly<SqliteConnection>) -> User {
    users::table
        .filter(users::id.eq(1))
        .first_ro(conn)
        .expect("Failed to retrieve users")
}

/// Does not compile : tried to Insert a new entry in the database, but the connection is read-only 
fn try_insert_with_read_only(conn: &mut ReadOnly<SqliteConnection>, name: &str) {
    diesel::insert_into(users::table)
        .values(User {
            id: 2,
            name: name.to_string(),
        })
        .execute(conn)
        .expect("Failed to insert");
}

#[derive(Debug, Queryable, Insertable)]
#[diesel(table_name = users)]
struct User {
    id: i32,
    name: String,
}

Design

My very first design is based off the idea to have an analog to the RunQueryDsl trait but for read-only calls, that prevents one to run a statement that is not read-only in case we're given such a connection. In no way do I claim this design is perfect, but it's worth because it allows me to demonstrate what I mean. I first tried implementing it as a separate crate, but ultimately decided to draft this as a fork of Diesel for easier experimentation. After looking at how diesel-async integrates with the rest of the query builder, I actually do believe it's feasible.

To my knowledge, there is no such thing as "read-only queries" in SQL. The way I would typically prevent an SQL connection from inserting something (at runtime) is by connecting with a user that has been granted certain permissions, but of course it's not Diesel's responsibility to know about any of this. This means in my opinion that this feature can only realistically be implemented as an extension to query_dsl and possibly query_builder. So here's what I have on my branch :

• ReadOnly<C> is a wrapper for a connection that makes it read-only (so, it blocks certain statements from compiling)
• There is an extension trait ReadOnlyExt but to be fair it can be replaced with a fn read_only(self) -> ReadOnly<Self> inside the Connection trait. This is an artifact of me trying to implement this as a separate crate.
• There is a marker trait ReadOnlyStatement that is implemented for every statement that can be executed on a read-only connection
• There is a trait RunReadOnlyQueryDsl, which is basically a copy of RunQueryDsl but with functions suffixed with _ro to prevent function names clashes.

Pros

• It's composable so it's kinda nice, you can reuse it on top of every Connection implementor
• It makes my example compile so we have a basis to discuss the advantages of that extension

Cons

• This does not prevent an unsafe handwritten SQL query from being executed at compile time. Though to be fair, nothing will. To me this is acceptable because handwritten SQL queries means the user is opting-out of type-safety, somewhat like a call to unsafe
• My current design forces one to suffix their calls with _ro (see example, where I use first_ro instead of ro). This is a bigger usability concern that I would like to know how to address.
• The error message is not exactly clear : it just says ReadOnly<C> does not implement Connection. This is to me a big redflag, as a ReadOnly<Conn> should probably still be a Connection. See alternative ideas for why it isn't the case in my draft.

Alternative ideas

• When I first implemented this, I tried to implement ReadOnly as not only a wrapper for a Connection, but also a wrapper for a Backend implementor (think impl Connection for ReadOnly<C: Conn> { type Backend = ReadOnly<C::Backend>; [...] }). Turns out the internals of Diesel makes this impractical because of AstPass and AstPassInternals being generic over DB: Backend. I could never get it to compile. Maybe someone has relevant comments ?
• I'm waiting for anyone's alternative design :)

[weiznich]

Thanks for writing this down 👍 I will try to have a look at it.
#1684 Might be relevant here as well.

[weiznich]

So I had some more time to think about this. Generally speaking I see the use case for this and I agree that we can provide something to solve this issue at least partially. It's up to discussion of everything of this solution should be part of diesel itself, or if some parts should better live in a distinct crate.

Now let me come to the details. It's definitively good to have some code around here, so that we could talk already about specific things.

• ReadOnly is a wrapper for a connection that makes it read-only (so, it blocks certain statements from compiling)

That's a good choice in my opinion.

• There is an extension trait ReadOnlyExt but to be fair it can be replaced with a fn read_only(self) -> ReadOnly inside the Connection trait. This is an artifact of me trying to implement this as a separate crate.

• There are two other alternatives here:
  • Provide an explicit constructor for ReadOnly<_> that accepts an inner connection type
  • Just implement From/TryFrom for ReadOnly<_> and existing connection types

(I would likely choose the explicit constructor method)

• There is a marker trait ReadOnlyStatement that is implemented for every statement that can be executed on a read-only connection

That's reasonable and that's something that should be provided by diesel itself. The question is if we provide a ReadOnlyStatement marker trait or if we would rather like to use a bit more complex, but more flexible solution here. Something like:

trait StatementType {
    type Type: StatementTypeChoice;
}

struct Read; 
impl StatementTypeChoice for Read{ };

struct Write;
impl StatementTypeChoice for Write {};

struct Mixed;
impl StatementTypeChoice for Mixed{}

That would allow a more fine grained classification of existing types. For example marking SqlQuery as Mixed instead of Readable and it would allow to use this infrastructure to provide a WriteOnly connection as well.

• There is a trait RunReadOnlyQueryDsl, which is basically a copy of RunQueryDsl but with functions suffixed with _ro to prevent function names clashes.

That's the point I do dislike most with the current proposed implementation. I would like to see if there is an alternative way here without duplicating. Not sure about possible options here, although…

When I first implemented this, I tried to implement ReadOnly as not only a wrapper for a Connection, but also a wrapper for a Backend implementor (think impl Connection for ReadOnly<C: Conn> { type Backend = ReadOnly<C::Backend>; [...] }). Turns out the internals of Diesel makes this impractical because of AstPass and AstPassInternals being generic over DB: Backend. I could never get it to compile. Maybe someone has relevant comments ?

I would be interested about knowing something more about why it did not work at all. Maybe there are some tweaks to the AstPass system that could be easily applied so that this would be possible and we don't need the RunReadOnlyQueryDsl duplication at all?