From 9119af46f5db7f50746b21f4fc2a99fd5422d843 Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Thu, 7 Dec 2023 09:40:48 +0100 Subject: [PATCH 01/19] serialize/writers: get rid of counting writers They are not going to be used after in the adjusted design, so remove them. --- scylla-cql/src/types/serialize/mod.rs | 2 +- scylla-cql/src/types/serialize/writers.rs | 125 +--------------------- 2 files changed, 2 insertions(+), 125 deletions(-) diff --git a/scylla-cql/src/types/serialize/mod.rs b/scylla-cql/src/types/serialize/mod.rs index 617fbc5f88..5cb8cc37c0 100644 --- a/scylla-cql/src/types/serialize/mod.rs +++ b/scylla-cql/src/types/serialize/mod.rs @@ -8,7 +8,7 @@ pub mod writers; pub use writers::{ BufBackedCellValueBuilder, BufBackedCellWriter, BufBackedRowWriter, CellValueBuilder, - CellWriter, CountingCellWriter, RowWriter, + CellWriter, RowWriter, }; #[derive(Debug, Clone, Error)] pub struct SerializationError(Arc); diff --git a/scylla-cql/src/types/serialize/writers.rs b/scylla-cql/src/types/serialize/writers.rs index ecb8a1fcc1..3562a14983 100644 --- a/scylla-cql/src/types/serialize/writers.rs +++ b/scylla-cql/src/types/serialize/writers.rs @@ -247,122 +247,10 @@ impl<'buf> CellValueBuilder for BufBackedCellValueBuilder<'buf> { } } -/// A row writer that does not actually write anything, just counts the bytes. -pub struct CountingRowWriter<'buf> { - buf: &'buf mut usize, -} - -impl<'buf> CountingRowWriter<'buf> { - /// Creates a new writer which increments the counter under given reference - /// when bytes are appended. - #[inline] - pub fn new(buf: &'buf mut usize) -> Self { - CountingRowWriter { buf } - } -} - -impl<'buf> RowWriter for CountingRowWriter<'buf> { - type CellWriter<'a> = CountingCellWriter<'a> where Self: 'a; - - #[inline] - fn make_cell_writer(&mut self) -> Self::CellWriter<'_> { - CountingCellWriter::new(self.buf) - } -} - -/// A cell writer that does not actually write anything, just counts the bytes. -pub struct CountingCellWriter<'buf> { - buf: &'buf mut usize, -} - -impl<'buf> CountingCellWriter<'buf> { - /// Creates a new writer which increments the counter under given reference - /// when bytes are appended. - #[inline] - fn new(buf: &'buf mut usize) -> Self { - CountingCellWriter { buf } - } -} - -impl<'buf> CellWriter for CountingCellWriter<'buf> { - type ValueBuilder = CountingCellValueBuilder<'buf>; - - type WrittenCellProof = (); - - #[inline] - fn set_null(self) { - *self.buf += 4; - } - - #[inline] - fn set_unset(self) { - *self.buf += 4; - } - - #[inline] - fn set_value(self, contents: &[u8]) -> Result<(), CellOverflowError> { - if contents.len() > i32::MAX as usize { - return Err(CellOverflowError); - } - *self.buf += 4 + contents.len(); - Ok(()) - } - - #[inline] - fn into_value_builder(self) -> Self::ValueBuilder { - *self.buf += 4; - CountingCellValueBuilder::new(self.buf) - } -} - -pub struct CountingCellValueBuilder<'buf> { - buf: &'buf mut usize, - - starting_pos: usize, -} - -impl<'buf> CountingCellValueBuilder<'buf> { - /// Creates a new builder which increments the counter under given reference - /// when bytes are appended. - #[inline] - fn new(buf: &'buf mut usize) -> Self { - let starting_pos = *buf; - CountingCellValueBuilder { buf, starting_pos } - } -} - -impl<'buf> CellValueBuilder for CountingCellValueBuilder<'buf> { - type SubCellWriter<'a> = CountingCellWriter<'a> - where - Self: 'a; - - type WrittenCellProof = (); - - #[inline] - fn append_bytes(&mut self, bytes: &[u8]) { - *self.buf += bytes.len(); - } - - #[inline] - fn make_sub_writer(&mut self) -> Self::SubCellWriter<'_> { - CountingCellWriter::new(self.buf) - } - - #[inline] - fn finish(self) -> Result { - if *self.buf - self.starting_pos > i32::MAX as usize { - return Err(CellOverflowError); - } - Ok(()) - } -} - #[cfg(test)] mod tests { - use crate::types::serialize::writers::CountingRowWriter; - use super::{ - BufBackedCellWriter, BufBackedRowWriter, CellValueBuilder, CellWriter, CountingCellWriter, + BufBackedCellWriter, BufBackedRowWriter, CellValueBuilder, CellWriter, RowWriter, }; @@ -377,12 +265,6 @@ mod tests { let mut data = Vec::new(); let writer = BufBackedCellWriter::new(&mut data); c.check(writer); - - let mut byte_count = 0usize; - let counting_writer = CountingCellWriter::new(&mut byte_count); - c.check(counting_writer); - - assert_eq!(data.len(), byte_count); data } @@ -441,11 +323,6 @@ mod tests { let mut writer = BufBackedRowWriter::new(&mut data); c.check(&mut writer); - let mut byte_count = 0usize; - let mut counting_writer = CountingRowWriter::new(&mut byte_count); - c.check(&mut counting_writer); - - assert_eq!(data.len(), byte_count); data } From ff9e363afd14e9bdc788f3387c6bafd9a2bd5989 Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Thu, 7 Dec 2023 09:44:48 +0100 Subject: [PATCH 02/19] serialize/writers: simplify tests The tests in the `writers` module used a helper function to run the same code with both counting writers and buffer-backed writers, which required implementing a trait (Rust doesn't have generic closures). This complexity is no longer necessary. --- scylla-cql/src/types/serialize/writers.rs | 79 ++++++----------------- 1 file changed, 19 insertions(+), 60 deletions(-) diff --git a/scylla-cql/src/types/serialize/writers.rs b/scylla-cql/src/types/serialize/writers.rs index 3562a14983..6c67177adc 100644 --- a/scylla-cql/src/types/serialize/writers.rs +++ b/scylla-cql/src/types/serialize/writers.rs @@ -249,42 +249,21 @@ impl<'buf> CellValueBuilder for BufBackedCellValueBuilder<'buf> { #[cfg(test)] mod tests { - use super::{ - BufBackedCellWriter, BufBackedRowWriter, CellValueBuilder, CellWriter, - RowWriter, - }; - - // We want to perform the same computation for both buf backed writer - // and counting writer, but Rust does not support generic closures. - // This trait comes to the rescue. - trait CellSerializeCheck { - fn check(&self, writer: W); - } - - fn check_cell_serialize(c: C) -> Vec { - let mut data = Vec::new(); - let writer = BufBackedCellWriter::new(&mut data); - c.check(writer); - data - } + use super::{BufBackedCellWriter, BufBackedRowWriter, CellValueBuilder, CellWriter, RowWriter}; #[test] fn test_cell_writer() { - struct Check; - impl CellSerializeCheck for Check { - fn check(&self, writer: W) { - let mut sub_writer = writer.into_value_builder(); - sub_writer.make_sub_writer().set_null(); - sub_writer - .make_sub_writer() - .set_value(&[1, 2, 3, 4]) - .unwrap(); - sub_writer.make_sub_writer().set_unset(); - sub_writer.finish().unwrap(); - } - } + let mut data = Vec::new(); + let writer = BufBackedCellWriter::new(&mut data); + let mut sub_writer = writer.into_value_builder(); + sub_writer.make_sub_writer().set_null(); + sub_writer + .make_sub_writer() + .set_value(&[1, 2, 3, 4]) + .unwrap(); + sub_writer.make_sub_writer().set_unset(); + sub_writer.finish().unwrap(); - let data = check_cell_serialize(Check); assert_eq!( data, [ @@ -298,14 +277,10 @@ mod tests { #[test] fn test_poisoned_appender() { - struct Check; - impl CellSerializeCheck for Check { - fn check(&self, writer: W) { - let _ = writer.into_value_builder(); - } - } + let mut data = Vec::new(); + let writer = BufBackedCellWriter::new(&mut data); + let _ = writer.into_value_builder(); - let data = check_cell_serialize(Check); assert_eq!( data, [ @@ -314,30 +289,14 @@ mod tests { ); } - trait RowSerializeCheck { - fn check(&self, writer: &mut W); - } - - fn check_row_serialize(c: C) -> Vec { - let mut data = Vec::new(); - let mut writer = BufBackedRowWriter::new(&mut data); - c.check(&mut writer); - - data - } - #[test] fn test_row_writer() { - struct Check; - impl RowSerializeCheck for Check { - fn check(&self, writer: &mut W) { - writer.make_cell_writer().set_null(); - writer.make_cell_writer().set_value(&[1, 2, 3, 4]).unwrap(); - writer.make_cell_writer().set_unset(); - } - } + let mut data = Vec::new(); + let mut writer = BufBackedRowWriter::new(&mut data); + writer.make_cell_writer().set_null(); + writer.make_cell_writer().set_value(&[1, 2, 3, 4]).unwrap(); + writer.make_cell_writer().set_unset(); - let data = check_row_serialize(Check); assert_eq!( data, [ From a7aa5d5ba845b0ea11679f3e3854c607eb7130a1 Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Wed, 6 Dec 2023 02:21:41 +0100 Subject: [PATCH 03/19] serialize/writers: add WrittenCellProof The writer traits are using a type-level trick to force implementors of `SerializeCql::serialize` to call `CellWriter::finish()`: the latter returns `CellWriter::WrittenCellProof` which the former is required to return. The fact that the proof is an associated type of the writer and the `serialize` method is generic over the writer type makes sure that the proof truly comes from the `finish()` call on the writer provided to the method and not from any other writer. We are going to change `CellWriter` into a struct, so this trick will no longer be applicable. Instead, introduce a `WrittenCellProof` struct, which is a zero-sized type generic over an invariant lifetime. The `CellWriter` struct will consume itself and return a `WrittenCellProof` with the same lifetime parameter as the original cell writer; `serialize` will require the implementor to return it. The lifetime invariance makes sure that one proof cannot be assigned to another if their lifetimes are not exactly the same. --- scylla-cql/src/types/serialize/writers.rs | 51 +++++++++++++++++++---- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/scylla-cql/src/types/serialize/writers.rs b/scylla-cql/src/types/serialize/writers.rs index 6c67177adc..59f564ad53 100644 --- a/scylla-cql/src/types/serialize/writers.rs +++ b/scylla-cql/src/types/serialize/writers.rs @@ -105,6 +105,39 @@ pub trait CellValueBuilder { fn finish(self) -> Result; } +/// An object that indicates a type-level proof that something was written +/// by a [`CellWriter`] or [`CellValueBuilder`] with lifetime parameter `'buf`. +/// +/// This type is returned by [`set_null`](CellWriter::set_null), +/// [`set_unset`](CellWriter::set_unset), +/// [`set_value`](CellWriter::set_value) +/// and also [`CellValueBuilder::finish`] - generally speaking, after +/// the value is fully initialized and the `CellWriter` is destroyed. +/// +/// The purpose of this type is to enforce the contract of +/// [`SerializeCql::serialize`](super::value::SerializeCql::serialize): either +/// the method succeeds and returns a proof that it serialized itself +/// into the given value, or it fails and returns an error or panics. +pub struct WrittenCellProof<'buf> { + /// Using *mut &'buf () is deliberate and makes WrittenCellProof invariant + /// on the 'buf lifetime parameter. + /// Ref: + _phantom: std::marker::PhantomData<*mut &'buf ()>, +} + +impl<'buf> WrittenCellProof<'buf> { + /// A shorthand for creating the proof. + /// + /// Do not make it public! It's important that only the row writer defined + /// in this module is able to create a proof. + #[inline] + fn new() -> Self { + WrittenCellProof { + _phantom: std::marker::PhantomData, + } + } +} + /// There was an attempt to produce a CQL value over the maximum size limit (i32::MAX) #[derive(Debug, Clone, Copy, Error)] #[error("CQL cell overflowed the maximum allowed size of 2^31 - 1")] @@ -169,24 +202,26 @@ impl<'buf> BufBackedCellWriter<'buf> { impl<'buf> CellWriter for BufBackedCellWriter<'buf> { type ValueBuilder = BufBackedCellValueBuilder<'buf>; - type WrittenCellProof = (); + type WrittenCellProof = WrittenCellProof<'buf>; #[inline] - fn set_null(self) { + fn set_null(self) -> Self::WrittenCellProof { self.buf.extend_from_slice(&(-1i32).to_be_bytes()); + WrittenCellProof::new() } #[inline] - fn set_unset(self) { + fn set_unset(self) -> Self::WrittenCellProof { self.buf.extend_from_slice(&(-2i32).to_be_bytes()); + WrittenCellProof::new() } #[inline] - fn set_value(self, bytes: &[u8]) -> Result<(), CellOverflowError> { + fn set_value(self, bytes: &[u8]) -> Result { let value_len: i32 = bytes.len().try_into().map_err(|_| CellOverflowError)?; self.buf.extend_from_slice(&value_len.to_be_bytes()); self.buf.extend_from_slice(bytes); - Ok(()) + Ok(WrittenCellProof::new()) } #[inline] @@ -224,7 +259,7 @@ impl<'buf> CellValueBuilder for BufBackedCellValueBuilder<'buf> { where Self: 'a; - type WrittenCellProof = (); + type WrittenCellProof = WrittenCellProof<'buf>; #[inline] fn append_bytes(&mut self, bytes: &[u8]) { @@ -237,13 +272,13 @@ impl<'buf> CellValueBuilder for BufBackedCellValueBuilder<'buf> { } #[inline] - fn finish(self) -> Result<(), CellOverflowError> { + fn finish(self) -> Result { let value_len: i32 = (self.buf.len() - self.starting_pos - 4) .try_into() .map_err(|_| CellOverflowError)?; self.buf[self.starting_pos..self.starting_pos + 4] .copy_from_slice(&value_len.to_be_bytes()); - Ok(()) + Ok(WrittenCellProof::new()) } } From e696663c949c0a4376bd808b6b525c526334c7d1 Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Thu, 7 Dec 2023 09:49:12 +0100 Subject: [PATCH 04/19] serialize/row: de-genericise SerializeRow Adjust the interface to use the buffer backed row writer. Rename BufBackedRowWriter to RowWriter to make our life easier later when we replace the writer trait with the buffer-backed row writer struct. --- scylla-cql/src/types/serialize/row.rs | 36 +++++++++++++-------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/scylla-cql/src/types/serialize/row.rs b/scylla-cql/src/types/serialize/row.rs index b5fd862cee..cc89c17607 100644 --- a/scylla-cql/src/types/serialize/row.rs +++ b/scylla-cql/src/types/serialize/row.rs @@ -10,7 +10,7 @@ use crate::frame::value::{SerializedValues, ValueList}; use crate::frame::{response::result::ColumnSpec, types::RawValue}; use super::value::SerializeCql; -use super::{CellWriter, RowWriter, SerializationError}; +use super::{BufBackedRowWriter as RowWriter, CellWriter, RowWriter as _, SerializationError}; /// Contains information needed to serialize a row. pub struct RowSerializationContext<'a> { @@ -48,10 +48,10 @@ pub trait SerializeRow { /// /// The function may assume that `preliminary_type_check` was called, /// though it must not do anything unsafe if this assumption does not hold. - fn serialize( + fn serialize( &self, ctx: &RowSerializationContext<'_>, - writer: &mut W, + writer: &mut RowWriter, ) -> Result<(), SerializationError>; fn is_empty(&self) -> bool; @@ -64,10 +64,10 @@ macro_rules! fallback_impl_contents { ) -> Result<(), SerializationError> { Ok(()) } - fn serialize( + fn serialize( &self, ctx: &RowSerializationContext<'_>, - writer: &mut W, + writer: &mut RowWriter, ) -> Result<(), SerializationError> { serialize_legacy_row(self, ctx, writer) } @@ -94,10 +94,10 @@ macro_rules! impl_serialize_row_for_unit { Ok(()) } - fn serialize( + fn serialize( &self, _ctx: &RowSerializationContext<'_>, - _writer: &mut W, + _writer: &mut RowWriter, ) -> Result<(), SerializationError> { // Row is empty - do nothing Ok(()) @@ -136,10 +136,10 @@ macro_rules! impl_serialize_row_for_slice { Ok(()) } - fn serialize( + fn serialize( &self, ctx: &RowSerializationContext<'_>, - writer: &mut W, + writer: &mut RowWriter, ) -> Result<(), SerializationError> { if ctx.columns().len() != self.len() { return Err(mk_typck_err::( @@ -197,10 +197,10 @@ macro_rules! impl_serialize_row_for_map { Ok(()) } - fn serialize( + fn serialize( &self, ctx: &RowSerializationContext<'_>, - writer: &mut W, + writer: &mut RowWriter, ) -> Result<(), SerializationError> { // Unfortunately, column names aren't guaranteed to be unique. // We need to track not-yet-used columns in order to see @@ -272,10 +272,10 @@ impl SerializeRow for &T { ::preliminary_type_check(ctx) } - fn serialize( + fn serialize( &self, ctx: &RowSerializationContext<'_>, - writer: &mut W, + writer: &mut RowWriter, ) -> Result<(), SerializationError> { ::serialize(self, ctx, writer) } @@ -326,10 +326,10 @@ macro_rules! impl_tuple { Ok(()) } - fn serialize( + fn serialize( &self, ctx: &RowSerializationContext<'_>, - writer: &mut W, + writer: &mut RowWriter, ) -> Result<(), SerializationError> { let ($($tidents,)*) = match ctx.columns() { [$($tidents),*] => ($($tidents,)*), @@ -452,10 +452,10 @@ macro_rules! impl_serialize_row_via_value_list { ::std::result::Result::Ok(()) } - fn serialize( + fn serialize( &self, ctx: &$crate::types::serialize::row::RowSerializationContext<'_>, - writer: &mut W, + writer: &mut $crate::types::serialize::writers::BufBackedRowWriter, ) -> ::std::result::Result<(), $crate::types::serialize::SerializationError> { $crate::types::serialize::row::serialize_legacy_row(self, ctx, writer) } @@ -492,7 +492,7 @@ macro_rules! impl_serialize_row_via_value_list { pub fn serialize_legacy_row( r: &T, ctx: &RowSerializationContext<'_>, - writer: &mut impl RowWriter, + writer: &mut RowWriter, ) -> Result<(), SerializationError> { let serialized = ::serialized(r).map_err(|err| SerializationError(Arc::new(err)))?; From f374ba7d9a7836c9978d30110c29c2aa16ce3ac7 Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Thu, 7 Dec 2023 09:54:14 +0100 Subject: [PATCH 05/19] serialize/value: de-genericise SerializeCql Adjust the interface to use the buffer backed cell writer. Rename BufBackedCellWriter to CellWriter to make our life easier later when we replace the writer trait with the buffer-backed cell writer struct. --- scylla-cql/src/types/serialize/value.rs | 147 ++++++++++++------------ 1 file changed, 75 insertions(+), 72 deletions(-) diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index 5d81cdb938..121eccf3ab 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -24,7 +24,10 @@ use crate::frame::value::{ #[cfg(feature = "chrono")] use crate::frame::value::ValueOverflow; -use super::{CellValueBuilder, CellWriter, SerializationError}; +use super::writers::WrittenCellProof; +use super::{ + BufBackedCellWriter as CellWriter, CellValueBuilder, CellWriter as _, SerializationError, +}; pub trait SerializeCql { /// Given a CQL type, checks if it _might_ be possible to serialize to that type. @@ -41,11 +44,11 @@ pub trait SerializeCql { /// /// The function may assume that `preliminary_type_check` was called, /// though it must not do anything unsafe if this assumption does not hold. - fn serialize( + fn serialize<'b>( &self, typ: &ColumnType, - writer: W, - ) -> Result; + writer: CellWriter<'b>, + ) -> Result, SerializationError>; } macro_rules! impl_exact_preliminary_type_check { @@ -69,11 +72,11 @@ macro_rules! impl_serialize_via_writer { impl_serialize_via_writer!(|$me, _typ, $writer| $e); }; (|$me:ident, $typ:ident, $writer:ident| $e:expr) => { - fn serialize( + fn serialize<'b>( &self, typ: &ColumnType, - writer: W, - ) -> Result { + writer: CellWriter<'b>, + ) -> Result, SerializationError> { let $writer = writer; let $typ = typ; let $me = self; @@ -182,11 +185,11 @@ impl SerializeCql for Secret { fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { V::preliminary_type_check(typ) } - fn serialize( + fn serialize<'b>( &self, typ: &ColumnType, - writer: W, - ) -> Result { + writer: CellWriter<'b>, + ) -> Result, SerializationError> { V::serialize(self.expose_secret(), typ, writer) } } @@ -270,11 +273,11 @@ impl SerializeCql for Option { fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { T::preliminary_type_check(typ) } - fn serialize( + fn serialize<'b>( &self, typ: &ColumnType, - writer: W, - ) -> Result { + writer: CellWriter<'b>, + ) -> Result, SerializationError> { match self { Some(v) => v.serialize(typ, writer), None => Ok(writer.set_null()), @@ -308,11 +311,11 @@ impl SerializeCql for MaybeUnset { fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { V::preliminary_type_check(typ) } - fn serialize( + fn serialize<'b>( &self, typ: &ColumnType, - writer: W, - ) -> Result { + writer: CellWriter<'b>, + ) -> Result, SerializationError> { match self { MaybeUnset::Set(v) => v.serialize(typ, writer), MaybeUnset::Unset => Ok(writer.set_unset()), @@ -323,11 +326,11 @@ impl SerializeCql for &T { fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { T::preliminary_type_check(typ) } - fn serialize( + fn serialize<'b>( &self, typ: &ColumnType, - writer: W, - ) -> Result { + writer: CellWriter<'b>, + ) -> Result, SerializationError> { T::serialize(*self, typ, writer) } } @@ -335,11 +338,11 @@ impl SerializeCql for Box { fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { T::preliminary_type_check(typ) } - fn serialize( + fn serialize<'b>( &self, typ: &ColumnType, - writer: W, - ) -> Result { + writer: CellWriter<'b>, + ) -> Result, SerializationError> { T::serialize(&**self, typ, writer) } } @@ -359,11 +362,11 @@ impl SerializeCql for HashSet { } } - fn serialize( + fn serialize<'b>( &self, typ: &ColumnType, - writer: W, - ) -> Result { + writer: CellWriter<'b>, + ) -> Result, SerializationError> { serialize_sequence( std::any::type_name::(), self.len(), @@ -389,11 +392,11 @@ impl SerializeCql for HashMap< } } - fn serialize( + fn serialize<'b>( &self, typ: &ColumnType, - writer: W, - ) -> Result { + writer: CellWriter<'b>, + ) -> Result, SerializationError> { serialize_mapping( std::any::type_name::(), self.len(), @@ -419,11 +422,11 @@ impl SerializeCql for BTreeSet { } } - fn serialize( + fn serialize<'b>( &self, typ: &ColumnType, - writer: W, - ) -> Result { + writer: CellWriter<'b>, + ) -> Result, SerializationError> { serialize_sequence( std::any::type_name::(), self.len(), @@ -449,11 +452,11 @@ impl SerializeCql for BTreeMap { } } - fn serialize( + fn serialize<'b>( &self, typ: &ColumnType, - writer: W, - ) -> Result { + writer: CellWriter<'b>, + ) -> Result, SerializationError> { serialize_mapping( std::any::type_name::(), self.len(), @@ -481,11 +484,11 @@ impl SerializeCql for Vec { } } - fn serialize( + fn serialize<'b>( &self, typ: &ColumnType, - writer: W, - ) -> Result { + writer: CellWriter<'b>, + ) -> Result, SerializationError> { serialize_sequence( std::any::type_name::(), self.len(), @@ -513,11 +516,11 @@ impl<'a, T: SerializeCql + 'a> SerializeCql for &'a [T] { } } - fn serialize( + fn serialize<'b>( &self, typ: &ColumnType, - writer: W, - ) -> Result { + writer: CellWriter<'b>, + ) -> Result, SerializationError> { serialize_sequence( std::any::type_name::(), self.len(), @@ -538,20 +541,20 @@ impl SerializeCql for CqlValue { } } - fn serialize( + fn serialize<'b>( &self, typ: &ColumnType, - writer: W, - ) -> Result { + writer: CellWriter<'b>, + ) -> Result, SerializationError> { serialize_cql_value(self, typ, writer).map_err(fix_cql_value_name_in_err) } } -fn serialize_cql_value( +fn serialize_cql_value<'b>( value: &CqlValue, typ: &ColumnType, - writer: W, -) -> Result { + writer: CellWriter<'b>, +) -> Result, SerializationError> { match value { CqlValue::Ascii(a) => check_and_serialize(a, typ, writer), CqlValue::Boolean(b) => check_and_serialize(b, typ, writer), @@ -666,22 +669,22 @@ fn fix_cql_value_name_in_err(mut err: SerializationError) -> SerializationError err } -fn check_and_serialize( +fn check_and_serialize<'b, V: SerializeCql>( v: &V, typ: &ColumnType, - writer: W, -) -> Result { + writer: CellWriter<'b>, +) -> Result, SerializationError> { V::preliminary_type_check(typ)?; v.serialize(typ, writer) } -fn serialize_udt( +fn serialize_udt<'b>( typ: &ColumnType, keyspace: &str, type_name: &str, values: &[(String, Option)], - writer: W, -) -> Result { + writer: CellWriter<'b>, +) -> Result, SerializationError> { let (dst_type_name, dst_keyspace, field_types) = match typ { ColumnType::UserDefinedType { type_name, @@ -747,12 +750,12 @@ fn serialize_udt( .map_err(|_| mk_ser_err::(typ, BuiltinSerializationErrorKind::SizeOverflow)) } -fn serialize_tuple_like<'t, W: CellWriter>( +fn serialize_tuple_like<'t, 'b>( typ: &ColumnType, field_types: impl Iterator, field_values: impl Iterator>, - writer: W, -) -> Result { + writer: CellWriter<'b>, +) -> Result, SerializationError> { let mut builder = writer.into_value_builder(); for (index, (el, typ)) in field_values.zip(field_types).enumerate() { @@ -818,11 +821,11 @@ macro_rules! impl_tuple { Ok(()) } - fn serialize( + fn serialize<'b>( &self, typ: &ColumnType, - writer: W, - ) -> Result { + writer: CellWriter<'b>, + ) -> Result, SerializationError> { let ($($tidents,)*) = match typ { ColumnType::Tuple(typs) => match typs.as_slice() { [$($tidents),*] => ($($tidents,)*), @@ -892,13 +895,13 @@ impl_tuples!( 16 ); -fn serialize_sequence<'t, T: SerializeCql + 't, W: CellWriter>( +fn serialize_sequence<'t, 'b, T: SerializeCql + 't>( rust_name: &'static str, len: usize, iter: impl Iterator, typ: &ColumnType, - writer: W, -) -> Result { + writer: CellWriter<'b>, +) -> Result, SerializationError> { let elt = match typ { ColumnType::List(elt) | ColumnType::Set(elt) => elt, _ => { @@ -936,13 +939,13 @@ fn serialize_sequence<'t, T: SerializeCql + 't, W: CellWriter>( .map_err(|_| mk_ser_err_named(rust_name, typ, BuiltinSerializationErrorKind::SizeOverflow)) } -fn serialize_mapping<'t, K: SerializeCql + 't, V: SerializeCql + 't, W: CellWriter>( +fn serialize_mapping<'t, 'b, K: SerializeCql + 't, V: SerializeCql + 't>( rust_name: &'static str, len: usize, iter: impl Iterator, typ: &ColumnType, - writer: W, -) -> Result { + writer: CellWriter<'b>, +) -> Result, SerializationError> { let (ktyp, vtyp) = match typ { ColumnType::Map(k, v) => (k, v), _ => { @@ -1009,7 +1012,7 @@ fn serialize_mapping<'t, K: SerializeCql + 't, V: SerializeCql + 't, W: CellWrit /// # use scylla_cql::impl_serialize_cql_via_value; /// struct NoGenerics {} /// impl Value for NoGenerics { -/// fn serialize(&self, _buf: &mut Vec) -> Result<(), ValueTooBig> { +/// fn serialize<'b>(&self, _buf: &mut Vec) -> Result<(), ValueTooBig> { /// Ok(()) /// } /// } @@ -1019,7 +1022,7 @@ fn serialize_mapping<'t, K: SerializeCql + 't, V: SerializeCql + 't, W: CellWrit /// // struct/enum contains any. /// struct WithGenerics(T, U); /// impl Value for WithGenerics { -/// fn serialize(&self, buf: &mut Vec) -> Result<(), ValueTooBig> { +/// fn serialize<'b>(&self, buf: &mut Vec) -> Result<(), ValueTooBig> { /// self.0.serialize(buf)?; /// self.1.clone().serialize(buf)?; /// Ok(()) @@ -1042,12 +1045,12 @@ macro_rules! impl_serialize_cql_via_value { ::std::result::Result::Ok(()) } - fn serialize( + fn serialize<'b>( &self, _typ: &$crate::frame::response::result::ColumnType, - writer: W, + writer: $crate::types::serialize::writers::BufBackedCellWriter<'b>, ) -> ::std::result::Result< - W::WrittenCellProof, + $crate::types::serialize::writers::WrittenCellProof<'b>, $crate::types::serialize::SerializationError, > { $crate::types::serialize::value::serialize_legacy_value(self, writer) @@ -1069,10 +1072,10 @@ macro_rules! impl_serialize_cql_via_value { /// /// See [`impl_serialize_cql_via_value`] which generates a boilerplate /// [`SerializeCql`] implementation that uses this function. -pub fn serialize_legacy_value( +pub fn serialize_legacy_value<'b, T: Value>( v: &T, - writer: W, -) -> Result { + writer: CellWriter<'b>, +) -> Result, SerializationError> { // It's an inefficient and slightly tricky but correct implementation. let mut buf = Vec::new(); ::serialize(v, &mut buf).map_err(|err| SerializationError(Arc::new(err)))?; From d37b2c4eb48f17d65bce7f49707a7cbc1c722a84 Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Thu, 7 Dec 2023 12:36:11 +0100 Subject: [PATCH 06/19] serialize/writers: replace writer traits with buffer-backed impls Remove RowWriter/CellWriter/CellValueBuilder traits and rename their buffer-backed implementations to take their place as structs. Fix the existing direct uses of buf-backed structs to refer to them by their new name. --- scylla-cql/src/frame/value_tests.rs | 10 +- scylla-cql/src/types/serialize/mod.rs | 5 +- scylla-cql/src/types/serialize/row.rs | 10 +- scylla-cql/src/types/serialize/value.rs | 10 +- scylla-cql/src/types/serialize/writers.rs | 214 ++++++++-------------- 5 files changed, 88 insertions(+), 161 deletions(-) diff --git a/scylla-cql/src/frame/value_tests.rs b/scylla-cql/src/frame/value_tests.rs index 280d5d055b..ecb678ecae 100644 --- a/scylla-cql/src/frame/value_tests.rs +++ b/scylla-cql/src/frame/value_tests.rs @@ -1,7 +1,7 @@ use crate::frame::{response::result::CqlValue, types::RawValue, value::BatchValuesIterator}; use crate::types::serialize::row::{RowSerializationContext, SerializeRow}; use crate::types::serialize::value::SerializeCql; -use crate::types::serialize::{BufBackedCellWriter, BufBackedRowWriter}; +use crate::types::serialize::{CellWriter, RowWriter}; use super::response::result::{ColumnSpec, ColumnType, TableSpec}; use super::value::{ @@ -27,7 +27,7 @@ where T::preliminary_type_check(&typ).unwrap(); let mut new_result: Vec = Vec::new(); - let writer = BufBackedCellWriter::new(&mut new_result); + let writer = CellWriter::new(&mut new_result); SerializeCql::serialize(&val, &typ, writer).unwrap(); assert_eq!(result, new_result); @@ -37,7 +37,7 @@ where fn serialized_only_new(val: T, typ: ColumnType) -> Vec { let mut result: Vec = Vec::new(); - let writer = BufBackedCellWriter::new(&mut result); + let writer = CellWriter::new(&mut result); SerializeCql::serialize(&val, &typ, writer).unwrap(); result } @@ -997,7 +997,7 @@ fn serialize_values( let ctx = RowSerializationContext { columns }; ::preliminary_type_check(&ctx).unwrap(); let mut new_serialized = vec![0, 0]; - let mut writer = BufBackedRowWriter::new(&mut new_serialized); + let mut writer = RowWriter::new(&mut new_serialized); ::serialize(&vl, &ctx, &mut writer).unwrap(); let value_count: u16 = writer.value_count().try_into().unwrap(); let is_empty = writer.value_count() == 0; @@ -1016,7 +1016,7 @@ fn serialize_values_only_new(vl: T, columns: &[ColumnSpec]) -> let ctx = RowSerializationContext { columns }; ::preliminary_type_check(&ctx).unwrap(); let mut serialized = vec![0, 0]; - let mut writer = BufBackedRowWriter::new(&mut serialized); + let mut writer = RowWriter::new(&mut serialized); ::serialize(&vl, &ctx, &mut writer).unwrap(); let value_count: u16 = writer.value_count().try_into().unwrap(); let is_empty = writer.value_count() == 0; diff --git a/scylla-cql/src/types/serialize/mod.rs b/scylla-cql/src/types/serialize/mod.rs index 5cb8cc37c0..230462759d 100644 --- a/scylla-cql/src/types/serialize/mod.rs +++ b/scylla-cql/src/types/serialize/mod.rs @@ -6,10 +6,7 @@ pub mod row; pub mod value; pub mod writers; -pub use writers::{ - BufBackedCellValueBuilder, BufBackedCellWriter, BufBackedRowWriter, CellValueBuilder, - CellWriter, RowWriter, -}; +pub use writers::{CellValueBuilder, CellWriter, RowWriter}; #[derive(Debug, Clone, Error)] pub struct SerializationError(Arc); diff --git a/scylla-cql/src/types/serialize/row.rs b/scylla-cql/src/types/serialize/row.rs index cc89c17607..0da2a4ac10 100644 --- a/scylla-cql/src/types/serialize/row.rs +++ b/scylla-cql/src/types/serialize/row.rs @@ -10,7 +10,7 @@ use crate::frame::value::{SerializedValues, ValueList}; use crate::frame::{response::result::ColumnSpec, types::RawValue}; use super::value::SerializeCql; -use super::{BufBackedRowWriter as RowWriter, CellWriter, RowWriter as _, SerializationError}; +use super::{RowWriter, SerializationError}; /// Contains information needed to serialize a row. pub struct RowSerializationContext<'a> { @@ -455,7 +455,7 @@ macro_rules! impl_serialize_row_via_value_list { fn serialize( &self, ctx: &$crate::types::serialize::row::RowSerializationContext<'_>, - writer: &mut $crate::types::serialize::writers::BufBackedRowWriter, + writer: &mut $crate::types::serialize::writers::RowWriter, ) -> ::std::result::Result<(), $crate::types::serialize::SerializationError> { $crate::types::serialize::row::serialize_legacy_row(self, ctx, writer) } @@ -660,7 +660,7 @@ pub enum ValueListToSerializeRowAdapterError { mod tests { use crate::frame::response::result::{ColumnSpec, ColumnType, TableSpec}; use crate::frame::value::{MaybeUnset, SerializedValues, ValueList}; - use crate::types::serialize::BufBackedRowWriter; + use crate::types::serialize::RowWriter; use super::{RowSerializationContext, SerializeRow}; @@ -688,7 +688,7 @@ mod tests { <_ as ValueList>::write_to_request(&row, &mut legacy_data).unwrap(); let mut new_data = Vec::new(); - let mut new_data_writer = BufBackedRowWriter::new(&mut new_data); + let mut new_data_writer = RowWriter::new(&mut new_data); let ctx = RowSerializationContext { columns: &[ col_spec("a", ColumnType::Int), @@ -725,7 +725,7 @@ mod tests { unsorted_row.add_named_value("c", &None::).unwrap(); let mut unsorted_row_data = Vec::new(); - let mut unsorted_row_data_writer = BufBackedRowWriter::new(&mut unsorted_row_data); + let mut unsorted_row_data_writer = RowWriter::new(&mut unsorted_row_data); let ctx = RowSerializationContext { columns: &[ col_spec("a", ColumnType::Int), diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index 121eccf3ab..28754d9a11 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -25,9 +25,7 @@ use crate::frame::value::{ use crate::frame::value::ValueOverflow; use super::writers::WrittenCellProof; -use super::{ - BufBackedCellWriter as CellWriter, CellValueBuilder, CellWriter as _, SerializationError, -}; +use super::{CellWriter, SerializationError}; pub trait SerializeCql { /// Given a CQL type, checks if it _might_ be possible to serialize to that type. @@ -1048,7 +1046,7 @@ macro_rules! impl_serialize_cql_via_value { fn serialize<'b>( &self, _typ: &$crate::frame::response::result::ColumnType, - writer: $crate::types::serialize::writers::BufBackedCellWriter<'b>, + writer: $crate::types::serialize::writers::CellWriter<'b>, ) -> ::std::result::Result< $crate::types::serialize::writers::WrittenCellProof<'b>, $crate::types::serialize::SerializationError, @@ -1576,7 +1574,7 @@ pub enum ValueToSerializeCqlAdapterError { mod tests { use crate::frame::response::result::ColumnType; use crate::frame::value::{MaybeUnset, Value}; - use crate::types::serialize::BufBackedCellWriter; + use crate::types::serialize::CellWriter; use super::SerializeCql; @@ -1585,7 +1583,7 @@ mod tests { ::serialize(&v, &mut legacy_data).unwrap(); let mut new_data = Vec::new(); - let new_data_writer = BufBackedCellWriter::new(&mut new_data); + let new_data_writer = CellWriter::new(&mut new_data); ::serialize(&v, &ColumnType::Int, new_data_writer).unwrap(); assert_eq!(legacy_data, new_data); diff --git a/scylla-cql/src/types/serialize/writers.rs b/scylla-cql/src/types/serialize/writers.rs index 59f564ad53..b42c8a7da1 100644 --- a/scylla-cql/src/types/serialize/writers.rs +++ b/scylla-cql/src/types/serialize/writers.rs @@ -3,14 +3,22 @@ use thiserror::Error; /// An interface that facilitates writing values for a CQL query. -pub trait RowWriter { - type CellWriter<'a>: CellWriter - where - Self: 'a; +pub struct RowWriter<'buf> { + // Buffer that this value should be serialized to. + buf: &'buf mut Vec, + // Number of values written so far. + value_count: usize, +} + +impl<'buf> RowWriter<'buf> { /// Appends a new value to the sequence and returns an object that allows /// to fill it in. - fn make_cell_writer(&mut self) -> Self::CellWriter<'_>; + #[inline] + pub fn make_cell_writer(&mut self) -> CellWriter<'_> { + self.value_count += 1; + CellWriter::new(self.buf) + } } /// Represents a handle to a CQL value that needs to be written into. @@ -19,11 +27,11 @@ pub trait RowWriter { /// (via [`set_null`](CellWriter::set_null), /// [`set_unset`](CellWriter::set_unset) /// or [`set_value`](CellWriter::set_value) or transformed into -/// the [`CellWriter::ValueBuilder`] in order to gradually initialize +/// the [`CellValueBuilder`] in order to gradually initialize /// the value when the contents are not available straight away. /// /// After the value is fully initialized, the handle is consumed and -/// a [`WrittenCellProof`](CellWriter::WrittenCellProof) object is returned +/// a [`WrittenCellProof`] object is returned /// in its stead. This is a type-level proof that the value was fully initialized /// and is used in [`SerializeCql::serialize`](`super::value::SerializeCql::serialize`) /// in order to enforce the implementor to fully initialize the provided handle @@ -31,33 +39,24 @@ pub trait RowWriter { /// /// Dropping this type without calling any of its methods will result /// in nothing being written. -pub trait CellWriter { - /// The type of the value builder, returned by the [`CellWriter::set_value`] - /// method. - type ValueBuilder: CellValueBuilder; - - /// An object that serves as a proof that the cell was fully initialized. - /// - /// This type is returned by [`set_null`](CellWriter::set_null), - /// [`set_unset`](CellWriter::set_unset), - /// [`set_value`](CellWriter::set_value) - /// and also [`CellValueBuilder::finish`] - generally speaking, after - /// the value is fully initialized and the `CellWriter` is destroyed. - /// - /// The purpose of this type is to enforce the contract of - /// [`SerializeCql::serialize`](super::value::SerializeCql::serialize): either - /// the method succeeds and returns a proof that it serialized itself - /// into the given value, or it fails and returns an error or panics. - /// The exact type of [`WrittenCellProof`](CellWriter::WrittenCellProof) - /// is not important as the value is not used at all - it's only - /// a compile-time check. - type WrittenCellProof; +pub struct CellWriter<'buf> { + buf: &'buf mut Vec, +} +impl<'buf> CellWriter<'buf> { /// Sets this value to be null, consuming this object. - fn set_null(self) -> Self::WrittenCellProof; + #[inline] + pub fn set_null(self) -> WrittenCellProof<'buf> { + self.buf.extend_from_slice(&(-1i32).to_be_bytes()); + WrittenCellProof::new() + } /// Sets this value to represent an unset value, consuming this object. - fn set_unset(self) -> Self::WrittenCellProof; + #[inline] + pub fn set_unset(self) -> WrittenCellProof<'buf> { + self.buf.extend_from_slice(&(-2i32).to_be_bytes()); + WrittenCellProof::new() + } /// Sets this value to a non-zero, non-unset value with given contents. /// @@ -67,7 +66,13 @@ pub trait CellWriter { /// /// Fails if the contents size overflows the maximum allowed CQL cell size /// (which is i32::MAX). - fn set_value(self, contents: &[u8]) -> Result; + #[inline] + pub fn set_value(self, contents: &[u8]) -> Result, CellOverflowError> { + let value_len: i32 = contents.len().try_into().map_err(|_| CellOverflowError)?; + self.buf.extend_from_slice(&value_len.to_be_bytes()); + self.buf.extend_from_slice(contents); + Ok(WrittenCellProof::new()) + } /// Turns this writter into a [`CellValueBuilder`] which can be used /// to gradually initialize the CQL value. @@ -75,7 +80,10 @@ pub trait CellWriter { /// This method should be used if you don't have all of the data /// up front, e.g. when serializing compound types such as collections /// or UDTs. - fn into_value_builder(self) -> Self::ValueBuilder; + #[inline] + pub fn into_value_builder(self) -> CellValueBuilder<'buf> { + CellValueBuilder::new(self.buf) + } } /// Allows appending bytes to a non-null, non-unset cell. @@ -84,25 +92,41 @@ pub trait CellWriter { /// serialized. Failing to drop this value will result in a payload that will /// not be parsed by the database correctly, but otherwise should not cause /// data to be misinterpreted. -pub trait CellValueBuilder { - type SubCellWriter<'a>: CellWriter - where - Self: 'a; +pub struct CellValueBuilder<'buf> { + // Buffer that this value should be serialized to. + buf: &'buf mut Vec, - type WrittenCellProof; + // Starting position of the value in the buffer. + starting_pos: usize, +} +impl<'buf> CellValueBuilder<'buf> { /// Appends raw bytes to this cell. - fn append_bytes(&mut self, bytes: &[u8]); + #[inline] + pub fn append_bytes(&mut self, bytes: &[u8]) { + self.buf.extend_from_slice(bytes); + } /// Appends a sub-value to the end of the current contents of the cell /// and returns an object that allows to fill it in. - fn make_sub_writer(&mut self) -> Self::SubCellWriter<'_>; + #[inline] + pub fn make_sub_writer(&mut self) -> CellWriter<'_> { + CellWriter::new(self.buf) + } /// Finishes serializing the value. /// /// Fails if the constructed cell size overflows the maximum allowed /// CQL cell size (which is i32::MAX). - fn finish(self) -> Result; + #[inline] + pub fn finish(self) -> Result, CellOverflowError> { + let value_len: i32 = (self.buf.len() - self.starting_pos - 4) + .try_into() + .map_err(|_| CellOverflowError)?; + self.buf[self.starting_pos..self.starting_pos + 4] + .copy_from_slice(&value_len.to_be_bytes()); + Ok(WrittenCellProof::new()) + } } /// An object that indicates a type-level proof that something was written @@ -143,16 +167,7 @@ impl<'buf> WrittenCellProof<'buf> { #[error("CQL cell overflowed the maximum allowed size of 2^31 - 1")] pub struct CellOverflowError; -/// A row writer backed by a buffer (vec). -pub struct BufBackedRowWriter<'buf> { - // Buffer that this value should be serialized to. - buf: &'buf mut Vec, - - // Number of values written so far. - value_count: usize, -} - -impl<'buf> BufBackedRowWriter<'buf> { +impl<'buf> RowWriter<'buf> { /// Creates a new row writer based on an existing Vec. /// /// The newly created row writer will append data to the end of the vec. @@ -174,72 +189,17 @@ impl<'buf> BufBackedRowWriter<'buf> { } } -impl<'buf> RowWriter for BufBackedRowWriter<'buf> { - type CellWriter<'a> = BufBackedCellWriter<'a> where Self: 'a; - - #[inline] - fn make_cell_writer(&mut self) -> Self::CellWriter<'_> { - self.value_count += 1; - BufBackedCellWriter::new(self.buf) - } -} - -/// A cell writer backed by a buffer (vec). -pub struct BufBackedCellWriter<'buf> { - buf: &'buf mut Vec, -} - -impl<'buf> BufBackedCellWriter<'buf> { +impl<'buf> CellWriter<'buf> { /// Creates a new cell writer based on an existing Vec. /// /// The newly created row writer will append data to the end of the vec. #[inline] pub fn new(buf: &'buf mut Vec) -> Self { - BufBackedCellWriter { buf } + Self { buf } } } -impl<'buf> CellWriter for BufBackedCellWriter<'buf> { - type ValueBuilder = BufBackedCellValueBuilder<'buf>; - - type WrittenCellProof = WrittenCellProof<'buf>; - - #[inline] - fn set_null(self) -> Self::WrittenCellProof { - self.buf.extend_from_slice(&(-1i32).to_be_bytes()); - WrittenCellProof::new() - } - - #[inline] - fn set_unset(self) -> Self::WrittenCellProof { - self.buf.extend_from_slice(&(-2i32).to_be_bytes()); - WrittenCellProof::new() - } - - #[inline] - fn set_value(self, bytes: &[u8]) -> Result { - let value_len: i32 = bytes.len().try_into().map_err(|_| CellOverflowError)?; - self.buf.extend_from_slice(&value_len.to_be_bytes()); - self.buf.extend_from_slice(bytes); - Ok(WrittenCellProof::new()) - } - - #[inline] - fn into_value_builder(self) -> Self::ValueBuilder { - BufBackedCellValueBuilder::new(self.buf) - } -} - -/// A cell value builder backed by a buffer (vec). -pub struct BufBackedCellValueBuilder<'buf> { - // Buffer that this value should be serialized to. - buf: &'buf mut Vec, - - // Starting position of the value in the buffer. - starting_pos: usize, -} - -impl<'buf> BufBackedCellValueBuilder<'buf> { +impl<'buf> CellValueBuilder<'buf> { #[inline] fn new(buf: &'buf mut Vec) -> Self { // "Length" of a [bytes] frame can either be a non-negative i32, @@ -250,46 +210,18 @@ impl<'buf> BufBackedCellValueBuilder<'buf> { // won't be misinterpreted. let starting_pos = buf.len(); buf.extend_from_slice(&(-3i32).to_be_bytes()); - BufBackedCellValueBuilder { buf, starting_pos } - } -} - -impl<'buf> CellValueBuilder for BufBackedCellValueBuilder<'buf> { - type SubCellWriter<'a> = BufBackedCellWriter<'a> - where - Self: 'a; - - type WrittenCellProof = WrittenCellProof<'buf>; - - #[inline] - fn append_bytes(&mut self, bytes: &[u8]) { - self.buf.extend_from_slice(bytes); - } - - #[inline] - fn make_sub_writer(&mut self) -> Self::SubCellWriter<'_> { - BufBackedCellWriter::new(self.buf) - } - - #[inline] - fn finish(self) -> Result { - let value_len: i32 = (self.buf.len() - self.starting_pos - 4) - .try_into() - .map_err(|_| CellOverflowError)?; - self.buf[self.starting_pos..self.starting_pos + 4] - .copy_from_slice(&value_len.to_be_bytes()); - Ok(WrittenCellProof::new()) + Self { buf, starting_pos } } } #[cfg(test)] mod tests { - use super::{BufBackedCellWriter, BufBackedRowWriter, CellValueBuilder, CellWriter, RowWriter}; + use super::{CellWriter, RowWriter}; #[test] fn test_cell_writer() { let mut data = Vec::new(); - let writer = BufBackedCellWriter::new(&mut data); + let writer = CellWriter::new(&mut data); let mut sub_writer = writer.into_value_builder(); sub_writer.make_sub_writer().set_null(); sub_writer @@ -313,7 +245,7 @@ mod tests { #[test] fn test_poisoned_appender() { let mut data = Vec::new(); - let writer = BufBackedCellWriter::new(&mut data); + let writer = CellWriter::new(&mut data); let _ = writer.into_value_builder(); assert_eq!( @@ -327,7 +259,7 @@ mod tests { #[test] fn test_row_writer() { let mut data = Vec::new(); - let mut writer = BufBackedRowWriter::new(&mut data); + let mut writer = RowWriter::new(&mut data); writer.make_cell_writer().set_null(); writer.make_cell_writer().set_value(&[1, 2, 3, 4]).unwrap(); writer.make_cell_writer().set_unset(); From 458c4de10e752a006a04d7e24fe72a7de0f6ba0d Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Thu, 7 Dec 2023 12:37:21 +0100 Subject: [PATCH 07/19] serialze/writers: move `new`/`value_count` methods higher Put them in the same `impl` block as other methods of the same structs for improved visual clarity. --- scylla-cql/src/types/serialize/writers.rs | 88 +++++++++++------------ 1 file changed, 41 insertions(+), 47 deletions(-) diff --git a/scylla-cql/src/types/serialize/writers.rs b/scylla-cql/src/types/serialize/writers.rs index b42c8a7da1..4d350adc75 100644 --- a/scylla-cql/src/types/serialize/writers.rs +++ b/scylla-cql/src/types/serialize/writers.rs @@ -12,6 +12,26 @@ pub struct RowWriter<'buf> { } impl<'buf> RowWriter<'buf> { + /// Creates a new row writer based on an existing Vec. + /// + /// The newly created row writer will append data to the end of the vec. + #[inline] + pub fn new(buf: &'buf mut Vec) -> Self { + Self { + buf, + value_count: 0, + } + } + + /// Returns the number of values that were written so far. + /// + /// Note that the protocol allows at most u16::MAX to be written into a query, + /// but the writer's interface allows more to be written. + #[inline] + pub fn value_count(&self) -> usize { + self.value_count + } + /// Appends a new value to the sequence and returns an object that allows /// to fill it in. #[inline] @@ -44,6 +64,14 @@ pub struct CellWriter<'buf> { } impl<'buf> CellWriter<'buf> { + /// Creates a new cell writer based on an existing Vec. + /// + /// The newly created row writer will append data to the end of the vec. + #[inline] + pub fn new(buf: &'buf mut Vec) -> Self { + Self { buf } + } + /// Sets this value to be null, consuming this object. #[inline] pub fn set_null(self) -> WrittenCellProof<'buf> { @@ -101,6 +129,19 @@ pub struct CellValueBuilder<'buf> { } impl<'buf> CellValueBuilder<'buf> { + #[inline] + fn new(buf: &'buf mut Vec) -> Self { + // "Length" of a [bytes] frame can either be a non-negative i32, + // -1 (null) or -1 (not set). Push an invalid value here. It will be + // overwritten eventually either by set_null, set_unset or Drop. + // If the CellSerializer is not dropped as it should, this will trigger + // an error on the DB side and the serialized data + // won't be misinterpreted. + let starting_pos = buf.len(); + buf.extend_from_slice(&(-3i32).to_be_bytes()); + Self { buf, starting_pos } + } + /// Appends raw bytes to this cell. #[inline] pub fn append_bytes(&mut self, bytes: &[u8]) { @@ -167,53 +208,6 @@ impl<'buf> WrittenCellProof<'buf> { #[error("CQL cell overflowed the maximum allowed size of 2^31 - 1")] pub struct CellOverflowError; -impl<'buf> RowWriter<'buf> { - /// Creates a new row writer based on an existing Vec. - /// - /// The newly created row writer will append data to the end of the vec. - #[inline] - pub fn new(buf: &'buf mut Vec) -> Self { - Self { - buf, - value_count: 0, - } - } - - /// Returns the number of values that were written so far. - /// - /// Note that the protocol allows at most u16::MAX to be written into a query, - /// but the writer's interface allows more to be written. - #[inline] - pub fn value_count(&self) -> usize { - self.value_count - } -} - -impl<'buf> CellWriter<'buf> { - /// Creates a new cell writer based on an existing Vec. - /// - /// The newly created row writer will append data to the end of the vec. - #[inline] - pub fn new(buf: &'buf mut Vec) -> Self { - Self { buf } - } -} - -impl<'buf> CellValueBuilder<'buf> { - #[inline] - fn new(buf: &'buf mut Vec) -> Self { - // "Length" of a [bytes] frame can either be a non-negative i32, - // -1 (null) or -1 (not set). Push an invalid value here. It will be - // overwritten eventually either by set_null, set_unset or Drop. - // If the CellSerializer is not dropped as it should, this will trigger - // an error on the DB side and the serialized data - // won't be misinterpreted. - let starting_pos = buf.len(); - buf.extend_from_slice(&(-3i32).to_be_bytes()); - Self { buf, starting_pos } - } -} - #[cfg(test)] mod tests { use super::{CellWriter, RowWriter}; From 9dc39fd7f3b87d92e0facccc079a673852a2daea Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Thu, 7 Dec 2023 18:32:42 +0100 Subject: [PATCH 08/19] serialize: provide default preliminary typecheck impls We are going to remove `preliminary_type_check` methods by moving the type checking logic to `serialize`. Add default `preliminary_type_check` impls in the trait definitions to make it possible to gradually remove their impls. --- scylla-cql/src/types/serialize/row.rs | 6 +++++- scylla-cql/src/types/serialize/value.rs | 4 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/scylla-cql/src/types/serialize/row.rs b/scylla-cql/src/types/serialize/row.rs index 0da2a4ac10..1fa06c7295 100644 --- a/scylla-cql/src/types/serialize/row.rs +++ b/scylla-cql/src/types/serialize/row.rs @@ -42,7 +42,11 @@ pub trait SerializeRow { /// Sometimes, a row cannot be fully type checked right away without knowing /// the exact values of the columns (e.g. when deserializing to `CqlValue`), /// but it's fine to do full type checking later in `serialize`. - fn preliminary_type_check(ctx: &RowSerializationContext<'_>) -> Result<(), SerializationError>; + fn preliminary_type_check( + _ctx: &RowSerializationContext<'_>, + ) -> Result<(), SerializationError> { + Ok(()) + } /// Serializes the row according to the information in the given context. /// diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index 28754d9a11..9056fb785f 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -36,7 +36,9 @@ pub trait SerializeCql { /// Some types cannot be type checked without knowing the exact value, /// this is the case e.g. for `CqlValue`. It's also fine to do it later in /// `serialize`. - fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError>; + fn preliminary_type_check(_typ: &ColumnType) -> Result<(), SerializationError> { + Ok(()) + } /// Serializes the value to given CQL type. /// From 6d7cc980467251375c85f645308c6558777774e1 Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Thu, 7 Dec 2023 18:45:46 +0100 Subject: [PATCH 09/19] serialize/value: move exact typechecks to serialize Moves the type checking logic of types that used `impl_exact_preliminary_type_check` into their `serialize` method. --- scylla-cql/src/types/serialize/value.rs | 120 +++++++++++++----------- 1 file changed, 67 insertions(+), 53 deletions(-) diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index 9056fb785f..b3e04566c2 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -51,18 +51,16 @@ pub trait SerializeCql { ) -> Result, SerializationError>; } -macro_rules! impl_exact_preliminary_type_check { - ($($cql:tt),*) => { - fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { - match typ { - $(ColumnType::$cql)|* => Ok(()), - _ => Err(mk_typck_err::( - typ, - BuiltinTypeCheckErrorKind::MismatchedType { - expected: &[$(ColumnType::$cql),*], - } - )) - } +macro_rules! exact_type_check { + ($typ:ident, $($cql:tt),*) => { + match $typ { + $(ColumnType::$cql)|* => {}, + _ => return Err(mk_typck_err::( + $typ, + BuiltinTypeCheckErrorKind::MismatchedType { + expected: &[$(ColumnType::$cql),*], + } + )) } }; } @@ -87,24 +85,32 @@ macro_rules! impl_serialize_via_writer { } impl SerializeCql for i8 { - impl_exact_preliminary_type_check!(TinyInt); - impl_serialize_via_writer!(|me, writer| writer.set_value(me.to_be_bytes().as_slice()).unwrap()); + impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, TinyInt); + writer.set_value(me.to_be_bytes().as_slice()).unwrap() + }); } impl SerializeCql for i16 { - impl_exact_preliminary_type_check!(SmallInt); - impl_serialize_via_writer!(|me, writer| writer.set_value(me.to_be_bytes().as_slice()).unwrap()); + impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, SmallInt); + writer.set_value(me.to_be_bytes().as_slice()).unwrap() + }); } impl SerializeCql for i32 { - impl_exact_preliminary_type_check!(Int); - impl_serialize_via_writer!(|me, writer| writer.set_value(me.to_be_bytes().as_slice()).unwrap()); + impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Int); + writer.set_value(me.to_be_bytes().as_slice()).unwrap() + }); } impl SerializeCql for i64 { - impl_exact_preliminary_type_check!(BigInt); - impl_serialize_via_writer!(|me, writer| writer.set_value(me.to_be_bytes().as_slice()).unwrap()); + impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, BigInt); + writer.set_value(me.to_be_bytes().as_slice()).unwrap() + }); } impl SerializeCql for BigDecimal { - impl_exact_preliminary_type_check!(Decimal); impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Decimal); let mut builder = writer.into_value_builder(); let (value, scale) = me.as_bigint_and_exponent(); let scale: i32 = scale @@ -118,41 +124,41 @@ impl SerializeCql for BigDecimal { }); } impl SerializeCql for CqlDate { - impl_exact_preliminary_type_check!(Date); - impl_serialize_via_writer!(|me, writer| { + impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Date); writer.set_value(me.0.to_be_bytes().as_slice()).unwrap() }); } impl SerializeCql for CqlTimestamp { - impl_exact_preliminary_type_check!(Timestamp); - impl_serialize_via_writer!(|me, writer| { + impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Timestamp); writer.set_value(me.0.to_be_bytes().as_slice()).unwrap() }); } impl SerializeCql for CqlTime { - impl_exact_preliminary_type_check!(Time); - impl_serialize_via_writer!(|me, writer| { + impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Time); writer.set_value(me.0.to_be_bytes().as_slice()).unwrap() }); } #[cfg(feature = "chrono")] impl SerializeCql for NaiveDate { - impl_exact_preliminary_type_check!(Date); impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Date); ::serialize(&(*me).into(), typ, writer)? }); } #[cfg(feature = "chrono")] impl SerializeCql for DateTime { - impl_exact_preliminary_type_check!(Timestamp); impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Timestamp); ::serialize(&(*me).into(), typ, writer)? }); } #[cfg(feature = "chrono")] impl SerializeCql for NaiveTime { - impl_exact_preliminary_type_check!(Time); impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Time); let cql_time = CqlTime::try_from(*me).map_err(|_: ValueOverflow| { mk_ser_err::(typ, BuiltinSerializationErrorKind::ValueOverflow) })?; @@ -161,22 +167,22 @@ impl SerializeCql for NaiveTime { } #[cfg(feature = "chrono")] impl SerializeCql for time::Date { - impl_exact_preliminary_type_check!(Date); impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Date); ::serialize(&(*me).into(), typ, writer)? }); } #[cfg(feature = "chrono")] impl SerializeCql for time::OffsetDateTime { - impl_exact_preliminary_type_check!(Timestamp); impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Timestamp); ::serialize(&(*me).into(), typ, writer)? }); } #[cfg(feature = "chrono")] impl SerializeCql for time::Time { - impl_exact_preliminary_type_check!(Time); impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Time); ::serialize(&(*me).into(), typ, writer)? }); } @@ -194,24 +200,32 @@ impl SerializeCql for Secret { } } impl SerializeCql for bool { - impl_exact_preliminary_type_check!(Boolean); - impl_serialize_via_writer!(|me, writer| writer.set_value(&[*me as u8]).unwrap()); + impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Boolean); + writer.set_value(&[*me as u8]).unwrap() + }); } impl SerializeCql for f32 { - impl_exact_preliminary_type_check!(Float); - impl_serialize_via_writer!(|me, writer| writer.set_value(me.to_be_bytes().as_slice()).unwrap()); + impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Float); + writer.set_value(me.to_be_bytes().as_slice()).unwrap() + }); } impl SerializeCql for f64 { - impl_exact_preliminary_type_check!(Double); - impl_serialize_via_writer!(|me, writer| writer.set_value(me.to_be_bytes().as_slice()).unwrap()); + impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Double); + writer.set_value(me.to_be_bytes().as_slice()).unwrap() + }); } impl SerializeCql for Uuid { - impl_exact_preliminary_type_check!(Uuid, Timeuuid); - impl_serialize_via_writer!(|me, writer| writer.set_value(me.as_bytes().as_ref()).unwrap()); + impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Uuid, Timeuuid); + writer.set_value(me.as_bytes().as_ref()).unwrap() + }); } impl SerializeCql for BigInt { - impl_exact_preliminary_type_check!(Varint); impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Varint); // TODO: The allocation here can be avoided and we can reimplement // `to_signed_bytes_be` by using `to_u64_digits` and a bit of custom // logic. Need better tests in order to do this. @@ -221,40 +235,40 @@ impl SerializeCql for BigInt { }); } impl SerializeCql for &str { - impl_exact_preliminary_type_check!(Ascii, Text); impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Ascii, Text); writer .set_value(me.as_bytes()) .map_err(|_| mk_ser_err::(typ, BuiltinSerializationErrorKind::SizeOverflow))? }); } impl SerializeCql for Vec { - impl_exact_preliminary_type_check!(Blob); impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Blob); writer .set_value(me.as_ref()) .map_err(|_| mk_ser_err::(typ, BuiltinSerializationErrorKind::SizeOverflow))? }); } impl SerializeCql for &[u8] { - impl_exact_preliminary_type_check!(Blob); impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Blob); writer .set_value(me) .map_err(|_| mk_ser_err::(typ, BuiltinSerializationErrorKind::SizeOverflow))? }); } impl SerializeCql for [u8; N] { - impl_exact_preliminary_type_check!(Blob); impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Blob); writer .set_value(me.as_ref()) .map_err(|_| mk_ser_err::(typ, BuiltinSerializationErrorKind::SizeOverflow))? }); } impl SerializeCql for IpAddr { - impl_exact_preliminary_type_check!(Inet); - impl_serialize_via_writer!(|me, writer| { + impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Inet); match me { IpAddr::V4(ip) => writer.set_value(&ip.octets()).unwrap(), IpAddr::V6(ip) => writer.set_value(&ip.octets()).unwrap(), @@ -262,8 +276,8 @@ impl SerializeCql for IpAddr { }); } impl SerializeCql for String { - impl_exact_preliminary_type_check!(Ascii, Text); impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Ascii, Text); writer .set_value(me.as_bytes()) .map_err(|_| mk_ser_err::(typ, BuiltinSerializationErrorKind::SizeOverflow))? @@ -291,14 +305,14 @@ impl SerializeCql for Unset { impl_serialize_via_writer!(|_me, writer| writer.set_unset()); } impl SerializeCql for Counter { - impl_exact_preliminary_type_check!(Counter); - impl_serialize_via_writer!(|me, writer| { + impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Counter); writer.set_value(me.0.to_be_bytes().as_slice()).unwrap() }); } impl SerializeCql for CqlDuration { - impl_exact_preliminary_type_check!(Duration); - impl_serialize_via_writer!(|me, writer| { + impl_serialize_via_writer!(|me, typ, writer| { + exact_type_check!(typ, Duration); // TODO: adjust vint_encode to use CellValueBuilder or something like that let mut buf = Vec::with_capacity(27); // worst case size is 27 vint_encode(me.months as i64, &mut buf); From cc0a1a0de906896f2aa0b05f9a53e7926061acbc Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Fri, 8 Dec 2023 03:36:38 +0100 Subject: [PATCH 10/19] serialize/value: move CqlValue's typecheck to serialize --- scylla-cql/src/types/serialize/value.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index b3e04566c2..20c3394540 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -545,14 +545,8 @@ impl<'a, T: SerializeCql + 'a> SerializeCql for &'a [T] { } } impl SerializeCql for CqlValue { - fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { - match typ { - ColumnType::Custom(_) => Err(mk_typck_err::( - typ, - BuiltinTypeCheckErrorKind::CustomTypeUnsupported, - )), - _ => Ok(()), - } + fn preliminary_type_check(_typ: &ColumnType) -> Result<(), SerializationError> { + Ok(()) } fn serialize<'b>( @@ -569,6 +563,12 @@ fn serialize_cql_value<'b>( typ: &ColumnType, writer: CellWriter<'b>, ) -> Result, SerializationError> { + if let ColumnType::Custom(_) = typ { + return Err(mk_typck_err::( + typ, + BuiltinTypeCheckErrorKind::CustomTypeUnsupported, + )); + } match value { CqlValue::Ascii(a) => check_and_serialize(a, typ, writer), CqlValue::Boolean(b) => check_and_serialize(b, typ, writer), From 69cb8f41720005b2b8babb63e39f5f2a35d970cc Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Fri, 8 Dec 2023 06:47:51 +0100 Subject: [PATCH 11/19] serialize/value: remove nested typecheck errors As we get rid of `preliminary_type_check`, error variants that were used to represent type check failure of some sub-type (e.g. a tuple element) will not be used anymore. Remove them now and adjust the rest of the code. --- scylla-cql/src/types/serialize/value.rs | 105 ++---------------------- 1 file changed, 8 insertions(+), 97 deletions(-) diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index 20c3394540..da4fd4e304 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -363,12 +363,7 @@ impl SerializeCql for Box { impl SerializeCql for HashSet { fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { match typ { - ColumnType::Set(elt) => V::preliminary_type_check(elt).map_err(|err| { - mk_typck_err::( - typ, - SetOrListTypeCheckErrorKind::ElementTypeCheckFailed(err), - ) - }), + ColumnType::Set(_) => Ok(()), _ => Err(mk_typck_err::( typ, SetOrListTypeCheckErrorKind::NotSetOrList, @@ -393,15 +388,7 @@ impl SerializeCql for HashSet { impl SerializeCql for HashMap { fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { match typ { - ColumnType::Map(k, v) => { - K::preliminary_type_check(k).map_err(|err| { - mk_typck_err::(typ, MapTypeCheckErrorKind::KeyTypeCheckFailed(err)) - })?; - V::preliminary_type_check(v).map_err(|err| { - mk_typck_err::(typ, MapTypeCheckErrorKind::ValueTypeCheckFailed(err)) - })?; - Ok(()) - } + ColumnType::Map(_, _) => Ok(()), _ => Err(mk_typck_err::(typ, MapTypeCheckErrorKind::NotMap)), } } @@ -423,12 +410,7 @@ impl SerializeCql for HashMap< impl SerializeCql for BTreeSet { fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { match typ { - ColumnType::Set(elt) => V::preliminary_type_check(elt).map_err(|err| { - mk_typck_err::( - typ, - SetOrListTypeCheckErrorKind::ElementTypeCheckFailed(err), - ) - }), + ColumnType::Set(_) => Ok(()), _ => Err(mk_typck_err::( typ, SetOrListTypeCheckErrorKind::NotSetOrList, @@ -453,15 +435,7 @@ impl SerializeCql for BTreeSet { impl SerializeCql for BTreeMap { fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { match typ { - ColumnType::Map(k, v) => { - K::preliminary_type_check(k).map_err(|err| { - mk_typck_err::(typ, MapTypeCheckErrorKind::KeyTypeCheckFailed(err)) - })?; - V::preliminary_type_check(v).map_err(|err| { - mk_typck_err::(typ, MapTypeCheckErrorKind::ValueTypeCheckFailed(err)) - })?; - Ok(()) - } + ColumnType::Map(_, _) => Ok(()), _ => Err(mk_typck_err::(typ, MapTypeCheckErrorKind::NotMap)), } } @@ -483,14 +457,7 @@ impl SerializeCql for BTreeMap { impl SerializeCql for Vec { fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { match typ { - ColumnType::List(elt) | ColumnType::Set(elt) => { - T::preliminary_type_check(elt).map_err(|err| { - mk_typck_err::( - typ, - SetOrListTypeCheckErrorKind::ElementTypeCheckFailed(err), - ) - }) - } + ColumnType::List(_) | ColumnType::Set(_) => Ok(()), _ => Err(mk_typck_err::( typ, SetOrListTypeCheckErrorKind::NotSetOrList, @@ -515,14 +482,7 @@ impl SerializeCql for Vec { impl<'a, T: SerializeCql + 'a> SerializeCql for &'a [T] { fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { match typ { - ColumnType::List(elt) | ColumnType::Set(elt) => { - T::preliminary_type_check(elt).map_err(|err| { - mk_typck_err::( - typ, - SetOrListTypeCheckErrorKind::ElementTypeCheckFailed(err), - ) - }) - } + ColumnType::List(_) | ColumnType::Set(_) => Ok(()), _ => Err(mk_typck_err::( typ, SetOrListTypeCheckErrorKind::NotSetOrList, @@ -803,21 +763,8 @@ macro_rules! impl_tuple { match typ { ColumnType::Tuple(typs) => match typs.as_slice() { [$($tidents),*, ..] => { - let index = 0; - $( - <$typs as SerializeCql>::preliminary_type_check($tidents) - .map_err(|err| - mk_typck_err::( - typ, - TupleTypeCheckErrorKind::ElementTypeCheckFailed { - index, - err, - } - ) - )?; - let index = index + 1; - )* - let _ = index; + // Suppress the "unused" warning + let _ = ($($tidents),*,); } _ => return Err(mk_typck_err::( typ, @@ -1341,12 +1288,6 @@ impl Display for BuiltinSerializationErrorKind { pub enum MapTypeCheckErrorKind { /// The CQL type is not a map. NotMap, - - /// Checking the map key type failed. - KeyTypeCheckFailed(SerializationError), - - /// Checking the map value type failed. - ValueTypeCheckFailed(SerializationError), } impl Display for MapTypeCheckErrorKind { @@ -1358,12 +1299,6 @@ impl Display for MapTypeCheckErrorKind { "the CQL type the map was attempted to be serialized to was not map" ) } - MapTypeCheckErrorKind::KeyTypeCheckFailed(err) => { - write!(f, "failed to type check one of the keys: {}", err) - } - MapTypeCheckErrorKind::ValueTypeCheckFailed(err) => { - write!(f, "failed to type check one of the values: {}", err) - } } } } @@ -1405,9 +1340,6 @@ impl Display for MapSerializationErrorKind { pub enum SetOrListTypeCheckErrorKind { /// The CQL type is neither a set not a list. NotSetOrList, - - /// Checking the type of the set/list element failed. - ElementTypeCheckFailed(SerializationError), } impl Display for SetOrListTypeCheckErrorKind { @@ -1419,9 +1351,6 @@ impl Display for SetOrListTypeCheckErrorKind { "the CQL type the tuple was attempted to was neither a set or a list" ) } - SetOrListTypeCheckErrorKind::ElementTypeCheckFailed(err) => { - write!(f, "failed to type check one of the elements: {err}") - } } } } @@ -1464,12 +1393,6 @@ pub enum TupleTypeCheckErrorKind { /// than the corresponding CQL type, but not more. The additional, unknown /// elements will be set to null. WrongElementCount { actual: usize, asked_for: usize }, - - /// One of the tuple elements failed to type check. - ElementTypeCheckFailed { - index: usize, - err: SerializationError, - }, } impl Display for TupleTypeCheckErrorKind { @@ -1483,9 +1406,6 @@ impl Display for TupleTypeCheckErrorKind { f, "wrong tuple element count: CQL type has {asked_for}, the Rust tuple has {actual}" ), - TupleTypeCheckErrorKind::ElementTypeCheckFailed { index, err } => { - write!(f, "element no. {index} failed to type check: {err}") - } } } } @@ -1521,12 +1441,6 @@ pub enum UdtTypeCheckErrorKind { /// The Rust data contains a field that is not present in the UDT UnexpectedFieldInDestination { field_name: String }, - - /// One of the fields failed to type check. - FieldTypeCheckFailed { - field_name: String, - err: SerializationError, - }, } impl Display for UdtTypeCheckErrorKind { @@ -1547,9 +1461,6 @@ impl Display for UdtTypeCheckErrorKind { f, "the field {field_name} present in the Rust data is not present in the CQL type" ), - UdtTypeCheckErrorKind::FieldTypeCheckFailed { field_name, err } => { - write!(f, "field {field_name} failed to type check: {err}") - } } } } From 53b73493c2659e8588c7466a50bf4513ff41bdec Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Fri, 8 Dec 2023 06:50:45 +0100 Subject: [PATCH 12/19] serialize/value: remove typechecks of compound types Type checks for compound types only check that the type is of the right shape (e.g. it's a UDT, a list, etc.). This is already checked in `serialize`, so remove the `preliminary_type_check` impls. --- scylla-cql/src/types/serialize/value.rs | 77 ------------------------- 1 file changed, 77 deletions(-) diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index da4fd4e304..433592aead 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -361,16 +361,6 @@ impl SerializeCql for Box { } } impl SerializeCql for HashSet { - fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { - match typ { - ColumnType::Set(_) => Ok(()), - _ => Err(mk_typck_err::( - typ, - SetOrListTypeCheckErrorKind::NotSetOrList, - )), - } - } - fn serialize<'b>( &self, typ: &ColumnType, @@ -386,13 +376,6 @@ impl SerializeCql for HashSet { } } impl SerializeCql for HashMap { - fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { - match typ { - ColumnType::Map(_, _) => Ok(()), - _ => Err(mk_typck_err::(typ, MapTypeCheckErrorKind::NotMap)), - } - } - fn serialize<'b>( &self, typ: &ColumnType, @@ -408,16 +391,6 @@ impl SerializeCql for HashMap< } } impl SerializeCql for BTreeSet { - fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { - match typ { - ColumnType::Set(_) => Ok(()), - _ => Err(mk_typck_err::( - typ, - SetOrListTypeCheckErrorKind::NotSetOrList, - )), - } - } - fn serialize<'b>( &self, typ: &ColumnType, @@ -433,13 +406,6 @@ impl SerializeCql for BTreeSet { } } impl SerializeCql for BTreeMap { - fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { - match typ { - ColumnType::Map(_, _) => Ok(()), - _ => Err(mk_typck_err::(typ, MapTypeCheckErrorKind::NotMap)), - } - } - fn serialize<'b>( &self, typ: &ColumnType, @@ -455,16 +421,6 @@ impl SerializeCql for BTreeMap { } } impl SerializeCql for Vec { - fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { - match typ { - ColumnType::List(_) | ColumnType::Set(_) => Ok(()), - _ => Err(mk_typck_err::( - typ, - SetOrListTypeCheckErrorKind::NotSetOrList, - )), - } - } - fn serialize<'b>( &self, typ: &ColumnType, @@ -480,16 +436,6 @@ impl SerializeCql for Vec { } } impl<'a, T: SerializeCql + 'a> SerializeCql for &'a [T] { - fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { - match typ { - ColumnType::List(_) | ColumnType::Set(_) => Ok(()), - _ => Err(mk_typck_err::( - typ, - SetOrListTypeCheckErrorKind::NotSetOrList, - )), - } - } - fn serialize<'b>( &self, typ: &ColumnType, @@ -759,29 +705,6 @@ macro_rules! impl_tuple { $length:expr ) => { impl<$($typs: SerializeCql),*> SerializeCql for ($($typs,)*) { - fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { - match typ { - ColumnType::Tuple(typs) => match typs.as_slice() { - [$($tidents),*, ..] => { - // Suppress the "unused" warning - let _ = ($($tidents),*,); - } - _ => return Err(mk_typck_err::( - typ, - TupleTypeCheckErrorKind::WrongElementCount { - actual: $length, - asked_for: typs.len(), - } - )) - } - _ => return Err(mk_typck_err::( - typ, - TupleTypeCheckErrorKind::NotTuple - )), - }; - Ok(()) - } - fn serialize<'b>( &self, typ: &ColumnType, From e8a92ad300814cbde55f59a78725f221502b275a Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Fri, 8 Dec 2023 06:51:01 +0100 Subject: [PATCH 13/19] serialize/value: remove remaining preliminary type check impls Remaining `SerializeCql::preliminary_type_check` impls can be removed because they either not do anything or call `preliminary_type_check` on other types. --- scylla-cql/src/types/serialize/value.rs | 29 ------------------------- 1 file changed, 29 deletions(-) diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index 433592aead..82f79c81a2 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -188,9 +188,6 @@ impl SerializeCql for time::Time { } #[cfg(feature = "secret")] impl SerializeCql for Secret { - fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { - V::preliminary_type_check(typ) - } fn serialize<'b>( &self, typ: &ColumnType, @@ -284,9 +281,6 @@ impl SerializeCql for String { }); } impl SerializeCql for Option { - fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { - T::preliminary_type_check(typ) - } fn serialize<'b>( &self, typ: &ColumnType, @@ -299,9 +293,6 @@ impl SerializeCql for Option { } } impl SerializeCql for Unset { - fn preliminary_type_check(_typ: &ColumnType) -> Result<(), SerializationError> { - Ok(()) // Fits everything - } impl_serialize_via_writer!(|_me, writer| writer.set_unset()); } impl SerializeCql for Counter { @@ -322,9 +313,6 @@ impl SerializeCql for CqlDuration { }); } impl SerializeCql for MaybeUnset { - fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { - V::preliminary_type_check(typ) - } fn serialize<'b>( &self, typ: &ColumnType, @@ -337,9 +325,6 @@ impl SerializeCql for MaybeUnset { } } impl SerializeCql for &T { - fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { - T::preliminary_type_check(typ) - } fn serialize<'b>( &self, typ: &ColumnType, @@ -349,9 +334,6 @@ impl SerializeCql for &T { } } impl SerializeCql for Box { - fn preliminary_type_check(typ: &ColumnType) -> Result<(), SerializationError> { - T::preliminary_type_check(typ) - } fn serialize<'b>( &self, typ: &ColumnType, @@ -451,10 +433,6 @@ impl<'a, T: SerializeCql + 'a> SerializeCql for &'a [T] { } } impl SerializeCql for CqlValue { - fn preliminary_type_check(_typ: &ColumnType) -> Result<(), SerializationError> { - Ok(()) - } - fn serialize<'b>( &self, typ: &ColumnType, @@ -922,13 +900,6 @@ macro_rules! impl_serialize_cql_via_value { where Self: $crate::frame::value::Value, { - fn preliminary_type_check( - _typ: &$crate::frame::response::result::ColumnType, - ) -> ::std::result::Result<(), $crate::types::serialize::SerializationError> { - // No-op - the old interface didn't offer type safety - ::std::result::Result::Ok(()) - } - fn serialize<'b>( &self, _typ: &$crate::frame::response::result::ColumnType, From 6e074d59d33314746754cf10493b724b70ddddf3 Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Fri, 8 Dec 2023 03:49:27 +0100 Subject: [PATCH 14/19] serialize/row: fix incorrect error being returned In case when serialization of one of the values fails, a row represented by a map would return a type check error. A serialization error should be returned instead. --- scylla-cql/src/types/serialize/row.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scylla-cql/src/types/serialize/row.rs b/scylla-cql/src/types/serialize/row.rs index 1fa06c7295..25398e880c 100644 --- a/scylla-cql/src/types/serialize/row.rs +++ b/scylla-cql/src/types/serialize/row.rs @@ -223,8 +223,8 @@ macro_rules! impl_serialize_row_for_map { Some(v) => { ::serialize(v, &col.typ, writer.make_cell_writer()) .map_err(|err| { - mk_typck_err::( - BuiltinTypeCheckErrorKind::ColumnTypeCheckFailed { + mk_ser_err::( + BuiltinSerializationErrorKind::ColumnSerializationFailed { name: col.name.clone(), err, }, From 5eddc08cd114f8f9a5bd009d63c061608c8c0f5c Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Fri, 8 Dec 2023 03:50:10 +0100 Subject: [PATCH 15/19] serialize/row: remove preliminary type check impls There's not many of them, so remove them all in one go. The ColumnTypeCheckFailed serialization error variant is also removed as it is no longer used. --- scylla-cql/src/types/serialize/row.rs | 93 +-------------------------- 1 file changed, 3 insertions(+), 90 deletions(-) diff --git a/scylla-cql/src/types/serialize/row.rs b/scylla-cql/src/types/serialize/row.rs index 25398e880c..72c0bf5a23 100644 --- a/scylla-cql/src/types/serialize/row.rs +++ b/scylla-cql/src/types/serialize/row.rs @@ -63,11 +63,6 @@ pub trait SerializeRow { macro_rules! fallback_impl_contents { () => { - fn preliminary_type_check( - _ctx: &RowSerializationContext<'_>, - ) -> Result<(), SerializationError> { - Ok(()) - } fn serialize( &self, ctx: &RowSerializationContext<'_>, @@ -84,8 +79,10 @@ macro_rules! fallback_impl_contents { macro_rules! impl_serialize_row_for_unit { () => { - fn preliminary_type_check( + fn serialize( + &self, ctx: &RowSerializationContext<'_>, + _writer: &mut RowWriter, ) -> Result<(), SerializationError> { if !ctx.columns().is_empty() { return Err(mk_typck_err::( @@ -95,14 +92,6 @@ macro_rules! impl_serialize_row_for_unit { }, )); } - Ok(()) - } - - fn serialize( - &self, - _ctx: &RowSerializationContext<'_>, - _writer: &mut RowWriter, - ) -> Result<(), SerializationError> { // Row is empty - do nothing Ok(()) } @@ -124,22 +113,6 @@ impl SerializeRow for [u8; 0] { macro_rules! impl_serialize_row_for_slice { () => { - fn preliminary_type_check( - ctx: &RowSerializationContext<'_>, - ) -> Result<(), SerializationError> { - // While we don't know how many columns will be there during serialization, - // we can at least check that all provided columns match T. - for col in ctx.columns() { - ::preliminary_type_check(&col.typ).map_err(|err| { - mk_typck_err::(BuiltinTypeCheckErrorKind::ColumnTypeCheckFailed { - name: col.name.clone(), - err, - }) - })?; - } - Ok(()) - } - fn serialize( &self, ctx: &RowSerializationContext<'_>, @@ -185,22 +158,6 @@ impl SerializeRow for Vec { macro_rules! impl_serialize_row_for_map { () => { - fn preliminary_type_check( - ctx: &RowSerializationContext<'_>, - ) -> Result<(), SerializationError> { - // While we don't know the column count or their names, - // we can go over all columns and check that their types match T. - for col in ctx.columns() { - ::preliminary_type_check(&col.typ).map_err(|err| { - mk_typck_err::(BuiltinTypeCheckErrorKind::ColumnTypeCheckFailed { - name: col.name.clone(), - err, - }) - })?; - } - Ok(()) - } - fn serialize( &self, ctx: &RowSerializationContext<'_>, @@ -272,10 +229,6 @@ impl SerializeRow for HashMap<&str, T, S> { } impl SerializeRow for &T { - fn preliminary_type_check(ctx: &RowSerializationContext<'_>) -> Result<(), SerializationError> { - ::preliminary_type_check(ctx) - } - fn serialize( &self, ctx: &RowSerializationContext<'_>, @@ -306,30 +259,6 @@ macro_rules! impl_tuple { $length:expr ) => { impl<$($typs: SerializeCql),*> SerializeRow for ($($typs,)*) { - fn preliminary_type_check( - ctx: &RowSerializationContext<'_>, - ) -> Result<(), SerializationError> { - match ctx.columns() { - [$($tidents),*] => { - $( - <$typs as SerializeCql>::preliminary_type_check(&$tidents.typ).map_err(|err| { - mk_typck_err::(BuiltinTypeCheckErrorKind::ColumnTypeCheckFailed { - name: $tidents.name.clone(), - err, - }) - })?; - )* - } - _ => return Err(mk_typck_err::( - BuiltinTypeCheckErrorKind::WrongColumnCount { - actual: $length, - asked_for: ctx.columns().len(), - }, - )), - }; - Ok(()) - } - fn serialize( &self, ctx: &RowSerializationContext<'_>, @@ -449,13 +378,6 @@ macro_rules! impl_serialize_row_via_value_list { where Self: $crate::frame::value::ValueList, { - fn preliminary_type_check( - _ctx: &$crate::types::serialize::row::RowSerializationContext<'_>, - ) -> ::std::result::Result<(), $crate::types::serialize::SerializationError> { - // No-op - the old interface didn't offer type safety - ::std::result::Result::Ok(()) - } - fn serialize( &self, ctx: &$crate::types::serialize::row::RowSerializationContext<'_>, @@ -600,12 +522,6 @@ pub enum BuiltinTypeCheckErrorKind { /// A value required by the statement is not provided by the Rust type. ColumnMissingForValue { name: String }, - - /// One of the columns failed to type check. - ColumnTypeCheckFailed { - name: String, - err: SerializationError, - }, } impl Display for BuiltinTypeCheckErrorKind { @@ -626,9 +542,6 @@ impl Display for BuiltinTypeCheckErrorKind { "value for column {name} was provided, but there is no bind marker for this column in the query" ) } - BuiltinTypeCheckErrorKind::ColumnTypeCheckFailed { name, err } => { - write!(f, "failed to check column {name}: {err}") - } } } } From 006dc3522cc4bddba9ddcee2a2c5a5e4291bee70 Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Fri, 8 Dec 2023 03:51:21 +0100 Subject: [PATCH 16/19] serialize: remove `preliminary_type_check` method As we removed all the non-default impls of `preliminary_type_check`, the method now does nothing. Remove it and all the remaining callers. --- scylla-cql/src/frame/value_tests.rs | 4 ---- scylla-cql/src/types/serialize/row.rs | 18 ------------------ scylla-cql/src/types/serialize/value.rs | 16 ---------------- 3 files changed, 38 deletions(-) diff --git a/scylla-cql/src/frame/value_tests.rs b/scylla-cql/src/frame/value_tests.rs index ecb678ecae..0ded4b4ed0 100644 --- a/scylla-cql/src/frame/value_tests.rs +++ b/scylla-cql/src/frame/value_tests.rs @@ -24,8 +24,6 @@ where let mut result: Vec = Vec::new(); Value::serialize(&val, &mut result).unwrap(); - T::preliminary_type_check(&typ).unwrap(); - let mut new_result: Vec = Vec::new(); let writer = CellWriter::new(&mut new_result); SerializeCql::serialize(&val, &typ, writer).unwrap(); @@ -995,7 +993,6 @@ fn serialize_values( serialized.write_to_request(&mut old_serialized); let ctx = RowSerializationContext { columns }; - ::preliminary_type_check(&ctx).unwrap(); let mut new_serialized = vec![0, 0]; let mut writer = RowWriter::new(&mut new_serialized); ::serialize(&vl, &ctx, &mut writer).unwrap(); @@ -1014,7 +1011,6 @@ fn serialize_values( fn serialize_values_only_new(vl: T, columns: &[ColumnSpec]) -> Vec { let ctx = RowSerializationContext { columns }; - ::preliminary_type_check(&ctx).unwrap(); let mut serialized = vec![0, 0]; let mut writer = RowWriter::new(&mut serialized); ::serialize(&vl, &ctx, &mut writer).unwrap(); diff --git a/scylla-cql/src/types/serialize/row.rs b/scylla-cql/src/types/serialize/row.rs index 72c0bf5a23..925ceeb4ed 100644 --- a/scylla-cql/src/types/serialize/row.rs +++ b/scylla-cql/src/types/serialize/row.rs @@ -33,25 +33,7 @@ impl<'a> RowSerializationContext<'a> { } pub trait SerializeRow { - /// Checks if it _might_ be possible to serialize the row according to the - /// information in the context. - /// - /// This function is intended to serve as an optimization in the future, - /// if we were ever to introduce prepared statements parametrized by types. - /// - /// Sometimes, a row cannot be fully type checked right away without knowing - /// the exact values of the columns (e.g. when deserializing to `CqlValue`), - /// but it's fine to do full type checking later in `serialize`. - fn preliminary_type_check( - _ctx: &RowSerializationContext<'_>, - ) -> Result<(), SerializationError> { - Ok(()) - } - /// Serializes the row according to the information in the given context. - /// - /// The function may assume that `preliminary_type_check` was called, - /// though it must not do anything unsafe if this assumption does not hold. fn serialize( &self, ctx: &RowSerializationContext<'_>, diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index 82f79c81a2..224b678c0a 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -28,22 +28,7 @@ use super::writers::WrittenCellProof; use super::{CellWriter, SerializationError}; pub trait SerializeCql { - /// Given a CQL type, checks if it _might_ be possible to serialize to that type. - /// - /// This function is intended to serve as an optimization in the future, - /// if we were ever to introduce prepared statements parametrized by types. - /// - /// Some types cannot be type checked without knowing the exact value, - /// this is the case e.g. for `CqlValue`. It's also fine to do it later in - /// `serialize`. - fn preliminary_type_check(_typ: &ColumnType) -> Result<(), SerializationError> { - Ok(()) - } - /// Serializes the value to given CQL type. - /// - /// The function may assume that `preliminary_type_check` was called, - /// though it must not do anything unsafe if this assumption does not hold. fn serialize<'b>( &self, typ: &ColumnType, @@ -572,7 +557,6 @@ fn check_and_serialize<'b, V: SerializeCql>( typ: &ColumnType, writer: CellWriter<'b>, ) -> Result, SerializationError> { - V::preliminary_type_check(typ)?; v.serialize(typ, writer) } From ef6e428e39c833c88c8565c2052e5fa07d8cf30b Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Fri, 8 Dec 2023 07:09:10 +0100 Subject: [PATCH 17/19] serialize/value: inline check_and_serialize into callers --- scylla-cql/src/types/serialize/value.rs | 52 +++++++++++-------------- 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index 224b678c0a..4359eee497 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -439,14 +439,14 @@ fn serialize_cql_value<'b>( )); } match value { - CqlValue::Ascii(a) => check_and_serialize(a, typ, writer), - CqlValue::Boolean(b) => check_and_serialize(b, typ, writer), - CqlValue::Blob(b) => check_and_serialize(b, typ, writer), - CqlValue::Counter(c) => check_and_serialize(c, typ, writer), - CqlValue::Decimal(d) => check_and_serialize(d, typ, writer), - CqlValue::Date(d) => check_and_serialize(d, typ, writer), - CqlValue::Double(d) => check_and_serialize(d, typ, writer), - CqlValue::Duration(d) => check_and_serialize(d, typ, writer), + CqlValue::Ascii(a) => <_ as SerializeCql>::serialize(&a, typ, writer), + CqlValue::Boolean(b) => <_ as SerializeCql>::serialize(&b, typ, writer), + CqlValue::Blob(b) => <_ as SerializeCql>::serialize(&b, typ, writer), + CqlValue::Counter(c) => <_ as SerializeCql>::serialize(&c, typ, writer), + CqlValue::Decimal(d) => <_ as SerializeCql>::serialize(&d, typ, writer), + CqlValue::Date(d) => <_ as SerializeCql>::serialize(&d, typ, writer), + CqlValue::Double(d) => <_ as SerializeCql>::serialize(&d, typ, writer), + CqlValue::Duration(d) => <_ as SerializeCql>::serialize(&d, typ, writer), CqlValue::Empty => { if !typ.supports_special_empty_value() { return Err(mk_typck_err::( @@ -456,13 +456,13 @@ fn serialize_cql_value<'b>( } Ok(writer.set_value(&[]).unwrap()) } - CqlValue::Float(f) => check_and_serialize(f, typ, writer), - CqlValue::Int(i) => check_and_serialize(i, typ, writer), - CqlValue::BigInt(b) => check_and_serialize(b, typ, writer), - CqlValue::Text(t) => check_and_serialize(t, typ, writer), - CqlValue::Timestamp(t) => check_and_serialize(t, typ, writer), - CqlValue::Inet(i) => check_and_serialize(i, typ, writer), - CqlValue::List(l) => check_and_serialize(l, typ, writer), + CqlValue::Float(f) => <_ as SerializeCql>::serialize(&f, typ, writer), + CqlValue::Int(i) => <_ as SerializeCql>::serialize(&i, typ, writer), + CqlValue::BigInt(b) => <_ as SerializeCql>::serialize(&b, typ, writer), + CqlValue::Text(t) => <_ as SerializeCql>::serialize(&t, typ, writer), + CqlValue::Timestamp(t) => <_ as SerializeCql>::serialize(&t, typ, writer), + CqlValue::Inet(i) => <_ as SerializeCql>::serialize(&i, typ, writer), + CqlValue::List(l) => <_ as SerializeCql>::serialize(&l, typ, writer), CqlValue::Map(m) => serialize_mapping( std::any::type_name::(), m.len(), @@ -470,16 +470,16 @@ fn serialize_cql_value<'b>( typ, writer, ), - CqlValue::Set(s) => check_and_serialize(s, typ, writer), + CqlValue::Set(s) => <_ as SerializeCql>::serialize(&s, typ, writer), CqlValue::UserDefinedType { keyspace, type_name, fields, } => serialize_udt(typ, keyspace, type_name, fields, writer), - CqlValue::SmallInt(s) => check_and_serialize(s, typ, writer), - CqlValue::TinyInt(t) => check_and_serialize(t, typ, writer), - CqlValue::Time(t) => check_and_serialize(t, typ, writer), - CqlValue::Timeuuid(t) => check_and_serialize(t, typ, writer), + CqlValue::SmallInt(s) => <_ as SerializeCql>::serialize(&s, typ, writer), + CqlValue::TinyInt(t) => <_ as SerializeCql>::serialize(&t, typ, writer), + CqlValue::Time(t) => <_ as SerializeCql>::serialize(&t, typ, writer), + CqlValue::Timeuuid(t) => <_ as SerializeCql>::serialize(&t, typ, writer), CqlValue::Tuple(t) => { // We allow serializing tuples that have less fields // than the database tuple, but not the other way around. @@ -505,8 +505,8 @@ fn serialize_cql_value<'b>( }; serialize_tuple_like(typ, fields.iter(), t.iter(), writer) } - CqlValue::Uuid(u) => check_and_serialize(u, typ, writer), - CqlValue::Varint(v) => check_and_serialize(v, typ, writer), + CqlValue::Uuid(u) => <_ as SerializeCql>::serialize(&u, typ, writer), + CqlValue::Varint(v) => <_ as SerializeCql>::serialize(&v, typ, writer), } } @@ -552,14 +552,6 @@ fn fix_cql_value_name_in_err(mut err: SerializationError) -> SerializationError err } -fn check_and_serialize<'b, V: SerializeCql>( - v: &V, - typ: &ColumnType, - writer: CellWriter<'b>, -) -> Result, SerializationError> { - v.serialize(typ, writer) -} - fn serialize_udt<'b>( typ: &ColumnType, keyspace: &str, From 6a1ea41e716d4de49376b26894c811f227d81ddc Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Fri, 8 Dec 2023 03:58:53 +0100 Subject: [PATCH 18/19] serialize/row: relax restriction on SerializeRow impl on reference The bound on `impl SerializeRow for &T` implicitly requires the type `T` to be sized, preventing it from being used with `dyn SerializeRow`. Relax the restriction by adding `+ ?Sized` to be able to use it with trait objects. --- scylla-cql/src/types/serialize/row.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scylla-cql/src/types/serialize/row.rs b/scylla-cql/src/types/serialize/row.rs index 925ceeb4ed..c5256a2eb8 100644 --- a/scylla-cql/src/types/serialize/row.rs +++ b/scylla-cql/src/types/serialize/row.rs @@ -210,7 +210,7 @@ impl SerializeRow for HashMap<&str, T, S> { impl_serialize_row_for_map!(); } -impl SerializeRow for &T { +impl SerializeRow for &T { fn serialize( &self, ctx: &RowSerializationContext<'_>, From 5733a2f5a34e9e8d458c93ba81415eb35eb809d5 Mon Sep 17 00:00:00 2001 From: Piotr Dulikowski Date: Fri, 8 Dec 2023 04:02:20 +0100 Subject: [PATCH 19/19] serialize: tests for dyn SerializeCql/dyn SerializeRow As a final confirmation of the work in the PR and to prevent regressions, add tests which explicitly use `dyn SerializeCql` and `dyn SerializeRow`. --- scylla-cql/src/types/serialize/row.rs | 33 +++++++++++++++++++++++++ scylla-cql/src/types/serialize/value.rs | 15 +++++++++++ 2 files changed, 48 insertions(+) diff --git a/scylla-cql/src/types/serialize/row.rs b/scylla-cql/src/types/serialize/row.rs index c5256a2eb8..d8702100b6 100644 --- a/scylla-cql/src/types/serialize/row.rs +++ b/scylla-cql/src/types/serialize/row.rs @@ -639,4 +639,37 @@ mod tests { // Skip the value count assert_eq!(&sorted_row_data[2..], unsorted_row_data); } + + #[test] + fn test_dyn_serialize_row() { + let row = ( + 1i32, + "Ala ma kota", + None::, + MaybeUnset::Unset::, + ); + let ctx = RowSerializationContext { + columns: &[ + col_spec("a", ColumnType::Int), + col_spec("b", ColumnType::Text), + col_spec("c", ColumnType::BigInt), + col_spec("d", ColumnType::Ascii), + ], + }; + + let mut typed_data = Vec::new(); + let mut typed_data_writer = RowWriter::new(&mut typed_data); + <_ as SerializeRow>::serialize(&row, &ctx, &mut typed_data_writer).unwrap(); + + let row = &row as &dyn SerializeRow; + let mut erased_data = Vec::new(); + let mut erased_data_writer = RowWriter::new(&mut erased_data); + <_ as SerializeRow>::serialize(&row, &ctx, &mut erased_data_writer).unwrap(); + + assert_eq!( + typed_data_writer.value_count(), + erased_data_writer.value_count(), + ); + assert_eq!(typed_data, erased_data); + } } diff --git a/scylla-cql/src/types/serialize/value.rs b/scylla-cql/src/types/serialize/value.rs index 4359eee497..37244f7073 100644 --- a/scylla-cql/src/types/serialize/value.rs +++ b/scylla-cql/src/types/serialize/value.rs @@ -1392,4 +1392,19 @@ mod tests { check_compat(None::); check_compat(MaybeUnset::Unset::); } + + #[test] + fn test_dyn_serialize_cql() { + let v: i32 = 123; + let mut typed_data = Vec::new(); + let typed_data_writer = CellWriter::new(&mut typed_data); + <_ as SerializeCql>::serialize(&v, &ColumnType::Int, typed_data_writer).unwrap(); + + let v = &v as &dyn SerializeCql; + let mut erased_data = Vec::new(); + let erased_data_writer = CellWriter::new(&mut erased_data); + <_ as SerializeCql>::serialize(&v, &ColumnType::Int, erased_data_writer).unwrap(); + + assert_eq!(typed_data, erased_data); + } }