UnexpectedNullError when deserializing (Null,Boolean,Boolean) into Option<(a,b,c)>

[Ten0]

Setup

Versions

• Rust: 1.40.0
• Diesel: master (bd349ce)
• Database: Postgresql
• Operating System Linux

Feature Flags

• diesel: "postgres", "r2d2", "serde_json", "chrono"

Problem Description

Hitting an unexpected UnexpectedNullError at runtime

What are you trying to accomplish?

    xxx::table
        .find(xxx_id)
        .inner_join(yyy::table)
        .left_join(zzz::table)
        .select((
            xxx::integer,
            (
                zzz::integer,
                zzz::nullable_integer.is_not_null(),
                yyy::nullable_integer
                    .is_not_null()
                    .or(zzz::nullable_enum.is_not_null()),
            )
                .nullable(),
        ))
        .get_result::<(Option<i32>, Option<(i32, bool, bool)>)>(db)?;

where postgres' tuple result of that query is NULL, NULL, FALSE, FALSE

What is the expected output?

Ok((None, None))

What is the actual output?

Err(DeserializationError(UnexpectedNullError))

Are you seeing any additional errors?

Stack trace of where this error ends up being triggered is:

  14: diesel::type_impls::integers::<impl diesel::deserialize::FromSql<diesel::sql_types::Integer,DB> for i32>::from_sql
             at /home/ten/Projects/diesel/diesel/src/type_impls/integers.rs:47
  15: diesel::type_impls::primitives::foreign_impls::_::<impl diesel::deserialize::FromSqlRow<__ST,__DB> for i32>::build_from_row
             at /home/ten/Projects/diesel/diesel/src/type_impls/primitives.rs:28 <-- This probably comes from that I'm actually using an intermediate type but it's unrelated to our issue: same thing happens when using directly an i32
  16: diesel::type_impls::tuples::<impl diesel::deserialize::FromSqlRow<(SA, SB, SC, SD),__DB> for (A, B, C, D)>::build_from_row
             at ./src/lib.rs:1
  17: diesel::type_impls::option::<impl diesel::deserialize::FromSqlRow<diesel::sql_types::Nullable<ST>,DB> for core::option::Option<T>>::build_from_row
             at /home/ten/Projects/diesel/diesel/src/type_impls/option.rs:98
  18: diesel::type_impls::tuples::<impl diesel::deserialize::FromSqlRow<(SA, SB),__DB> for (A, B)>::build_from_row
             at ./src/lib.rs:1
  19: <diesel::pg::connection::cursor::Cursor<ST,T> as core::iter::traits::iterator::Iterator>::next
             at /home/ten/Projects/diesel/diesel/src/pg/connection/cursor.rs:41

(replaced the return Err(UnexpectedNullError) of the not_none! macro by a panic to get the stack trace so I hope this error would not have been caught and handled, but it's pretty consistent with why i'd expect this error to trigger so I think this is the actual issue.

Checklist

• I have already looked over the issue tracker for similar issues.
• This issue can be reproduced on Rust's stable channel. (Your issue will be
  closed if this is not the case)

Solving

Digged into that stacktrace a bit, and found this:

diesel/diesel/src/type_impls/option.rs

Lines 84 to 101 in bd349ce

	impl<T, ST, DB> FromSqlRow<Nullable<ST>, DB> for Option<T>
	where
	T: FromSqlRow<ST, DB>,
	DB: Backend,
	ST: NotNull,
	{
	const FIELDS_NEEDED: usize = T::FIELDS_NEEDED;
	fn build_from_row<R: crate::row::Row<DB>>(row: &mut R) -> deserialize::Result<Self> {
	let fields_needed = Self::FIELDS_NEEDED;
	if row.next_is_null(fields_needed) {
	row.advance(fields_needed);
	Ok(None)
	} else {
	T::build_from_row(row).map(Some)
	}
	}
	}

where in particular, next_is_null is documented as making sure that "the next count columns are all NULL", hence the current behaviour.

Note: Just checking whether any of these columns is null is not satisfactory because we still want to deserialize 1, NULL into Option<i32, Option<i32>> as Some(1, None) and not just None.

Whether it should be solved

Clearly in my case whether these booleans are defined doesn't have any meaning if I couldn't join on the zzz table, so I'm very happy to not have access to them if just any of these fields is NULL, which is why I'm expecting this behaviour.

I can't think of a scenario where I'd prefer a runtime error: cases where I could unintentionally "forget" a field should be covered by the typing after the query: "Why can't I get the value of this field? Oh it's because it was packed in this Option...)

[weiznich]

@Ten0 I've added a fix for this as part of #2182 in 9433438. Any input there would be welcome.