Unclear reason for why is Executor not implemented for String

[luke-biel]

I've came upon a comment above Execute implementation for &str:

// NOTE: `Execute` is explicitly not implemented for String and &String to make it slightly more
//       involved to write `conn.execute(format!("SELECT {}", val))`

I'm glad this is noted somewhere, as I would scratch my head on where's String implementation of Execute, but... it says nothing about the reasoning. Ok - we have writer slightly more involved when they have to put &format!(.... We have them way more involved if futures are at stake, I can guarantee you that. But the information I'm lacking is why did author of the code decide we shouldn't have String implementation?
Is this to improve user's code performance?
Is this some security reason?
I have to write my own String wrapper in order to be able to use execute and I'm not sure if I'm not doing something potentially dangerous. I'd be glad if somebody could explain this to me.

Also, I would reword this comment a bit to include said explanation.

PS: I've looked through other issues/discussions, found nothing on this topic, but that may be me not being able to specify search criteria

[abonander]

Is this some security reason?

This. If String was possible to use directly, it would be extremely tempting to just format user input directly into the query instead of using bind parameters, which introduces SQL injection vulnerabilities. For example, to co-opt XKCD #327:

async fn insert_student(conn: &mut PgConnection, student_name: &str) -> sqlx::Result<StudentId> {
    sqlx::query_scalar(format!("INSERT INTO students(name) VALUES('{}') RETURNING student_id", student_name))
        .fetch_one(conn)
        .await
}

// imagine this is user input, and wasn't properly sanitized
let student_name = "Robert'); DROP TABLE students; --";

let student_id = insert_student(&mut conn, &student_name).await?;

Then suddenly, you've lost all student data for the year because the query that got executed looked like this:

INSERT INTO students(name) VALUES('Robert'); DROP TABLE students; -- ') RETURNING student_id

(Postgres would reject the attempt to prepare a query string with more than one statement, but MySQL and SQLite wouldn't.)

The idea is that by requiring dereffing to a string slice, it will at least make the code start to smell a bit and hopefully lead to the user re-evaluating their implementation.

Bind parameters don't have this vulnerability because the database knows not to interpret anything in a bind parameter as SQL; by comparison, this implementation is completely injection-safe, and is the approach that the API design was intended to push you towards:

async fn insert_student(conn: &mut PgConnection, student_name: &str) -> sqlx::Result<StudentId> {
    sqlx::query_scalar("INSERT INTO students(name) VALUES(?) RETURNING student_id")
        .bind(student_name)
        .fetch_one(conn)
        .await
}

I've recently spent some more time thinking about this API, because yeah the intent here is not clear at all, and always requiring a string slice makes it annoying to construct query builders. I was thinking of adopting a similar strategy to std's UnwindSafe trait: https://doc.rust-lang.org/stable/std/panic/trait.UnwindSafe.html

But I'll talk about that more in a proposal issue which I've been putting off writing.

[abonander]

#1594