fix(sqlite): correctly infer nullability for INTEGER PRIMARY KEY in RETURNING

sqlx-sqlite/src/connection/describe.rs

@@ -7,7 +7,6 @@ use crate::type_info::DataType;
 use crate::{Sqlite, SqliteColumn};
 use sqlx_core::sql_str::SqlStr;
 use sqlx_core::Either;
-use std::convert::identity;
 
 pub(crate) fn describe(
     conn: &mut ConnectionState,
@@ -78,9 +77,8 @@ pub(crate) fn describe(
                 ty
             };
 
-            // check explain
             let col_nullable = stmt.handle.column_nullable(col)?;
-            let exp_nullable = fallback_nullable.get(col).copied().and_then(identity);
+            let exp_nullable = fallback_nullable.get(col).copied().flatten();
 
             nullable.push(exp_nullable.or(col_nullable));
 

sqlx-sqlite/src/connection/explain.rs

@@ -373,17 +373,17 @@ fn opcode_to_type(op: &str) -> DataType {
 fn root_block_columns(
     conn: &mut ConnectionState,
 ) -> Result<HashMap<(i64, i64), IntMap<ColumnType>>, Error> {
-    let table_block_columns: Vec<(i64, i64, i64, String, bool)> = execute::iter(
+    let table_block_columns: Vec<(i64, i64, i64, String, bool, i64)> = execute::iter(
         conn,
-        "SELECT s.dbnum, s.rootpage, col.cid as colnum, col.type, col.\"notnull\"
+        "SELECT s.dbnum, s.rootpage, col.cid as colnum, col.type, col.\"notnull\", col.pk
          FROM (
              select 1 dbnum, tss.* from temp.sqlite_schema tss
              UNION ALL select 0 dbnum, mss.* from main.sqlite_schema mss
              ) s
          JOIN pragma_table_info(s.name) AS col
          WHERE s.type = 'table'
          UNION ALL
-         SELECT s.dbnum, s.rootpage, idx.seqno as colnum, col.type, col.\"notnull\"
+         SELECT s.dbnum, s.rootpage, idx.seqno as colnum, col.type, col.\"notnull\", col.pk
          FROM (
              select 1 dbnum, tss.* from temp.sqlite_schema tss
              UNION ALL select 0 dbnum, mss.* from main.sqlite_schema mss
@@ -400,13 +400,13 @@ fn root_block_columns(
     .collect::<Result<Vec<_>, Error>>()?;
 
     let mut row_info: HashMap<(i64, i64), IntMap<ColumnType>> = HashMap::new();
-    for (dbnum, block, colnum, datatype, notnull) in table_block_columns {
+    for (dbnum, block, colnum, datatype, notnull, pk) in table_block_columns {
         let row_info = row_info.entry((dbnum, block)).or_default();
         row_info.insert(
             colnum,
             ColumnType::Single {
                 datatype: datatype.parse().unwrap_or(DataType::Null),
-                nullable: Some(!notnull),
+                nullable: Some(!(notnull || (pk > 0 && datatype.to_lowercase() == "integer"))),
             },
         );
     }
@@ -1640,7 +1640,7 @@ fn test_root_block_columns_has_types() {
         assert_eq!(
             Some(&ColumnType::Single {
                 datatype: DataType::Integer,
-                nullable: Some(true) //sqlite primary key columns are nullable unless declared not null
+                nullable: Some(false)
             }),
             root_block_cols[&table_db_block].get(&0)
         );
@@ -1665,7 +1665,7 @@ fn test_root_block_columns_has_types() {
         assert_eq!(
             Some(&ColumnType::Single {
                 datatype: DataType::Integer,
-                nullable: Some(true) //sqlite primary key columns are nullable unless declared not null
+                nullable: Some(false)
             }),
             root_block_cols[&table_db_block].get(&0)
         );
@@ -1683,7 +1683,7 @@ fn test_root_block_columns_has_types() {
         assert_eq!(
             Some(&ColumnType::Single {
                 datatype: DataType::Integer,
-                nullable: Some(true) //sqlite primary key columns are nullable unless declared not null
+                nullable: Some(false)
             }),
             root_block_cols[&table_db_block].get(&0)
         );

sqlx-sqlite/src/statement/handle.rs

@@ -212,9 +212,9 @@ impl StatementHandle {
 
     /// Use sqlite3_column_metadata to determine if a specific column is nullable.
     ///
-    /// Returns None in the case of INTEGER PRIMARY KEYs
+    /// Returns Some(false) in the case of INTEGER PRIMARY KEYs
     /// This is because this column is an alias to rowid if the table does not use a compound
-    /// primary key. In this case the row is not nullable, and the output of
+    /// primary key. In this case the column is not nullable, and the output of
     /// sqlite3_column_metadata may be incorrect.
     pub(crate) fn column_nullable(&self, index: usize) -> Result<Option<bool>, Error> {
         unsafe {
@@ -271,7 +271,7 @@ impl StatementHandle {
                         .to_bytes()
                         .eq_ignore_ascii_case("integer".as_bytes())
                 {
-                    None
+                    Some(false)
                 } else {
                     Some(not_null == 0)
                 },

sqlx-sqlite/src/types/time.rs

@@ -192,6 +192,7 @@ fn decode_datetime_from_text(value: &str) -> Option<PrimitiveDateTime> {
     None
 }
 
+#[allow(deprecated)]
 mod formats {
     use time::format_description::BorrowedFormatItem::{Component, Literal, Optional};
     use time::format_description::{modifier, BorrowedFormatItem, Component::*};
@@ -270,12 +271,12 @@ mod formats {
             Literal(b":"),
             MINUTE,
             Optional(&Literal(b":")),
-            Optional(&SECOND),
+            SECOND,
             Optional(&Literal(b".")),
-            Optional(&SUBSECOND),
-            Optional(&OFFSET_HOUR),
+            SUBSECOND,
+            OFFSET_HOUR,
             Optional(&Literal(b":")),
-            Optional(&OFFSET_MINUTE),
+            OFFSET_MINUTE,
         ]
     };
 
@@ -291,9 +292,9 @@ mod formats {
             Literal(b":"),
             MINUTE,
             Optional(&Literal(b":")),
-            Optional(&SECOND),
+            SECOND,
             Optional(&Literal(b".")),
-            Optional(&SUBSECOND),
+            SUBSECOND,
             Optional(&Literal(b"Z")),
         ]
     };
@@ -310,9 +311,9 @@ mod formats {
             Literal(b":"),
             MINUTE,
             Optional(&Literal(b":")),
-            Optional(&SECOND),
+            SECOND,
             Optional(&Literal(b".")),
-            Optional(&SUBSECOND),
+            SUBSECOND,
             Optional(&Literal(b"Z")),
         ]
     };

tests/sqlite/macros.rs

@@ -358,4 +358,32 @@ async fn test_column_override_exact_nullable() -> anyhow::Result<()> {
     Ok(())
 }
 
-// we don't emit bind parameter typechecks for SQLite so testing the overrides is redundant
+// Regression test: INTEGER PRIMARY KEY (a rowid alias) must not be nullable.
+// Before the fix, `query!` would infer `id` as `Option<i64>` and the
+// assignment `let _: i64 = row.id` would fail to compile.
+#[sqlx_macros::test]
+async fn test_returning_primary_key_is_not_nullable() -> anyhow::Result<()> {
+    let mut conn = new::<Sqlite>().await?;
+
+    let row =
+        sqlx::query!(r#"INSERT INTO accounts_no_not_null ( name ) VALUES ( 'test' ) RETURNING id"#)
+            .fetch_one(&mut conn)
+            .await?;
+
+    let _: i64 = row.id;
+
+    Ok(())
+}
+
+#[sqlx_macros::test]
+async fn test_returning_with_foreign_key_is_not_nullable() -> anyhow::Result<()> {
+    let mut conn = new::<Sqlite>().await?;
+
+    let _id: i64 = sqlx::query_scalar!(
+        r#"INSERT INTO packages ( project_id ) VALUES ( 1 ) RETURNING package_id"#
+    )
+    .fetch_one(&mut conn)
+    .await?;
+
+    Ok(())
+}

tests/sqlite/setup.sql

@@ -23,6 +23,10 @@ CREATE TABLE accounts (
     name TEXT NOT NULL,
     is_active BOOLEAN
 );
+CREATE TABLE accounts_no_not_null (
+    id INTEGER PRIMARY KEY,
+    name TEXT NOT NULL
+);
 INSERT INTO accounts(id, name, is_active)
 VALUES (1, 'Herp Derpinson', 1);
 CREATE VIEW accounts_view as
@@ -35,3 +39,8 @@ CREATE TABLE products (
     price NUMERIC,
     CONSTRAINT price_greater_than_zero CHECK (price > 0)
 );
+
+CREATE TABLE packages (
+    package_id INTEGER PRIMARY KEY,
+    project_id INTEGER
+);