New Validation panics instead of returning Error::NotEnoughData #341
Description
Describe the bug
Steps to reproduce
- Start streaming SELECT via the client, like this:
let mut cursor = client
.query(
r#"
SELECT
...
FROM ... FINAL
WHERE ...
ORDER BY create_time ASC
"#,
)
.fetch::<BillingItemSummary>()?;
loop {
let Some(bi) = cursor.next().await? else {
break;
};
counter += 1;
log::info!("{counter}, create_time: {:?} done", bi.create_time);
}
- make sure you have at least 1MiB of data in CH
- it will panic wit:
thread 'tokio-runtime-worker' panicked at /usr/local/cargo/registry/src/index.crates.io-1949cf8c6b5b557f/clickhouse-0.14.0/src/rowbinary/validation.rs:404:13:
While processing column BillingItemSummary.api_key_id defined as UUID: tuple was not fully (de)serialized; remaining elements: UInt64; likely, the field definition is incomplete
stack backtrace:
0: __rustc::rust_begin_unwind
1: core::panicking::panic_fmt
2: core::ptr::drop_in_place<clickhouse::rowbinary::validation::InnerDataTypeValidator<BillingItemSummary>>
3: clickhouse::serde::uuid::deserialize
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
Expected behaviour
It does not panic
It looks like this issue happens when uuid (which consists of two uin64) is split. In my concrete repro example
self.bytes.remaining() evaluates to 1048630 - 1048623 == 7
clickhouse-0.14.0/src/cursors/row.rs
loop {
if self.bytes.remaining() > 0 {
let mut slice = self.bytes.slice();
match parse_rbwnat_columns_header(&mut slice) {
Ok(columns) if !columns.is_empty() => {
self.bytes.set_remaining(slice.len());
self.row_metadata = Some(RowMetadata::new_for_cursor::<T>(columns));
return Ok(());
}
Ok(_) => {
// This does not panic, as it could be a network issue
// or a malformed response from the server or LB,
// and a simple retry might help in certain cases.
return Err(Error::BadResponse(
"Expected at least one column in the header".to_string(),
));
}
Err(TypesError::NotEnoughData(_)) => {}
Err(err) => {
return Err(Error::InvalidColumnsHeader(err.into()));
}
}
}
match self.raw.next().await? {
Some(chunk) => self.bytes.extend(chunk),
None if self.row_metadata.is_none() => {
// Similar to the other BadResponse branch above
return Err(Error::BadResponse(
"Could not read columns header".to_string(),
));
}
// if the result set is empty, there is only the columns header
None => return Ok(()),
}
}Note, the code above correctly handles the case TypesError::NotEnoughData, but instead of returning the error, validation struct panics in its drop().
Suggestions / Questions
- Why does a library code panics at all? Library should not panic. You could return
Result<>. - Also, apparently, there are no tests for this?..
- Phrasing of the error is misleading. It says "tuple was not fully (de)serialized; remaining elements: UInt64". It should be the vice-versa: tuple was not fully (de)serialized; MISSING elements: UInt64. It failed deserialisation because of missing data / elements, not because of remaining elements.