From 2c4dbe6e686199ff097d9487e967cf02b1e3d27e Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Sun, 8 Dec 2019 20:20:17 +0100 Subject: [PATCH] Re-work some in-creation file handling This effectively reverts https://github.com/deltachat/deltachat-core-rust/pull/964 for chat.rs, which in that PR was thought to fix something. So maybe something is still broken? But after improving tests the previous code seems to be correct. - Update Python bindings to not always use dc_prepare_msg path when sending messages with attachements. When using dc_prepare_msg the blobs need to be created in the blobdir since they will not get copied and many tests where not doing this. - Add a test that ensures that calling dc_prepare_msg with a file **not** in the blobdir fails. - Add a test that ensures that calling dc_send_msg directly with a file **not** in the blobdir copies the file to the blobdir. This test cheats a little by knowing what the filename in the blobdir will be which is implementation-dependent and thus a bit brittle. But for now it proves correct behaviour so let's go with this. - Improve the test_forward_increation test to ensure that the in-creation file only has it's final state before calling dc_send_msg. This checks the correct file data is sent out and not the preparing data, this fails with the chat.rs changes in #964 (reverted here to make this work again). Also fix the test to actually create the in-creation file in the blobdir. - Fix test_send_file_twice_unicode_filename_mangling to not use in-creation. It was not creating it's files in the blobdir and that is an error when using in-creation and it didn't seem it was trying to test something about the in-creation logic (which is tested in test_increation.py already). - Fix Message._msgtate code which presumably was not used before? - Rename `BlobObject::create_from_path` to `BlobObject::new_from_path`. All the `BlobObject::create*` calls now always create new files which is much more consistent. APIs should do what is obious. --- python/src/deltachat/chat.py | 42 +++++++++++++++++++++++---- python/src/deltachat/message.py | 2 +- python/tests/test_account.py | 5 +--- python/tests/test_increation.py | 51 ++++++++++++++++++++++++++++++--- python/tox.ini | 4 +-- src/blob.rs | 8 +++--- src/chat.rs | 19 ++++-------- src/config.rs | 2 +- src/param.rs | 2 +- 9 files changed, 98 insertions(+), 37 deletions(-) diff --git a/python/src/deltachat/chat.py b/python/src/deltachat/chat.py index ca204193f..00787b795 100644 --- a/python/src/deltachat/chat.py +++ b/python/src/deltachat/chat.py @@ -109,6 +109,30 @@ class Chat(object): # ------ chat messaging API ------------------------------ + def send_msg(self, msg): + """send a message by using a ready Message object. + + :param msg: a :class:`deltachat.message.Message` instance + previously returned by + e.g. :meth:`deltachat.message.Message.new_empty` or + :meth:`prepare_file`. + :raises ValueError: if message can not be sent. + + :returns: a :class:`deltachat.message.Message` instance as + sent out. This is the same object as was passed in, which + has been modified with the new state of the core. + """ + if msg.is_out_preparing(): + assert msg.id != 0 + # get a fresh copy of dc_msg, the core needs it + msg = Message.from_db(self.account, msg.id) + sent_id = lib.dc_send_msg(self._dc_context, self.id, msg._dc_msg) + if sent_id == 0: + raise ValueError("message could not be sent") + # modify message in place to avoid bad state for the caller + msg._dc_msg = Message.from_db(self.account, sent_id)._dc_msg + return msg + def send_text(self, text): """ send a text message and return the resulting Message instance. @@ -130,9 +154,12 @@ class Chat(object): :raises ValueError: if message can not be send/chat does not exist. :returns: the resulting :class:`deltachat.message.Message` instance """ - msg = self.prepare_message_file(path=path, mime_type=mime_type) - self.send_prepared(msg) - return msg + msg = Message.new_empty(self.account, view_type="file") + msg.set_file(path, mime_type) + sent_id = lib.dc_send_msg(self._dc_context, self.id, msg._dc_msg) + if sent_id == 0: + raise ValueError("message could not be sent") + return Message.from_db(self.account, sent_id) def send_image(self, path): """ send an image message and return the resulting Message instance. @@ -142,9 +169,12 @@ class Chat(object): :returns: the resulting :class:`deltachat.message.Message` instance """ mime_type = mimetypes.guess_type(path)[0] - msg = self.prepare_message_file(path=path, mime_type=mime_type, view_type="image") - self.send_prepared(msg) - return msg + msg = Message.new_empty(self.account, view_type="image") + msg.set_file(path, mime_type) + sent_id = lib.dc_send_msg(self._dc_context, self.id, msg._dc_msg) + if sent_id == 0: + raise ValueError("message could not be sent") + return Message.from_db(self.account, sent_id) def prepare_message(self, msg): """ create a new prepared message. diff --git a/python/src/deltachat/message.py b/python/src/deltachat/message.py index 2c1dd3703..d8128bfb0 100644 --- a/python/src/deltachat/message.py +++ b/python/src/deltachat/message.py @@ -174,7 +174,7 @@ class Message(object): @property def _msgstate(self): if self.id == 0: - dc_msg = self.message._dc_msg + dc_msg = self._dc_msg else: # load message from db to get a fresh/current state dc_msg = ffi.gc( diff --git a/python/tests/test_account.py b/python/tests/test_account.py index 0d3c15a41..cc6db441f 100644 --- a/python/tests/test_account.py +++ b/python/tests/test_account.py @@ -456,10 +456,7 @@ class TestOnlineAccount: msg1 = Message.new_empty(ac1, "file") msg1.set_text("withfile") msg1.set_file(p) - message = chat.prepare_message(msg1) - assert message.is_out_preparing() - assert message.text == "withfile" - chat.send_prepared(message) + chat.send_msg(msg1) lp.sec("ac2: receive message") ev = ac2._evlogger.get_matching("DC_EVENT_INCOMING_MSG|DC_EVENT_MSGS_CHANGED") diff --git a/python/tests/test_increation.py b/python/tests/test_increation.py index 7692c6776..0b6735dda 100644 --- a/python/tests/test_increation.py +++ b/python/tests/test_increation.py @@ -1,10 +1,49 @@ from __future__ import print_function + +import os.path +import shutil + +import pytest from filecmp import cmp -from deltachat import const + from conftest import wait_configuration_progress, wait_msgs_changed +from deltachat import const class TestOnlineInCreation: + def test_increation_not_blobdir(self, tmpdir, acfactory, lp): + ac1 = acfactory.get_online_configuring_account() + ac2 = acfactory.get_online_configuring_account() + wait_configuration_progress(ac1, 1000) + wait_configuration_progress(ac2, 1000) + + c2 = ac1.create_contact(email=ac2.get_config("addr")) + chat = ac1.create_chat_by_contact(c2) + + lp.sec("Creating in-creation file outside of blobdir") + assert tmpdir.strpath != ac1.get_blobdir() + src = tmpdir.join('file.txt').ensure(file=1) + with pytest.raises(Exception): + chat.prepare_message_file(src.strpath) + + def test_no_increation_copies_to_blobdir(self, tmpdir, acfactory, lp): + ac1 = acfactory.get_online_configuring_account() + ac2 = acfactory.get_online_configuring_account() + wait_configuration_progress(ac1, 1000) + wait_configuration_progress(ac2, 1000) + + c2 = ac1.create_contact(email=ac2.get_config("addr")) + chat = ac1.create_chat_by_contact(c2) + + lp.sec("Creating file outside of blobdir") + assert tmpdir.strpath != ac1.get_blobdir() + src = tmpdir.join('file.txt') + src.write("hello there\n") + chat.send_file(src.strpath) + + blob_src = os.path.join(ac1.get_blobdir(), 'file.txt') + assert os.path.exists(blob_src), "file.txt not copied to blobdir" + def test_forward_increation(self, acfactory, data, lp): ac1 = acfactory.get_online_configuring_account() ac2 = acfactory.get_online_configuring_account() @@ -17,7 +56,10 @@ class TestOnlineInCreation: wait_msgs_changed(ac1, 0, 0) # why no chat id? lp.sec("create a message with a file in creation") - path = data.get_path("d.png") + orig = data.get_path("d.png") + path = os.path.join(ac1.get_blobdir(), 'd.png') + with open(path, "x") as fp: + fp.write("preparing") prepared_original = chat.prepare_message_file(path) assert prepared_original.is_out_preparing() wait_msgs_changed(ac1, chat.id, prepared_original.id) @@ -38,6 +80,7 @@ class TestOnlineInCreation: lp.sec("finish creating the file and send it") assert prepared_original.is_out_preparing() + shutil.copyfile(orig, path) chat.send_prepared(prepared_original) assert prepared_original.is_out_pending() or prepared_original.is_out_delivered() wait_msgs_changed(ac1, chat.id, prepared_original.id) @@ -59,11 +102,11 @@ class TestOnlineInCreation: ev1 = ac2._evlogger.get_matching("DC_EVENT_MSGS_CHANGED") assert ev1[1] > const.DC_CHAT_ID_LAST_SPECIAL received_original = ac2.get_message_by_id(ev1[2]) - assert cmp(received_original.filename, path, False) + assert cmp(received_original.filename, orig, shallow=False) lp.sec("wait2 for original or forwarded messages to arrive") ev2 = ac2._evlogger.get_matching("DC_EVENT_MSGS_CHANGED") assert ev2[1] > const.DC_CHAT_ID_LAST_SPECIAL assert ev2[1] != ev1[1] received_copy = ac2.get_message_by_id(ev2[2]) - assert cmp(received_copy.filename, path, False) + assert cmp(received_copy.filename, orig, shallow=False) diff --git a/python/tox.ini b/python/tox.ini index 29f1c3c43..fb0b65eb0 100644 --- a/python/tox.ini +++ b/python/tox.ini @@ -65,8 +65,8 @@ commands = [pytest] -addopts = -v -rs -python_files = tests/test_*.py +addopts = -v -ra +python_files = tests/test_*.py norecursedirs = .tox xfail_strict=true timeout = 60 diff --git a/src/blob.rs b/src/blob.rs index b16972eae..cbc3d8a33 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -159,7 +159,7 @@ impl<'a> BlobObject<'a> { /// This merely delegates to the [BlobObject::create_and_copy] and /// the [BlobObject::from_path] methods. See those for possible /// errors. - pub fn create_from_path( + pub fn new_from_path( context: &Context, src: impl AsRef, ) -> std::result::Result { @@ -559,14 +559,14 @@ mod tests { let src_ext = t.dir.path().join("external"); fs::write(&src_ext, b"boo").unwrap(); - let blob = BlobObject::create_from_path(&t.ctx, &src_ext).unwrap(); + let blob = BlobObject::new_from_path(&t.ctx, &src_ext).unwrap(); assert_eq!(blob.as_name(), "$BLOBDIR/external"); let data = fs::read(blob.to_abs_path()).unwrap(); assert_eq!(data, b"boo"); let src_int = t.ctx.get_blobdir().join("internal"); fs::write(&src_int, b"boo").unwrap(); - let blob = BlobObject::create_from_path(&t.ctx, &src_int).unwrap(); + let blob = BlobObject::new_from_path(&t.ctx, &src_int).unwrap(); assert_eq!(blob.as_name(), "$BLOBDIR/internal"); let data = fs::read(blob.to_abs_path()).unwrap(); assert_eq!(data, b"boo"); @@ -576,7 +576,7 @@ mod tests { let t = dummy_context(); let src_ext = t.dir.path().join("autocrypt-setup-message-4137848473.html"); fs::write(&src_ext, b"boo").unwrap(); - let blob = BlobObject::create_from_path(&t.ctx, &src_ext).unwrap(); + let blob = BlobObject::new_from_path(&t.ctx, &src_ext).unwrap(); assert_eq!( blob.as_name(), "$BLOBDIR/autocrypt-setup-message-4137848473.html" diff --git a/src/chat.rs b/src/chat.rs index 0a308bdc6..693489487 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -725,20 +725,11 @@ fn prepare_msg_blob(context: &Context, msg: &mut Message) -> Result<(), Error> { if msg.type_0 == Viewtype::Text { // the caller should check if the message text is empty } else if msgtype_has_file(msg.type_0) { - let blob = if let Some(f) = msg.param.get_file(Param::File, context)? { - match f { - ParamsFile::Blob(blob) => blob, - ParamsFile::FsPath(path) => { - // path is outside the blobdir, let's copy - let blob = BlobObject::create_and_copy(context, path)?; - msg.param.set(Param::File, blob.as_name()); - - blob - } - } - } else { - bail!("Attachment missing for message of type #{}", msg.type_0); - }; + let blob = msg + .param + .get_blob(Param::File, context, !msg.is_increation())? + .ok_or_else(|| format_err!("Attachment missing for message of type #{}", msg.type_0))?; + msg.param.set(Param::File, blob.as_name()); if msg.type_0 == Viewtype::File || msg.type_0 == Viewtype::Image { // Correct the type, take care not to correct already very special diff --git a/src/config.rs b/src/config.rs index d8b5240e2..b13eb4f29 100644 --- a/src/config.rs +++ b/src/config.rs @@ -128,7 +128,7 @@ impl Context { pub fn set_config(&self, key: Config, value: Option<&str>) -> crate::sql::Result<()> { match key { Config::Selfavatar if value.is_some() => { - let blob = BlobObject::create_from_path(&self, value.unwrap())?; + let blob = BlobObject::new_from_path(&self, value.unwrap())?; self.sql.set_raw_config(self, key, Some(blob.as_name())) } Config::InboxWatch => { diff --git a/src/param.rs b/src/param.rs index 0979cd1a0..3276f2afe 100644 --- a/src/param.rs +++ b/src/param.rs @@ -250,7 +250,7 @@ impl Params { let file = ParamsFile::from_param(context, val)?; let blob = match file { ParamsFile::FsPath(path) => match create { - true => BlobObject::create_from_path(context, path)?, + true => BlobObject::new_from_path(context, path)?, false => BlobObject::from_path(context, path)?, }, ParamsFile::Blob(blob) => blob,