[Feature request] Add chunk API to QueryBuilder

[PoignardAzur]

I was recently looking at a Rust app building some SQL queries, and trying to rewrite it to avoid injection vulnerabilities.

The unsafe version has code like this:

// Builds the WHERE clause
fn where_sql(request: &RequestData) -> String {
    let mut where_clauses = vec![];

    for key in request.group_keys.iter() {
        where_clauses.push(format!("{} = '{}'", key, some_value));
    }

    format!("WHERE {}", where_clauses.join(" AND "))
}

The safe version becomes:

// Builds the WHERE clause
fn where_sql(builder: &mut QueryBuilder<Db>, request: &RequestData) {
    query.push("WHERE ");

    let query_sep = query.separated(" AND ");
    for key in request.group_keys.iter() {
        query_sep.push_bind(key);
        query_sep.push_unseparated(" = '");
        query_sep.push_bind_unseparated(some_value);
        query_sep.push_unseparated("'");
    }
}

I have several problems with the safe version:

• It's much more verbose.
• The general structure of the query is lost. Instead of reading {} = '{}', your brain has to stitch that string together from multiple lines of code.
• There's a very sneaky footgun in the loop: my first "push" call has to include a separator, and every call after that in the block has to be push_unseparated. I missed that footgun the first time I wrote that function.

My proposed solution:

• Add a push_fragment() method to both QueryBuilder and Separated that takes a QueryBuilder.
• Create a query_fragment macro which returns a QueryBuilder.

With those two features, the code above becomes:

// Builds the WHERE clause
fn where_sql(builder: &mut QueryBuilder<Db>, request: &RequestData) {
    query.push("WHERE ");

    let query_sep = query.separated(" AND ");
    for key in request.group_keys.iter() {
        query_sep.push_fragment(query_fragment!("{} = '{}'", key, some_value));
    }
}

I believe that version is much more readable and less footgun-y.

---

Is this something that would fit sqlx's API? I would be willing to write the PR if there's interest.

[abonander]

This seems like something that can incubate as its own crate. I think it's gonna take some significant iteration to land on an API that's flexible enough for most uses without being too arcane.

[PoignardAzur]

I've given it a quick try and it's not trivial:

• First, you can't add methods to sqlx::QueryBuilder from an external crate. You can simulate it with an external trait or use free functions, so it's not too bad.
• A QueryBuilder's string and argument list are private. This adds complexity, especially for the arguments: there's just no way to get the argument list from a QueryBuilder. (I could build it and use take_arguments, but the method is private.)

Basically an external crate would need to create its own QueryBuilder type from scratch.

By itself that's not infeasible, QueryBuilder isn't used from sqlx anyway, but that's an awkward approach.

[PoignardAzur]

Gave it a try, and even copying the QueryBuilder code has problems, because Query and QueryAs have no public constructors.

This means you need to build a temporary QueryBuilder to create a Query; you can't do it in MyQueryBuilder::build() without having lifetime problems.

[PoignardAzur]

After spending a few hours on this, my takeaway is that it's feasible, but not without maintainer support for changes to sqlx internals.

The biggest break from sqlx's current design philosophy is that, while sqlx encourages a "push everything into one buffer" approach, the API I'm recommending in this issue is closer to "create intermediary buffers, push to them, then push the intermediary buffer to the main buffer". It's the difference between using write! and join().

The second approach is more expressive, but leads to more memcpys and more intermediary allocations. This is something sqlx tries really hard to avoid right now (as can be seen in the implementations of the Arguments trait), but I don't think it really matters.

Building a query is almost always going to be infinitely faster than executing the query. I don't think skipping a few mallocs is worth the extra complexity of the current design.

In any case, I'm available if someone from the project wants to discuss this further. Otherwise, without maintainer interest, I think this feature is dead in the water.