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

@@ -193,127 +193,103 @@ fn decode_datetime_from_text(value: &str) -> Option<PrimitiveDateTime> {
 }
 
 mod formats {
-    use time::format_description::BorrowedFormatItem::{Component, Literal, Optional};
+    use time::format_description::BorrowedFormatItem::{Component, Optional, StringLiteral};
     use time::format_description::{modifier, BorrowedFormatItem, Component::*};
 
-    const YEAR: BorrowedFormatItem<'_> = Component(Year({
-        let mut value = modifier::Year::default();
-        value.padding = modifier::Padding::Zero;
-        value.repr = modifier::YearRepr::Full;
-        value.iso_week_based = false;
-        value.sign_is_mandatory = false;
-        value
-    }));
-
-    const MONTH: BorrowedFormatItem<'_> = Component(Month({
-        let mut value = modifier::Month::default();
-        value.padding = modifier::Padding::Zero;
-        value.repr = modifier::MonthRepr::Numerical;
-        value.case_sensitive = true;
-        value
-    }));
-
-    const DAY: BorrowedFormatItem<'_> = Component(Day({
-        let mut value = modifier::Day::default();
-        value.padding = modifier::Padding::Zero;
-        value
-    }));
-
-    const HOUR: BorrowedFormatItem<'_> = Component(Hour({
-        let mut value = modifier::Hour::default();
-        value.padding = modifier::Padding::Zero;
-        value.is_12_hour_clock = false;
-        value
-    }));
-
-    const MINUTE: BorrowedFormatItem<'_> = Component(Minute({
-        let mut value = modifier::Minute::default();
-        value.padding = modifier::Padding::Zero;
-        value
-    }));
-
-    const SECOND: BorrowedFormatItem<'_> = Component(Second({
-        let mut value = modifier::Second::default();
-        value.padding = modifier::Padding::Zero;
-        value
-    }));
-
-    const SUBSECOND: BorrowedFormatItem<'_> = Component(Subsecond({
-        let mut value = modifier::Subsecond::default();
-        value.digits = modifier::SubsecondDigits::OneOrMore;
-        value
-    }));
-
-    const OFFSET_HOUR: BorrowedFormatItem<'_> = Component(OffsetHour({
-        let mut value = modifier::OffsetHour::default();
-        value.sign_is_mandatory = true;
-        value.padding = modifier::Padding::Zero;
-        value
-    }));
-
-    const OFFSET_MINUTE: BorrowedFormatItem<'_> = Component(OffsetMinute({
-        let mut value = modifier::OffsetMinute::default();
-        value.padding = modifier::Padding::Zero;
-        value
-    }));
+    const YEAR: BorrowedFormatItem<'_> = Component(CalendarYearFullStandardRange(
+        modifier::CalendarYearFullStandardRange::default()
+            .with_padding(modifier::Padding::Zero)
+            .with_sign_is_mandatory(false),
+    ));
+
+    const MONTH: BorrowedFormatItem<'_> = Component(MonthNumerical(
+        modifier::MonthNumerical::default().with_padding(modifier::Padding::Zero),
+    ));
+
+    const DAY: BorrowedFormatItem<'_> =
+        Component(Day(modifier::Day::default().with_padding(modifier::Padding::Zero)));
+
+    const HOUR: BorrowedFormatItem<'_> =
+        Component(Hour24(modifier::Hour24::default().with_padding(modifier::Padding::Zero)));
+
+    const MINUTE: BorrowedFormatItem<'_> =
+        Component(Minute(modifier::Minute::default().with_padding(modifier::Padding::Zero)));
+
+    const SECOND: BorrowedFormatItem<'_> = Component(Second(
+        modifier::Second::default().with_padding(modifier::Padding::Zero),
+    ));
+
+    const SUBSECOND: BorrowedFormatItem<'_> = Component(Subsecond(
+        modifier::Subsecond::default().with_digits(modifier::SubsecondDigits::OneOrMore),
+    ));
+
+    const OFFSET_HOUR: BorrowedFormatItem<'_> = Component(OffsetHour(
+        modifier::OffsetHour::default()
+            .with_sign_is_mandatory(true)
+            .with_padding(modifier::Padding::Zero),
+    ));
+
+    const OFFSET_MINUTE: BorrowedFormatItem<'_> = Component(OffsetMinute(
+        modifier::OffsetMinute::default().with_padding(modifier::Padding::Zero),
+    ));
 
     pub(super) const OFFSET_DATE_TIME: &[BorrowedFormatItem<'_>] = {
         &[
             YEAR,
-            Literal(b"-"),
+            StringLiteral("-"),
             MONTH,
-            Literal(b"-"),
+            StringLiteral("-"),
             DAY,
-            Optional(&Literal(b" ")),
-            Optional(&Literal(b"T")),
+            Optional(&StringLiteral(" ")),
+            Optional(&StringLiteral("T")),
             HOUR,
-            Literal(b":"),
+            StringLiteral(":"),
             MINUTE,
-            Optional(&Literal(b":")),
-            Optional(&SECOND),
-            Optional(&Literal(b".")),
-            Optional(&SUBSECOND),
-            Optional(&OFFSET_HOUR),
-            Optional(&Literal(b":")),
-            Optional(&OFFSET_MINUTE),
+            Optional(&StringLiteral(":")),
+            SECOND,
+            Optional(&StringLiteral(".")),
+            SUBSECOND,
+            OFFSET_HOUR,
+            Optional(&StringLiteral(":")),
+            OFFSET_MINUTE,
         ]
     };
 
     pub(super) const PRIMITIVE_DATE_TIME_SPACE_SEPARATED: &[BorrowedFormatItem<'_>] = {
         &[
             YEAR,
-            Literal(b"-"),
+            StringLiteral("-"),
             MONTH,
-            Literal(b"-"),
+            StringLiteral("-"),
             DAY,
-            Literal(b" "),
+            StringLiteral(" "),
             HOUR,
-            Literal(b":"),
+            StringLiteral(":"),
             MINUTE,
-            Optional(&Literal(b":")),
-            Optional(&SECOND),
-            Optional(&Literal(b".")),
-            Optional(&SUBSECOND),
-            Optional(&Literal(b"Z")),
+            Optional(&StringLiteral(":")),
+            SECOND,
+            Optional(&StringLiteral(".")),
+            SUBSECOND,
+            Optional(&StringLiteral("Z")),
         ]
     };
 
     pub(super) const PRIMITIVE_DATE_TIME_T_SEPARATED: &[BorrowedFormatItem<'_>] = {
         &[
             YEAR,
-            Literal(b"-"),
+            StringLiteral("-"),
             MONTH,
-            Literal(b"-"),
+            StringLiteral("-"),
             DAY,
-            Literal(b"T"),
+            StringLiteral("T"),
             HOUR,
-            Literal(b":"),
+            StringLiteral(":"),
             MINUTE,
-            Optional(&Literal(b":")),
-            Optional(&SECOND),
-            Optional(&Literal(b".")),
-            Optional(&SUBSECOND),
-            Optional(&Literal(b"Z")),
+            Optional(&StringLiteral(":")),
+            SECOND,
+            Optional(&StringLiteral(".")),
+            SUBSECOND,
+            Optional(&StringLiteral("Z")),
         ]
     };
 }

tests/sqlite/macros.rs

@@ -358,4 +358,37 @@ 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?;
+
+    sqlx::query("INSERT INTO projects ( project_id ) VALUES ( 1 )")
+        .execute(&mut conn)
+        .await?;
+
+    let _id: i64 = sqlx::query_scalar!(
+        r#"INSERT INTO foo ( 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,13 @@ CREATE TABLE products (
     price NUMERIC,
     CONSTRAINT price_greater_than_zero CHECK (price > 0)
 );
+
+CREATE TABLE projects (
+    project_id INTEGER PRIMARY KEY
+);
+
+CREATE TABLE foo (
+    package_id INTEGER PRIMARY KEY NOT NULL CHECK(package_id >= 0),
+    project_id INTEGER NOT NULL,
+    FOREIGN KEY(project_id) REFERENCES projects(project_id)
+);