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

@@ -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)
+        msg = Message.new_empty(self.account, view_type="file")
-        self.send_prepared(msg)
+        msg.set_file(path, mime_type)
-        return msg
+        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")
+        msg = Message.new_empty(self.account, view_type="image")
-        self.send_prepared(msg)
+        msg.set_file(path, mime_type)
-        return msg
+        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.

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(

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)
+            chat.send_msg(msg1)
-            assert message.is_out_preparing()
-            assert message.text == "withfile"
-            chat.send_prepared(message)
 
             lp.sec("ac2: receive message")
             ev = ac2._evlogger.get_matching("DC_EVENT_INCOMING_MSG|DC_EVENT_MSGS_CHANGED")

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)

python/tox.ini

@@ -65,8 +65,8 @@ commands =
 
 
 [pytest]
-addopts = -v -rs 
+addopts = -v -ra
-python_files = tests/test_*.py 
+python_files = tests/test_*.py
 norecursedirs = .tox 
 xfail_strict=true
 timeout = 60 

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<Path>,
     ) -> std::result::Result<BlobObject, BlobError> {
@@ -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"

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)? {
+        let blob = msg
-            match f {
+            .param
-                ParamsFile::Blob(blob) => blob,
+            .get_blob(Param::File, context, !msg.is_increation())?
-                ParamsFile::FsPath(path) => {
+            .ok_or_else(|| format_err!("Attachment missing for message of type #{}", msg.type_0))?;
-                    // path is outside the blobdir, let's copy
+        msg.param.set(Param::File, blob.as_name());
-                    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);
-        };
 
         if msg.type_0 == Viewtype::File || msg.type_0 == Viewtype::Image {
             // Correct the type, take care not to correct already very special

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 => {

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,