Skip to content

fix(query_row_optional): do not treat rows with NULL as missing rows #6014

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.

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

Merged
merged 1 commit into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion deltachat-jsonrpc/src/api/types/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,7 @@ pub struct MessageInfo {
error: Option<String>,
rfc724_mid: String,
server_urls: Vec<String>,
hop_info: Option<String>,
hop_info: String,
}

impl MessageInfo {
Expand Down
25 changes: 20 additions & 5 deletions src/chat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1042,7 +1042,13 @@ impl ChatId {
pub(crate) async fn get_timestamp(self, context: &Context) -> Result<Option<i64>> {
let timestamp = context
.sql
.query_get_value("SELECT MAX(timestamp) FROM msgs WHERE chat_id=?", (self,))
.query_get_value(
"SELECT MAX(timestamp)
FROM msgs
WHERE chat_id=?
HAVING COUNT(*) > 0",
(self,),
)
.await?;
Ok(timestamp)
}
Expand Down Expand Up @@ -1251,7 +1257,7 @@ impl ChatId {
) -> Result<Option<(String, String, String)>> {
self.parent_query(
context,
"rfc724_mid, mime_in_reply_to, mime_references",
"rfc724_mid, mime_in_reply_to, IFNULL(mime_references, '')",
state_out_min,
|row: &rusqlite::Row| {
let rfc724_mid: String = row.get(0)?;
Expand Down Expand Up @@ -1405,7 +1411,10 @@ impl ChatId {
context
.sql
.query_get_value(
"SELECT MAX(timestamp) FROM msgs WHERE chat_id=? AND state!=?",
"SELECT MAX(timestamp)
FROM msgs
WHERE chat_id=? AND state!=?
HAVING COUNT(*) > 0",
Comment on lines -1408 to +1417
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just FTR: It would also be possible to expect an Option<i64> in the Rust code, and then handle it within Rust instead of SQL:

@@ -1402,7 +1402,7 @@ pub(crate) async fn calc_sort_timestamp(
     ) -> Result<i64> {
         let mut sort_timestamp = cmp::min(message_timestamp, smeared_time(context));
 
-        let last_msg_time: Option<i64> = if always_sort_to_bottom {
+        let last_msg_time: Option<Option<i64>> = if always_sort_to_bottom {
             // get newest message for this chat
 
             // Let hidden messages also be ordered with protection messages because hidden messages
@@ -1413,8 +1413,7 @@ pub(crate) async fn calc_sort_timestamp(
                 .query_get_value(
                     "SELECT MAX(timestamp)
                      FROM msgs
-                     WHERE chat_id=? AND state!=?
-                     HAVING COUNT(*) > 0",
+                     WHERE chat_id=? AND state!=?",
                     (self, MessageState::OutDraft),
                 )
                 .await?
@@ -1432,8 +1431,7 @@ pub(crate) async fn calc_sort_timestamp(
                 .query_get_value(
                     "SELECT MAX(timestamp)
                      FROM msgs
-                     WHERE chat_id=? AND hidden=0 AND state>?
-                     HAVING COUNT(*) > 0",
+                     WHERE chat_id=? AND hidden=0 AND state>?",
                     (self, MessageState::InFresh),
                 )
                 .await?
@@ -1441,7 +1439,7 @@ pub(crate) async fn calc_sort_timestamp(
             None
         };
 
-        if let Some(last_msg_time) = last_msg_time {
+        if let Some(Some(last_msg_time)) = last_msg_time {
             if last_msg_time > sort_timestamp {
                 sort_timestamp = last_msg_time;
             }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Option::flatten() can also be used to make the code more brief

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just kept HAVING here, devs have to understand interaction between MIN, MAX and HAVING anyway and it is then easier to read than trying to think about Result<Option<Option<....

(self, MessageState::OutDraft),
)
.await?
Expand All @@ -1421,7 +1430,10 @@ impl ChatId {
context
.sql
.query_get_value(
"SELECT MAX(timestamp) FROM msgs WHERE chat_id=? AND hidden=0 AND state>?",
"SELECT MAX(timestamp)
FROM msgs
WHERE chat_id=? AND hidden=0 AND state>?
HAVING COUNT(*) > 0",
(self, MessageState::InFresh),
)
.await?
Expand Down Expand Up @@ -4345,7 +4357,10 @@ pub async fn add_device_msg_with_importance(
if let Some(last_msg_time) = context
.sql
.query_get_value(
"SELECT MAX(timestamp) FROM msgs WHERE chat_id=?",
"SELECT MAX(timestamp)
FROM msgs
WHERE chat_id=?
HAVING COUNT(*) > 0",
(chat_id,),
)
.await?
Expand Down
28 changes: 22 additions & 6 deletions src/ephemeral.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ use std::num::ParseIntError;
use std::str::FromStr;
use std::time::{Duration, UNIX_EPOCH};

use anyhow::{ensure, Result};
use anyhow::{ensure, Context as _, Result};
use async_channel::Receiver;
use serde::{Deserialize, Serialize};
use tokio::time::timeout;
Expand Down Expand Up @@ -176,9 +176,13 @@ impl ChatId {
pub async fn get_ephemeral_timer(self, context: &Context) -> Result<Timer> {
let timer = context
.sql
.query_get_value("SELECT ephemeral_timer FROM chats WHERE id=?;", (self,))
.await?;
Ok(timer.unwrap_or_default())
.query_get_value(
"SELECT IFNULL(ephemeral_timer, 0) FROM chats WHERE id=?",
(self,),
)
.await?
.with_context(|| format!("Chat {self} not found"))?;
Ok(timer)
}

/// Set ephemeral timer value without sending a message.
Expand Down Expand Up @@ -509,7 +513,8 @@ async fn next_delete_device_after_timestamp(context: &Context) -> Result<Option<
FROM msgs
WHERE chat_id > ?
AND chat_id != ?
AND chat_id != ?;
AND chat_id != ?
HAVING count(*) > 0
"#,
(DC_CHAT_ID_TRASH, self_chat_id, device_chat_id),
)
Expand All @@ -533,7 +538,8 @@ async fn next_expiration_timestamp(context: &Context) -> Option<i64> {
SELECT min(ephemeral_timestamp)
FROM msgs
WHERE ephemeral_timestamp != 0
AND chat_id != ?;
AND chat_id != ?
HAVING count(*) > 0
"#,
(DC_CHAT_ID_TRASH,), // Trash contains already deleted messages, skip them
)
Expand Down Expand Up @@ -1410,4 +1416,14 @@ mod tests {

Ok(())
}

/// Tests that `.get_ephemeral_timer()` returns an error for invalid chat ID.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_get_ephemeral_timer_wrong_chat_id() -> Result<()> {
let context = TestContext::new().await;
let chat_id = ChatId::new(12345);
assert!(chat_id.get_ephemeral_timer(&context).await.is_err());

Ok(())
}
}
20 changes: 14 additions & 6 deletions src/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,11 +219,13 @@ impl MsgId {
}

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

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

ret += "\n\n";
ret += &hop_info.unwrap_or_else(|| "No Hop Info".to_owned());
if hop_info.is_empty() {
ret += "No Hop Info";
} else {
ret += &hop_info;
}

Ok(ret)
}
Expand Down Expand Up @@ -1998,7 +2004,9 @@ pub(crate) async fn rfc724_mid_exists_ex(
.query_row_optional(
&("SELECT id, timestamp_sent, MIN(".to_string()
+ expr
+ ") FROM msgs WHERE rfc724_mid=? ORDER BY timestamp_sent DESC"),
+ ") FROM msgs WHERE rfc724_mid=?
HAVING COUNT(*) > 0 -- Prevent MIN(expr) from returning NULL when there are no rows.
ORDER BY timestamp_sent DESC"),
(rfc724_mid,),
|row| {
let msg_id: MsgId = row.get(0)?;
Expand Down
3 changes: 2 additions & 1 deletion src/mimefactory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,8 @@ impl MimeFactory {
let (in_reply_to, references) = context
.sql
.query_row(
"SELECT mime_in_reply_to, mime_references FROM msgs WHERE id=?",
"SELECT mime_in_reply_to, IFNULL(mime_references, '')
FROM msgs WHERE id=?",
(msg.id,),
|row| {
let in_reply_to: String = row.get(0)?;
Expand Down
4 changes: 1 addition & 3 deletions src/sql.rs
Original file line number Diff line number Diff line change
Expand Up @@ -558,15 +558,13 @@ impl Sql {
self.call(move |conn| match conn.query_row(sql.as_ref(), params, f) {
Ok(res) => Ok(Some(res)),
Err(rusqlite::Error::QueryReturnedNoRows) => Ok(None),
Err(rusqlite::Error::InvalidColumnType(_, _, rusqlite::types::Type::Null)) => Ok(None),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the main part of the PR.

Err(err) => Err(err.into()),
})
.await
}

/// Executes a query which is expected to return one row and one
/// column. If the query does not return a value or returns SQL
/// `NULL`, returns `Ok(None)`.
/// column. If the query does not return any rows, returns `Ok(None)`.
pub async fn query_get_value<T>(
&self,
query: &str,
Expand Down
Loading