Skip to content

Commit 8a88479

Browse files
link2xtlink2xt
committed
fix(query_row_optional): do not treat rows with NULL as missing rows
Instead of treating NULL type error as absence of the row, handle NULL values with SQL. Previously we sometimes accidentally treated a single column being NULL as the lack of the whole row.
1 parent 5711f2f commit 8a88479

File tree

6 files changed

+60
-22
lines changed

6 files changed

+60
-22
lines changed

‎deltachat-jsonrpc/src/api/types/message.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -640,7 +640,7 @@ pub struct MessageInfo {
640640
error: Option<String>,
641641
rfc724_mid: String,
642642
server_urls: Vec<String>,
643-
hop_info: Option<String>,
643+
hop_info: String,
644644
}
645645

646646
impl MessageInfo {

‎src/chat.rs

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1042,7 +1042,13 @@ impl ChatId {
10421042
pub(crate) async fn get_timestamp(self, context: &Context) -> Result<Option<i64>> {
10431043
let timestamp = context
10441044
.sql
1045-
.query_get_value("SELECT MAX(timestamp) FROM msgs WHERE chat_id=?", (self,))
1045+
.query_get_value(
1046+
"SELECT MAX(timestamp)
1047+
FROM msgs
1048+
WHERE chat_id=?
1049+
HAVING COUNT(*) > 0",
1050+
(self,),
1051+
)
10461052
.await?;
10471053
Ok(timestamp)
10481054
}
@@ -1251,7 +1257,7 @@ impl ChatId {
12511257
) -> Result<Option<(String, String, String)>> {
12521258
self.parent_query(
12531259
context,
1254-
"rfc724_mid, mime_in_reply_to, mime_references",
1260+
"rfc724_mid, mime_in_reply_to, IFNULL(mime_references, '')",
12551261
state_out_min,
12561262
|row: &rusqlite::Row| {
12571263
let rfc724_mid: String = row.get(0)?;
@@ -1405,7 +1411,10 @@ impl ChatId {
14051411
context
14061412
.sql
14071413
.query_get_value(
1408-
"SELECT MAX(timestamp) FROM msgs WHERE chat_id=? AND state!=?",
1414+
"SELECT MAX(timestamp)
1415+
FROM msgs
1416+
WHERE chat_id=? AND state!=?
1417+
HAVING COUNT(*) > 0",
14091418
(self, MessageState::OutDraft),
14101419
)
14111420
.await?
@@ -1421,7 +1430,10 @@ impl ChatId {
14211430
context
14221431
.sql
14231432
.query_get_value(
1424-
"SELECT MAX(timestamp) FROM msgs WHERE chat_id=? AND hidden=0 AND state>?",
1433+
"SELECT MAX(timestamp)
1434+
FROM msgs
1435+
WHERE chat_id=? AND hidden=0 AND state>?
1436+
HAVING COUNT(*) > 0",
14251437
(self, MessageState::InFresh),
14261438
)
14271439
.await?
@@ -4345,7 +4357,10 @@ pub async fn add_device_msg_with_importance(
43454357
if let Some(last_msg_time) = context
43464358
.sql
43474359
.query_get_value(
4348-
"SELECT MAX(timestamp) FROM msgs WHERE chat_id=?",
4360+
"SELECT MAX(timestamp)
4361+
FROM msgs
4362+
WHERE chat_id=?
4363+
HAVING COUNT(*) > 0",
43494364
(chat_id,),
43504365
)
43514366
.await?

‎src/ephemeral.rs

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ use std::num::ParseIntError;
6969
use std::str::FromStr;
7070
use std::time::{Duration, UNIX_EPOCH};
7171

72-
use anyhow::{ensure, Result};
72+
use anyhow::{ensure, Context as _, Result};
7373
use async_channel::Receiver;
7474
use serde::{Deserialize, Serialize};
7575
use tokio::time::timeout;
@@ -176,9 +176,13 @@ impl ChatId {
176176
pub async fn get_ephemeral_timer(self, context: &Context) -> Result<Timer> {
177177
let timer = context
178178
.sql
179-
.query_get_value("SELECT ephemeral_timer FROM chats WHERE id=?;", (self,))
180-
.await?;
181-
Ok(timer.unwrap_or_default())
179+
.query_get_value(
180+
"SELECT IFNULL(ephemeral_timer, 0) FROM chats WHERE id=?",
181+
(self,),
182+
)
183+
.await?
184+
.with_context(|| format!("Chat {self} not found"))?;
185+
Ok(timer)
182186
}
183187

184188
/// Set ephemeral timer value without sending a message.
@@ -509,7 +513,8 @@ async fn next_delete_device_after_timestamp(context: &Context) -> Result<Option<
509513
FROM msgs
510514
WHERE chat_id > ?
511515
AND chat_id != ?
512-
AND chat_id != ?;
516+
AND chat_id != ?
517+
HAVING count(*) > 0
513518
"#,
514519
(DC_CHAT_ID_TRASH, self_chat_id, device_chat_id),
515520
)
@@ -533,7 +538,8 @@ async fn next_expiration_timestamp(context: &Context) -> Option<i64> {
533538
SELECT min(ephemeral_timestamp)
534539
FROM msgs
535540
WHERE ephemeral_timestamp != 0
536-
AND chat_id != ?;
541+
AND chat_id != ?
542+
HAVING count(*) > 0
537543
"#,
538544
(DC_CHAT_ID_TRASH,), // Trash contains already deleted messages, skip them
539545
)
@@ -1410,4 +1416,14 @@ mod tests {
14101416

14111417
Ok(())
14121418
}
1419+
1420+
/// Tests that `.get_ephemeral_timer()` returns an error for invalid chat ID.
1421+
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
1422+
async fn test_get_ephemeral_timer_wrong_chat_id() -> Result<()> {
1423+
let context = TestContext::new().await;
1424+
let chat_id = ChatId::new(12345);
1425+
assert!(chat_id.get_ephemeral_timer(&context).await.is_err());
1426+
1427+
Ok(())
1428+
}
14131429
}

‎src/message.rs

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -219,11 +219,13 @@ impl MsgId {
219219
}
220220

221221
/// Returns information about hops of a message, used for message info
222-
pub async fn hop_info(self, context: &Context) -> Result<Option<String>> {
223-
context
222+
pub async fn hop_info(self, context: &Context) -> Result<String> {
223+
let hop_info = context
224224
.sql
225-
.query_get_value("SELECT hop_info FROM msgs WHERE id=?", (self,))
226-
.await
225+
.query_get_value("SELECT IFNULL(hop_info, '') FROM msgs WHERE id=?", (self,))
226+
.await?
227+
.with_context(|| format!("Message {self} not found"))?;
228+
Ok(hop_info)
227229
}
228230

229231
/// Returns detailed message information in a multi-line text form.
@@ -366,7 +368,11 @@ impl MsgId {
366368
let hop_info = self.hop_info(context).await?;
367369

368370
ret += "\n\n";
369-
ret += &hop_info.unwrap_or_else(|| "No Hop Info".to_owned());
371+
if hop_info.is_empty() {
372+
ret += "No Hop Info";
373+
} else {
374+
ret += &hop_info;
375+
}
370376

371377
Ok(ret)
372378
}
@@ -1998,7 +2004,9 @@ pub(crate) async fn rfc724_mid_exists_ex(
19982004
.query_row_optional(
19992005
&("SELECT id, timestamp_sent, MIN(".to_string()
20002006
+ expr
2001-
+ ") FROM msgs WHERE rfc724_mid=? ORDER BY timestamp_sent DESC"),
2007+
+ ") FROM msgs WHERE rfc724_mid=?
2008+
HAVING COUNT(*) > 0 -- Prevent MIN(expr) from returning NULL when there are no rows.
2009+
ORDER BY timestamp_sent DESC"),
20022010
(rfc724_mid,),
20032011
|row| {
20042012
let msg_id: MsgId = row.get(0)?;

‎src/mimefactory.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,8 @@ impl MimeFactory {
201201
let (in_reply_to, references) = context
202202
.sql
203203
.query_row(
204-
"SELECT mime_in_reply_to, mime_references FROM msgs WHERE id=?",
204+
"SELECT mime_in_reply_to, IFNULL(mime_references, '')
205+
FROM msgs WHERE id=?",
205206
(msg.id,),
206207
|row| {
207208
let in_reply_to: String = row.get(0)?;

‎src/sql.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -558,15 +558,13 @@ impl Sql {
558558
self.call(move |conn| match conn.query_row(sql.as_ref(), params, f) {
559559
Ok(res) => Ok(Some(res)),
560560
Err(rusqlite::Error::QueryReturnedNoRows) => Ok(None),
561-
Err(rusqlite::Error::InvalidColumnType(_, _, rusqlite::types::Type::Null)) => Ok(None),
562561
Err(err) => Err(err.into()),
563562
})
564563
.await
565564
}
566565

567566
/// Executes a query which is expected to return one row and one
568-
/// column. If the query does not return a value or returns SQL
569-
/// `NULL`, returns `Ok(None)`.
567+
/// column. If the query does not return any rows, returns `Ok(None)`.
570568
pub async fn query_get_value<T>(
571569
&self,
572570
query: &str,

0 commit comments

Comments
 (0)