Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add support for
INSERT INTO table (...) SELECT ...
This feature has been in the works for a very long time, and has a lot of context... I've added headers so if you already know about the iteration of the API and the evolution of `InsertStatement` internally, skip to the third section. Getting to this API === I'd like to give a bit of context on the APIs that have been considered, and how I landed on this one. To preface, all of the iterations around this have been trying to carefully balance three things: - Easy to discover in the API - Being syntactically close to the generated SQL - Avoiding rightward drift For most of Diesel's life, our API was `insert(values).into(table)`. That API was originally introduced in 0.2 "to mirror `update` and `delete` (it didn't mirror them. It was backwards. It's always been backwards). My main concern with the old API actually was related to this feature. I couldn't come up with a decent API that had you specify the column list (you basically always need to specify the column list for this feature). So in 0.99 we changed it to what we have now, and I had toyed around with `insert_into(table).columns(columns).values(select)`, as well as `insert_into(table).from_select(columns, select)`. I was leaning towards the second one for a while (I didn't realize at the time that it was exactly SQLAlchemy's API). I hated the `columns` method because it was unclear what it was doing until you saw you were inserting from a select statement. It also implied an interaction with tuples that didn't exist. However, another thing that happened in 0.99 was that we deprecated our old upsert API. The `from_select` form reminded me far too much of the old `on_conflict` API, which we had just removed. In practice what that API would give you was something like this: ```rust insert_into(posts::table) .from_select( (posts::user_id, posts::title), users::table .select(( users::id, users::name.concat("'s First Post"), )), ) ``` That's just far too much rightward drift for me. Yes, you can assign the args to local variables, but they're awkward to name and now your code reads weird. I thought moving the columns list to another method call would help, but it doesn't. ```rust insert_into(posts::table) .from_select( users::table .select(( users::id, users::name.concat("'s First Post"), )), ) .into_columns((posts::user_id, posts::title)) ``` Eventually a member of the Diesel team had the idea of making this an `insert_into` method on select statements. This solves all of my concerns around rightward drift, though at the cost of the other two priorities. The API now looked like this: ``` users::table .select(( users::id, users::name.concat("'s First Post"), )) .insert_into(posts::table) .into_columns((posts::user_id, posts::title)) ``` I liked the way the code flowed, but I had concerns around discoverability, and didn't like how backwards it was from SQL. But I could live with it and started implementing. Up until this point I had been assuming that we would have an `InsertFromSelectStatement` struct, which was distinct from `InsertStatement` and necessitated the differently named methods. I realized when I started digging into it though, that we really just want to re-use `InsertStatement` for this. It seems obvious in hindsight. And if we were going to use that structure, that meant that it wouldn't be much harder to just make passing `SelectStatement` to `values` work. This automatically solves most of my concerns around discoverability, since it now just works exactly like every other form of insert. That said, I really don't like the rightward drift. I liked the `.insert_into` form for being able to avoid that. But for the final implementation, I just generalized that feature. *Anything* that can be written as `insert_into(table).values(values)` can now be written as `values.insert_into(table)`. Context around InsertStatement === This file has churned more than just about any other part of Diesel. I feel like I've re-written it nearly every release at this point. I think the reason it's churned so much is for two reasons. The first is that it's kept a fundamental design flaw through 1.1 (which I'll get to), and we've been constantly working around it. The second is that `INSERT` is actually one of the most complex queries in SQL. It has less variations than `SELECT`, but far more of its variations are backend specific, or have different syntaxes between backends. `InsertStatement` was originally added for 0.2 in c9894b3 which has a very helpful commit message "WIP" (good job past Sean). At the time we only supported PG, so we actually had two versions -- `InsertStatement` and `InsertQuery`, the latter having a returning clause. I'm honestly not sure why I didn't do the `ReturningClause<T>` `NoReturningClause` dance we do now and did back then in `SelectStatement`. Maybe I thought it'd be easier? Anyway this file had to go through some pretty major changes in 0.5 when we added SQLite support. We needed to disallow batch insert and returning clauses on that backend. Additionally, the way we handle default values had to work differently on SQLite since it doesn't support the `DEFAULT` keyword. At this point though, it still a few things. The query starts with `INSERT INTO`, had a columns list, then the word `VALUES` and then some values. It also managed all parenthesis. (Yes it was trivial to make it generate invalid SQL at that point). Fast forward to 0.9, I decided to support batch insert on SQLite. This needs to do one query per row, and I felt that meant we needed a separate struct. I didn't want to have `BatchInsertStatement` and `BatchInsertQuery` and `InsertStatement` and `InsertQuery`, so we got the `NoReturningClause` struct, and got a little closer to how literally every other part of Diesel works. In 0.14 we added `insert_default_values` which left us with `InsertStatement`, `BatchInsertStatement`, and `DefaultInsertStatement`, which eventually went back down to the two. The last time the file went through a big rewrite was in 0.99 when I finally unified it down to the one struct we have today. However, it still had the fatal flaw I mentioned earlier. It was trying to handle too much logic, and was too disjoint from the rest of the query builder. It assumed that the two forms were `DEFAULT VALUES` or `(columns) VALUES ...`, and also handled parens. We've gone through a lot of refactoring to get rid of that. I finally think this struct is at a point where it will stop churning, mostly because it looks like the rest of Diesel now. It doesn't do anything at all, and the only thing it assumes is that the word `INTO` appears in the query (which I'm pretty sure actually is true). The actual implementation of this commit ==== With all that said, this commit is relatively simple. The main addition is the `InsertFromSelect` struct. It's definitely a Diesel struct, in that it basically does nothing, and all the logic is in its `where` clauses. I tried to make `Insertable` work roughly everywhere that methods like `filter` work. I couldn't actually do a blanket impl for tables in Rust itself, because it made rustc vomit on recursion with our impls on `Option`. That shouldn't be happening, but I suspect it's a lot of work to fix that in the language. I've also implemented it for references, since most of the time that you pass values to `.values`, you pass a reference. That should "just work" here unless we have a good reason not to. The majority of the additions here are tests. This is a feature that fundamentally interacts with many of our most complex features, and I've tried to be exhaustive. Theres ~3 lines of tests for every line of code added. I've done at least a minimal test of this feature's interaction with every other feature on inserts that I could think of. I am not so much worried about whether they work, but I was worried about if there was a syntactic edge case I didn't know about. There weren't. Same with the compile tests, I've tried to think of every way the feature could be accidentally misused (bad arguments to `into_columns` basically), as well as all the nonsense things we should make sure don't work (putting it in a tuple or vec). I didn't add any compile tests on making sure that the select statement itself is valid. The fact that the `Query` bound only matches valid complete select statements is already very well tested, and we don't need to re-test that here. I also did not explicitly disallow selecting from the same table as the insert, as this appears to be one of the few places where the table can appear twice with no ambiguity. Fixes #1106
- Loading branch information