-
-
Notifications
You must be signed in to change notification settings - Fork 110
remove: partial downloads (remove creation of the stub messages) #7373
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
base: pre-messages
Are you sure you want to change the base?
remove: partial downloads (remove creation of the stub messages) #7373
Conversation
0d46626 to
81ca8c2
Compare
|
| info!(context, "Message already partly in DB, replacing."); | ||
| Some(msg.chat_id) | ||
|
|
||
| // TODO: look at this place |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there may be existing message-stubs that are not downloaded yet. After updating, the "Download" button will probably become non-functional; this is fine. We should make sure that DC doesn't crash when clicking "Download".
currently I'm still asking myself whether we have to to make this sacrifice or if we can still have a last version that has an option to download old message stubs and remove it in the version after...
As of the time of writing the download logic still works, because I have only removed the partial download logic that creates the stubs so far.
my current feeling/idea is to remove it in a dedicated pr as soon as it makes problems / the implementation of the new method harder - that way this pr would also be focused on the removal of just the stub creation part of partial download.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not that big of a sacrifice:
- download-on-demand is just an option (i.e. many people don't have it activated, since they don't change the settings)
- this option never worked really well
- It can already happen that you can't fully download a message, because it has been deleted on the server already.
- The user sees that there is a failure, and can ask the sender to send the message again
It does make sense to remove only the partial-download logic in the PR here, and then remove the replace_msg_id logic in a later PR. But if there is any kind of effort in it, it's not worth it, esp. as we need pre-messages rather sooner than later for calls etc.
6fe6723 to
8140ebc
Compare
6c84e01 to
938c890
Compare
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | ||
| async fn test_partial_receive_imf() -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to replace this with a test which adds a partially downloaded message to the db in some hacky way (probably by adding a full message as usual and then modifying it so that it becomes a partial download) and then receives a full message (probably the same message again). This would be a backward compat test for messages received before the upgrade.
I general other tests can be changed the same way, but i suggest not to do much work here, maybe only cover the most important scenarios which may happen after upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting #7367:
Note that there may be existing message-stubs that are not downloaded yet. After updating, the "Download" button will probably become non-functional; this is fine. We should make sure that DC doesn't crash when clicking "Download".
I.e. there is not a lot to test here, only that clicking "Download" doesn't result in a crash (the download state can just become "failed"). This could get a test if it's not too much effort.
A test can be added rather easily in this way, probably:
- Remove everything after the first call to
receive_imf_from_inboxin this test here (on main, not on this branch here) DELTACHAT_SAVE_TMP_DB=1 cargo test test_partial_receive_imfsqlite3 test-account-alice.db.dump- Copy the SQL statement inserting the partially-downloaded message. This SQL statement can be used to create an old message stub.
...but this is just to share some knowledge about what's possible. This can anyways not be done in the branch here, but in #7410
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | ||
| async fn test_webxdc_update_for_not_downloaded_instance() -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be modified to employ message reordering instead of a partially downloaded instance
| use crate::test_utils::{E2EE_INFO_MSGS, TestContext, TestContextManager}; | ||
|
|
||
| #[test] | ||
| fn test_downloadstate_values() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the test if DownloadState doesn't change yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right this test needs to be restored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! LGTM once all comments are addressed (i.e. either resolved them, or commented why it's not worth it / doesn't make sense)
BTW, in general, it's good to request a review from some people using the Github UI once you've finished your PR, in order to get speedier reviews
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | ||
| async fn test_partial_receive_imf() -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting #7367:
Note that there may be existing message-stubs that are not downloaded yet. After updating, the "Download" button will probably become non-functional; this is fine. We should make sure that DC doesn't crash when clicking "Download".
I.e. there is not a lot to test here, only that clicking "Download" doesn't result in a crash (the download state can just become "failed"). This could get a test if it's not too much effort.
A test can be added rather easily in this way, probably:
- Remove everything after the first call to
receive_imf_from_inboxin this test here (on main, not on this branch here) DELTACHAT_SAVE_TMP_DB=1 cargo test test_partial_receive_imfsqlite3 test-account-alice.db.dump- Copy the SQL statement inserting the partially-downloaded message. This SQL statement can be used to create an old message stub.
...but this is just to share some knowledge about what's possible. This can anyways not be done in the branch here, but in #7410
| info!(context, "Message already partly in DB, replacing."); | ||
| Some(msg.chat_id) | ||
|
|
||
| // TODO: look at this place |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not that big of a sacrifice:
- download-on-demand is just an option (i.e. many people don't have it activated, since they don't change the settings)
- this option never worked really well
- It can already happen that you can't fully download a message, because it has been deleted on the server already.
- The user sees that there is a failure, and can ask the sender to send the message again
It does make sense to remove only the partial-download logic in the PR here, and then remove the replace_msg_id logic in a later PR. But if there is any kind of effort in it, it's not worth it, esp. as we need pre-messages rather sooner than later for calls etc.
506ecc8 to
e564a9b
Compare
system as it is only ever applied to messages that have a pre-message.
`Config::SimulateReceiveImfError`
e564a9b to
05a1967
Compare
make download full message methods fail for now with not implemented error and comment what should happen laterdoesn't need to change at this time (and possibly not at all)I kept
partial_download_msg_bodystock string for now, as it may be reused in the new solution. because I made the linter ignore it, I added a to do item to #7367 to check if it was used in the end.removed partial download tests:
core/deltachat-rpc-client/tests/test_something.py
Line 623 in 0d46626
core/src/download.rs
Line 346 in 8b4c718
core/src/download.rs
Line 391 in 8b4c718
core/src/download.rs
Line 425 in 8b4c718
core/src/download.rs
Line 536 in 8b4c718
core/src/reaction.rs
Line 929 in 8b4c718
core/src/receive_imf/receive_imf_tests.rs
Line 4473 in 8b4c718
core/src/receive_imf/receive_imf_tests.rs
Line 5288 in 8b4c718
core/src/webxdc/webxdc_tests.rs
Line 333 in 8b4c718
core/src/calls/calls_tests.rs
Line 617 in 8b4c718
core/src/message/message_tests.rs
Line 331 in 8b4c718
core/src/message/message_tests.rs
Line 404 in 8b4c718
core/src/receive_imf/receive_imf_tests.rs
Line 4398 in 8b4c718
core/src/receive_imf/receive_imf_tests.rs
Line 4333 in 8b4c718
core/src/receive_imf/receive_imf_tests.rs
Line 4792 in 8b4c718
core/deltachat-rpc-client/tests/test_chatlist_events.py
Line 114 in 56370c2
core/deltachat-rpc-client/tests/test_something.py
Line 723 in 56370c2
core/python/tests/test_1_online.py
Line 225 in 9bc2aee
part of #7367