Initial support for SQLite3 #152

sgrif · 2016-01-31T00:36:02Z

This PR adds all of the guts for SQLite3 support to Diesel. There are several outstanding issues which will need to be resolved before this is ready to be merged into master. This PR is targeting a new branch, diesel-sqlite-support, and is intended to be merged into that branch without addressing these issues. That branch will not be merged into master until these issues have been resolved. The git history of this PR will be cleaned up before merging.

Ultimately the most difficult remaining problem which we need to address is the lack of a DEFAULT keyword. I will detail why this is a problem and what I think we can do to solve it in a separate issue.

SQLite has a fundamentally different mechanism for handling bind parameters than pretty much every other backend out there, where the function you call differs based on the type, rather than sending an array of bytes and telling the backend how to interpret it. FromSql had to be updated to address this (see 8a33029 for details on that). I have not applied the same change to ToSql, as it was possible to shoehorn SQLite into our existing structure. While this may end up changing in the future, I do not believe it is necessary for 0.5.

I'm not super happy about the structure of StatementIterator, SqliteRow, and SqliteValue, as ultimately the Row is a giant lie and Statement is the the value that maintains all of the state. Ultimately this comes out of the weird interaction between the fact that Row#take returns a reference, but sqlite's C api doesn't actually return a single value that works in that sense (and really doesn't have the concept of a row at all). As with much of our code overall, this can probably be cleaned up in the future by having the Backend::RawValue directly be a reference for Pg. I need to figure out a way to accomplish this without adding a lifetime to the Pg struct.

I've broken our test suite into postgres specific and general where possible, but this is a very incomplete process. There are several modules which are entirely scoped to PG that do not need to be, which can be addressed independently in later PRs. Additionally, the entire expression module needs to be for expressions which are specific to PG. I do not believe that SQLite has anything specific to it which is not also supported by PG.

Fixes #39 (mostly. Enough to close the issue at least)

sgrif · 2016-01-31T00:37:40Z

Review? @mfpiccolo @mcasper @samphippen

fables-tales · 2016-01-31T00:41:42Z

diesel/src/connection/sqlite/mod.rs

