Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correct mistaken GROUP BY fxa_kid (vs userid) #246

Merged
merged 2 commits into from
Oct 4, 2019
Merged

fix: correct mistaken GROUP BY fxa_kid (vs userid) #246

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c35270f
fix: correct mistaken GROUP BY fxa_kid (vs userid)
pjenvey Oct 4, 2019
041cb11
Merge branch 'master' into fix/fix-group-by-kid
jrconlin Oct 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 30 additions & 45 deletions src/db/spanner/models.rs
View file
Open in desktop
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 @@ -175,7 +174,7 @@ impl SpannerDb {
)?
.params(params! {
"name" => name.to_string(),
"collectionid" => cmp::max(id, 100).to_string(),
"collectionid" => id.to_string(),
})
.execute(&self.conn)?;
self.coll_cache.put(id, name.to_owned())?;
Expand Down Expand Up @@ -225,9 +224,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 @@ -248,8 +244,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 @@ -498,16 +494,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 @@ -521,8 +515,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 @@ -536,8 +530,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 @@ -547,8 +539,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 @@ -623,9 +615,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 @@ -636,8 +625,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 @@ -723,7 +712,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 @@ -777,18 +766,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,
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was more me trying to be consistent about how params were being defined for functions. The clone was definitely unneeded. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, I think this is actually leftover from removing a second unneeded DELETE query anyway

.execute(&self.conn)?;
Ok(())
}
Expand All @@ -804,28 +792,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 @@ -889,7 +875,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 @@ -901,8 +886,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 @@ -938,8 +923,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