Codereview feedback

matrix-org · Dec 9, 2024 · 013da78 · 013da78
1 parent b1ef4ca
commit 013da78
Show file tree

Hide file tree

Showing 6 changed files with 31 additions and 26 deletions.
diff --git a/crates/matrix-sdk-base/src/store/memory_store.rs b/crates/matrix-sdk-base/src/store/memory_store.rs
@@ -815,7 +815,7 @@ impl StateStore for MemoryStore {
                 transaction_id,
                 error: None,
                 priority,
-                enqueue_time: Some(MilliSecondsSinceUnixEpoch::now()),
+                created_at: Some(MilliSecondsSinceUnixEpoch::now()),
             },
         );
         Ok(())
@@ -915,7 +915,7 @@ impl StateStore for MemoryStore {
                 parent_transaction_id: parent_transaction_id.to_owned(),
                 own_transaction_id,
                 parent_key: None,
-                enqueue_time: None,
+                created_at: None,
             },
         );
         Ok(())

diff --git a/crates/matrix-sdk-base/src/store/send_queue.rs b/crates/matrix-sdk-base/src/store/send_queue.rs
@@ -134,7 +134,7 @@ pub struct QueuedRequest {
     pub priority: usize,
 
     /// The time that the request was original attempted.
-    pub enqueue_time: Option<MilliSecondsSinceUnixEpoch>,
+    pub created_at: Option<MilliSecondsSinceUnixEpoch>,
 }
 
 impl QueuedRequest {
@@ -371,7 +371,8 @@ pub struct DependentQueuedRequest {
     pub parent_key: Option<SentRequestKey>,
 
     /// The time that the request was original attempted.
-    pub enqueue_time: Option<MilliSecondsSinceUnixEpoch>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub created_at: Option<MilliSecondsSinceUnixEpoch>,
 }
 
 impl DependentQueuedRequest {

diff --git a/crates/matrix-sdk-indexeddb/src/state_store/mod.rs b/crates/matrix-sdk-indexeddb/src/state_store/mod.rs
@@ -439,13 +439,15 @@ struct PersistedQueuedRequest {
 
     priority: Option<usize>,
 
+    /// The time the original message was first attempted to be sent at.
+    #[serde(skip_serializing_if = "Option::is_none")]
+    created_at: Option<MilliSecondsSinceUnixEpoch>,
+
     // Migrated fields: keep these private, they're not used anymore elsewhere in the code base.
     /// Deprecated (from old format), now replaced with error field.
     is_wedged: Option<bool>,
 
     event: Option<SerializableEventContent>,
-
-    enqueue_time: Option<MilliSecondsSinceUnixEpoch>,
 }
 
 impl PersistedQueuedRequest {
@@ -471,7 +473,7 @@ impl PersistedQueuedRequest {
             transaction_id: self.transaction_id,
             error,
             priority,
-            enqueue_time: self.enqueue_time,
+            created_at: self.created_at,
         })
     }
 }
@@ -1397,7 +1399,7 @@ impl_state_store!({
             is_wedged: None,
             event: None,
             priority: Some(priority),
-            enqueue_time: Some(MilliSecondsSinceUnixEpoch::now()),
+            created_at: Some(MilliSecondsSinceUnixEpoch::now()),
         });
 
         // Save the new vector into db.
@@ -1593,7 +1595,7 @@ impl_state_store!({
             parent_transaction_id: parent_txn_id.to_owned(),
             own_transaction_id: own_txn_id,
             parent_key: None,
-            enqueue_time: None,
+            created_at: None,
         });
 
         // Save the new vector into db.

diff --git a/crates/matrix-sdk-sqlite/migrations/state_store/010_send_queue_enqueue_time.sql b/crates/matrix-sdk-sqlite/migrations/state_store/010_send_queue_enqueue_time.sql
@@ -1,6 +1,6 @@
--- Migration script to add the enqueue_time column to the send_queue_events table
+-- Migration script to add the created_at column to the send_queue_events table
 ALTER TABLE "send_queue_events"
-ADD COLUMN "enqueue_time" INTEGER NOT NULL DEFAULT (strftime('%s', 'now'));
+ADD COLUMN "created_at" INTEGER NOT NULL DEFAULT (strftime('%s', 'now'));
 
 ALTER TABLE "dependent_send_queue_events"
