Skip to content

Commit

Permalink
Merge pull request #246 from mozilla-services/fix/fix-group-by-kid
Browse files Browse the repository at this point in the history 
fix: correct mistaken GROUP BY fxa_kid (vs userid)
  • Loading branch information
@pjenvey
pjenvey authored Oct 4, 2019
2 parents 4e5672d + 041cb11 commit f3bc384
Showing 1 changed file with 30 additions and 45 deletions.
75 changes: 30 additions & 45 deletions src/db/spanner/models.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use futures::lazy;
use diesel::r2d2::PooledConnection;

use std::cell::RefCell;
use std::cmp;
use std::collections::HashMap;
use std::convert::TryInto;
use std::fmt;
Expand Down Expand Up @@ -177,7 +176,7 @@ impl SpannerDb {
)?
.params(params! {
"name" => name.to_string(),
"collectionid" => cmp::max(id, 100).to_string(),
"collectionid" => id.to_string(),
})
.execute(&self.conn)?;
Ok(id)
Expand Down Expand Up @@ -226,9 +225,6 @@ impl SpannerDb {
pub fn lock_for_write_sync(&self, params: params::LockCollection) -> Result<()> {
// Begin a transaction
self.begin(true)?;

let fxa_uid = params.user_id.fxa_uid.clone();
let fxa_kid = params.user_id.fxa_kid.clone();
let collection_id = self.get_or_create_collection_id(&params.collection)?;
if let Some(CollectionLock::Read) = self
.inner
Expand All @@ -249,8 +245,8 @@ impl SpannerDb {
AND collection = @collectionid",
)?
.params(params! {
"fxa_uid" => fxa_uid,
"fxa_kid" => fxa_kid,
"fxa_uid" => params.user_id.fxa_uid.clone(),
"fxa_kid" => params.user_id.fxa_kid.clone(),
"collectionid" => collection_id.to_string(),
})
.execute(&self.conn)?
Expand Down Expand Up @@ -499,16 +495,14 @@ impl SpannerDb {
&self,
params: params::GetCollectionTimestamp,
) -> Result<SyncTimestamp> {
let fxa_uid = params.user_id.fxa_uid.clone();
let fxa_kid = params.user_id.fxa_kid.clone();
dbg!("!!QQQ get_collection_timestamp_sync", &params.collection);

let collection_id = self.get_collection_id(&params.collection)?;
if let Some(modified) = self
.session
.borrow()
.coll_modified_cache
.get(&(params.user_id, collection_id))
.get(&(params.user_id.clone(), collection_id))
{
return Ok(*modified);
}
Expand All @@ -522,8 +516,8 @@ impl SpannerDb {
AND collection = @collectionid",
)?
.params(params! {
"fxa_uid" => fxa_uid,
"fxa_kid" => fxa_kid,
"fxa_uid" => params.user_id.fxa_uid,
"fxa_kid" => params.user_id.fxa_kid,
"collectionid" => collection_id.to_string(),
})
.execute(&self.conn)?
Expand All @@ -537,8 +531,6 @@ impl SpannerDb {
&self,
user_id: params::GetCollectionTimestamps,
) -> Result<results::GetCollectionTimestamps> {
let fxa_uid = user_id.fxa_uid;
let fxa_kid = user_id.fxa_kid;
let modifieds = self
.sql(
"SELECT collection, last_modified
Expand All @@ -548,8 +540,8 @@ impl SpannerDb {
AND collection != @collectionid",
)?
.params(params! {
"fxa_uid" => fxa_uid,
"fxa_kid" => fxa_kid,
"fxa_uid" => user_id.fxa_uid,
"fxa_kid" => user_id.fxa_kid,
"collectionid" => TOMBSTONE.to_string(),
})
.execute(&self.conn)?
Expand Down Expand Up @@ -626,9 +618,6 @@ impl SpannerDb {
&self,
user_id: params::GetCollectionCounts,
) -> Result<results::GetCollectionCounts> {
let fxa_uid = user_id.fxa_uid;
let fxa_kid = user_id.fxa_kid;

let counts = self
.sql(
"SELECT collection, COUNT(collection)
Expand All @@ -639,8 +628,8 @@ impl SpannerDb {
GROUP BY collection",
)?
.params(params! {
"fxa_uid" => fxa_uid,
"fxa_kid" => fxa_kid,
"fxa_uid" => user_id.fxa_uid,
"fxa_kid" => user_id.fxa_kid,
})
.execute(&self.conn)?
.map(|row| {
Expand Down Expand Up @@ -726,7 +715,7 @@ impl SpannerDb {
WHERE userid = @fxa_uid
AND fxa_kid = @fxa_kid
AND ttl > CURRENT_TIMESTAMP()
GROUP BY fxa_kid",
GROUP BY userid",
)?
.params(params! {
"fxa_uid" => user_id.fxa_uid,
Expand Down Expand Up @@ -780,18 +769,17 @@ impl SpannerDb {
}

pub fn delete_storage_sync(&self, user_id: params::DeleteStorage) -> Result<()> {
let params = params! {
"fxa_uid" => user_id.fxa_uid.clone(),
"fxa_kid" => user_id.fxa_kid.clone()
};
// Also deletes child bso rows (INTERLEAVE IN PARENT user_collections
// ON DELETE CASCADE
self.sql(
"DELETE FROM user_collections
WHERE userid = @fxa_uid
AND fxa_kid = @fxa_kid",
)?
.params(params.clone())
.params(params! {
"fxa_uid" => user_id.fxa_uid,
"fxa_kid" => user_id.fxa_kid,
})
.execute(&self.conn)?;
Ok(())
}
Expand All @@ -807,28 +795,26 @@ impl SpannerDb {
&self,
params: params::DeleteCollection,
) -> Result<results::DeleteCollection> {
let sqlparams = params! {
"fxa_uid" => params.user_id.fxa_uid.clone(),
"fxa_kid" => params.user_id.fxa_kid.clone(),
"collectionid" => self.get_collection_id(&params.collection)?.to_string(),
};
// Also deletes child bso rows (INTERLEAVE IN PARENT user_collections
// ON DELETE CASCADE
if self
let rs = self
.sql(
"DELETE FROM user_collections
WHERE userid = @fxa_uid
AND fxa_kid = @fxa_kid
AND collection = @collectionid",
)?
.params(sqlparams.clone())
.execute(&self.conn)?
.affected_rows()?
> 0
{
return self.erect_tombstone(&params.user_id);
.params(params! {
"fxa_uid" => params.user_id.fxa_uid.clone(),
"fxa_kid" => params.user_id.fxa_kid.clone(),
"collectionid" => self.get_collection_id(&params.collection)?.to_string(),
})
.execute(&self.conn)?;
if rs.affected_rows()? > 0 {
self.erect_tombstone(&params.user_id)
} else {
self.get_storage_timestamp_sync(params.user_id)
}
self.get_storage_timestamp_sync(params.user_id)
}

pub(super) fn touch_collection(
Expand Down Expand Up @@ -892,7 +878,6 @@ impl SpannerDb {
}

pub fn delete_bso_sync(&self, params: params::DeleteBso) -> Result<results::DeleteBso> {
let user_id = params.user_id.clone();
let collection_id = self.get_collection_id(&params.collection)?;
let touch = self.touch_collection(&params.user_id, collection_id)?;
let result = self
Expand All @@ -904,8 +889,8 @@ impl SpannerDb {
AND id = @bsoid",
)?
.params(params! {
"fxa_uid" => user_id.fxa_uid,
"fxa_kid" => user_id.fxa_kid,
"fxa_uid" => params.user_id.fxa_uid,
"fxa_kid" => params.user_id.fxa_kid,
"collectionid" => collection_id.to_string(),
"bsoid" => params.id.to_string(),
})
Expand Down Expand Up @@ -941,8 +926,8 @@ impl SpannerDb {

pub fn get_bsos_sync(&self, params: params::GetBsos) -> Result<results::GetBsos> {
let mut sqlparams = params! {
"fxa_uid" => params.user_id.fxa_uid.clone(),
"fxa_kid" => params.user_id.fxa_kid.clone(),
"fxa_uid" => params.user_id.fxa_uid,
"fxa_kid" => params.user_id.fxa_kid,
"collectionid" => self.get_collection_id(&params.collection)?.to_string(),
};
let BsoQueryParams {
Expand Down

0 comments on commit f3bc384

Please sign in to comment.