Skip to content

Commit 081dab3

Browse files
chenbachenba
committed
refactor: stop passing collection id to get_quota_usage
1 parent 1984772 commit 081dab3

File tree

15 files changed

+145
-137
lines changed

15 files changed

+145
-137
lines changed

‎syncserver/src/web/handlers.rs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -471,12 +471,10 @@ pub async fn post_collection_batch(
471471
.await?;
472472

473473
if is_valid {
474-
let collection_id = db.get_collection_id(&coll.collection).await?;
475474
let usage = db
476475
.get_quota_usage(params::GetQuotaUsage {
477476
user_id: coll.user_id.clone(),
478477
collection: coll.collection.clone(),
479-
collection_id,
480478
})
481479
.await?;
482480
CreateBatch {

‎syncstorage-db-common/src/lib.rs‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -207,9 +207,7 @@ pub trait Db: BatchDb {
207207

208208
// Internal methods used by the db tests
209209

210-
// TODO: should be test only but currently isn't:
211-
// https://github.com/mozilla-services/syncstorage-rs/issues/1959
212-
//#[cfg(debug_assertions)]
210+
#[cfg(debug_assertions)]
213211
async fn get_collection_id(&mut self, name: &str) -> Result<i32, Self::Error>;
214212

215213
#[cfg(debug_assertions)]

‎syncstorage-db-common/src/params.rs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,6 @@ collection_data! {
193193
id: String,
194194
},
195195
GetQuotaUsage {
196-
collection_id: i32,
197196
},
198197
}
199198

‎syncstorage-db/src/mock.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,7 @@ impl Db for MockDb {
206206
Default::default()
207207
}
208208

209+
#[cfg(debug_assertions)]
209210
async fn get_collection_id(
210211
&mut self,
211212
_params: &str,

‎syncstorage-db/src/tests/db.rs‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -430,7 +430,7 @@ async fn get_storage_timestamp() -> Result<(), DbError> {
430430
#[tokio::test]
431431
async fn get_collection_id() -> Result<(), DbError> {
432432
with_test_transaction(None, async |db: &mut dyn Db<Error = DbError>| {
433-
db.get_collection_id("bookmarks").await?;
433+
db._get_collection_id("bookmarks").await?;
434434
Ok(())
435435
})
436436
.await
@@ -442,7 +442,7 @@ async fn create_collection() -> Result<(), DbError> {
442442
let name = "NewCollection";
443443
let cid = db.create_collection(name).await?;
444444
assert_ne!(cid, 0);
445-
let cid2 = db.get_collection_id(name).await?;
445+
let cid2 = db._get_collection_id(name).await?;
446446
assert_eq!(cid2, cid);
447447
Ok(())
448448
})
@@ -648,12 +648,11 @@ async fn get_collection_usage() -> Result<(), DbError> {
648648
assert_eq!(total, sum as u64);
649649
let settings = Settings::test_settings();
650650
if settings.syncstorage.enable_quota {
651-
let collection_id = db.get_collection_id("bookmarks").await?;
651+
let collection_id = db._get_collection_id("bookmarks").await?;
652652
let quota = db
653653
.get_quota_usage(params::GetQuotaUsage {
654654
user_id: hid(uid),
655655
collection: "ignored".to_owned(),
656-
collection_id,
657656
})
658657
.await?;
659658
assert_eq!(
@@ -1032,7 +1031,7 @@ async fn delete_storage() -> Result<(), DbError> {
10321031
assert!(result.is_none());
10331032

10341033
// collection data sticks around
1035-
let cid2 = db.get_collection_id("my_collection").await?;
1034+
let cid2 = db._get_collection_id("my_collection").await?;
10361035
assert_eq!(cid2, cid);
10371036

10381037
let collections = db.get_collection_counts(hid(uid)).await?;
@@ -1076,7 +1075,7 @@ async fn lock_for_read() -> Result<(), DbError> {
10761075
collection: coll.to_owned(),
10771076
})
10781077
.await?;
1079-
let result = db.get_collection_id("NewCollection").await;
1078+
let result = db._get_collection_id("NewCollection").await;
10801079
assert!(result.unwrap_err().is_collection_not_found());
10811080
Ok(())
10821081
}

‎syncstorage-mysql/src/db/batch_impl.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ impl BatchDb for MysqlDb {
3232
params: params::CreateBatch,
3333
) -> DbResult<results::CreateBatch> {
3434
let user_id = params.user_id.legacy_id as i64;
35-
let collection_id = self.get_collection_id(&params.collection).await?;
35+
let collection_id = self._get_collection_id(&params.collection).await?;
3636
// Careful, there's some weirdness here!
3737
//
3838
// Sync timestamps are in seconds and quantized to two decimal places, so
@@ -85,7 +85,7 @@ impl BatchDb for MysqlDb {
8585
}
8686

8787
let user_id = params.user_id.legacy_id as i64;
88-
let collection_id = self.get_collection_id(&params.collection).await?;
88+
let collection_id = self._get_collection_id(&params.collection).await?;
8989
let exists = batch_uploads::table
9090
.select(sql::<Integer>("1"))
9191
.filter(batch_uploads::batch_id.eq(&batch_id))
@@ -111,7 +111,7 @@ impl BatchDb for MysqlDb {
111111
}
112112

113113
let batch_id = decode_id(&params.batch.id)?;
114-
let collection_id = self.get_collection_id(&params.collection).await?;
114+
let collection_id = self._get_collection_id(&params.collection).await?;
115115
do_append(self, batch_id, params.user_id, collection_id, params.bsos).await?;
116116
Ok(())
117117
}
@@ -135,7 +135,7 @@ impl BatchDb for MysqlDb {
135135
async fn delete_batch(&mut self, params: params::DeleteBatch) -> DbResult<()> {
136136
let batch_id = decode_id(&params.id)?;
137137
let user_id = params.user_id.legacy_id as i64;
138-
let collection_id = self.get_collection_id(&params.collection).await?;
138+
let collection_id = self._get_collection_id(&params.collection).await?;
139139
diesel::delete(batch_uploads::table)
140140
.filter(batch_uploads::batch_id.eq(&batch_id))
141141
.filter(batch_uploads::user_id.eq(&user_id))
@@ -157,7 +157,7 @@ impl BatchDb for MysqlDb {
157157
) -> DbResult<results::CommitBatch> {
158158
let batch_id = decode_id(&params.batch.id)?;
159159
let user_id = params.user_id.legacy_id as i64;
160-
let collection_id = self.get_collection_id(&params.collection).await?;
160+
let collection_id = self._get_collection_id(&params.collection).await?;
161161
let timestamp = self.session.timestamp;
162162
sql_query(include_str!("batch_commit.sql"))
163163
.bind::<BigInt, _>(user_id)

‎syncstorage-mysql/src/db/db_impl.rs‎

Lines changed: 16 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ impl Db for MysqlDb {
4343
// Lock the db
4444
self.begin(false).await?;
4545
let collection_id = self
46-
.get_collection_id(&params.collection)
46+
._get_collection_id(&params.collection)
4747
.await
4848
.or_else(|e| {
4949
if e.is_collection_not_found() {
@@ -165,7 +165,7 @@ impl Db for MysqlDb {
165165
params: params::DeleteCollection,
166166
) -> DbResult<SyncTimestamp> {
167167
let user_id = params.user_id.legacy_id as i64;
168-
let collection_id = self.get_collection_id(&params.collection).await?;
168+
let collection_id = self._get_collection_id(&params.collection).await?;
169169
let mut count = delete(bso::table)
170170
.filter(bso::user_id.eq(user_id))
171171
.filter(bso::collection_id.eq(&collection_id))
@@ -184,28 +184,6 @@ impl Db for MysqlDb {
184184
self.get_storage_timestamp(params.user_id).await
185185
}
186186

187-
async fn get_collection_id(&mut self, name: &str) -> DbResult<i32> {
188-
if let Some(id) = self.coll_cache.get_id(name)? {
189-
return Ok(id);
190-
}
191-
192-
let id = sql_query(
193-
"SELECT id
194-
FROM collections
195-
WHERE name = ?",
196-
)
197-
.bind::<Text, _>(name)
198-
.get_result::<IdResult>(&mut self.conn)
199-
.await
200-
.optional()?
201-
.ok_or_else(DbError::collection_not_found)?
202-
.id;
203-
if !self.session.in_write_transaction {
204-
self.coll_cache.put(id, name.to_owned())?;
205-
}
206-
Ok(id)
207-
}
208-
209187
async fn put_bso(&mut self, bso: params::PutBso) -> DbResult<results::PutBso> {
210188
/*
211189
if bso.payload.is_none() && bso.sortindex.is_none() && bso.ttl.is_none() {
@@ -223,7 +201,6 @@ impl Db for MysqlDb {
223201
.get_quota_usage(params::GetQuotaUsage {
224202
user_id: bso.user_id.clone(),
225203
collection: bso.collection.clone(),
226-
collection_id,
227204
})
228205
.await?;
229206
if usage.total_bytes >= self.quota.size {
@@ -311,7 +288,7 @@ impl Db for MysqlDb {
311288

312289
async fn get_bsos(&mut self, params: params::GetBsos) -> DbResult<results::GetBsos> {
313290
let user_id = params.user_id.legacy_id as i64;
314-
let collection_id = self.get_collection_id(&params.collection).await?;
291+
let collection_id = self._get_collection_id(&params.collection).await?;
315292
let now = self.session.timestamp.as_i64();
316293
let mut query = bso::table
317294
.select((
@@ -412,7 +389,7 @@ impl Db for MysqlDb {
412389

413390
async fn get_bso_ids(&mut self, params: params::GetBsos) -> DbResult<results::GetBsoIds> {
414391
let user_id = params.user_id.legacy_id as i64;
415-
let collection_id = self.get_collection_id(&params.collection).await?;
392+
let collection_id = self._get_collection_id(&params.collection).await?;
416393
let mut query = bso::table
417394
.select(bso::id)
418395
.filter(bso::user_id.eq(user_id))
@@ -475,7 +452,7 @@ impl Db for MysqlDb {
475452

476453
async fn get_bso(&mut self, params: params::GetBso) -> DbResult<Option<results::GetBso>> {
477454
let user_id = params.user_id.legacy_id as i64;
478-
let collection_id = self.get_collection_id(&params.collection).await?;
455+
let collection_id = self._get_collection_id(&params.collection).await?;
479456
Ok(bso::table
480457
.select((
481458
bso::id,
@@ -495,7 +472,7 @@ impl Db for MysqlDb {
495472

496473
async fn delete_bso(&mut self, params: params::DeleteBso) -> DbResult<results::DeleteBso> {
497474
let user_id = params.user_id.legacy_id;
498-
let collection_id = self.get_collection_id(&params.collection).await?;
475+
let collection_id = self._get_collection_id(&params.collection).await?;
499476
let affected_rows = delete(bso::table)
500477
.filter(bso::user_id.eq(user_id as i64))
501478
.filter(bso::collection_id.eq(&collection_id))
@@ -516,7 +493,7 @@ impl Db for MysqlDb {
516493

517494
async fn delete_bsos(&mut self, params: params::DeleteBsos) -> DbResult<results::DeleteBsos> {
518495
let user_id = params.user_id.legacy_id as i64;
519-
let collection_id = self.get_collection_id(&params.collection).await?;
496+
let collection_id = self._get_collection_id(&params.collection).await?;
520497
delete(bso::table)
521498
.filter(bso::user_id.eq(user_id))
522499
.filter(bso::collection_id.eq(&collection_id))
@@ -572,7 +549,7 @@ impl Db for MysqlDb {
572549
params: params::GetCollectionTimestamp,
573550
) -> DbResult<SyncTimestamp> {
574551
let user_id = params.user_id.legacy_id as u32;
575-
let collection_id = self.get_collection_id(&params.collection).await?;
552+
let collection_id = self._get_collection_id(&params.collection).await?;
576553
if let Some(modified) = self
577554
.session
578555
.coll_modified_cache
@@ -595,7 +572,7 @@ impl Db for MysqlDb {
595572
params: params::GetBsoTimestamp,
596573
) -> DbResult<SyncTimestamp> {
597574
let user_id = params.user_id.legacy_id as i64;
598-
let collection_id = self.get_collection_id(&params.collection).await?;
575+
let collection_id = self._get_collection_id(&params.collection).await?;
599576
let modified = bso::table
600577
.select(bso::modified)
601578
.filter(bso::user_id.eq(user_id))
@@ -705,13 +682,14 @@ impl Db for MysqlDb {
705682
params: params::GetQuotaUsage,
706683
) -> DbResult<results::GetQuotaUsage> {
707684
let uid = params.user_id.legacy_id as i64;
685+
let collection_id = self._get_collection_id(&params.collection).await?;
708686
let (total_bytes, count): (i64, i32) = user_collections::table
709687
.select((
710688
sql::<BigInt>("COALESCE(SUM(COALESCE(total_bytes, 0)), 0)"),
711689
sql::<Integer>("COALESCE(SUM(COALESCE(count, 0)), 0)"),
712690
))
713691
.filter(user_collections::user_id.eq(uid))
714-
.filter(user_collections::collection_id.eq(params.collection_id))
692+
.filter(user_collections::collection_id.eq(collection_id))
715693
.get_result(&mut self.conn)
716694
.await
717695
.optional()?
@@ -769,6 +747,11 @@ impl Db for MysqlDb {
769747
self._create_collection(name).await
770748
}
771749

750+
#[cfg(debug_assertions)]
751+
async fn get_collection_id(&mut self, name: &str) -> DbResult<i32> {
752+
self._get_collection_id(name).await
753+
}
754+
772755
#[cfg(debug_assertions)]
773756
fn timestamp(&self) -> SyncTimestamp {
774757
self.session.timestamp
@@ -795,12 +778,6 @@ impl Db for MysqlDb {
795778
}
796779
}
797780

798-
#[derive(Debug, QueryableByName)]
799-
struct IdResult {
800-
#[diesel(sql_type = Integer)]
801-
id: i32,
802-
}
803-
804781
#[derive(Debug, QueryableByName)]
805782
struct UserCollectionsResult {
806783
// Can't substitute column names here.

‎syncstorage-mysql/src/db/mod.rs‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ impl MysqlDb {
113113
}
114114

115115
pub(super) async fn get_or_create_collection_id(&mut self, name: &str) -> DbResult<i32> {
116-
match self.get_collection_id(name).await {
116+
match self._get_collection_id(name).await {
117117
Err(e) if e.is_collection_not_found() => self._create_collection(name).await,
118118
result => result,
119119
}
@@ -145,6 +145,28 @@ impl MysqlDb {
145145
Ok(collection_id)
146146
}
147147

148+
async fn _get_collection_id(&mut self, name: &str) -> DbResult<i32> {
149+
if let Some(id) = self.coll_cache.get_id(name)? {
150+
return Ok(id);
151+
}
152+
153+
let id = sql_query(
154+
"SELECT id
155+
FROM collections
156+
WHERE name = ?",
157+
)
158+
.bind::<Text, _>(name)
159+
.get_result::<IdResult>(&mut self.conn)
160+
.await
161+
.optional()?
162+
.ok_or_else(DbError::collection_not_found)?
163+
.id;
164+
if !self.session.in_write_transaction {
165+
self.coll_cache.put(id, name.to_owned())?;
166+
}
167+
Ok(id)
168+
}
169+
148170
async fn map_collection_names<T>(
149171
&mut self,
150172
by_id: HashMap<i32, T>,
@@ -223,3 +245,9 @@ struct NameResult {
223245
#[diesel(sql_type = Text)]
224246
name: String,
225247
}
248+
249+
#[derive(Debug, QueryableByName)]
250+
struct IdResult {
251+
#[diesel(sql_type = Integer)]
252+
id: i32,
253+
}

‎syncstorage-mysql/src/test.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ async fn static_collection_id() -> DbResult<()> {
7474
}
7575

7676
for (id, name) in &cols {
77-
let result = db.get_collection_id(name).await?;
77+
let result = db._get_collection_id(name).await?;
7878
assert_eq!(result, *id);
7979
}
8080

‎syncstorage-postgres/src/db/batch_impl.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ impl BatchDb for PgDb {
167167
) -> DbResult<results::DeleteBatch> {
168168
let batch_id = validate_batch_id(&params.id)?;
169169
let user_id = params.user_id.legacy_id as i64;
170-
let collection_id = self.get_collection_id(&params.collection).await?;
170+
let collection_id = self._get_collection_id(&params.collection).await?;
171171

172172
delete(
173173
batches::table

0 commit comments

Comments
 (0)