-ADD COLUMN "enqueue_time" INTEGER NOT NULL DEFAULT (strftime('%s', 'now'));
+ADD COLUMN "created_at" INTEGER NOT NULL DEFAULT (strftime('%s', 'now'));
diff --git a/crates/matrix-sdk-sqlite/src/state_store.rs b/crates/matrix-sdk-sqlite/src/state_store.rs
@@ -1838,7 +1838,7 @@ impl StateStore for SqliteStateStore {
             .acquire()
             .await?
             .prepare(
-                "SELECT transaction_id, content, wedge_reason, priority, enqueue_time FROM send_queue_events WHERE room_id = ? ORDER BY priority DESC, ROWID",
+                "SELECT transaction_id, content, wedge_reason, priority, created_at FROM send_queue_events WHERE room_id = ? ORDER BY priority DESC, ROWID",
                 |mut stmt| {
                     stmt.query((room_id,))?
                         .mapped(|row| Ok((row.get(0)?, row.get(1)?, row.get(2)?, row.get(3)?, row.get(4)?)))
@@ -1849,13 +1849,13 @@ impl StateStore for SqliteStateStore {
 
         let mut requests = Vec::with_capacity(res.len());
         for entry in res {
-            let enqueue_time = MilliSecondsSinceUnixEpoch(UInt::new(entry.4).unwrap());
+            let created_at = UInt::new(entry.4).map(|e| MilliSecondsSinceUnixEpoch(e));
             requests.push(QueuedRequest {
                 transaction_id: entry.0.into(),
                 kind: self.deserialize_json(&entry.1)?,
                 error: entry.2.map(|v| self.deserialize_value(&v)).transpose()?,
                 priority: entry.3,
-                enqueue_time: Some(enqueue_time),
+                created_at,
             });
         }
 
@@ -1921,6 +1921,7 @@ impl StateStore for SqliteStateStore {
         // See comment in `save_send_queue_event`.
         let parent_txn_id = parent_txn_id.to_string();
         let own_txn_id = own_txn_id.to_string();
+
         self.acquire()
             .await?
             .with_transaction(move |txn| {
@@ -2026,7 +2027,7 @@ impl StateStore for SqliteStateStore {
             .acquire()
             .await?
             .prepare(
-                "SELECT own_transaction_id, parent_transaction_id, parent_key, content, enqueue_time FROM dependent_send_queue_events WHERE room_id = ? ORDER BY ROWID",
+                "SELECT own_transaction_id, parent_transaction_id, parent_key, content, created_at FROM dependent_send_queue_events WHERE room_id = ? ORDER BY ROWID",
                 |mut stmt| {
                     stmt.query((room_id,))?
                         .mapped(|row| Ok((row.get(0)?, row.get(1)?, row.get(2)?, row.get(3)?, row.get(4)?)))
@@ -2037,13 +2038,13 @@ impl StateStore for SqliteStateStore {
 
         let mut dependent_events = Vec::with_capacity(res.len());
         for entry in res {
-            let enqueue_time = MilliSecondsSinceUnixEpoch(UInt::new(entry.4).unwrap());
+            let created_at = UInt::new(entry.4).map(|e| MilliSecondsSinceUnixEpoch(e));
             dependent_events.push(DependentQueuedRequest {
                 own_transaction_id: entry.0.into(),
                 parent_transaction_id: entry.1.into(),
                 parent_key: entry.2.map(|bytes| self.deserialize_value(&bytes)).transpose()?,
                 kind: self.deserialize_json(&entry.3)?,
-                enqueue_time: Some(enqueue_time),
+                created_at,
             });
         }
 
@@ -2440,6 +2441,7 @@ mod migration_tests {
         let room_id_value = this.serialize_value(&room_id.to_owned())?;
 
         let content = this.serialize_json(&content)?;
+
         txn.prepare_cached("INSERT INTO send_queue_events (room_id, room_id_val, transaction_id, content, wedged) VALUES (?, ?, ?, ?, ?)")?
             .execute((room_id_key, room_id_value, transaction_id.to_string(), content, is_wedged))?;
 

diff --git a/crates/matrix-sdk/src/send_queue.rs b/crates/matrix-sdk/src/send_queue.rs
@@ -2275,7 +2275,7 @@ mod tests {
                 .unwrap(),
             },
             parent_key: None,
-            enqueue_time: None,
+            created_at: None,
         };
         let res = canonicalize_dependent_requests(&[edit]);
 
@@ -2296,7 +2296,7 @@ mod tests {
             parent_transaction_id: txn.clone(),
             kind: DependentQueuedRequestKind::RedactEvent,
             parent_key: None,
-            enqueue_time: None,
+            created_at: None,
         };
 
         let edit = DependentQueuedRequest {
@@ -2309,7 +2309,7 @@ mod tests {
                 .unwrap(),
             },
             parent_key: None,
-            enqueue_time: None,
+            created_at: None,
         };
 
         inputs.push({
@@ -2349,7 +2349,7 @@ mod tests {
                     .unwrap(),
                 },
                 parent_key: None,
-                enqueue_time: None,
+                created_at: None,
             })
             .collect::<Vec<_>>();
 
@@ -2381,7 +2381,7 @@ mod tests {
                 kind: DependentQueuedRequestKind::RedactEvent,
                 parent_transaction_id: txn1.clone(),
                 parent_key: None,
-                enqueue_time: None,
+                created_at: None,
             },
             // This one pertains to txn2.
             DependentQueuedRequest {
@@ -2394,7 +2394,7 @@ mod tests {
                 },
                 parent_transaction_id: txn2.clone(),
                 parent_key: None,
-                enqueue_time: None,
+                created_at: None,
             },
         ];
 
@@ -2425,7 +2425,7 @@ mod tests {
             kind: DependentQueuedRequestKind::ReactEvent { key: "🧠".to_owned() },
             parent_transaction_id: txn.clone(),
             parent_key: None,
-            enqueue_time: None,
+            created_at: None,
         };
 
         let edit_id = ChildTransactionId::new();
@@ -2439,7 +2439,7 @@ mod tests {
             },
             parent_transaction_id: txn,
             parent_key: None,
-            enqueue_time: None,
+            created_at: None,
         };
 
         let res = canonicalize_dependent_requests(&[react, edit]);