From 52f0103c8d3f828323e6327052b888abdce3851a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 8 Nov 2024 14:39:39 +0100 Subject: [PATCH 01/14] f_errors: introduce PreparedMetadataParseError Previously, the `ResultMetadataParseError` error type was common for both Result and Prepared metadata. It does not make much sense, though. Obviously, some of the error variants are shared between these two types, but not all of them. --- scylla-cql/src/frame/frame_errors.rs | 41 +++++++++++++++++++++++-- scylla-cql/src/frame/response/result.rs | 15 ++++----- 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/scylla-cql/src/frame/frame_errors.rs b/scylla-cql/src/frame/frame_errors.rs index 94e455bb79..3fe8d24c1a 100644 --- a/scylla-cql/src/frame/frame_errors.rs +++ b/scylla-cql/src/frame/frame_errors.rs @@ -301,7 +301,7 @@ pub enum PreparedParseError { #[error("Invalid result metadata: {0}")] ResultMetadataParseError(ResultMetadataParseError), #[error("Invalid prepared metadata: {0}")] - PreparedMetadataParseError(ResultMetadataParseError), + PreparedMetadataParseError(PreparedMetadataParseError), #[error("Non-zero paging state in result metadata: {0:?}")] NonZeroPagingState(Arc<[u8]>), } @@ -327,22 +327,57 @@ pub enum RowsParseError { } /// An error type returned when deserialization -/// of `[Result/Prepared]Metadata` failed. +/// of statement's prepared metadata failed. #[non_exhaustive] #[derive(Error, Debug, Clone)] -pub enum ResultMetadataParseError { +pub enum PreparedMetadataParseError { + /// Failed to parse metadata flags. #[error("Malformed metadata flags: {0}")] FlagsParseError(LowLevelDeserializationError), + + /// Failed to parse column count. #[error("Malformed column count: {0}")] ColumnCountParseError(LowLevelDeserializationError), + + /// Failed to parse partition key count. #[error("Malformed partition key count: {0}")] PkCountParseError(LowLevelDeserializationError), + + /// Failed to parse partition key index. #[error("Malformed partition key index: {0}")] PkIndexParseError(LowLevelDeserializationError), + + /// Failed to parse global table spec. + #[error("Invalid global table spec: {0}")] + GlobalTableSpecParseError(#[from] TableSpecParseError), + + /// Failed to parse column spec. + #[error("Invalid column spec: {0}")] + ColumnSpecParseError(#[from] ColumnSpecParseError), +} + +/// An error type returned when deserialization +/// of result metadata failed. +#[non_exhaustive] +#[derive(Error, Debug, Clone)] +pub enum ResultMetadataParseError { + /// Failed to parse metadata flags. + #[error("Malformed metadata flags: {0}")] + FlagsParseError(LowLevelDeserializationError), + + /// Failed to parse column count. + #[error("Malformed column count: {0}")] + ColumnCountParseError(LowLevelDeserializationError), + + /// Failed to parse paging state response. #[error("Malformed paging state: {0}")] PagingStateParseError(LowLevelDeserializationError), + + /// Failed to parse global table spec. #[error("Invalid global table spec: {0}")] GlobalTableSpecParseError(#[from] TableSpecParseError), + + /// Failed to parse column spec. #[error("Invalid column spec: {0}")] ColumnSpecParseError(#[from] ColumnSpecParseError), } diff --git a/scylla-cql/src/frame/response/result.rs b/scylla-cql/src/frame/response/result.rs index 3a351b4341..8741a261f4 100644 --- a/scylla-cql/src/frame/response/result.rs +++ b/scylla-cql/src/frame/response/result.rs @@ -1,8 +1,9 @@ use crate::cql_to_rust::{FromRow, FromRowError}; use crate::frame::frame_errors::{ ColumnSpecParseError, ColumnSpecParseErrorKind, CqlResultParseError, CqlTypeParseError, - LowLevelDeserializationError, PreparedParseError, ResultMetadataParseError, RowsParseError, - SchemaChangeEventParseError, SetKeyspaceParseError, TableSpecParseError, + LowLevelDeserializationError, PreparedMetadataParseError, PreparedParseError, + ResultMetadataParseError, RowsParseError, SchemaChangeEventParseError, SetKeyspaceParseError, + TableSpecParseError, }; use crate::frame::request::query::PagingStateResponse; use crate::frame::response::event::SchemaChangeEvent; @@ -1274,22 +1275,22 @@ impl RawMetadataAndRawRows { fn deser_prepared_metadata( buf: &mut &[u8], -) -> StdResult { +) -> StdResult { let flags = types::read_int(buf) - .map_err(|err| ResultMetadataParseError::FlagsParseError(err.into()))?; + .map_err(|err| PreparedMetadataParseError::FlagsParseError(err.into()))?; let global_tables_spec = flags & 0x0001 != 0; let col_count = - types::read_int_length(buf).map_err(ResultMetadataParseError::ColumnCountParseError)?; + types::read_int_length(buf).map_err(PreparedMetadataParseError::ColumnCountParseError)?; let pk_count: usize = - types::read_int_length(buf).map_err(ResultMetadataParseError::PkCountParseError)?; + types::read_int_length(buf).map_err(PreparedMetadataParseError::PkCountParseError)?; let mut pk_indexes = Vec::with_capacity(pk_count); for i in 0..pk_count { pk_indexes.push(PartitionKeyIndex { index: types::read_short(buf) - .map_err(|err| ResultMetadataParseError::PkIndexParseError(err.into()))? + .map_err(|err| PreparedMetadataParseError::PkIndexParseError(err.into()))? as u16, sequence: i as u16, }); From 962ae5059ce744ecbad0937ad0de66a1dec70f6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 8 Nov 2024 15:02:46 +0100 Subject: [PATCH 02/14] f_errors: RawRowsAndPagingStateResponseParseError An error that occurred during initial deserialization of `RESULT:Rows` response. Since the deserialization of rows is lazy, we initially only need to deserialize: - result metadata flags - column count (result metadata) - paging state response We ultimately want to get rid of current `RowsParseError` since its usage is abused in a lot of places throughout the scylla crate. We start by narrowing the return type of `deser_rows()` function. --- scylla-cql/src/frame/frame_errors.rs | 27 +++++++++++++++++++++++++ scylla-cql/src/frame/response/result.rs | 15 +++++++------- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/scylla-cql/src/frame/frame_errors.rs b/scylla-cql/src/frame/frame_errors.rs index 3fe8d24c1a..0e0a1c2856 100644 --- a/scylla-cql/src/frame/frame_errors.rs +++ b/scylla-cql/src/frame/frame_errors.rs @@ -227,6 +227,11 @@ pub enum CqlResultParseError { #[error("RESULT:Prepared response deserialization failed: {0}")] PreparedParseError(#[from] PreparedParseError), #[error("RESULT:Rows response deserialization failed: {0}")] + RawRowsParseError(#[from] RawRowsAndPagingStateResponseParseError), + + // TODO: This is required for `From for QueryError conversion`. + // It will be removed in later commits. + #[error("RESULT:Rows response deserialization failed: {0}")] RowsParseError(#[from] RowsParseError), } @@ -306,6 +311,28 @@ pub enum PreparedParseError { NonZeroPagingState(Arc<[u8]>), } +/// An error that occurred during initial deserialization of +/// `RESULT:Rows` response. Since the deserialization of rows is lazy, +/// we initially only need to deserialize: +/// - result metadata flags +/// - column count (result metadata) +/// - paging state response +#[non_exhaustive] +#[derive(Debug, Error, Clone)] +pub enum RawRowsAndPagingStateResponseParseError { + /// Failed to parse metadata flags. + #[error("Malformed metadata flags: {0}")] + FlagsParseError(LowLevelDeserializationError), + + /// Failed to parse column count. + #[error("Malformed column count: {0}")] + ColumnCountParseError(LowLevelDeserializationError), + + /// Failed to parse paging state response. + #[error("Malformed paging state: {0}")] + PagingStateParseError(LowLevelDeserializationError), +} + /// An error type returned when deserialization /// of `RESULT::Rows` response fails. #[non_exhaustive] diff --git a/scylla-cql/src/frame/response/result.rs b/scylla-cql/src/frame/response/result.rs index 8741a261f4..f96afe8416 100644 --- a/scylla-cql/src/frame/response/result.rs +++ b/scylla-cql/src/frame/response/result.rs @@ -2,8 +2,8 @@ use crate::cql_to_rust::{FromRow, FromRowError}; use crate::frame::frame_errors::{ ColumnSpecParseError, ColumnSpecParseErrorKind, CqlResultParseError, CqlTypeParseError, LowLevelDeserializationError, PreparedMetadataParseError, PreparedParseError, - ResultMetadataParseError, RowsParseError, SchemaChangeEventParseError, SetKeyspaceParseError, - TableSpecParseError, + RawRowsAndPagingStateResponseParseError, ResultMetadataParseError, RowsParseError, + SchemaChangeEventParseError, SetKeyspaceParseError, TableSpecParseError, }; use crate::frame::request::query::PagingStateResponse; use crate::frame::response::event::SchemaChangeEvent; @@ -1143,20 +1143,20 @@ impl RawMetadataAndRawRows { fn deserialize( frame: &mut FrameSlice, cached_metadata: Option>>, - ) -> StdResult<(Self, PagingStateResponse), RowsParseError> { + ) -> StdResult<(Self, PagingStateResponse), RawRowsAndPagingStateResponseParseError> { let flags = types::read_int(frame.as_slice_mut()) - .map_err(|err| ResultMetadataParseError::FlagsParseError(err.into()))?; + .map_err(|err| RawRowsAndPagingStateResponseParseError::FlagsParseError(err.into()))?; let global_tables_spec = flags & 0x0001 != 0; let has_more_pages = flags & 0x0002 != 0; let no_metadata = flags & 0x0004 != 0; let col_count = types::read_int_length(frame.as_slice_mut()) - .map_err(ResultMetadataParseError::ColumnCountParseError)?; + .map_err(RawRowsAndPagingStateResponseParseError::ColumnCountParseError)?; let raw_paging_state = has_more_pages .then(|| { types::read_bytes(frame.as_slice_mut()) - .map_err(ResultMetadataParseError::PagingStateParseError) + .map_err(RawRowsAndPagingStateResponseParseError::PagingStateParseError) }) .transpose()?; @@ -1474,7 +1474,8 @@ pub fn deser_cql_value( fn deser_rows( buf_bytes: Bytes, cached_metadata: Option<&Arc>>, -) -> StdResult<(RawMetadataAndRawRows, PagingStateResponse), RowsParseError> { +) -> StdResult<(RawMetadataAndRawRows, PagingStateResponse), RawRowsAndPagingStateResponseParseError> +{ let mut frame_slice = FrameSlice::new(&buf_bytes); RawMetadataAndRawRows::deserialize(&mut frame_slice, cached_metadata.cloned()) } From 256705db0a3cd7d5b638fdd0a52eb2c30839d571 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 8 Nov 2024 16:51:00 +0100 Subject: [PATCH 03/14] errors: IntoLegacyQueryResultError An error returned by `QueryResult::into_legacy_result()`. Previously, the `QueryResult::into_legacy_result()` would return `RowsParseError`. Note that `RowsParseError` is (for now) included in new error type. This is because `RawMetadataAndRawRows::deserialize_metadata()` still returns `RowsParseError`. This will be adjusted in following commit once we introduce a new error type for `deserialize_metadata` method. --- scylla/src/transport/errors.rs | 15 ++++++++++++- scylla/src/transport/legacy_query_result.rs | 21 +++++++++++++++++++ .../src/transport/load_balancing/default.rs | 3 ++- scylla/src/transport/query_result.rs | 4 ++-- scylla/src/transport/speculative_execution.rs | 3 ++- 5 files changed, 41 insertions(+), 5 deletions(-) diff --git a/scylla/src/transport/errors.rs b/scylla/src/transport/errors.rs index 349d968a40..d1e04782b3 100644 --- a/scylla/src/transport/errors.rs +++ b/scylla/src/transport/errors.rs @@ -34,7 +34,7 @@ use thiserror::Error; use crate::{authentication::AuthError, frame::response}; -use super::query_result::SingleRowError; +use super::{legacy_query_result::IntoLegacyQueryResultError, query_result::SingleRowError}; /// Error that occurred during query execution #[derive(Error, Debug, Clone)] @@ -100,6 +100,11 @@ pub enum QueryError { /// Client timeout occurred before any response arrived #[error("Request timeout: {0}")] RequestTimeout(String), + + /// Failed to convert [`QueryResult`][crate::transport::query_result::QueryResult] + /// into [`LegacyQueryResult`][crate::transport::legacy_query_result::LegacyQueryResult]. + #[error("Failed to convert `QueryResult` into `LegacyQueryResult`: {0}")] + IntoLegacyQueryResultError(#[from] IntoLegacyQueryResultError), } impl From for QueryError { @@ -164,6 +169,9 @@ impl From for NewSessionError { QueryError::BrokenConnection(e) => NewSessionError::BrokenConnection(e), QueryError::UnableToAllocStreamId => NewSessionError::UnableToAllocStreamId, QueryError::RequestTimeout(msg) => NewSessionError::RequestTimeout(msg), + QueryError::IntoLegacyQueryResultError(e) => { + NewSessionError::IntoLegacyQueryResultError(e) + } } } } @@ -266,6 +274,11 @@ pub enum NewSessionError { /// during `Session` creation. #[error("Client timeout: {0}")] RequestTimeout(String), + + /// Failed to convert [`QueryResult`][crate::transport::query_result::QueryResult] + /// into [`LegacyQueryResult`][crate::transport::legacy_query_result::LegacyQueryResult]. + #[error("Failed to convert `QueryResult` into `LegacyQueryResult`: {0}")] + IntoLegacyQueryResultError(#[from] IntoLegacyQueryResultError), } /// A protocol error. diff --git a/scylla/src/transport/legacy_query_result.rs b/scylla/src/transport/legacy_query_result.rs index 46818a297e..a0fc2b1c70 100644 --- a/scylla/src/transport/legacy_query_result.rs +++ b/scylla/src/transport/legacy_query_result.rs @@ -1,7 +1,9 @@ use crate::frame::response::cql_to_rust::{FromRow, FromRowError}; use crate::frame::response::result::ColumnSpec; use crate::frame::response::result::Row; +use scylla_cql::frame::frame_errors::RowsParseError; use scylla_cql::frame::response::result::{self, ResultMetadataHolder}; +use scylla_cql::types::deserialize::{DeserializationError, TypeCheckError}; use thiserror::Error; use uuid::Uuid; @@ -174,6 +176,25 @@ impl LegacyQueryResult { } } +/// An error that occurred during [`QueryResult`](crate::transport::query_result::QueryResult) +/// to [`LegacyQueryResult`] conversion. +#[non_exhaustive] +#[derive(Error, Clone, Debug)] +pub enum IntoLegacyQueryResultError { + // TODO: Replace RowsParseError with narrower error. Done in later commit. + /// Failed to lazily deserialize result metadata. + #[error("Failed to lazily deserialize result metadata: {0}")] + ResultMetadataParseError(#[from] RowsParseError), + + /// Failed to perform the typecheck against [`Row`] type. + #[error("Typecheck error: {0}")] + TypecheckError(#[from] TypeCheckError), + + /// Failed to deserialize rows. + #[error("Failed to deserialize rows: {0}")] + DeserializationError(#[from] DeserializationError), +} + /// [`LegacyQueryResult::rows()`](LegacyQueryResult::rows) or a similar function called on a bad LegacyQueryResult.\ /// Expected `LegacyQueryResult.rows` to be `Some`, but it was `None`.\ /// `LegacyQueryResult.rows` is `Some` for queries that can return rows (e.g `SELECT`).\ diff --git a/scylla/src/transport/load_balancing/default.rs b/scylla/src/transport/load_balancing/default.rs index 51db7f97f5..f10c139eae 100644 --- a/scylla/src/transport/load_balancing/default.rs +++ b/scylla/src/transport/load_balancing/default.rs @@ -2860,7 +2860,8 @@ mod latency_awareness { | QueryError::MetadataError(_) | QueryError::ProtocolError(_) | QueryError::TimeoutError - | QueryError::RequestTimeout(_) => true, + | QueryError::RequestTimeout(_) + | QueryError::IntoLegacyQueryResultError(_) => true, } } } diff --git a/scylla/src/transport/query_result.rs b/scylla/src/transport/query_result.rs index 52326ba325..363292a9b5 100644 --- a/scylla/src/transport/query_result.rs +++ b/scylla/src/transport/query_result.rs @@ -11,7 +11,7 @@ use scylla_cql::types::deserialize::result::TypedRowIterator; use scylla_cql::types::deserialize::row::DeserializeRow; use scylla_cql::types::deserialize::{DeserializationError, TypeCheckError}; -use super::legacy_query_result::LegacyQueryResult; +use super::legacy_query_result::{IntoLegacyQueryResultError, LegacyQueryResult}; /// A view over specification of a table in the database. #[derive(Debug, Clone, Copy)] @@ -243,7 +243,7 @@ impl QueryResult { /// Transforms itself into the legacy result type, by eagerly deserializing rows /// into the Row type. This is inefficient, and should only be used during transition /// period to the new API. - pub fn into_legacy_result(self) -> Result { + pub fn into_legacy_result(self) -> Result { if let Some(raw_rows) = self.raw_metadata_and_rows { let raw_rows_with_metadata = raw_rows.deserialize_metadata()?; diff --git a/scylla/src/transport/speculative_execution.rs b/scylla/src/transport/speculative_execution.rs index d66a9bb6f7..24dfff474b 100644 --- a/scylla/src/transport/speculative_execution.rs +++ b/scylla/src/transport/speculative_execution.rs @@ -110,7 +110,8 @@ fn can_be_ignored(result: &Result) -> bool { QueryError::EmptyPlan => false, // Errors that should not appear here, thus should not be ignored - QueryError::TimeoutError + QueryError::IntoLegacyQueryResultError(_) + | QueryError::TimeoutError | QueryError::RequestTimeout(_) | QueryError::MetadataError(_) => false, From 9b625efdb78259266bb6d99b7fcd1865e6ae4cee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Wed, 13 Nov 2024 14:17:49 +0100 Subject: [PATCH 04/14] metadata: remove column count mismath check This could only happen if there is a serious bug in the driver. `deser_col_specs_generic` takes `column_count` as argument and deserializes the col specs `column_count` times in a loop. Thus, this check does not make sense. --- scylla-cql/src/frame/frame_errors.rs | 5 ----- scylla-cql/src/frame/response/result.rs | 6 ------ 2 files changed, 11 deletions(-) diff --git a/scylla-cql/src/frame/frame_errors.rs b/scylla-cql/src/frame/frame_errors.rs index 0e0a1c2856..646d65c4d9 100644 --- a/scylla-cql/src/frame/frame_errors.rs +++ b/scylla-cql/src/frame/frame_errors.rs @@ -340,11 +340,6 @@ pub enum RawRowsAndPagingStateResponseParseError { pub enum RowsParseError { #[error("Invalid result metadata: {0}")] ResultMetadataParseError(#[from] ResultMetadataParseError), - #[error("Invalid result metadata, server claims {col_count} columns, received {col_specs_count} col specs.")] - ColumnCountMismatch { - col_count: usize, - col_specs_count: usize, - }, #[error("Malformed rows count: {0}")] RowsCountParseError(LowLevelDeserializationError), #[error("Data type check prior to deserialization failed: {0}")] diff --git a/scylla-cql/src/frame/response/result.rs b/scylla-cql/src/frame/response/result.rs index f96afe8416..fda06a65f6 100644 --- a/scylla-cql/src/frame/response/result.rs +++ b/scylla-cql/src/frame/response/result.rs @@ -1204,12 +1204,6 @@ impl RawMetadataAndRawRows { col_specs, } }; - if server_metadata.col_count() != server_metadata.col_specs().len() { - return Err(RowsParseError::ColumnCountMismatch { - col_count: server_metadata.col_count(), - col_specs_count: server_metadata.col_specs().len(), - }); - } Ok(server_metadata) } } From b2d60b3fd01b1602f94a8cc776fae36efac5966f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 8 Nov 2024 17:21:10 +0100 Subject: [PATCH 05/14] metadata: return ResultMetadataAndRowsCountParseError in deserialize_metadata Adjusted the error returned by `RawMetadataAndRawRows::deserialize_metadata()`. Again, to narrow the return error type of following methods: - RawMetadataAndRawRows::deserialize_metadata() - QueryResult::into_rows_result() Adjusted the callers of `QueryResult::into_rows_result` in topology.rs and session.rs and added corresponding variants for `TracingProtocolError` and `SchemaVersionFetchError` Temporarily, we need to introduce a corresponding variant to `QueryError` and `NewSessionError`. This is because internal API makes use of `into_rows_result()` in multiple places. The callers will be adjusted later in this PR, allowing us to remove the variant from `Query/NSError`. --- scylla-cql/src/frame/frame_errors.rs | 14 ++++++++ scylla-cql/src/frame/response/result.rs | 22 ++++++------ scylla/src/transport/connection.rs | 7 +++- scylla/src/transport/errors.rs | 35 ++++++++++++++++++- scylla/src/transport/legacy_query_result.rs | 5 ++- .../src/transport/load_balancing/default.rs | 1 + scylla/src/transport/query_result.rs | 6 ++-- scylla/src/transport/session.rs | 23 ++++++++---- scylla/src/transport/speculative_execution.rs | 1 + 9 files changed, 90 insertions(+), 24 deletions(-) diff --git a/scylla-cql/src/frame/frame_errors.rs b/scylla-cql/src/frame/frame_errors.rs index 646d65c4d9..ad95125a94 100644 --- a/scylla-cql/src/frame/frame_errors.rs +++ b/scylla-cql/src/frame/frame_errors.rs @@ -378,6 +378,20 @@ pub enum PreparedMetadataParseError { ColumnSpecParseError(#[from] ColumnSpecParseError), } +/// An error returned when lazy deserialization of +/// result metadata and rows count fails. +#[non_exhaustive] +#[derive(Error, Debug, Clone)] +pub enum ResultMetadataAndRowsCountParseError { + /// Failed to deserialize result metadata. + #[error("Failed to lazily deserialize result metadata: {0}")] + ResultMetadataParseError(#[from] ResultMetadataParseError), + + /// Received malformed rows count from the server. + #[error("Malformed rows count: {0}")] + RowsCountParseError(LowLevelDeserializationError), +} + /// An error type returned when deserialization /// of result metadata failed. #[non_exhaustive] diff --git a/scylla-cql/src/frame/response/result.rs b/scylla-cql/src/frame/response/result.rs index fda06a65f6..de720aa91c 100644 --- a/scylla-cql/src/frame/response/result.rs +++ b/scylla-cql/src/frame/response/result.rs @@ -2,8 +2,9 @@ use crate::cql_to_rust::{FromRow, FromRowError}; use crate::frame::frame_errors::{ ColumnSpecParseError, ColumnSpecParseErrorKind, CqlResultParseError, CqlTypeParseError, LowLevelDeserializationError, PreparedMetadataParseError, PreparedParseError, - RawRowsAndPagingStateResponseParseError, ResultMetadataParseError, RowsParseError, - SchemaChangeEventParseError, SetKeyspaceParseError, TableSpecParseError, + RawRowsAndPagingStateResponseParseError, ResultMetadataAndRowsCountParseError, + ResultMetadataParseError, SchemaChangeEventParseError, SetKeyspaceParseError, + TableSpecParseError, }; use crate::frame::request::query::PagingStateResponse; use crate::frame::response::event::SchemaChangeEvent; @@ -1187,17 +1188,16 @@ impl RawMetadataAndRawRows { fn metadata_deserializer( col_count: usize, global_tables_spec: bool, - ) -> impl for<'frame> FnOnce(&mut &'frame [u8]) -> StdResult, RowsParseError> - { + ) -> impl for<'frame> FnOnce( + &mut &'frame [u8], + ) -> StdResult, ResultMetadataParseError> { move |buf| { let server_metadata = { let global_table_spec = global_tables_spec .then(|| deser_table_spec(buf)) - .transpose() - .map_err(ResultMetadataParseError::from)?; + .transpose()?; - let col_specs = deser_col_specs_borrowed(buf, global_table_spec, col_count) - .map_err(ResultMetadataParseError::from)?; + let col_specs = deser_col_specs_borrowed(buf, global_table_spec, col_count)?; ResultMetadata { col_count, @@ -1212,7 +1212,9 @@ impl RawMetadataAndRawRows { /// /// If metadata is cached (in the PreparedStatement), it is reused (shared) from cache /// instead of deserializing. - pub fn deserialize_metadata(self) -> StdResult { + pub fn deserialize_metadata( + self, + ) -> StdResult { let (metadata_deserialized, row_count_and_raw_rows) = match self.cached_metadata { Some(cached) if self.no_metadata => { // Server sent no metadata, but we have metadata cached. This means that we asked the server @@ -1257,7 +1259,7 @@ impl RawMetadataAndRawRows { let mut frame_slice = FrameSlice::new(&row_count_and_raw_rows); let rows_count: usize = types::read_int_length(frame_slice.as_slice_mut()) - .map_err(RowsParseError::RowsCountParseError)?; + .map_err(ResultMetadataAndRowsCountParseError::RowsCountParseError)?; Ok(DeserializedMetadataAndRawRows { metadata: metadata_deserialized, diff --git a/scylla/src/transport/connection.rs b/scylla/src/transport/connection.rs index 185286d2fa..1ce685b61c 100644 --- a/scylla/src/transport/connection.rs +++ b/scylla/src/transport/connection.rs @@ -1434,7 +1434,12 @@ impl Connection { let (version_id,) = self .query_unpaged(LOCAL_VERSION) .await? - .into_rows_result()? + .into_rows_result() + .map_err(|err| { + QueryError::ProtocolError(ProtocolError::SchemaVersionFetch( + SchemaVersionFetchError::ResultMetadataParseError(err), + )) + })? .ok_or(QueryError::ProtocolError( ProtocolError::SchemaVersionFetch(SchemaVersionFetchError::ResultNotRows), ))? diff --git a/scylla/src/transport/errors.rs b/scylla/src/transport/errors.rs index d1e04782b3..ddd18fe0af 100644 --- a/scylla/src/transport/errors.rs +++ b/scylla/src/transport/errors.rs @@ -18,7 +18,8 @@ use scylla_cql::{ CqlAuthChallengeParseError, CqlAuthSuccessParseError, CqlAuthenticateParseError, CqlErrorParseError, CqlEventParseError, CqlRequestSerializationError, CqlResponseParseError, CqlResultParseError, CqlSupportedParseError, - FrameBodyExtensionsParseError, FrameHeaderParseError, RowsParseError, + FrameBodyExtensionsParseError, FrameHeaderParseError, + ResultMetadataAndRowsCountParseError, RowsParseError, }, request::CqlRequestKind, response::CqlResponseKind, @@ -101,6 +102,12 @@ pub enum QueryError { #[error("Request timeout: {0}")] RequestTimeout(String), + // TODO: This should not belong here, but it requires changes to error types + // returned in `iter` API. It's going to be addressed later in this PR. + /// Failed to lazily deserialize result metadata. + #[error("Failed to lazily deserialize result metadata: {0}")] + ResultMetadataParseError(#[from] ResultMetadataAndRowsCountParseError), + /// Failed to convert [`QueryResult`][crate::transport::query_result::QueryResult] /// into [`LegacyQueryResult`][crate::transport::legacy_query_result::LegacyQueryResult]. #[error("Failed to convert `QueryResult` into `LegacyQueryResult`: {0}")] @@ -172,6 +179,7 @@ impl From for NewSessionError { QueryError::IntoLegacyQueryResultError(e) => { NewSessionError::IntoLegacyQueryResultError(e) } + QueryError::ResultMetadataParseError(e) => NewSessionError::ResultMetadataParseError(e), } } } @@ -275,6 +283,12 @@ pub enum NewSessionError { #[error("Client timeout: {0}")] RequestTimeout(String), + // TODO: This should not belong here, but it requires changes to error types + // returned in `iter` API. It's going to be addressed later in this PR. + /// Failed to lazily deserialize result metadata. + #[error("Failed to lazily deserialize result metadata: {0}")] + ResultMetadataParseError(#[from] ResultMetadataAndRowsCountParseError), + /// Failed to convert [`QueryResult`][crate::transport::query_result::QueryResult] /// into [`LegacyQueryResult`][crate::transport::legacy_query_result::LegacyQueryResult]. #[error("Failed to convert `QueryResult` into `LegacyQueryResult`: {0}")] @@ -364,8 +378,15 @@ pub enum UseKeyspaceProtocolError { #[derive(Error, Debug, Clone)] #[non_exhaustive] pub enum SchemaVersionFetchError { + /// Schema version query returned non-rows result. #[error("Schema version query returned non-rows result")] ResultNotRows, + + /// Failed to lazily deserialize result metadata. + #[error("Failed to lazily deserialize result metadata")] + ResultMetadataParseError(ResultMetadataAndRowsCountParseError), + + /// Failed to deserialize a single row from schema version query response. #[error(transparent)] SingleRowError(SingleRowError), } @@ -378,6 +399,12 @@ pub enum TracingProtocolError { #[error("Response to system_traces.session is not RESULT:Rows")] TracesSessionNotRows, + /// Failed to lazily deserialize result metadata from response to system_traces.session query. + #[error( + "Failed to lazily deserialize result metadata from response to system_traces.session query" + )] + TracesSessionResultMetadataParseError(ResultMetadataAndRowsCountParseError), + /// system_traces.session has invalid column type. #[error("system_traces.session has invalid column type: {0}")] TracesSessionInvalidColumnType(TypeCheckError), @@ -390,6 +417,12 @@ pub enum TracingProtocolError { #[error("Response to system_traces.events is not RESULT:Rows")] TracesEventsNotRows, + /// Failed to lazily deserialize result metadata from response to system_traces.events query. + #[error( + "Failed to lazily deserialize result metadata from response to system_traces.events query" + )] + TracesEventsResultMetadataParseError(ResultMetadataAndRowsCountParseError), + /// system_traces.events has invalid column type. #[error("system_traces.events has invalid column type: {0}")] TracesEventsInvalidColumnType(TypeCheckError), diff --git a/scylla/src/transport/legacy_query_result.rs b/scylla/src/transport/legacy_query_result.rs index a0fc2b1c70..8dd6b72142 100644 --- a/scylla/src/transport/legacy_query_result.rs +++ b/scylla/src/transport/legacy_query_result.rs @@ -1,7 +1,7 @@ use crate::frame::response::cql_to_rust::{FromRow, FromRowError}; use crate::frame::response::result::ColumnSpec; use crate::frame::response::result::Row; -use scylla_cql::frame::frame_errors::RowsParseError; +use scylla_cql::frame::frame_errors::ResultMetadataAndRowsCountParseError; use scylla_cql::frame::response::result::{self, ResultMetadataHolder}; use scylla_cql::types::deserialize::{DeserializationError, TypeCheckError}; use thiserror::Error; @@ -181,10 +181,9 @@ impl LegacyQueryResult { #[non_exhaustive] #[derive(Error, Clone, Debug)] pub enum IntoLegacyQueryResultError { - // TODO: Replace RowsParseError with narrower error. Done in later commit. /// Failed to lazily deserialize result metadata. #[error("Failed to lazily deserialize result metadata: {0}")] - ResultMetadataParseError(#[from] RowsParseError), + ResultMetadataAndRowsCountParseError(#[from] ResultMetadataAndRowsCountParseError), /// Failed to perform the typecheck against [`Row`] type. #[error("Typecheck error: {0}")] diff --git a/scylla/src/transport/load_balancing/default.rs b/scylla/src/transport/load_balancing/default.rs index f10c139eae..4b380615c2 100644 --- a/scylla/src/transport/load_balancing/default.rs +++ b/scylla/src/transport/load_balancing/default.rs @@ -2861,6 +2861,7 @@ mod latency_awareness { | QueryError::ProtocolError(_) | QueryError::TimeoutError | QueryError::RequestTimeout(_) + | QueryError::ResultMetadataParseError(_) | QueryError::IntoLegacyQueryResultError(_) => true, } } diff --git a/scylla/src/transport/query_result.rs b/scylla/src/transport/query_result.rs index 363292a9b5..742c75be73 100644 --- a/scylla/src/transport/query_result.rs +++ b/scylla/src/transport/query_result.rs @@ -3,7 +3,7 @@ use std::fmt::Debug; use thiserror::Error; use uuid::Uuid; -use scylla_cql::frame::frame_errors::RowsParseError; +use scylla_cql::frame::frame_errors::ResultMetadataAndRowsCountParseError; use scylla_cql::frame::response::result::{ ColumnSpec, ColumnType, DeserializedMetadataAndRawRows, RawMetadataAndRawRows, Row, TableSpec, }; @@ -222,7 +222,9 @@ impl QueryResult { /// # } /// /// ``` - pub fn into_rows_result(self) -> Result, RowsParseError> { + pub fn into_rows_result( + self, + ) -> Result, ResultMetadataAndRowsCountParseError> { let QueryResult { raw_metadata_and_rows, tracing_id, diff --git a/scylla/src/transport/session.rs b/scylla/src/transport/session.rs index 4db0bbde20..45f1a74c46 100644 --- a/scylla/src/transport/session.rs +++ b/scylla/src/transport/session.rs @@ -1799,7 +1799,12 @@ where // Get tracing info let maybe_tracing_info: Option = traces_session_res - .into_rows_result()? + .into_rows_result() + .map_err(|err| { + ProtocolError::Tracing(TracingProtocolError::TracesSessionResultMetadataParseError( + err, + )) + })? .ok_or(ProtocolError::Tracing( TracingProtocolError::TracesSessionNotRows, ))? @@ -1819,12 +1824,16 @@ where }; // Get tracing events - let tracing_event_rows_result = - traces_events_res - .into_rows_result()? - .ok_or(ProtocolError::Tracing( - TracingProtocolError::TracesEventsNotRows, - ))?; + let tracing_event_rows_result = traces_events_res + .into_rows_result() + .map_err(|err| { + ProtocolError::Tracing(TracingProtocolError::TracesEventsResultMetadataParseError( + err, + )) + })? + .ok_or(ProtocolError::Tracing( + TracingProtocolError::TracesEventsNotRows, + ))?; let tracing_event_rows = tracing_event_rows_result.rows().map_err(|err| match err { RowsError::TypeCheckFailed(err) => { ProtocolError::Tracing(TracingProtocolError::TracesEventsInvalidColumnType(err)) diff --git a/scylla/src/transport/speculative_execution.rs b/scylla/src/transport/speculative_execution.rs index 24dfff474b..814e825577 100644 --- a/scylla/src/transport/speculative_execution.rs +++ b/scylla/src/transport/speculative_execution.rs @@ -96,6 +96,7 @@ fn can_be_ignored(result: &Result) -> bool { Err(e) => match e { // Errors that will almost certainly appear for other nodes as well QueryError::BadQuery(_) + | QueryError::ResultMetadataParseError(_) | QueryError::CqlRequestSerialization(_) | QueryError::BodyExtensionsParseError(_) | QueryError::CqlResultParseError(_) From e0e27c0e04bff72fd8c00bc77013587fdc5f14c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 8 Nov 2024 17:31:45 +0100 Subject: [PATCH 06/14] topology: remove RowsParseError usage from query_peers --- scylla/src/transport/errors.rs | 8 ++++++++ scylla/src/transport/topology.rs | 13 ++++++------- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/scylla/src/transport/errors.rs b/scylla/src/transport/errors.rs index ddd18fe0af..4a8a0933d5 100644 --- a/scylla/src/transport/errors.rs +++ b/scylla/src/transport/errors.rs @@ -477,6 +477,14 @@ pub enum MetadataError { #[derive(Error, Debug, Clone)] #[non_exhaustive] pub enum PeersMetadataError { + /// system.peers has invalid column type. + #[error("system.peers has invalid column type: {0}")] + SystemPeersInvalidColumnType(TypeCheckError), + + /// system.local has invalid column type. + #[error("system.local has invalid column type: {0}")] + SystemLocalInvalidColumnType(TypeCheckError), + /// Empty peers list returned during peers metadata fetch. #[error("Peers list is empty")] EmptyPeers, diff --git a/scylla/src/transport/topology.rs b/scylla/src/transport/topology.rs index ab29cd46b2..2b83b7ade2 100644 --- a/scylla/src/transport/topology.rs +++ b/scylla/src/transport/topology.rs @@ -15,7 +15,6 @@ use futures::stream::{self, StreamExt, TryStreamExt}; use futures::Stream; use rand::seq::SliceRandom; use rand::{thread_rng, Rng}; -use scylla_cql::frame::frame_errors::RowsParseError; use scylla_cql::types::deserialize::TypeCheckError; use scylla_macros::DeserializeRow; use std::borrow::BorrowMut; @@ -806,9 +805,9 @@ async fn query_peers(conn: &Arc, connect_port: u16) -> Result() - .map_err(RowsParseError::from)?; + let rows_stream = pager.rows_stream::().map_err(|err| { + MetadataError::Peers(PeersMetadataError::SystemPeersInvalidColumnType(err)) + })?; Ok::<_, QueryError>(rows_stream) }) .into_stream() @@ -823,9 +822,9 @@ async fn query_peers(conn: &Arc, connect_port: u16) -> Result() - .map_err(RowsParseError::from)?; + let rows_stream = pager.rows_stream::().map_err(|err| { + MetadataError::Peers(PeersMetadataError::SystemLocalInvalidColumnType(err)) + })?; Ok::<_, QueryError>(rows_stream) }) .into_stream() From 6a137fd805b8f5c09b66a37061db4b415bc454c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 8 Nov 2024 17:54:46 +0100 Subject: [PATCH 07/14] NextRowError -> LegacyNextRowError We will re-introduce `NewRowError` as an error type returned by current async iterator API. --- scylla/src/transport/iterator.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scylla/src/transport/iterator.rs b/scylla/src/transport/iterator.rs index a46de9d42c..56f9fe4959 100644 --- a/scylla/src/transport/iterator.rs +++ b/scylla/src/transport/iterator.rs @@ -1104,7 +1104,7 @@ mod legacy { /// Couldn't get next typed row from the iterator #[derive(Error, Debug, Clone)] - pub enum NextRowError { + pub enum LegacyNextRowError { /// Query to fetch next page has failed #[error(transparent)] QueryError(#[from] QueryError), @@ -1117,7 +1117,7 @@ mod legacy { /// Fetching pages is asynchronous so `LegacyTypedRowIterator` does not implement the `Iterator` trait.\ /// Instead it uses the asynchronous `Stream` trait impl Stream for LegacyTypedRowIterator { - type Item = Result; + type Item = Result; fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { let mut s = self.as_mut(); @@ -1131,4 +1131,4 @@ mod legacy { // LegacyTypedRowIterator can be moved freely for any RowT so it's Unpin impl Unpin for LegacyTypedRowIterator {} } -pub use legacy::{LegacyRowIterator, LegacyTypedRowIterator, NextRowError}; +pub use legacy::{LegacyNextRowError, LegacyRowIterator, LegacyTypedRowIterator}; From 4717ac1e8b216bd2cbb811ea2a78340653b93acc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 8 Nov 2024 18:11:12 +0100 Subject: [PATCH 08/14] errors: NextPageError and NextRowError These are errors returned by async iterator API. Ultimately, we want them to be independent types returned by the public API. However, as of now, NextRowError needs to be wrapped in QueryError - to address that, further changes to iter API need to applied. These, however, are not in a scope of this PR. This partial change of iter API is introduced because we want to get rid of RowsParseError dependency in this module. --- scylla/src/transport/errors.rs | 24 +++++++++- scylla/src/transport/iterator.rs | 47 ++++++++++++++++--- .../src/transport/load_balancing/default.rs | 1 + scylla/src/transport/speculative_execution.rs | 3 +- 4 files changed, 66 insertions(+), 9 deletions(-) diff --git a/scylla/src/transport/errors.rs b/scylla/src/transport/errors.rs index 4a8a0933d5..b3363a5e43 100644 --- a/scylla/src/transport/errors.rs +++ b/scylla/src/transport/errors.rs @@ -35,7 +35,10 @@ use thiserror::Error; use crate::{authentication::AuthError, frame::response}; -use super::{legacy_query_result::IntoLegacyQueryResultError, query_result::SingleRowError}; +use super::{ + iterator::NextRowError, legacy_query_result::IntoLegacyQueryResultError, + query_result::SingleRowError, +}; /// Error that occurred during query execution #[derive(Error, Debug, Clone)] @@ -102,6 +105,14 @@ pub enum QueryError { #[error("Request timeout: {0}")] RequestTimeout(String), + // TODO: This should not belong here, but it requires changes to error types + // returned in async iterator API. This should be handled in separate PR. + // The reason this needs to be included is that topology.rs makes use of iter API and returns QueryError. + // Once iter API is adjusted, we can then adjust errors returned by topology module (e.g. refactor MetadataError and not include it in QueryError). + /// An error occurred during async iteration over rows of result. + #[error("An error occurred during async iteration over rows of result: {0}")] + NextRowError(#[from] NextRowError), + // TODO: This should not belong here, but it requires changes to error types // returned in `iter` API. It's going to be addressed later in this PR. /// Failed to lazily deserialize result metadata. @@ -180,6 +191,7 @@ impl From for NewSessionError { NewSessionError::IntoLegacyQueryResultError(e) } QueryError::ResultMetadataParseError(e) => NewSessionError::ResultMetadataParseError(e), + QueryError::NextRowError(e) => NewSessionError::NextRowError(e), } } } @@ -284,7 +296,15 @@ pub enum NewSessionError { RequestTimeout(String), // TODO: This should not belong here, but it requires changes to error types - // returned in `iter` API. It's going to be addressed later in this PR. + // returned in async iterator API. This should be handled in separate PR. + // The reason this needs to be included is that topology.rs makes use of iter API and returns QueryError. + // Once iter API is adjusted, we can then adjust errors returned by topology module (e.g. refactor MetadataError and not include it in QueryError). + /// An error occurred during async iteration over rows of result. + #[error("An error occurred during async iteration over rows of result: {0}")] + NextRowError(#[from] NextRowError), + + // TODO: This should not belong here, but it requires changes to error types + // returned in `iter` API. This should be handled in separate PR. /// Failed to lazily deserialize result metadata. #[error("Failed to lazily deserialize result metadata: {0}")] ResultMetadataParseError(#[from] ResultMetadataAndRowsCountParseError), diff --git a/scylla/src/transport/iterator.rs b/scylla/src/transport/iterator.rs index 56f9fe4959..bccb85afa8 100644 --- a/scylla/src/transport/iterator.rs +++ b/scylla/src/transport/iterator.rs @@ -8,12 +8,12 @@ use std::sync::Arc; use std::task::{Context, Poll}; use futures::Stream; -use scylla_cql::frame::frame_errors::RowsParseError; +use scylla_cql::frame::frame_errors::{ResultMetadataAndRowsCountParseError, RowsParseError}; use scylla_cql::frame::response::result::RawMetadataAndRawRows; use scylla_cql::frame::response::NonErrorResponse; use scylla_cql::types::deserialize::result::RawRowLendingIterator; use scylla_cql::types::deserialize::row::{ColumnIterator, DeserializeRow}; -use scylla_cql::types::deserialize::TypeCheckError; +use scylla_cql::types::deserialize::{DeserializationError, TypeCheckError}; use scylla_cql::types::serialize::row::SerializedValues; use std::result::Result; use thiserror::Error; @@ -587,7 +587,7 @@ impl QueryPager { self.current_page .next() .unwrap() - .map_err(|e| RowsParseError::from(e).into()), + .map_err(|err| NextRowError::RowDeserializationError(err).into()), ) } @@ -622,7 +622,14 @@ impl QueryPager { let mut s = self.as_mut(); let received_page = ready_some_ok!(Pin::new(&mut s.page_receiver).poll_recv(cx)); - let raw_rows_with_deserialized_metadata = received_page.rows.deserialize_metadata()?; + + // TODO: see my other comment next to QueryError::NextRowError + // This is the place where conversion happens. To fix this, we need to refactor error types in iterator API. + // The `page_receiver`'s error type should be narrowed from QueryError to some other error type. + let raw_rows_with_deserialized_metadata = + received_page.rows.deserialize_metadata().map_err(|err| { + NextRowError::NextPageError(NextPageError::ResultMetadataParseError(err)) + })?; s.current_page = RawRowLendingIterator::new(raw_rows_with_deserialized_metadata); if let Some(tracing_id) = received_page.tracing_id { @@ -935,7 +942,10 @@ impl QueryPager { // - That future is polled in a tokio::task which isn't going to be // cancelled let page_received = receiver.recv().await.unwrap()?; - let raw_rows_with_deserialized_metadata = page_received.rows.deserialize_metadata()?; + let raw_rows_with_deserialized_metadata = + page_received.rows.deserialize_metadata().map_err(|err| { + NextRowError::NextPageError(NextPageError::ResultMetadataParseError(err)) + })?; Ok(Self { current_page: RawRowLendingIterator::new(raw_rows_with_deserialized_metadata), @@ -1018,7 +1028,7 @@ where self.raw_row_lending_stream.next().await.map(|res| { res.and_then(|column_iterator| { ::deserialize(column_iterator) - .map_err(|err| RowsParseError::from(err).into()) + .map_err(|err| NextRowError::RowDeserializationError(err).into()) }) }) }; @@ -1029,6 +1039,31 @@ where } } +/// An error returned that occurred during next page fetch. +#[derive(Error, Debug, Clone)] +#[non_exhaustive] +pub enum NextPageError { + /// Failed to deserialize result metadata associated with next page response. + #[error("Failed to deserialize result metadata associated with next page response: {0}")] + ResultMetadataParseError(#[from] ResultMetadataAndRowsCountParseError), + // TODO: This should also include a variant representing an error that occurred during + // query that fetches the next page. However, as of now, it would require that we include QueryError here. + // This would introduce a cyclic dependency: QueryError -> NextRowError -> NextPageError -> QueryError. +} + +/// An error returned by async iterator API. +#[derive(Error, Debug, Clone)] +#[non_exhaustive] +pub enum NextRowError { + /// Failed to fetch next page of result. + #[error("Failed to fetch next page of result: {0}")] + NextPageError(#[from] NextPageError), + + /// An error occurred during row deserialization. + #[error("Row deserialization error: {0}")] + RowDeserializationError(#[from] DeserializationError), +} + mod legacy { use super::*; diff --git a/scylla/src/transport/load_balancing/default.rs b/scylla/src/transport/load_balancing/default.rs index 4b380615c2..edf3a1b916 100644 --- a/scylla/src/transport/load_balancing/default.rs +++ b/scylla/src/transport/load_balancing/default.rs @@ -2862,6 +2862,7 @@ mod latency_awareness { | QueryError::TimeoutError | QueryError::RequestTimeout(_) | QueryError::ResultMetadataParseError(_) + | QueryError::NextRowError(_) | QueryError::IntoLegacyQueryResultError(_) => true, } } diff --git a/scylla/src/transport/speculative_execution.rs b/scylla/src/transport/speculative_execution.rs index 814e825577..53bc4afe36 100644 --- a/scylla/src/transport/speculative_execution.rs +++ b/scylla/src/transport/speculative_execution.rs @@ -111,7 +111,8 @@ fn can_be_ignored(result: &Result) -> bool { QueryError::EmptyPlan => false, // Errors that should not appear here, thus should not be ignored - QueryError::IntoLegacyQueryResultError(_) + QueryError::NextRowError(_) + | QueryError::IntoLegacyQueryResultError(_) | QueryError::TimeoutError | QueryError::RequestTimeout(_) | QueryError::MetadataError(_) => false, From c2a700f42a5f2eefa1cbc9187b9d6d9786527a62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 8 Nov 2024 18:16:09 +0100 Subject: [PATCH 09/14] errors: remove QueryError::ResultMetadataParseError variant It's not used anymore after adjustments to iter API. It was introduced temporarily earlier, and can now be safely removed. --- scylla/src/transport/errors.rs | 13 ------------- scylla/src/transport/load_balancing/default.rs | 1 - scylla/src/transport/speculative_execution.rs | 1 - 3 files changed, 15 deletions(-) diff --git a/scylla/src/transport/errors.rs b/scylla/src/transport/errors.rs index b3363a5e43..28bd270f11 100644 --- a/scylla/src/transport/errors.rs +++ b/scylla/src/transport/errors.rs @@ -113,12 +113,6 @@ pub enum QueryError { #[error("An error occurred during async iteration over rows of result: {0}")] NextRowError(#[from] NextRowError), - // TODO: This should not belong here, but it requires changes to error types - // returned in `iter` API. It's going to be addressed later in this PR. - /// Failed to lazily deserialize result metadata. - #[error("Failed to lazily deserialize result metadata: {0}")] - ResultMetadataParseError(#[from] ResultMetadataAndRowsCountParseError), - /// Failed to convert [`QueryResult`][crate::transport::query_result::QueryResult] /// into [`LegacyQueryResult`][crate::transport::legacy_query_result::LegacyQueryResult]. #[error("Failed to convert `QueryResult` into `LegacyQueryResult`: {0}")] @@ -190,7 +184,6 @@ impl From for NewSessionError { QueryError::IntoLegacyQueryResultError(e) => { NewSessionError::IntoLegacyQueryResultError(e) } - QueryError::ResultMetadataParseError(e) => NewSessionError::ResultMetadataParseError(e), QueryError::NextRowError(e) => NewSessionError::NextRowError(e), } } @@ -303,12 +296,6 @@ pub enum NewSessionError { #[error("An error occurred during async iteration over rows of result: {0}")] NextRowError(#[from] NextRowError), - // TODO: This should not belong here, but it requires changes to error types - // returned in `iter` API. This should be handled in separate PR. - /// Failed to lazily deserialize result metadata. - #[error("Failed to lazily deserialize result metadata: {0}")] - ResultMetadataParseError(#[from] ResultMetadataAndRowsCountParseError), - /// Failed to convert [`QueryResult`][crate::transport::query_result::QueryResult] /// into [`LegacyQueryResult`][crate::transport::legacy_query_result::LegacyQueryResult]. #[error("Failed to convert `QueryResult` into `LegacyQueryResult`: {0}")] diff --git a/scylla/src/transport/load_balancing/default.rs b/scylla/src/transport/load_balancing/default.rs index edf3a1b916..b445dea5fb 100644 --- a/scylla/src/transport/load_balancing/default.rs +++ b/scylla/src/transport/load_balancing/default.rs @@ -2861,7 +2861,6 @@ mod latency_awareness { | QueryError::ProtocolError(_) | QueryError::TimeoutError | QueryError::RequestTimeout(_) - | QueryError::ResultMetadataParseError(_) | QueryError::NextRowError(_) | QueryError::IntoLegacyQueryResultError(_) => true, } diff --git a/scylla/src/transport/speculative_execution.rs b/scylla/src/transport/speculative_execution.rs index 53bc4afe36..60344d0a02 100644 --- a/scylla/src/transport/speculative_execution.rs +++ b/scylla/src/transport/speculative_execution.rs @@ -96,7 +96,6 @@ fn can_be_ignored(result: &Result) -> bool { Err(e) => match e { // Errors that will almost certainly appear for other nodes as well QueryError::BadQuery(_) - | QueryError::ResultMetadataParseError(_) | QueryError::CqlRequestSerialization(_) | QueryError::BodyExtensionsParseError(_) | QueryError::CqlResultParseError(_) From 827dcb52d8ce69fc216295b08b37cbc74d1b618b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 8 Nov 2024 18:21:42 +0100 Subject: [PATCH 10/14] legacy: adjust LegacyNextRowError So we do not depend on RowsParseError anymore, new variant is introduced that represents a row deserialization failure. --- scylla/src/transport/iterator.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/scylla/src/transport/iterator.rs b/scylla/src/transport/iterator.rs index bccb85afa8..bda210b31a 100644 --- a/scylla/src/transport/iterator.rs +++ b/scylla/src/transport/iterator.rs @@ -8,7 +8,7 @@ use std::sync::Arc; use std::task::{Context, Poll}; use futures::Stream; -use scylla_cql::frame::frame_errors::{ResultMetadataAndRowsCountParseError, RowsParseError}; +use scylla_cql::frame::frame_errors::ResultMetadataAndRowsCountParseError; use scylla_cql::frame::response::result::RawMetadataAndRawRows; use scylla_cql::frame::response::NonErrorResponse; use scylla_cql::types::deserialize::result::RawRowLendingIterator; @@ -1075,7 +1075,7 @@ mod legacy { } impl Stream for LegacyRowIterator { - type Item = Result; + type Item = Result; fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { let mut s = self.as_mut(); @@ -1085,8 +1085,8 @@ mod legacy { let next_column_iter = ready_some_ok!(next_fut.poll(cx)); - let next_ready_row = - Row::deserialize(next_column_iter).map_err(|e| RowsParseError::from(e).into()); + let next_ready_row = Row::deserialize(next_column_iter) + .map_err(LegacyNextRowError::RowDeserializationError); Poll::Ready(Some(next_ready_row)) } @@ -1147,6 +1147,10 @@ mod legacy { /// Parsing values in row as given types failed #[error(transparent)] FromRowError(#[from] FromRowError), + + /// Row deserialization error + #[error("Row deserialization error: {0}")] + RowDeserializationError(#[from] DeserializationError), } /// Fetching pages is asynchronous so `LegacyTypedRowIterator` does not implement the `Iterator` trait.\ From e9c446c5ddaf7751b64805670a9329da1a6247e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 8 Nov 2024 18:22:59 +0100 Subject: [PATCH 11/14] errors: remove RowsParseError (finally!) --- scylla-cql/src/frame/frame_errors.rs | 21 --------------------- scylla/src/transport/errors.rs | 14 +------------- 2 files changed, 1 insertion(+), 34 deletions(-) diff --git a/scylla-cql/src/frame/frame_errors.rs b/scylla-cql/src/frame/frame_errors.rs index ad95125a94..1f12a6008e 100644 --- a/scylla-cql/src/frame/frame_errors.rs +++ b/scylla-cql/src/frame/frame_errors.rs @@ -14,7 +14,6 @@ pub use super::request::{ use super::response::result::TableSpec; use super::response::CqlResponseKind; use super::TryFromPrimitiveError; -use crate::types::deserialize::{DeserializationError, TypeCheckError}; use thiserror::Error; /// An error returned by `parse_response_body_extensions`. @@ -228,11 +227,6 @@ pub enum CqlResultParseError { PreparedParseError(#[from] PreparedParseError), #[error("RESULT:Rows response deserialization failed: {0}")] RawRowsParseError(#[from] RawRowsAndPagingStateResponseParseError), - - // TODO: This is required for `From for QueryError conversion`. - // It will be removed in later commits. - #[error("RESULT:Rows response deserialization failed: {0}")] - RowsParseError(#[from] RowsParseError), } #[non_exhaustive] @@ -333,21 +327,6 @@ pub enum RawRowsAndPagingStateResponseParseError { PagingStateParseError(LowLevelDeserializationError), } -/// An error type returned when deserialization -/// of `RESULT::Rows` response fails. -#[non_exhaustive] -#[derive(Debug, Error, Clone)] -pub enum RowsParseError { - #[error("Invalid result metadata: {0}")] - ResultMetadataParseError(#[from] ResultMetadataParseError), - #[error("Malformed rows count: {0}")] - RowsCountParseError(LowLevelDeserializationError), - #[error("Data type check prior to deserialization failed: {0}")] - IncomingDataTypeCheckError(#[from] TypeCheckError), - #[error("Data deserialization failed: {0}")] - DataDeserializationError(#[from] DeserializationError), -} - /// An error type returned when deserialization /// of statement's prepared metadata failed. #[non_exhaustive] diff --git a/scylla/src/transport/errors.rs b/scylla/src/transport/errors.rs index 28bd270f11..9b24a199db 100644 --- a/scylla/src/transport/errors.rs +++ b/scylla/src/transport/errors.rs @@ -19,7 +19,7 @@ use scylla_cql::{ CqlErrorParseError, CqlEventParseError, CqlRequestSerializationError, CqlResponseParseError, CqlResultParseError, CqlSupportedParseError, FrameBodyExtensionsParseError, FrameHeaderParseError, - ResultMetadataAndRowsCountParseError, RowsParseError, + ResultMetadataAndRowsCountParseError, }, request::CqlRequestKind, response::CqlResponseKind, @@ -201,18 +201,6 @@ impl From for QueryError { } } -impl From for QueryError { - fn from(err: RowsParseError) -> Self { - let err: CqlResultParseError = err.into(); - let err: CqlResponseParseError = err.into(); - let err: RequestError = err.into(); - let err: UserRequestError = err.into(); - let err: QueryError = err.into(); - - err - } -} - /// Error that occurred during session creation #[derive(Error, Debug, Clone)] #[non_exhaustive] From 9f876161220190c1523056ba7ca618c42ddc42ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 8 Nov 2024 18:28:04 +0100 Subject: [PATCH 12/14] treewide: fix typo occure... -> occurre... --- scylla-cql/src/types/deserialize/row.rs | 4 ++-- scylla-cql/src/types/deserialize/value.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/scylla-cql/src/types/deserialize/row.rs b/scylla-cql/src/types/deserialize/row.rs index ad6514518c..8de97489d7 100644 --- a/scylla-cql/src/types/deserialize/row.rs +++ b/scylla-cql/src/types/deserialize/row.rs @@ -347,7 +347,7 @@ pub enum BuiltinTypeCheckErrorKind { /// Duplicated column in DB metadata. DuplicatedColumn { - /// Column index of the second occurence of the column with the same name. + /// Column index of the second occurrence of the column with the same name. column_index: usize, /// The name of the duplicated column. @@ -401,7 +401,7 @@ impl Display for BuiltinTypeCheckErrorKind { ), BuiltinTypeCheckErrorKind::DuplicatedColumn { column_name, column_index } => write!( f, - "column {} occurs more than once in DB metadata; second occurence is at column index {}", + "column {} occurs more than once in DB metadata; second occurrence is at column index {}", column_name, column_index, ), diff --git a/scylla-cql/src/types/deserialize/value.rs b/scylla-cql/src/types/deserialize/value.rs index b07c6eb478..492984c3fd 100644 --- a/scylla-cql/src/types/deserialize/value.rs +++ b/scylla-cql/src/types/deserialize/value.rs @@ -1572,7 +1572,7 @@ pub enum TupleTypeCheckErrorKind { /// The index of the field whose type check failed. position: usize, - /// The type check error that occured. + /// The type check error that occurred. err: TypeCheckError, }, } @@ -1651,7 +1651,7 @@ pub enum UdtTypeCheckErrorKind { /// The name of the field whose type check failed. field_name: String, - /// Inner type check error that occured. + /// Inner type check error that occurred. err: TypeCheckError, }, } From ed4a4270ba6426e01b2ad57cf1f4c71bfa1f7ac0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 8 Nov 2024 19:02:57 +0100 Subject: [PATCH 13/14] qr: flatten return type of QueryResult::into_rows_result to Result<...> Instead of returning Result, _>, we will simply return a Result. IntoRowsResultError is introduced specifically for this method. Adjusted all of usages of this API. Most of the changes were simply to replace `unwrap().unwrap()` to single `unwrap()`. There are 4 places that require more focus during review: - print_result() method in `cql-sh.rs` example. - changes to SchemaVersionFetchError (and corresponding code in connection.rs) - changes to TracingProtocolError (and corresponding code in session.rs) - adjustment to `scylla_supports_tablets` in test_utils.rs --- docs/source/queries/paged.md | 2 +- docs/source/queries/result.md | 32 +++++---- docs/source/queries/simple.md | 4 +- examples/compare-tokens.rs | 1 - examples/cqlsh-rs.rs | 43 ++++++------ examples/custom_deserialization.rs | 5 +- examples/get_by_name.rs | 5 +- examples/select-paging.rs | 8 +-- examples/tower.rs | 3 +- scylla/src/lib.rs | 9 +-- scylla/src/transport/caching_session.rs | 8 +-- scylla/src/transport/connection.rs | 6 +- scylla/src/transport/cql_collections_test.rs | 1 - scylla/src/transport/cql_types_test.rs | 37 ----------- scylla/src/transport/cql_value_test.rs | 2 - scylla/src/transport/errors.rs | 40 ++++-------- scylla/src/transport/query_result.rs | 65 ++++++++++--------- scylla/src/transport/session.rs | 30 ++------- scylla/src/transport/session_test.rs | 31 +-------- .../transport/silent_prepare_batch_test.rs | 1 - scylla/src/utils/test_utils.rs | 6 +- .../integration/skip_metadata_optimization.rs | 2 - 22 files changed, 112 insertions(+), 229 deletions(-) diff --git a/docs/source/queries/paged.md b/docs/source/queries/paged.md index 516a149ab9..3944de5682 100644 --- a/docs/source/queries/paged.md +++ b/docs/source/queries/paged.md @@ -202,7 +202,7 @@ loop { .execute_single_page(&paged_prepared, &[], paging_state) .await?; - let rows_res = res.into_rows_result()?.unwrap(); + let rows_res = res.into_rows_result()?; println!( "Paging state response from the prepared statement execution: {:#?} ({} rows)", diff --git a/docs/source/queries/result.md b/docs/source/queries/result.md index 5e88e15109..db63637e7b 100644 --- a/docs/source/queries/result.md +++ b/docs/source/queries/result.md @@ -41,20 +41,18 @@ Additionally, [`QueryResult`](https://docs.rs/scylla/latest/scylla/transport/que let result = session .query_unpaged("SELECT a from ks.tab", &[]) .await? - .into_rows_result()? - .unwrap(); + .into_rows_result()?; for row in result.rows::<(i32,)>()? { let (int_value,): (i32,) = row?; } // first_row gets the first row and parses it as the given type -let first_int_val: Option<(i32,)> = session +let first_int_val: (i32,) = session .query_unpaged("SELECT a from ks.tab", &[]) .await? .into_rows_result()? - .map(|res| res.first_row::<(i32,)>()) - .transpose()?; + .first_row::<(i32,)>()?; // result_not_rows fails when the response is rows session.query_unpaged("INSERT INTO ks.tab (a) VALUES (0)", &[]).await?.result_not_rows()?; @@ -75,13 +73,13 @@ To properly handle `NULL` values parse column as an `Option<>`: use scylla::IntoTypedRows; // Parse row as two columns containing an int and text which might be null -if let Some(rows_result) = session.query_unpaged("SELECT a, b from ks.tab", &[]) +let rows_result = session + .query_unpaged("SELECT a, b from ks.tab", &[]) .await? - .into_rows_result()? -{ - for row in rows_result.rows::<(i32, Option<&str>)>()? { - let (int_value, str_or_null): (i32, Option<&str>) = row?; - } + .into_rows_result()?; + +for row in rows_result.rows::<(i32, Option<&str>)>()? { + let (int_value, str_or_null): (i32, Option<&str>) = row?; } # Ok(()) # } @@ -111,13 +109,13 @@ struct MyRow { } // Parse row as two columns containing an int and text which might be null -if let Some(result_rows) = session.query_unpaged("SELECT a, b from ks.tab", &[]) +let result_rows = session + .query_unpaged("SELECT a, b from ks.tab", &[]) .await? - .into_rows_result()? -{ - for row in result_rows.rows::()? { - let my_row: MyRow = row?; - } + .into_rows_result()?; + +for row in result_rows.rows::()? { + let my_row: MyRow = row?; } # Ok(()) # } diff --git a/docs/source/queries/simple.md b/docs/source/queries/simple.md index 468c10c93a..a917998593 100644 --- a/docs/source/queries/simple.md +++ b/docs/source/queries/simple.md @@ -103,8 +103,8 @@ use scylla::IntoTypedRows; // Query rows from the table and print them let result = session.query_unpaged("SELECT a FROM ks.tab", &[]) .await? - .into_rows_result()? - .unwrap(); + .into_rows_result()?; + let mut iter = result.rows::<(i32,)>()?; while let Some(read_row) = iter.next().transpose()? { println!("Read a value from row: {}", read_row.0); diff --git a/examples/compare-tokens.rs b/examples/compare-tokens.rs index 5350006b99..ab4bbb6b16 100644 --- a/examples/compare-tokens.rs +++ b/examples/compare-tokens.rs @@ -52,7 +52,6 @@ async fn main() -> Result<()> { ) .await? .into_rows_result()? - .expect("Got not Rows result") .single_row()?; assert_eq!(t, qt); println!("token for {}: {}", pk, t); diff --git a/examples/cqlsh-rs.rs b/examples/cqlsh-rs.rs index ba46519636..6eda4a35f0 100644 --- a/examples/cqlsh-rs.rs +++ b/examples/cqlsh-rs.rs @@ -4,9 +4,10 @@ use rustyline::error::ReadlineError; use rustyline::{CompletionType, Config, Context, Editor}; use rustyline_derive::{Helper, Highlighter, Hinter, Validator}; use scylla::frame::response::result::Row; +use scylla::transport::query_result::IntoRowsResultError; use scylla::transport::session::Session; use scylla::transport::Compression; -use scylla::QueryRowsResult; +use scylla::QueryResult; use scylla::SessionBuilder; use std::env; @@ -176,24 +177,27 @@ impl Completer for CqlHelper { } } -fn print_result(result: Option<&QueryRowsResult>) { - if let Some(rows_result) = result { - for row in rows_result.rows::().unwrap() { - let row = row.unwrap(); - for column in &row.columns { - print!("|"); - print!( - " {:16}", - match column { - None => "null".to_owned(), - Some(value) => format!("{:?}", value), - } - ); +fn print_result(result: QueryResult) -> Result<(), IntoRowsResultError> { + match result.into_rows_result() { + Ok(rows_result) => { + for row in rows_result.rows::().unwrap() { + let row = row.unwrap(); + for column in &row.columns { + print!("|"); + print!( + " {:16}", + match column { + None => "null".to_owned(), + Some(value) => format!("{:?}", value), + } + ); + } + println!("|"); } - println!("|") + Ok(()) } - } else { - println!("OK"); + Err(IntoRowsResultError::ResultNotRows) => Ok(println!("OK")), + Err(e) => Err(e), } } @@ -226,10 +230,7 @@ async fn main() -> Result<()> { let maybe_res = session.query_unpaged(line, &[]).await; match maybe_res { Err(err) => println!("Error: {}", err), - Ok(res) => { - let rows_res = res.into_rows_result()?; - print_result(rows_res.as_ref()) - } + Ok(res) => print_result(res)?, } } Err(ReadlineError::Interrupted) => continue, diff --git a/examples/custom_deserialization.rs b/examples/custom_deserialization.rs index 0306ebe879..5a5991edfe 100644 --- a/examples/custom_deserialization.rs +++ b/examples/custom_deserialization.rs @@ -1,4 +1,4 @@ -use anyhow::{Context, Result}; +use anyhow::Result; use scylla::deserialize::DeserializeValue; use scylla::frame::response::result::ColumnType; use scylla::transport::session::Session; @@ -55,8 +55,7 @@ async fn main() -> Result<()> { (), ) .await? - .into_rows_result()? - .context("Expected Result:Rows response, got a different Result response.")?; + .into_rows_result()?; let (v,) = rows_result.single_row::<(MyType,)>()?; assert_eq!(v, MyType("asdf")); diff --git a/examples/get_by_name.rs b/examples/get_by_name.rs index 1caca3e3df..4aca66f665 100644 --- a/examples/get_by_name.rs +++ b/examples/get_by_name.rs @@ -1,4 +1,4 @@ -use anyhow::{anyhow, Context as _, Result}; +use anyhow::{anyhow, Result}; use scylla::frame::response::result::Row; use scylla::transport::session::Session; use scylla::SessionBuilder; @@ -39,8 +39,7 @@ async fn main() -> Result<()> { let rows_result = session .query_unpaged("SELECT pk, ck, value FROM examples_ks.get_by_name", &[]) .await? - .into_rows_result()? - .context("Response is not of Rows type")?; + .into_rows_result()?; let col_specs = rows_result.column_specs(); let (ck_idx, _) = col_specs .get_by_name("ck") diff --git a/examples/select-paging.rs b/examples/select-paging.rs index b3c7501feb..00aa961fc8 100644 --- a/examples/select-paging.rs +++ b/examples/select-paging.rs @@ -51,9 +51,7 @@ async fn main() -> Result<()> { .query_single_page(paged_query.clone(), &[], paging_state) .await?; - let res = res - .into_rows_result()? - .expect("Got result different than Rows"); + let res = res.into_rows_result()?; println!( "Paging state: {:#?} ({} rows)", @@ -85,9 +83,7 @@ async fn main() -> Result<()> { .execute_single_page(&paged_prepared, &[], paging_state) .await?; - let res = res - .into_rows_result()? - .expect("Got result different than Rows"); + let res = res.into_rows_result()?; println!( "Paging state from the prepared statement execution: {:#?} ({} rows)", diff --git a/examples/tower.rs b/examples/tower.rs index c34c3f3986..f521b1b614 100644 --- a/examples/tower.rs +++ b/examples/tower.rs @@ -45,8 +45,7 @@ async fn main() -> anyhow::Result<()> { let rows_result = session .call("SELECT keyspace_name, table_name FROM system_schema.tables;".into()) .await? - .into_rows_result()? - .expect("Got result different than Rows"); + .into_rows_result()?; let print_text = |t: &Option| { t.as_ref() diff --git a/scylla/src/lib.rs b/scylla/src/lib.rs index 8b62c0c2b4..39387f6210 100644 --- a/scylla/src/lib.rs +++ b/scylla/src/lib.rs @@ -82,12 +82,9 @@ //! .await? //! .into_rows_result()?; //! -//! -//! if let Some(rows) = query_rows { -//! for row in rows.rows()? { -//! // Parse row as int and text \ -//! let (int_val, text_val): (i32, &str) = row?; -//! } +//! for row in query_rows.rows()? { +//! // Parse row as int and text \ +//! let (int_val, text_val): (i32, &str) = row?; //! } //! # Ok(()) //! # } diff --git a/scylla/src/transport/caching_session.rs b/scylla/src/transport/caching_session.rs index 192ad6dd46..108752e4c9 100644 --- a/scylla/src/transport/caching_session.rs +++ b/scylla/src/transport/caching_session.rs @@ -428,7 +428,7 @@ mod tests { .execute_unpaged("select * from test_table", &[]) .await .unwrap(); - let result_rows = result.into_rows_result().unwrap().unwrap(); + let result_rows = result.into_rows_result().unwrap(); assert_eq!(1, session.cache.len()); assert_eq!(1, result_rows.rows_num()); @@ -438,7 +438,7 @@ mod tests { .await .unwrap(); - let result_rows = result.into_rows_result().unwrap().unwrap(); + let result_rows = result.into_rows_result().unwrap(); assert_eq!(1, session.cache.len()); assert_eq!(1, result_rows.rows_num()); @@ -485,7 +485,7 @@ mod tests { .unwrap(); assert_eq!(1, session.cache.len()); - assert_eq!(1, result.into_rows_result().unwrap().unwrap().rows_num()); + assert_eq!(1, result.into_rows_result().unwrap().rows_num()); } async fn assert_test_batch_table_rows_contain( @@ -498,7 +498,6 @@ mod tests { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, i32)>() .unwrap() .map(|r| r.unwrap()) @@ -710,7 +709,6 @@ mod tests { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, i64)>() .unwrap() .collect::, _>>() diff --git a/scylla/src/transport/connection.rs b/scylla/src/transport/connection.rs index 1ce685b61c..26e41ae723 100644 --- a/scylla/src/transport/connection.rs +++ b/scylla/src/transport/connection.rs @@ -1437,12 +1437,9 @@ impl Connection { .into_rows_result() .map_err(|err| { QueryError::ProtocolError(ProtocolError::SchemaVersionFetch( - SchemaVersionFetchError::ResultMetadataParseError(err), + SchemaVersionFetchError::TracesEventsIntoRowsResultError(err), )) })? - .ok_or(QueryError::ProtocolError( - ProtocolError::SchemaVersionFetch(SchemaVersionFetchError::ResultNotRows), - ))? .single_row::<(Uuid,)>() .map_err(|err| { ProtocolError::SchemaVersionFetch(SchemaVersionFetchError::SingleRowError(err)) @@ -2625,7 +2622,6 @@ mod tests { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, Vec)>() .unwrap() .collect::, _>>() diff --git a/scylla/src/transport/cql_collections_test.rs b/scylla/src/transport/cql_collections_test.rs index 475bd47eeb..9cdb34ce5a 100644 --- a/scylla/src/transport/cql_collections_test.rs +++ b/scylla/src/transport/cql_collections_test.rs @@ -52,7 +52,6 @@ async fn insert_and_select( .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(SelectT,)>() .unwrap() .0; diff --git a/scylla/src/transport/cql_types_test.rs b/scylla/src/transport/cql_types_test.rs index 2863df76c0..30f406a1db 100644 --- a/scylla/src/transport/cql_types_test.rs +++ b/scylla/src/transport/cql_types_test.rs @@ -101,7 +101,6 @@ where .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(T,)>() .unwrap() .map(Result::unwrap) @@ -222,7 +221,6 @@ async fn test_cql_varint() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(CqlVarint,)>() .unwrap() .map(Result::unwrap) @@ -300,7 +298,6 @@ async fn test_counter() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(Counter,)>() .unwrap() .map(Result::unwrap) @@ -379,7 +376,6 @@ async fn test_naive_date_04() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(NaiveDate,)>() .unwrap() .next() @@ -405,7 +401,6 @@ async fn test_naive_date_04() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(NaiveDate,)>() .unwrap(); assert_eq!(read_date, *naive_date); @@ -447,7 +442,6 @@ async fn test_cql_date() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(CqlDate,)>() .unwrap(); @@ -533,7 +527,6 @@ async fn test_date_03() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(Date,)>() .ok() .map(|val| val.0); @@ -556,7 +549,6 @@ async fn test_date_03() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(Date,)>() .unwrap(); assert_eq!(read_date, *date); @@ -602,7 +594,6 @@ async fn test_cql_time() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(CqlTime,)>() .unwrap(); @@ -623,7 +614,6 @@ async fn test_cql_time() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(CqlTime,)>() .unwrap(); @@ -704,7 +694,6 @@ async fn test_naive_time_04() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(NaiveTime,)>() .unwrap(); @@ -725,7 +714,6 @@ async fn test_naive_time_04() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(NaiveTime,)>() .unwrap(); assert_eq!(read_time, *time); @@ -790,7 +778,6 @@ async fn test_time_03() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(Time,)>() .unwrap(); @@ -811,7 +798,6 @@ async fn test_time_03() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(Time,)>() .unwrap(); assert_eq!(read_time, *time); @@ -867,7 +853,6 @@ async fn test_cql_timestamp() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(CqlTimestamp,)>() .unwrap(); @@ -888,7 +873,6 @@ async fn test_cql_timestamp() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(CqlTimestamp,)>() .unwrap(); @@ -968,7 +952,6 @@ async fn test_date_time_04() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(DateTime,)>() .unwrap(); @@ -989,7 +972,6 @@ async fn test_date_time_04() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(DateTime,)>() .unwrap(); assert_eq!(read_datetime, *datetime); @@ -1020,7 +1002,6 @@ async fn test_date_time_04() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(DateTime,)>() .unwrap(); assert_eq!(read_datetime, nanosecond_precision_1st_half_rounded); @@ -1049,7 +1030,6 @@ async fn test_date_time_04() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(DateTime,)>() .unwrap(); assert_eq!(read_datetime, nanosecond_precision_2nd_half_rounded); @@ -1141,7 +1121,6 @@ async fn test_offset_date_time_03() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(OffsetDateTime,)>() .unwrap(); @@ -1162,7 +1141,6 @@ async fn test_offset_date_time_03() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(OffsetDateTime,)>() .unwrap(); assert_eq!(read_datetime, *datetime); @@ -1193,7 +1171,6 @@ async fn test_offset_date_time_03() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(OffsetDateTime,)>() .unwrap(); assert_eq!(read_datetime, nanosecond_precision_1st_half_rounded); @@ -1222,7 +1199,6 @@ async fn test_offset_date_time_03() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(OffsetDateTime,)>() .unwrap(); assert_eq!(read_datetime, nanosecond_precision_2nd_half_rounded); @@ -1274,7 +1250,6 @@ async fn test_timeuuid() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(CqlTimeuuid,)>() .unwrap(); @@ -1296,7 +1271,6 @@ async fn test_timeuuid() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(CqlTimeuuid,)>() .unwrap(); @@ -1368,7 +1342,6 @@ async fn test_timeuuid_ordering() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(CqlTimeuuid,)>() .unwrap() .map(|r| r.unwrap().0) @@ -1450,7 +1423,6 @@ async fn test_inet() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(IpAddr,)>() .unwrap(); @@ -1468,7 +1440,6 @@ async fn test_inet() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(IpAddr,)>() .unwrap(); @@ -1522,7 +1493,6 @@ async fn test_blob() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(Vec,)>() .unwrap(); @@ -1540,7 +1510,6 @@ async fn test_blob() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(Vec,)>() .unwrap(); @@ -1631,7 +1600,6 @@ async fn test_udt_after_schema_update() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(UdtV1,)>() .unwrap(); @@ -1651,7 +1619,6 @@ async fn test_udt_after_schema_update() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(UdtV1,)>() .unwrap(); @@ -1675,7 +1642,6 @@ async fn test_udt_after_schema_update() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(UdtV2,)>() .unwrap(); @@ -1708,7 +1674,6 @@ async fn test_empty() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(CqlValue,)>() .unwrap(); @@ -1728,7 +1693,6 @@ async fn test_empty() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .first_row::<(CqlValue,)>() .unwrap(); @@ -1817,7 +1781,6 @@ async fn test_udt_with_missing_field() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(TR,)>() .unwrap() .0; diff --git a/scylla/src/transport/cql_value_test.rs b/scylla/src/transport/cql_value_test.rs index c5c2eedd55..932b72934b 100644 --- a/scylla/src/transport/cql_value_test.rs +++ b/scylla/src/transport/cql_value_test.rs @@ -62,7 +62,6 @@ async fn test_cqlvalue_udt() { .await .unwrap() .into_rows_result() - .unwrap() .unwrap(); let (received_udt_cql_value,) = rows_result.single_row::<(CqlValue,)>().unwrap(); @@ -115,7 +114,6 @@ async fn test_cqlvalue_duration() { .await .unwrap() .into_rows_result() - .unwrap() .unwrap(); let mut rows_iter = rows_result.rows::<(CqlValue,)>().unwrap(); diff --git a/scylla/src/transport/errors.rs b/scylla/src/transport/errors.rs index 9b24a199db..778f33f295 100644 --- a/scylla/src/transport/errors.rs +++ b/scylla/src/transport/errors.rs @@ -19,7 +19,6 @@ use scylla_cql::{ CqlErrorParseError, CqlEventParseError, CqlRequestSerializationError, CqlResponseParseError, CqlResultParseError, CqlSupportedParseError, FrameBodyExtensionsParseError, FrameHeaderParseError, - ResultMetadataAndRowsCountParseError, }, request::CqlRequestKind, response::CqlResponseKind, @@ -36,8 +35,9 @@ use thiserror::Error; use crate::{authentication::AuthError, frame::response}; use super::{ - iterator::NextRowError, legacy_query_result::IntoLegacyQueryResultError, - query_result::SingleRowError, + iterator::NextRowError, + legacy_query_result::IntoLegacyQueryResultError, + query_result::{IntoRowsResultError, SingleRowError}, }; /// Error that occurred during query execution @@ -373,13 +373,9 @@ pub enum UseKeyspaceProtocolError { #[derive(Error, Debug, Clone)] #[non_exhaustive] pub enum SchemaVersionFetchError { - /// Schema version query returned non-rows result. - #[error("Schema version query returned non-rows result")] - ResultNotRows, - - /// Failed to lazily deserialize result metadata. - #[error("Failed to lazily deserialize result metadata")] - ResultMetadataParseError(ResultMetadataAndRowsCountParseError), + /// Failed to convert schema version query result into rows result. + #[error("Failed to convert schema version query result into rows result: {0}")] + TracesEventsIntoRowsResultError(IntoRowsResultError), /// Failed to deserialize a single row from schema version query response. #[error(transparent)] @@ -390,15 +386,9 @@ pub enum SchemaVersionFetchError { #[derive(Error, Debug, Clone)] #[non_exhaustive] pub enum TracingProtocolError { - /// Response to system_traces.session is not RESULT:Rows. - #[error("Response to system_traces.session is not RESULT:Rows")] - TracesSessionNotRows, - - /// Failed to lazily deserialize result metadata from response to system_traces.session query. - #[error( - "Failed to lazily deserialize result metadata from response to system_traces.session query" - )] - TracesSessionResultMetadataParseError(ResultMetadataAndRowsCountParseError), + /// Failed to convert result of system_traces.session query to rows result. + #[error("Failed to convert result of system_traces.session query to rows result")] + TracesSessionIntoRowsResultError(IntoRowsResultError), /// system_traces.session has invalid column type. #[error("system_traces.session has invalid column type: {0}")] @@ -408,15 +398,9 @@ pub enum TracingProtocolError { #[error("Response to system_traces.session failed to deserialize: {0}")] TracesSessionDeserializationFailed(DeserializationError), - /// Response to system_traces.events is not RESULT:Rows. - #[error("Response to system_traces.events is not RESULT:Rows")] - TracesEventsNotRows, - - /// Failed to lazily deserialize result metadata from response to system_traces.events query. - #[error( - "Failed to lazily deserialize result metadata from response to system_traces.events query" - )] - TracesEventsResultMetadataParseError(ResultMetadataAndRowsCountParseError), + /// Failed to convert result of system_traces.events query to rows result. + #[error("Failed to convert result of system_traces.events query to rows result")] + TracesEventsIntoRowsResultError(IntoRowsResultError), /// system_traces.events has invalid column type. #[error("system_traces.events has invalid column type: {0}")] diff --git a/scylla/src/transport/query_result.rs b/scylla/src/transport/query_result.rs index 742c75be73..780a8b8f61 100644 --- a/scylla/src/transport/query_result.rs +++ b/scylla/src/transport/query_result.rs @@ -203,28 +203,23 @@ impl QueryResult { /// Transforms itself into the Rows result type to enable deserializing rows. /// Deserializes result metadata and allocates it. /// - /// Returns `None` if the response is not of Rows kind. + /// Returns an error if the response is not of Rows kind or metadata deserialization failed. /// /// ```rust /// # use scylla::transport::query_result::{QueryResult, QueryRowsResult}; /// # fn example(query_result: QueryResult) -> Result<(), Box> { - /// let maybe_rows_result = query_result.into_rows_result()?; - /// if let Some(rows_result) = maybe_rows_result { - /// let mut rows_iter = rows_result.rows::<(i32, &str)>()?; - /// while let Some((num, text)) = rows_iter.next().transpose()? { - /// // do something with `num` and `text`` - /// } - /// } else { - /// // Response was not Result:Rows, but some other kind of Result. + /// let rows_result = query_result.into_rows_result()?; + /// + /// let mut rows_iter = rows_result.rows::<(i32, &str)>()?; + /// while let Some((num, text)) = rows_iter.next().transpose()? { + /// // do something with `num` and `text`` /// } /// /// Ok(()) /// # } /// /// ``` - pub fn into_rows_result( - self, - ) -> Result, ResultMetadataAndRowsCountParseError> { + pub fn into_rows_result(self) -> Result { let QueryResult { raw_metadata_and_rows, tracing_id, @@ -239,7 +234,7 @@ impl QueryResult { tracing_id, }) }) - .transpose() + .unwrap_or(Err(IntoRowsResultError::ResultNotRows)) } /// Transforms itself into the legacy result type, by eagerly deserializing rows @@ -291,14 +286,11 @@ impl QueryResult { /// ```rust /// # use scylla::transport::query_result::QueryResult; /// # fn example(query_result: QueryResult) -> Result<(), Box> { -/// let maybe_rows_result = query_result.into_rows_result()?; -/// if let Some(rows_result) = maybe_rows_result { -/// let mut rows_iter = rows_result.rows::<(i32, &str)>()?; -/// while let Some((num, text)) = rows_iter.next().transpose()? { -/// // do something with `num` and `text`` -/// } -/// } else { -/// // Response was not Result:Rows, but some other kind of Result. +/// let rows_result = query_result.into_rows_result()?; +/// +/// let mut rows_iter = rows_result.rows::<(i32, &str)>()?; +/// while let Some((num, text)) = rows_iter.next().transpose()? { +/// // do something with `num` and `text`` /// } /// /// Ok(()) @@ -419,6 +411,19 @@ impl QueryRowsResult { } } +/// An error returned by [`QueryResult::into_rows_result`] +#[derive(Debug, Error, Clone)] +pub enum IntoRowsResultError { + /// Result is not of Rows kind + #[error("Result is not of Rows kind")] + ResultNotRows, + + // transparent because the underlying error provides enough context. + /// Failed to lazily deserialize result metadata. + #[error(transparent)] + ResultMetadataLazyDeserializationError(#[from] ResultMetadataAndRowsCountParseError), +} + /// An error returned by [`QueryRowsResult::rows`]. #[derive(Debug, Error)] pub enum RowsError { @@ -566,8 +571,8 @@ mod tests { // Not RESULT::Rows response -> no column specs { let rqr = QueryResult::new(None, None, Vec::new()); - let qr = rqr.into_rows_result().unwrap(); - assert_matches!(qr, None); + let qr = rqr.into_rows_result(); + assert_matches!(qr, Err(IntoRowsResultError::ResultNotRows)); } // RESULT::Rows response -> some column specs @@ -577,7 +582,7 @@ mod tests { let rr = RawMetadataAndRawRows::new_for_test(None, Some(metadata), false, 0, &[]) .unwrap(); let rqr = QueryResult::new(Some(rr), None, Vec::new()); - let qr = rqr.into_rows_result().unwrap().unwrap(); + let qr = rqr.into_rows_result().unwrap(); let column_specs = qr.column_specs(); assert_eq!(column_specs.len(), n); @@ -624,8 +629,8 @@ mod tests { // Not RESULT::Rows { let rqr = QueryResult::new(None, None, Vec::new()); - let qr = rqr.into_rows_result().unwrap(); - assert_matches!(qr, None); + let qr = rqr.into_rows_result(); + assert_matches!(qr, Err(IntoRowsResultError::ResultNotRows)); } // RESULT::Rows with 0 rows @@ -634,7 +639,7 @@ mod tests { let rqr = QueryResult::new(Some(rr), None, Vec::new()); assert_matches!(rqr.result_not_rows(), Err(ResultNotRowsError)); - let qr = rqr.into_rows_result().unwrap().unwrap(); + let qr = rqr.into_rows_result().unwrap(); // Type check error { @@ -680,8 +685,8 @@ mod tests { assert_matches!(rqr.result_not_rows(), Err(ResultNotRowsError)); } - let qr_good_data = rqr_good_data.into_rows_result().unwrap().unwrap(); - let qr_bad_data = rqr_bad_data.into_rows_result().unwrap().unwrap(); + let qr_good_data = rqr_good_data.into_rows_result().unwrap(); + let qr_bad_data = rqr_bad_data.into_rows_result().unwrap(); for qr in [&qr_good_data, &qr_bad_data] { // Type check error @@ -737,7 +742,7 @@ mod tests { let rqr = QueryResult::new(Some(rr), None, Vec::new()); assert_matches!(rqr.result_not_rows(), Err(ResultNotRowsError)); - let qr = rqr.into_rows_result().unwrap().unwrap(); + let qr = rqr.into_rows_result().unwrap(); // Type check error { diff --git a/scylla/src/transport/session.rs b/scylla/src/transport/session.rs index 45f1a74c46..2ed5ca9016 100644 --- a/scylla/src/transport/session.rs +++ b/scylla/src/transport/session.rs @@ -514,11 +514,9 @@ impl GenericSession { /// .await? /// .into_rows_result()?; /// - /// if let Some(rows) = query_rows { - /// for row in rows.rows()? { - /// // Parse row as int and text. - /// let (int_val, text_val): (i32, &str) = row?; - /// } + /// for row in query_rows.rows()? { + /// // Parse row as int and text. + /// let (int_val, text_val): (i32, &str) = row?; /// } /// # Ok(()) /// # } @@ -562,7 +560,6 @@ impl GenericSession { /// // Do something with a single page of results. /// for row in res /// .into_rows_result()? - /// .unwrap() /// .rows::<(i32, &str)>()? /// { /// let (a, b) = row?; @@ -725,7 +722,6 @@ impl GenericSession { /// // Do something with a single page of results. /// for row in res /// .into_rows_result()? - /// .unwrap() /// .rows::<(i32, &str)>()? /// { /// let (a, b) = row?; @@ -1801,13 +1797,8 @@ where let maybe_tracing_info: Option = traces_session_res .into_rows_result() .map_err(|err| { - ProtocolError::Tracing(TracingProtocolError::TracesSessionResultMetadataParseError( - err, - )) + ProtocolError::Tracing(TracingProtocolError::TracesSessionIntoRowsResultError(err)) })? - .ok_or(ProtocolError::Tracing( - TracingProtocolError::TracesSessionNotRows, - ))? .maybe_first_row() .map_err(|err| match err { MaybeFirstRowError::TypeCheckFailed(e) => { @@ -1824,16 +1815,9 @@ where }; // Get tracing events - let tracing_event_rows_result = traces_events_res - .into_rows_result() - .map_err(|err| { - ProtocolError::Tracing(TracingProtocolError::TracesEventsResultMetadataParseError( - err, - )) - })? - .ok_or(ProtocolError::Tracing( - TracingProtocolError::TracesEventsNotRows, - ))?; + let tracing_event_rows_result = traces_events_res.into_rows_result().map_err(|err| { + ProtocolError::Tracing(TracingProtocolError::TracesEventsIntoRowsResultError(err)) + })?; let tracing_event_rows = tracing_event_rows_result.rows().map_err(|err| match err { RowsError::TypeCheckFailed(err) => { ProtocolError::Tracing(TracingProtocolError::TracesEventsInvalidColumnType(err)) diff --git a/scylla/src/transport/session_test.rs b/scylla/src/transport/session_test.rs index 6c4beeb4ae..8027360a74 100644 --- a/scylla/src/transport/session_test.rs +++ b/scylla/src/transport/session_test.rs @@ -109,7 +109,7 @@ async fn test_unprepared_statement() { .await .unwrap(); - let rows = query_result.into_rows_result().unwrap().unwrap(); + let rows = query_result.into_rows_result().unwrap(); let col_specs = rows.column_specs(); assert_eq!(col_specs.get_by_name("a").unwrap().0, 0); @@ -154,7 +154,6 @@ async fn test_unprepared_statement() { let mut page_results = rs_manual .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, i32, String)>() .unwrap() .collect::, _>>() @@ -244,7 +243,6 @@ async fn test_prepared_statement() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(i64,)>() .unwrap(); let token = Token::new(value); @@ -266,7 +264,6 @@ async fn test_prepared_statement() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(i64,)>() .unwrap(); let token = Token::new(value); @@ -291,7 +288,6 @@ async fn test_prepared_statement() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, i32, String)>() .unwrap() .collect::, _>>() @@ -312,7 +308,6 @@ async fn test_prepared_statement() { let mut page_results = rs_manual .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, i32, String)>() .unwrap() .collect::, _>>() @@ -336,7 +331,6 @@ async fn test_prepared_statement() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(i32, i32, String, i32, Option)>() .unwrap(); assert!(e.is_none()); @@ -385,7 +379,6 @@ async fn test_prepared_statement() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row() .unwrap(); assert_eq!(input, output) @@ -509,7 +502,6 @@ async fn test_batch() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, i32, String)>() .unwrap() .collect::>() @@ -549,7 +541,6 @@ async fn test_batch() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, i32, String)>() .unwrap() .collect::>() @@ -605,7 +596,6 @@ async fn test_token_calculation() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row::<(i64,)>() .unwrap(); let token = Token::new(value); @@ -716,7 +706,6 @@ async fn test_use_keyspace() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(String,)>() .unwrap() .map(|res| res.unwrap().0) @@ -768,7 +757,6 @@ async fn test_use_keyspace() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(String,)>() .unwrap() .map(|res| res.unwrap().0) @@ -831,7 +819,6 @@ async fn test_use_keyspace_case_sensitivity() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(String,)>() .unwrap() .map(|row| row.unwrap().0) @@ -849,7 +836,6 @@ async fn test_use_keyspace_case_sensitivity() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(String,)>() .unwrap() .map(|row| row.unwrap().0) @@ -893,7 +879,6 @@ async fn test_raw_use_keyspace() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(String,)>() .unwrap() .map(|res| res.unwrap().0) @@ -1188,7 +1173,6 @@ async fn assert_in_tracing_table(session: &Session, tracing_uuid: Uuid) { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows_num(); if rows_num > 0 { // Ok there was some row for this tracing_uuid @@ -1302,7 +1286,6 @@ async fn test_timestamp() { .await .unwrap() .into_rows_result() - .unwrap() .unwrap(); let mut results = query_rows_result @@ -1961,7 +1944,6 @@ async fn test_named_bind_markers() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, i32, i32)>() .unwrap() .map(|res| res.unwrap()) @@ -2115,7 +2097,6 @@ async fn test_unprepared_reprepare_in_execute() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, i32, i32)>() .unwrap() .map(|r| r.unwrap()) @@ -2173,7 +2154,6 @@ async fn test_unusual_valuelists() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, i32, String)>() .unwrap() .map(|r| r.unwrap()) @@ -2247,7 +2227,6 @@ async fn test_unprepared_reprepare_in_batch() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, i32, i32)>() .unwrap() .map(|r| r.unwrap()) @@ -2317,7 +2296,6 @@ async fn test_unprepared_reprepare_in_caching_session_execute() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, i32, i32)>() .unwrap() .map(|r| r.unwrap()) @@ -2387,7 +2365,6 @@ async fn assert_test_batch_table_rows_contain(sess: &Session, expected_rows: &[( .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, i32)>() .unwrap() .map(|r| r.unwrap()) @@ -2616,7 +2593,7 @@ async fn test_batch_lwts() { batch.append_statement("UPDATE tab SET r1 = 1 WHERE p1 = 0 AND c1 = 0 IF r2 = 0"); let batch_res: QueryResult = session.batch(&batch, ((), (), ())).await.unwrap(); - let batch_deserializer = batch_res.into_rows_result().unwrap().unwrap(); + let batch_deserializer = batch_res.into_rows_result().unwrap(); // Scylla returns 5 columns, but Cassandra returns only 1 let is_scylla: bool = batch_deserializer.column_specs().len() == 5; @@ -2660,7 +2637,6 @@ async fn test_batch_lwts_for_scylla( prepared_batch_res .into_rows_result() .unwrap() - .unwrap() .rows() .unwrap() .map(|r| r.unwrap()) @@ -2705,7 +2681,6 @@ async fn test_batch_lwts_for_cassandra( prepared_batch_res .into_rows_result() .unwrap() - .unwrap() .rows() .unwrap() .map(|r| r.unwrap()) @@ -2972,7 +2947,6 @@ async fn simple_strategy_test() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, i32, i32)>() .unwrap() .map(|r| r.unwrap()) @@ -3128,7 +3102,6 @@ async fn test_deserialize_empty_collections() { .await .unwrap() .into_rows_result() - .unwrap() .unwrap(); let (collection,) = query_rows_result.first_row::<(Collection,)>().unwrap(); diff --git a/scylla/src/transport/silent_prepare_batch_test.rs b/scylla/src/transport/silent_prepare_batch_test.rs index 48c0dc1f1e..bca8ef183a 100644 --- a/scylla/src/transport/silent_prepare_batch_test.rs +++ b/scylla/src/transport/silent_prepare_batch_test.rs @@ -98,7 +98,6 @@ async fn assert_test_batch_table_rows_contain(sess: &Session, expected_rows: &[( .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::<(i32, i32)>() .unwrap() .map(|r| r.unwrap()) diff --git a/scylla/src/utils/test_utils.rs b/scylla/src/utils/test_utils.rs index 2a7a21f690..d15284d61e 100644 --- a/scylla/src/utils/test_utils.rs +++ b/scylla/src/utils/test_utils.rs @@ -50,7 +50,6 @@ pub(crate) async fn supports_feature(session: &Session, feature: &str) -> bool { .unwrap() .into_rows_result() .unwrap() - .unwrap() .single_row() .unwrap(); @@ -108,10 +107,9 @@ pub async fn scylla_supports_tablets(session: &Session) -> bool { ) .await .unwrap() - .into_rows_result() - .unwrap(); + .into_rows_result(); - result.map_or(false, |rows_result| rows_result.single_row::().is_ok()) + result.is_ok_and(|rows_result| rows_result.single_row::().is_ok()) } #[cfg(test)] diff --git a/scylla/tests/integration/skip_metadata_optimization.rs b/scylla/tests/integration/skip_metadata_optimization.rs index 17f595400b..dba646e895 100644 --- a/scylla/tests/integration/skip_metadata_optimization.rs +++ b/scylla/tests/integration/skip_metadata_optimization.rs @@ -115,7 +115,6 @@ async fn test_skip_result_metadata() { .unwrap() .into_rows_result() .unwrap() - .unwrap() .rows::() .unwrap() .collect::, _>>() @@ -134,7 +133,6 @@ async fn test_skip_result_metadata() { .unwrap(); results_from_manual_paging.extend( rs_manual.into_rows_result() - .unwrap() .unwrap() .rows::() .unwrap() From 2054163c4304e999e908b9e2a2d8d6209bba37a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Wed, 13 Nov 2024 15:16:03 +0100 Subject: [PATCH 14/14] qr: return self in into_rows_result if response is not Rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Karol Baryła --- examples/cqlsh-rs.rs | 2 +- scylla-cql/src/frame/response/result.rs | 2 +- scylla/src/transport/query_result.rs | 93 ++++++++++++++++++++----- 3 files changed, 77 insertions(+), 20 deletions(-) diff --git a/examples/cqlsh-rs.rs b/examples/cqlsh-rs.rs index 6eda4a35f0..04e303d255 100644 --- a/examples/cqlsh-rs.rs +++ b/examples/cqlsh-rs.rs @@ -196,7 +196,7 @@ fn print_result(result: QueryResult) -> Result<(), IntoRowsResultError> { } Ok(()) } - Err(IntoRowsResultError::ResultNotRows) => Ok(println!("OK")), + Err(IntoRowsResultError::ResultNotRows(_)) => Ok(println!("OK")), Err(e) => Err(e), } } diff --git a/scylla-cql/src/frame/response/result.rs b/scylla-cql/src/frame/response/result.rs index de720aa91c..54a1567c76 100644 --- a/scylla-cql/src/frame/response/result.rs +++ b/scylla-cql/src/frame/response/result.rs @@ -616,7 +616,7 @@ impl Row { /// /// Flags and paging state are deserialized, remaining part of metadata /// as well as rows remain serialized. -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct RawMetadataAndRawRows { // Already deserialized part of metadata: col_count: usize, diff --git a/scylla/src/transport/query_result.rs b/scylla/src/transport/query_result.rs index 780a8b8f61..9a7745c506 100644 --- a/scylla/src/transport/query_result.rs +++ b/scylla/src/transport/query_result.rs @@ -133,7 +133,7 @@ impl<'res> ColumnSpecs<'res> { /// /// NOTE: this is a result of a single CQL request. If you use paging for your query, /// this will contain exactly one page. -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct QueryResult { raw_metadata_and_rows: Option, tracing_id: Option, @@ -219,22 +219,40 @@ impl QueryResult { /// # } /// /// ``` + /// + /// If the response is not of Rows kind, the original [`QueryResult`] (self) is + /// returned back to the user in the error type. See [`IntoRowsResultError`] documentation. + /// + /// ```rust + /// # use scylla::transport::query_result::{QueryResult, QueryRowsResult, IntoRowsResultError}; + /// # fn example(non_rows_query_result: QueryResult) -> Result<(), Box> { + /// let err = non_rows_query_result.into_rows_result().unwrap_err(); + /// + /// match err { + /// IntoRowsResultError::ResultNotRows(query_result) => { + /// // do something with original `query_result` + /// } + /// _ => { + /// // deserialization failed - query result is not recovered + /// } + /// } + /// + /// Ok(()) + /// # } + /// ``` pub fn into_rows_result(self) -> Result { - let QueryResult { - raw_metadata_and_rows, - tracing_id, + let Some(raw_metadata_and_rows) = self.raw_metadata_and_rows else { + return Err(IntoRowsResultError::ResultNotRows(self)); + }; + let tracing_id = self.tracing_id; + let warnings = self.warnings; + + let raw_rows_with_metadata = raw_metadata_and_rows.deserialize_metadata()?; + Ok(QueryRowsResult { + raw_rows_with_metadata, warnings, - } = self; - raw_metadata_and_rows - .map(|raw_rows| { - let raw_rows_with_metadata = raw_rows.deserialize_metadata()?; - Ok(QueryRowsResult { - raw_rows_with_metadata, - warnings, - tracing_id, - }) - }) - .unwrap_or(Err(IntoRowsResultError::ResultNotRows)) + tracing_id, + }) } /// Transforms itself into the legacy result type, by eagerly deserializing rows @@ -412,11 +430,14 @@ impl QueryRowsResult { } /// An error returned by [`QueryResult::into_rows_result`] +/// +/// The `ResultNotRows` variant contains original [`QueryResult`], +/// which otherwise would be consumed and lost. #[derive(Debug, Error, Clone)] pub enum IntoRowsResultError { /// Result is not of Rows kind #[error("Result is not of Rows kind")] - ResultNotRows, + ResultNotRows(QueryResult), // transparent because the underlying error provides enough context. /// Failed to lazily deserialize result metadata. @@ -572,7 +593,7 @@ mod tests { { let rqr = QueryResult::new(None, None, Vec::new()); let qr = rqr.into_rows_result(); - assert_matches!(qr, Err(IntoRowsResultError::ResultNotRows)); + assert_matches!(qr, Err(IntoRowsResultError::ResultNotRows(_))); } // RESULT::Rows response -> some column specs @@ -630,7 +651,7 @@ mod tests { { let rqr = QueryResult::new(None, None, Vec::new()); let qr = rqr.into_rows_result(); - assert_matches!(qr, Err(IntoRowsResultError::ResultNotRows)); + assert_matches!(qr, Err(IntoRowsResultError::ResultNotRows(_))); } // RESULT::Rows with 0 rows @@ -778,4 +799,40 @@ mod tests { } } } + + #[test] + fn test_query_result_returns_self_if_not_rows() { + // Check tracing ID + for tracing_id in [None, Some(Uuid::from_u128(0x_feed_dead))] { + let qr = QueryResult::new(None, tracing_id, vec![]); + let err = qr.into_rows_result().unwrap_err(); + match err { + IntoRowsResultError::ResultNotRows(query_result) => { + assert_eq!(query_result.tracing_id, tracing_id) + } + IntoRowsResultError::ResultMetadataLazyDeserializationError(_) => { + panic!("Expected ResultNotRows error") + } + } + } + + // Check warnings + { + let warnings = &["Ooops", "Meltdown..."]; + let qr = QueryResult::new( + None, + None, + warnings.iter().copied().map(String::from).collect(), + ); + let err = qr.into_rows_result().unwrap_err(); + match err { + IntoRowsResultError::ResultNotRows(query_result) => { + assert_eq!(query_result.warnings().collect_vec(), warnings) + } + IntoRowsResultError::ResultMetadataLazyDeserializationError(_) => { + panic!("Expected ResultNotRows error") + } + } + } + } }