Allow arrays containing null to be serialized #698
Conversation
diesel/src/pg/types/array.rs
Outdated
| try!(out.write_all(&buffer)); | ||
| buffer.clear(); | ||
| } else { | ||
| try!(out.write_i32::<NetworkEndian>(-1)); |
There was a problem hiding this comment.
I'm not familiar with Postgres' binary interface, so this is one of the places where I'd really like to have a link to some semi-official Postgres document that says "-1 is null here".
There was a problem hiding this comment.
I can link to the line in the source code? There is no documentation on this. But we also have
diesel/diesel/src/pg/types/array.rs
Lines 54 to 56 in 3d985d7
There was a problem hiding this comment.
Would you like me to link to https://github.com/postgres/postgres/blob/82f8107b92c9104ec9d9465f3f6a4c6dab4c124a/src/backend/utils/adt/arrayfuncs.c#L1461 ?
There was a problem hiding this comment.
Yes! It even points to something with postgres in the name! Perfect! 😄
No, but seriously, it helps see where this comes from (and where to look when something changed).
Deserialization already worked, so there's no reason we shouldn't just allow this. It's worth noting that there is no way to represent whether an array is allowed to contain nulls or not within postgres's type system. As such, the type `Array<Nullable<ST>>` will never be generated by `infer_schema!` (I do want to make `FromSql<Array<ST>> for `Vec<Option<T>>` just work though). Since we never provide a `ToSql<Array<ST>> for Vec<Option<T>>`, Diesel will prevent you from accidentally inserting nulls at compile time. Ultimately the user does just have to make sure they represent whether the array could contain null in the output type, however. One alternative would be to represent the fact that all arrays can contain null within the type system, and only provide `impl FromSql<Array<ST>> for Vec<Option<T>>`. I suspect this would hinder ergonomics pretty heavily, but we should consider it before 1.0. One note about the implementation -- Technically we should be setting `flags` to `1` if the array contains any nulls, as that's what happens on the data sent to us. However, setting it would require us to perform an additional level of intermediate buffering on all the values to determine whether the flag should be set, and that flag is currently ignored by PG. "But Sean, that's just relying on an implementation detail!" this is true, but our entire usage of the binary format is relying on an implementation detail (See #695).
sgrif
force-pushed
the
sg-arrays-of-null
branch
from
3d985d7 to
a571e41
Compare
Deserialization already worked, so there's no reason we shouldn't just
allow this. It's worth noting that there is no way to represent whether
an array is allowed to contain nulls or not within postgres's type
system. As such, the type
Array<Nullable<ST>>will never be generatedby
infer_schema!(I do want to makeFromSql<Array<ST>> forVec<Option>just work though). Since we never provide aToSql<Array> for Vec<Option>`, Diesel will prevent you fromaccidentally inserting nulls at compile time. Ultimately the user does
just have to make sure they represent whether the array could contain
null in the output type, however.
One alternative would be to represent the fact that all arrays can
contain null within the type system, and only provide
impl FromSql<Array<ST>> for Vec<Option<T>>. I suspect this would hinderergonomics pretty heavily, but we should consider it before 1.0.
One note about the implementation -- Technically we should be setting
flagsto1if the array contains any nulls, as that's what happenson the data sent to us. However, setting it would require us to perform
an additional level of intermediate buffering on all the values to
determine whether the flag should be set, and that flag is currently
ignored by PG. "But Sean, that's just relying on an implementation
detail!" this is true, but our entire usage of the binary format is
relying on an implementation detail (See #695).