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.

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(query_row_optional): do not treat rows with NULL as missing rows #6014

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

link2xt
Copy link
Collaborator

@link2xt link2xt commented Oct 2, 2024

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.

Fixes #5994

@link2xt link2xt marked this pull request as draft October 2, 2024 02:45
@link2xt link2xt force-pushed the link2xt/query-row-optional-no-null branch 2 times, most recently from dd1e9ed to 56fd0d9 Compare October 2, 2024 04:08
@link2xt
Copy link
Collaborator Author

link2xt commented Oct 2, 2024

Primary reason for this was a bug where NULL in mime_references of info messages prevented loading the whole row as if the message does not exist. As we skip info messages when trying to find parent message since #6023, the original problem is gone.

add_info_msg_with_cmd does not set mime_references for info messages, this is how they are created.
Not going to fix this as we have to deal with NULL in mime_references anyway and running a migration fixing this seems dangerous as the database may contain a lot of info messages to fix.

While trying to make a test for this one discovered another bug: #6021

@link2xt link2xt force-pushed the link2xt/query-row-optional-no-null branch 4 times, most recently from a5778e5 to aab29a1 Compare October 3, 2024 18:01
@link2xt link2xt force-pushed the link2xt/query-row-optional-no-null branch from aab29a1 to e26c419 Compare October 3, 2024 21:00
@link2xt link2xt changed the base branch from main to link2xt/dont-reference-info October 3, 2024 21:00
@link2xt link2xt force-pushed the link2xt/dont-reference-info branch from f81b531 to 637a3f6 Compare October 3, 2024 21:34
@link2xt link2xt force-pushed the link2xt/query-row-optional-no-null branch from e26c419 to 23e8df6 Compare October 3, 2024 21:34
Base automatically changed from link2xt/dont-reference-info to main October 3, 2024 21:49
@link2xt link2xt force-pushed the link2xt/query-row-optional-no-null branch 3 times, most recently from 9a3e779 to 5af3c26 Compare October 4, 2024 03:26
@link2xt link2xt marked this pull request as ready for review October 4, 2024 03:27
@link2xt link2xt requested review from iequidoo and Hocuri October 4, 2024 03:27
@link2xt link2xt force-pushed the link2xt/query-row-optional-no-null branch from 5af3c26 to 5708bba Compare October 4, 2024 03:28
@link2xt
Copy link
Collaborator Author

link2xt commented Oct 4, 2024

I added one test for ephemeral timer so this can be considered a fix, but otherwise this is mostly a refactoring to prevent more bugs similar to the one fixed in #6023.

@@ -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.

Copy link
Collaborator

@Hocuri Hocuri left a comment

Choose a reason for hiding this comment

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

LGTM, I have 2 non-blocking thoughts

src/ephemeral.rs Outdated Show resolved Hide resolved
Comment on lines -1408 to +1417
"SELECT MAX(timestamp) FROM msgs WHERE chat_id=? AND state!=?",
"SELECT MAX(timestamp)
FROM msgs
WHERE chat_id=? AND state!=?
HAVING COUNT(*) > 0",
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<....

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.
@link2xt link2xt force-pushed the link2xt/query-row-optional-no-null branch from 6ac053a to e8c239b Compare October 4, 2024 14:30
@link2xt link2xt enabled auto-merge (rebase) October 4, 2024 14:32
@link2xt link2xt merged commit 8a88479 into main Oct 4, 2024
37 checks passed
@link2xt link2xt deleted the link2xt/query-row-optional-no-null branch October 4, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get_parent_mime_headers does not handle NULL
3 participants