+
+    fn begin_transaction(&self) -> QueryResult<()>{
+        let transaction_depth = self.transaction_depth.get();
+        self.change_transaction_depth(1, if transaction_depth == 0 {


I'd like to see the conditional here pulled out into a function, expression, local or something. It feels like using it as an inline expression here is trying to be "too clever". Thoughts?

fables-tales · 2016-01-31T00:41:57Z

diesel/src/connection/sqlite/mod.rs

+
+    fn rollback_transaction(&self) -> QueryResult<()> {
+        let transaction_depth = self.transaction_depth.get();
+        self.change_transaction_depth(-1, if transaction_depth == 1 {


same as above

fables-tales · 2016-01-31T00:43:55Z

diesel/src/connection/sqlite/mod.rs

+}
+
+fn error_message(err_code: libc::c_int) -> &'static str {
+    let message_ptr = unsafe { ffi::sqlite3_errstr(err_code) };


is it advisable/good to combine the unsafe blocks here?

fables-tales · 2016-01-31T00:46:27Z

diesel/src/connection/sqlite/raw.rs

+        unsafe {
+            ffi::sqlite3_exec(
+                self.internal_connection,
+                query.as_ptr(),


These arguments to sqlite3_exec are pretty arcane. Is it easy to introduce either named variables or put comments here to explain what they do?

fables-tales · 2016-01-31T00:48:10Z

diesel/src/connection/sqlite/raw.rs

+                let bytes = CStr::from_ptr(err_msg).to_bytes();
+                str::from_utf8_unchecked(bytes).into()
+            };
+            unsafe { ffi::sqlite3_free(err_msg as *mut libc::c_void) };


A comment or named function here would help me understand what's going on. As it is, I had to pause to read this a couple of times.

fables-tales · 2016-01-31T00:49:01Z

diesel/src/connection/sqlite/sqlite_value.rs

+
+pub struct SqliteValue {
+    inner_statement: *mut ffi::sqlite3_stmt,
+    col_index: libc::c_int,


is this not being unsigned a sqlite implementation detail?

Yes. It ended up being entirely internal, so it made sense to just have this be a c_int on the struct rather than a usize, and skip the coercions later.

fables-tales · 2016-01-31T00:55:38Z

diesel/src/query_builder/sqlite.rs

+
+    fn push_identifier(&mut self, identifier: &str) -> BuildQueryResult {
+        self.push_sql("`");
+        self.push_sql(&identifier.replace("`", "``"));


The knowledge that "`" -> "``" escapes a sqlite identifier is really specific. Is it worth adding a comment or named function here?

I'm not sure this is required. This function is already SqliteQueryBuilder#push_identifier. I'm not sure what other information to convey other than that we're escaping identifiers for SQLite, which the method/class name already give.

mfpiccolo · 2016-01-31T01:13:07Z

diesel/src/connection/sqlite/mod.rs

@@ -0,0 +1,138 @@
+extern crate libsqlite3_sys as ffi;


Why refer to libsqlite3_sys as ffi? This is a bit confusing considering std::ffi.

Glob importing the crate causes namespace conflicts, and it felt like the most appropriate name to use as a namespace.

mfpiccolo · 2016-01-31T01:53:00Z

diesel_tests/tests/schema.rs

+pub fn connection_without_transaction() -> TestConnection {
+    let connection = ::diesel::connection::SqliteConnection::establish(":memory:").unwrap();
+    let migrations_dir = migrations::find_migrations_directory().unwrap().join("sqlite");
+    migrations::run_pending_migrations_in_directory(&connection, &migrations_dir).unwrap();


Can we silence the migration outputs during the tests? Looks like there is about 3200 or so.

The migration output should be swallowed, as is all stdout output during tests. I have no clue why it isn't. #156

This commit adds all of the guts for SQLite3 support to Diesel. There are several outstanding issues which will need to be resolved before this is ready to be merged into master. This commit will not be merged into master until these issues have been resolved. Ultimately the most difficult remaining problem which we need to address is the lack of a `DEFAULT` keyword. I will detail why this is a problem and what I think we can do to solve it in a separate issue. SQLite has a fundamentally different mechanism for handling bind parameters than pretty much every other backend out there, where the function you call differs based on the type, rather than sending an array of bytes and telling the backend how to interpret it. `FromSql` had to be updated to address this (see 8a33029 for details on that). I have not applied the same change to `ToSql`, as it was possible to shoehorn SQLite into our existing structure. While this may end up changing in the future, I do not believe it is necessary for 0.5. I'm not super happy about the structure of `StatementIterator`, `SqliteRow`, and `SqliteValue`, as ultimately the `Row` is a giant lie and `Statement` is the the value that maintains all of the state. Ultimately this comes out of the weird interaction between the fact that `Row#take` returns a reference, but sqlite's C api doesn't actually return a single value that works in that sense (and really doesn't have the concept of a row at all). As with much of our code overall, this can probably be cleaned up in the future by having the `Backend::RawValue` directly be a reference for `Pg`. I need to figure out a way to accomplish this without adding a lifetime to the `Pg` struct. I've broken our test suite into postgres specific and general where possible, but this is a *very* incomplete process. There are several modules which are entirely scoped to PG that do not need to be, which can be addressed independently in later PRs. Additionally, the entire `expression` module needs to be for expressions which are specific to PG. I do not believe that SQLite has anything specific to it which is not also supported by PG. Fixes #39 (mostly. Enough to close the issue at least)

All code which *directly* depends on these has been moved to a separate module and placed behind a cfg attr. There is still a ton of PG specific stuff hidden in the expressions module that simply isn't marked as such. I had to rework how we do our boilerplate type impls, as `sqlite` and `postgres` need to be able to co-exist in the same compile pass (even just for generating docs). While this PR is pretty large, it ultimately mostly comes down to clerical work. Fixes #152.