From b8858cea0756ee5920680ccd99133687c340200f Mon Sep 17 00:00:00 2001 From: Philip Jenvey Date: Wed, 4 May 2022 17:33:38 -0700 Subject: [PATCH] chore: to_spanner_value -> into_spanner_value (#1301) to avoid cloning, particularly of the payload Closes #1300 --- src/db/spanner/batch.rs | 29 ++++++++---------- src/db/spanner/macros.rs | 6 ++-- src/db/spanner/models.rs | 45 ++++++++++++++++----------- src/db/spanner/schema.ddl | 4 +-- src/db/spanner/support.rs | 64 +++++++++++++++++++-------------------- 5 files changed, 77 insertions(+), 71 deletions(-) diff --git a/src/db/spanner/batch.rs b/src/db/spanner/batch.rs index 61f0869c40..3031158a92 100644 --- a/src/db/spanner/batch.rs +++ b/src/db/spanner/batch.rs @@ -11,7 +11,7 @@ use protobuf::{ use uuid::Uuid; use super::models::{Result, SpannerDb, DEFAULT_BSO_TTL, PRETOUCH_TS}; -use super::support::{as_type, null_value, struct_type_field, ToSpannerValue}; +use super::support::{as_type, null_value, struct_type_field, IntoSpannerValue}; use crate::{ db::{params, results, util::to_rfc3339, DbError, DbErrorKind, BATCH_LIFETIME}, web::{extractors::HawkIdentifier, tags::Tags}, @@ -359,29 +359,26 @@ pub async fn do_append_async( } else { let sortindex = bso .sortindex - .as_ref() - .map(ToSpannerValue::to_spanner_value) + .map(IntoSpannerValue::into_spanner_value) .unwrap_or_else(null_value); let payload = bso .payload - .as_ref() - .map(ToSpannerValue::to_spanner_value) + .map(IntoSpannerValue::into_spanner_value) .unwrap_or_else(null_value); let ttl = bso .ttl - .as_ref() - .map(ToSpannerValue::to_spanner_value) + .map(IntoSpannerValue::into_spanner_value) .unwrap_or_else(null_value); // convert to a protobuf structure for direct insertion to // avoid some mutation limits. let mut row = ListValue::new(); row.set_values(RepeatedField::from_vec(vec![ - user_id.fxa_uid.clone().to_spanner_value(), - user_id.fxa_kid.clone().to_spanner_value(), - collection_id.to_spanner_value(), - batch.id.clone().to_spanner_value(), - bso.id.to_spanner_value(), + user_id.fxa_uid.clone().into_spanner_value(), + user_id.fxa_kid.clone().into_spanner_value(), + collection_id.into_spanner_value(), + batch.id.clone().into_spanner_value(), + bso.id.into_spanner_value(), sortindex, payload, ttl, @@ -480,18 +477,18 @@ pub async fn do_append_async( }; if let Some(sortindex) = val.sortindex { fields.push("sortindex"); - params.insert("sortindex".to_owned(), sortindex.to_spanner_value()); param_types.insert("sortindex".to_owned(), sortindex.spanner_type()); + params.insert("sortindex".to_owned(), sortindex.into_spanner_value()); } if let Some(payload) = val.payload { fields.push("payload"); - params.insert("payload".to_owned(), payload.to_spanner_value()); param_types.insert("payload".to_owned(), payload.spanner_type()); + params.insert("payload".to_owned(), payload.into_spanner_value()); }; if let Some(ttl) = val.ttl { fields.push("ttl"); - params.insert("ttl".to_owned(), ttl.to_spanner_value()); param_types.insert("ttl".to_owned(), ttl.spanner_type()); + params.insert("ttl".to_owned(), ttl.into_spanner_value()); } if fields.is_empty() { continue; @@ -551,7 +548,7 @@ async fn pretouch_collection_async( if result.is_none() { sqlparams.insert( "modified".to_owned(), - PRETOUCH_TS.to_owned().to_spanner_value(), + PRETOUCH_TS.to_owned().into_spanner_value(), ); sqlparam_types.insert("modified".to_owned(), as_type(TypeCode::TIMESTAMP)); let sql = if db.quota.enabled { diff --git a/src/db/spanner/macros.rs b/src/db/spanner/macros.rs index 945c28ccb1..651265a3d5 100644 --- a/src/db/spanner/macros.rs +++ b/src/db/spanner/macros.rs @@ -9,8 +9,8 @@ macro_rules! params { let mut _value_map = ::std::collections::HashMap::with_capacity(_cap); let mut _type_map = ::std::collections::HashMap::with_capacity(_cap); $( - _value_map.insert($key.to_owned(), ToSpannerValue::to_spanner_value(&$value)); - _type_map.insert($key.to_owned(), ToSpannerValue::spanner_type(&$value)); + _type_map.insert($key.to_owned(), IntoSpannerValue::spanner_type(&$value)); + _value_map.insert($key.to_owned(), IntoSpannerValue::into_spanner_value($value)); )* (_value_map, _type_map) } @@ -19,7 +19,7 @@ macro_rules! params { #[test] fn test_params_macro() { - use crate::db::spanner::support::ToSpannerValue; + use crate::db::spanner::support::IntoSpannerValue; use google_cloud_rust_raw::spanner::v1::type_pb::{Type, TypeCode}; use protobuf::{ well_known_types::{ListValue, Value}, diff --git a/src/db/spanner/models.rs b/src/db/spanner/models.rs index a376ad703f..083a02a027 100644 --- a/src/db/spanner/models.rs +++ b/src/db/spanner/models.rs @@ -44,7 +44,7 @@ use super::{ pool::{CollectionCache, Conn}, support::{ as_type, bso_from_row, bso_to_insert_row, bso_to_update_row, ExecuteSqlRequestBuilder, - StreamedResultSetAsync, ToSpannerValue, + IntoSpannerValue, StreamedResultSetAsync, }, }; @@ -645,7 +645,7 @@ impl SpannerDb { .into_iter() .map(|id| id.to_string()) .collect::>() - .to_spanner_value(), + .into_spanner_value(), ); let mut rs = self .sql( @@ -909,11 +909,11 @@ impl SpannerDb { if self.quota.enabled { sqlparams.insert( "total_bytes".to_owned(), - result[0].take_string_value().to_spanner_value(), + result[0].take_string_value().into_spanner_value(), ); sqlparams.insert( "count".to_owned(), - result[1].take_string_value().to_spanner_value(), + result[1].take_string_value().into_spanner_value(), ); sqltypes.insert( "total_bytes".to_owned(), @@ -1041,7 +1041,7 @@ impl SpannerDb { let (sqlparams, mut sqlparam_types) = params! { "fxa_uid" => params.user_id.fxa_uid.clone(), "fxa_kid" => params.user_id.fxa_kid.clone(), - "collection_id" => collection_id.clone(), + "collection_id" => collection_id, "pretouch_ts" => PRETOUCH_TS.to_owned(), }; sqlparam_types.insert("pretouch_ts".to_owned(), as_type(TypeCode::TIMESTAMP)); @@ -1233,8 +1233,8 @@ impl SpannerDb { if !ids.is_empty() { query = format!("{} AND bso_id IN UNNEST(@ids)", query); - sqlparams.insert("ids".to_owned(), ids.to_spanner_value()); sqlparam_types.insert("ids".to_owned(), ids.spanner_type()); + sqlparams.insert("ids".to_owned(), ids.into_spanner_value()); } // issue559: Dead code (timestamp always None) @@ -1244,7 +1244,7 @@ impl SpannerDb { Sorting::Newest => { sqlparams.insert( "older_eq".to_string(), - timestamp.as_rfc3339()?.to_spanner_value(), + timestamp.as_rfc3339()?.into_spanner_value(), ); sqlparam_types.insert("older_eq".to_string(), as_type(TypeCode::TIMESTAMP)); format!("{} AND modified <= @older_eq", query) @@ -1252,7 +1252,7 @@ impl SpannerDb { Sorting::Oldest => { sqlparams.insert( "newer_eq".to_string(), - timestamp.as_rfc3339()?.to_spanner_value(), + timestamp.as_rfc3339()?.into_spanner_value(), ); sqlparam_types.insert("newer_eq".to_string(), as_type(TypeCode::TIMESTAMP)); format!("{} AND modified >= @newer_eq", query) @@ -1263,12 +1263,18 @@ impl SpannerDb { */ if let Some(older) = older { query = format!("{} AND modified < @older", query); - sqlparams.insert("older".to_string(), older.as_rfc3339()?.to_spanner_value()); + sqlparams.insert( + "older".to_string(), + older.as_rfc3339()?.into_spanner_value(), + ); sqlparam_types.insert("older".to_string(), as_type(TypeCode::TIMESTAMP)); } if let Some(newer) = newer { query = format!("{} AND modified > @newer", query); - sqlparams.insert("newer".to_string(), newer.as_rfc3339()?.to_spanner_value()); + sqlparams.insert( + "newer".to_string(), + newer.as_rfc3339()?.into_spanner_value(), + ); sqlparam_types.insert("newer".to_string(), as_type(TypeCode::TIMESTAMP)); } @@ -1734,8 +1740,8 @@ impl SpannerDb { "{}{}", q, if let Some(sortindex) = bso.sortindex { - sqlparams.insert("sortindex".to_string(), sortindex.to_spanner_value()); sqlparam_types.insert("sortindex".to_string(), sortindex.spanner_type()); + sqlparams.insert("sortindex".to_string(), sortindex.into_spanner_value()); format!("{}{}", comma(&q), "sortindex = @sortindex") } else { @@ -1748,7 +1754,10 @@ impl SpannerDb { q, if let Some(ttl) = bso.ttl { let expiry = timestamp.as_i64() + (i64::from(ttl) * 1000); - sqlparams.insert("expiry".to_string(), to_rfc3339(expiry)?.to_spanner_value()); + sqlparams.insert( + "expiry".to_string(), + to_rfc3339(expiry)?.into_spanner_value(), + ); sqlparam_types.insert("expiry".to_string(), as_type(TypeCode::TIMESTAMP)); format!("{}{}", comma(&q), "expiry = @expiry") } else { @@ -1762,7 +1771,7 @@ impl SpannerDb { if bso.payload.is_some() || bso.sortindex.is_some() { sqlparams.insert( "modified".to_string(), - timestamp.as_rfc3339()?.to_spanner_value(), + timestamp.as_rfc3339()?.into_spanner_value(), ); sqlparam_types.insert("modified".to_string(), as_type(TypeCode::TIMESTAMP)); format!("{}{}", comma(&q), "modified = @modified") @@ -1775,8 +1784,8 @@ impl SpannerDb { "{}{}", q, if let Some(payload) = bso.payload { - sqlparams.insert("payload".to_string(), payload.to_spanner_value()); sqlparam_types.insert("payload".to_string(), payload.spanner_type()); + sqlparams.insert("payload".to_string(), payload.into_spanner_value()); format!("{}{}", comma(&q), "payload = @payload") } else { "".to_string() @@ -1820,14 +1829,14 @@ impl SpannerDb { use super::support::null_value; let sortindex = bso .sortindex - .map(|sortindex| sortindex.to_spanner_value()) + .map(|sortindex| sortindex.into_spanner_value()) .unwrap_or_else(null_value); sqlparams.insert("sortindex".to_string(), sortindex); sqlparam_types.insert("sortindex".to_string(), as_type(TypeCode::INT64)); } let payload = bso.payload.unwrap_or_else(|| "".to_owned()); - sqlparams.insert("payload".to_string(), payload.to_spanner_value()); sqlparam_types.insert("payload".to_owned(), payload.spanner_type()); + sqlparams.insert("payload".to_string(), payload.into_spanner_value()); let now_millis = timestamp.as_i64(); let ttl = bso.ttl.map_or(i64::from(DEFAULT_BSO_TTL), |ttl| { ttl.try_into() @@ -1838,12 +1847,12 @@ impl SpannerDb { "!!!!! [test] INSERT expirystring:{:?}, timestamp:{:?}, ttl:{:?}", &expirystring, timestamp, ttl ); - sqlparams.insert("expiry".to_string(), expirystring.to_spanner_value()); + sqlparams.insert("expiry".to_string(), expirystring.into_spanner_value()); sqlparam_types.insert("expiry".to_string(), as_type(TypeCode::TIMESTAMP)); sqlparams.insert( "modified".to_string(), - timestamp.as_rfc3339()?.to_spanner_value(), + timestamp.as_rfc3339()?.into_spanner_value(), ); sqlparam_types.insert("modified".to_string(), as_type(TypeCode::TIMESTAMP)); sql.to_owned() diff --git a/src/db/spanner/schema.ddl b/src/db/spanner/schema.ddl index 7215c7498e..092aa2e564 100644 --- a/src/db/spanner/schema.ddl +++ b/src/db/spanner/schema.ddl @@ -82,6 +82,6 @@ CREATE TABLE batch_bsos ( -- no "modified" column because the modification timestamp gets set on -- batch commit. --- *NOTE*: --- Newly created Spanner instances should pre-populate the `collections` table by +-- *NOTE*: +-- Newly created Spanner instances should pre-populate the `collections` table by -- running the content of `insert_standard_collections.sql ` diff --git a/src/db/spanner/support.rs b/src/db/spanner/support.rs index 6f88ede708..7cb777d5e3 100644 --- a/src/db/spanner/support.rs +++ b/src/db/spanner/support.rs @@ -26,10 +26,10 @@ use crate::{ use super::{models::Result, pool::Conn}; -pub trait ToSpannerValue { +pub trait IntoSpannerValue { const TYPE_CODE: TypeCode; - fn to_spanner_value(&self) -> Value; + fn into_spanner_value(self) -> Value; fn spanner_type(&self) -> Type { let mut t = Type::new(); @@ -38,43 +38,43 @@ pub trait ToSpannerValue { } } -impl ToSpannerValue for String { +impl IntoSpannerValue for String { const TYPE_CODE: TypeCode = TypeCode::STRING; - fn to_spanner_value(&self) -> Value { + fn into_spanner_value(self) -> Value { let mut value = Value::new(); - value.set_string_value(self.clone()); + value.set_string_value(self); value } } -impl ToSpannerValue for i32 { +impl IntoSpannerValue for i32 { const TYPE_CODE: TypeCode = TypeCode::INT64; - fn to_spanner_value(&self) -> Value { - self.to_string().to_spanner_value() + fn into_spanner_value(self) -> Value { + self.to_string().into_spanner_value() } } -impl ToSpannerValue for u32 { +impl IntoSpannerValue for u32 { const TYPE_CODE: TypeCode = TypeCode::INT64; - fn to_spanner_value(&self) -> Value { - self.to_string().to_spanner_value() + fn into_spanner_value(self) -> Value { + self.to_string().into_spanner_value() } } -impl ToSpannerValue for Vec +impl IntoSpannerValue for Vec where - T: ToSpannerValue + Clone, + T: IntoSpannerValue, Vec: SpannerArrayElementType, { const TYPE_CODE: TypeCode = TypeCode::ARRAY; - fn to_spanner_value(&self) -> Value { + fn into_spanner_value(self) -> Value { let mut list = ListValue::new(); list.set_values(RepeatedField::from_vec( - self.iter().map(|v| v.clone().to_spanner_value()).collect(), + self.into_iter().map(|v| v.into_spanner_value()).collect(), )); let mut value = Value::new(); value.set_list_value(list); @@ -358,7 +358,7 @@ fn merge_string(mut lhs: Value, rhs: &Value) -> Result { } let mut merged = lhs.take_string_value(); merged.push_str(rhs.get_string_value()); - Ok(merged.to_spanner_value()) + Ok(merged.into_spanner_value()) } pub fn bso_from_row(mut row: Vec) -> Result { @@ -390,21 +390,21 @@ pub fn bso_to_insert_row( ) -> Result { let sortindex = bso .sortindex - .map(|sortindex| sortindex.to_spanner_value()) + .map(|sortindex| sortindex.into_spanner_value()) .unwrap_or_else(null_value); let ttl = bso.ttl.unwrap_or(DEFAULT_BSO_TTL); let expiry = to_rfc3339(now.as_i64() + (i64::from(ttl) * 1000))?; let mut row = ListValue::new(); row.set_values(RepeatedField::from_vec(vec![ - user_id.fxa_uid.clone().to_spanner_value(), - user_id.fxa_kid.clone().to_spanner_value(), - collection_id.to_spanner_value(), - bso.id.to_spanner_value(), + user_id.fxa_uid.clone().into_spanner_value(), + user_id.fxa_kid.clone().into_spanner_value(), + collection_id.into_spanner_value(), + bso.id.into_spanner_value(), sortindex, - bso.payload.unwrap_or_default().to_spanner_value(), - now.as_rfc3339()?.to_spanner_value(), - expiry.to_spanner_value(), + bso.payload.unwrap_or_default().into_spanner_value(), + now.as_rfc3339()?.into_spanner_value(), + expiry.into_spanner_value(), ])); Ok(row) } @@ -417,29 +417,29 @@ pub fn bso_to_update_row( ) -> Result<(Vec<&'static str>, ListValue)> { let mut columns = vec!["fxa_uid", "fxa_kid", "collection_id", "bso_id"]; let mut values = vec![ - user_id.fxa_uid.clone().to_spanner_value(), - user_id.fxa_kid.clone().to_spanner_value(), - collection_id.to_spanner_value(), - bso.id.to_spanner_value(), + user_id.fxa_uid.clone().into_spanner_value(), + user_id.fxa_kid.clone().into_spanner_value(), + collection_id.into_spanner_value(), + bso.id.into_spanner_value(), ]; let modified = bso.payload.is_some() || bso.sortindex.is_some(); if let Some(sortindex) = bso.sortindex { columns.push("sortindex"); - values.push(sortindex.to_spanner_value()); + values.push(sortindex.into_spanner_value()); } if let Some(payload) = bso.payload { columns.push("payload"); - values.push(payload.to_spanner_value()); + values.push(payload.into_spanner_value()); } if modified { columns.push("modified"); - values.push(now.as_rfc3339()?.to_spanner_value()); + values.push(now.as_rfc3339()?.into_spanner_value()); } if let Some(ttl) = bso.ttl { columns.push("expiry"); let expiry = now.as_i64() + (i64::from(ttl) * 1000); - values.push(to_rfc3339(expiry)?.to_spanner_value()); + values.push(to_rfc3339(expiry)?.into_spanner_value()); } let mut row = ListValue::new();