From f8d5e213774f0706cb25900ffde2b0640a289257 Mon Sep 17 00:00:00 2001 From: Thomas BESSOU Date: Wed, 22 Jan 2020 17:22:57 +0100 Subject: [PATCH 1/3] Fix UnexpectedNullError when deserializing (Null,NotNull) into Option<(a,b)> Resolves #2274 --- diesel/src/type_impls/option.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/diesel/src/type_impls/option.rs b/diesel/src/type_impls/option.rs index 10414eefe5d2..07a781549d06 100644 --- a/diesel/src/type_impls/option.rs +++ b/diesel/src/type_impls/option.rs @@ -95,7 +95,16 @@ where row.advance(fields_needed); Ok(None) } else { - T::build_from_row(row).map(Some) + match T::build_from_row(row) { + Ok(v) => Ok(Some(v)), + Err(e) => { + if e.is::() { + Ok(None) + } else { + Err(e) + } + } + } } } } From 465c35e5f18cea4de28eaccbeb9a1fe529153dcc Mon Sep 17 00:00:00 2001 From: Thomas BESSOU Date: Mon, 17 Feb 2020 15:23:44 +0100 Subject: [PATCH 2/3] Add test (does not pass atm) --- diesel_tests/tests/joins.rs | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/diesel_tests/tests/joins.rs b/diesel_tests/tests/joins.rs index c7d4d5a02981..74e2810e2171 100644 --- a/diesel_tests/tests/joins.rs +++ b/diesel_tests/tests/joins.rs @@ -300,6 +300,35 @@ fn select_right_side_with_nullable_column_first() { assert_eq!(expected_data, actual_data); } +#[test] +fn select_left_join_right_side_with_non_null_inside() { + let connection = connection_with_sean_and_tess_in_users_table(); + + connection + .execute( + "INSERT INTO posts (user_id, title, body) VALUES + (1, 'Hello', 'Content') + ", + ) + .unwrap(); + + let expected_data = vec![ + (None, 2), + (Some((1, "Hello".to_string(), "Hello".to_string())), 1), + ]; + + let source = users::table + .left_outer_join(posts::table) + .select(( + (users::id, posts::title, posts::title).nullable(), + users::id, + )) + .order_by((users::id.desc(), posts::id.asc())); + let actual_data: Vec<_> = source.load(&connection).unwrap(); + + assert_eq!(expected_data, actual_data); +} + #[test] fn select_then_join() { use crate::schema::users::dsl::*; From f00c46eb1bdaff844aea59da1b436629ae28c390 Mon Sep 17 00:00:00 2001 From: Thomas BESSOU Date: Mon, 17 Feb 2020 18:15:11 +0100 Subject: [PATCH 3/3] Properly skip leftover fields in `Option` after recovering from UnexpectedNullError --- diesel/src/mysql/connection/stmt/iterator.rs | 4 ++++ diesel/src/pg/connection/row.rs | 4 ++++ diesel/src/row.rs | 6 ++++++ diesel/src/sqlite/connection/functions.rs | 8 +++++++- diesel/src/sqlite/connection/sqlite_value.rs | 4 ++++ diesel/src/type_impls/option.rs | 11 +++++++++++ 6 files changed, 36 insertions(+), 1 deletion(-) diff --git a/diesel/src/mysql/connection/stmt/iterator.rs b/diesel/src/mysql/connection/stmt/iterator.rs index 4911fcf29ff7..02230a39c2e9 100644 --- a/diesel/src/mysql/connection/stmt/iterator.rs +++ b/diesel/src/mysql/connection/stmt/iterator.rs @@ -59,6 +59,10 @@ impl<'a> Row for MysqlRow<'a> { fn next_is_null(&self, count: usize) -> bool { (0..count).all(|i| self.binds.field_data(self.col_idx + i).is_none()) } + + fn column_index(&self) -> usize { + self.col_idx + } } pub struct NamedStatementIterator<'a> { diff --git a/diesel/src/pg/connection/row.rs b/diesel/src/pg/connection/row.rs index 1ebf50cfb51c..ad1ffe5cae9f 100644 --- a/diesel/src/pg/connection/row.rs +++ b/diesel/src/pg/connection/row.rs @@ -31,6 +31,10 @@ impl<'a> Row for PgRow<'a> { fn next_is_null(&self, count: usize) -> bool { (0..count).all(|i| self.db_result.is_null(self.row_idx, self.col_idx + i)) } + + fn column_index(&self) -> usize { + self.col_idx + } } pub struct PgNamedRow<'a> { diff --git a/diesel/src/row.rs b/diesel/src/row.rs index f5e6a695c065..18989133b72a 100644 --- a/diesel/src/row.rs +++ b/diesel/src/row.rs @@ -19,6 +19,12 @@ pub trait Row { /// would all return `None`. fn next_is_null(&self, count: usize) -> bool; + /// Returns the index of the current column (next to be returned by take, zero based) + /// + /// May be used to skip the right number of fields when recovering for an `UnexpectedNullError` when + /// deserializing an `Option` + fn column_index(&self) -> usize; + /// Skips the next `count` columns. This method must be called if you are /// choosing not to call `take` as a result of `next_is_null` returning /// `true`. diff --git a/diesel/src/sqlite/connection/functions.rs b/diesel/src/sqlite/connection/functions.rs index 3e6a526531e5..eb86098daa14 100644 --- a/diesel/src/sqlite/connection/functions.rs +++ b/diesel/src/sqlite/connection/functions.rs @@ -30,7 +30,7 @@ where } conn.register_sql_function(fn_name, fields_needed, deterministic, move |conn, args| { - let mut row = FunctionRow { args }; + let mut row = FunctionRow { args, col_idx: 0 }; let args_row = Args::Row::build_from_row(&mut row).map_err(Error::DeserializationError)?; let args = Args::build(args_row); @@ -55,11 +55,13 @@ where struct FunctionRow<'a> { args: &'a [*mut ffi::sqlite3_value], + col_idx: usize, } impl<'a> Row for FunctionRow<'a> { fn take(&mut self) -> Option<&SqliteValue> { self.args.split_first().and_then(|(&first, rest)| { + self.col_idx += 1; self.args = rest; unsafe { SqliteValue::new(first) } }) @@ -70,4 +72,8 @@ impl<'a> Row for FunctionRow<'a> { .iter() .all(|&p| unsafe { SqliteValue::new(p) }.is_none()) } + + fn column_index(&self) -> usize { + self.col_idx + } } diff --git a/diesel/src/sqlite/connection/sqlite_value.rs b/diesel/src/sqlite/connection/sqlite_value.rs index 247f49210d20..6cd50bbedcd2 100644 --- a/diesel/src/sqlite/connection/sqlite_value.rs +++ b/diesel/src/sqlite/connection/sqlite_value.rs @@ -105,6 +105,10 @@ impl Row for SqliteRow { tpe == ffi::SQLITE_NULL }) } + + fn column_index(&self) -> usize { + self.next_col_index as usize + } } pub struct SqliteNamedRow<'a> { diff --git a/diesel/src/type_impls/option.rs b/diesel/src/type_impls/option.rs index 07a781549d06..7af0215027bb 100644 --- a/diesel/src/type_impls/option.rs +++ b/diesel/src/type_impls/option.rs @@ -95,10 +95,21 @@ where row.advance(fields_needed); Ok(None) } else { + let column_index_before_build = row.column_index(); match T::build_from_row(row) { Ok(v) => Ok(Some(v)), Err(e) => { if e.is::() { + // It is possible that we are recovering but not all fields have been taken. + // We need to skip those that are left that belong to this `Option`. + + // We are returning a deserialization error in case this goes unexpectedly + // to make it easier on people implementing `FromSqlRow` + row.advance( + (column_index_before_build + fields_needed) + .checked_sub(row.column_index()) + .ok_or("Inconsistent FromSqlRow impl: we went further than FIELDS_NEEDED")?, + ); Ok(None) } else { Err(e)