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

feat: Make dc_msg_get_filename() return the original attachment filename (#4309) #4555

Merged
merged 1 commit into from
Jul 27, 2023
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
16 changes: 8 additions & 8 deletions deltachat-ffi/deltachat.h
Original file line number Diff line number Diff line change
Expand Up @@ -3978,31 +3978,31 @@ char* dc_msg_get_text (const dc_msg_t* msg);
*/
char* dc_msg_get_subject (const dc_msg_t* msg);


/**
* Find out full path, file name and extension of the file associated with a
* message.
* Find out full path of the file associated with a message.
*
* Typically files are associated with images, videos, audios, documents.
* Plain text messages do not have a file.
* File name may be mangled. To obtain the original attachment filename use dc_msg_get_filename().
*
* @memberof dc_msg_t
* @param msg The message object.
* @return The full path, the file name, and the extension of the file associated with the message.
* @return The full path (with file name and extension) of the file associated with the message.
* If there is no file associated with the message, an empty string is returned.
* NULL is never returned and the returned value must be released using dc_str_unref().
iequidoo marked this conversation as resolved.
Show resolved Hide resolved
*/
char* dc_msg_get_file (const dc_msg_t* msg);


/**
* Get a base file name without the path. The base file name includes the extension; the path
* is not returned. To get the full path, use dc_msg_get_file().
* Get an original attachment filename, with extension but without the path. To get the full path,
* use dc_msg_get_file().
*
* @memberof dc_msg_t
* @param msg The message object.
* @return The base file name plus the extension without part. If there is no file
* associated with the message, an empty string is returned. The returned
* value must be released using dc_str_unref().
* @return The attachment filename. If there is no file associated with the message, an empty string
* is returned. The returned value must be released using dc_str_unref().
*/
char* dc_msg_get_filename (const dc_msg_t* msg);

Expand Down
3 changes: 2 additions & 1 deletion node/test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,8 @@ describe('Offline Tests with unconfigured account', function () {
context.setChatProfileImage(chatId, imagePath)
const blobPath = context.getChat(chatId).getProfileImage()
expect(blobPath.startsWith(blobs)).to.be.true
expect(blobPath.endsWith(image)).to.be.true
expect(blobPath.includes('image')).to.be.true
Copy link
Collaborator

Choose a reason for hiding this comment

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

These changes and most of the discussion on the PR are unrelated now, this is why I suggested opening a new branch and PR instead, but ok.

expect(blobPath.endsWith('.jpeg')).to.be.true

context.setChatProfileImage(chatId, null)
expect(context.getChat(chatId).getProfileImage()).to.be.equal(
Expand Down
19 changes: 12 additions & 7 deletions python/tests/test_1_online.py
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,9 @@ def test_send_file_twice_unicode_filename_mangling(tmp_path, acfactory, lp):
ac1, ac2 = acfactory.get_online_accounts(2)
chat = acfactory.get_accepted_chat(ac1, ac2)

basename = "somedäüta.html.zip"
p = tmp_path / basename
basename = "somedäüta"
ext = ".html.zip"
p = tmp_path / (basename + ext)
p.write_text("some data")

def send_and_receive_message():
Expand All @@ -181,23 +182,26 @@ def send_and_receive_message():
msg = send_and_receive_message()
assert msg.text == "withfile"
assert open(msg.filename).read() == "some data"
assert msg.filename.endswith(basename)
msg.filename.index(basename)
assert msg.filename.endswith(ext)

msg2 = send_and_receive_message()
assert msg2.text == "withfile"
assert open(msg2.filename).read() == "some data"
assert msg2.filename.endswith("html.zip")
msg2.filename.index(basename)
assert msg2.filename.endswith(ext)
assert msg.filename != msg2.filename


def test_send_file_html_attachment(tmp_path, acfactory, lp):
ac1, ac2 = acfactory.get_online_accounts(2)
chat = acfactory.get_accepted_chat(ac1, ac2)

basename = "test.html"
basename = "test"
ext = ".html"
content = "<html><body>text</body>data"

p = tmp_path / basename
p = tmp_path / (basename + ext)
# write wrong html to see if core tries to parse it
# (it shouldn't as it's a file attachment)
p.write_text(content)
Expand All @@ -211,7 +215,8 @@ def test_send_file_html_attachment(tmp_path, acfactory, lp):
msg = ac2.get_message_by_id(ev.data2)

assert open(msg.filename).read() == content
assert msg.filename.endswith(basename)
msg.filename.index(basename)
assert msg.filename.endswith(ext)


def test_html_message(acfactory, lp):
Expand Down
7 changes: 3 additions & 4 deletions python/tests/test_2_increation.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,9 @@ def test_no_increation_copies_to_blobdir(self, tmp_path, acfactory, lp):
assert str(tmp_path) != ac1.get_blobdir()
src = tmp_path / "file.txt"
src.write_text("hello there\n")
chat.send_file(str(src))

blob_src = os.path.join(ac1.get_blobdir(), "file.txt")
assert os.path.exists(blob_src), "file.txt not copied to blobdir"
msg = chat.send_file(str(src))
assert msg.filename.startswith(os.path.join(ac1.get_blobdir(), "file"))
assert msg.filename.endswith(".txt")

def test_forward_increation(self, acfactory, data, lp):
ac1, ac2 = acfactory.get_online_accounts(2)
Expand Down
10 changes: 9 additions & 1 deletion src/chat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2042,6 +2042,14 @@ async fn prepare_msg_blob(context: &Context, msg: &mut Message) -> Result<()> {
}
}
msg.param.set(Param::File, blob.as_name());
if let (Some(filename), Some(blob_ext)) = (msg.param.get(Param::Filename), blob.suffix()) {
let stem = match filename.rsplit_once('.') {
Some((stem, _)) => stem,
None => filename,
};
msg.param
.set(Param::Filename, stem.to_string() + "." + blob_ext);
}

if msg.viewtype == Viewtype::File || msg.viewtype == Viewtype::Image {
// Correct the type, take care not to correct already very special
Expand Down Expand Up @@ -5521,7 +5529,7 @@ mod tests {
let msg = bob.recv_msg(&sent_msg).await;
assert_eq!(msg.chat_id, bob_chat.id);
assert_eq!(msg.get_viewtype(), Viewtype::Sticker);
assert_eq!(msg.get_filename(), Some(filename.to_string()));
assert_eq!(msg.get_filename().unwrap(), filename);
assert_eq!(msg.get_width(), w);
assert_eq!(msg.get_height(), h);
assert!(msg.get_filebytes(&bob).await?.unwrap() > 250);
Expand Down
2 changes: 2 additions & 0 deletions src/imex/transfer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,9 @@ mod tests {
_ => panic!("wrong chat item"),
};
let msg = Message::load_from_db(&ctx1, *msgid).await.unwrap();

let path = msg.get_file(&ctx1).unwrap();
assert_eq!(path.with_file_name("hello.txt"), path);
let text = fs::read_to_string(&path).await.unwrap();
assert_eq!(text, "i am attachment");

Expand Down
11 changes: 9 additions & 2 deletions src/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -688,11 +688,13 @@ impl Message {
&self.subject
}

/// Returns base file name without the path.
/// The base file name includes the extension.
/// Returns original filename (as shown in chat).
///
/// To get the full path, use [`Self::get_file()`].
pub fn get_filename(&self) -> Option<String> {
if let Some(name) = self.param.get(Param::Filename) {
return Some(name.to_string());
}
self.param
.get(Param::File)
.and_then(|file| Path::new(file).file_name())
Expand Down Expand Up @@ -972,6 +974,11 @@ impl Message {
/// the file will only be used when the message is prepared
/// for sending.
pub fn set_file(&mut self, file: impl ToString, filemime: Option<&str>) {
if let Some(name) = Path::new(&file.to_string()).file_name() {
if let Some(name) = name.to_str() {
self.param.set(Param::Filename, name);
}
}
self.param.set(Param::File, file);
if let Some(filemime) = filemime {
self.param.set(Param::MimeType, filemime);
Expand Down
8 changes: 6 additions & 2 deletions src/mimefactory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1386,7 +1386,7 @@ async fn build_body_file(
.param
.get_blob(Param::File, context, true)
.await?
.context("msg has no filename")?;
.context("msg has no file")?;
let suffix = blob.suffix().unwrap_or("dat");

// Get file name to use for sending. For privacy purposes, we do
Expand Down Expand Up @@ -1431,7 +1431,11 @@ async fn build_body_file(
),
&suffix
),
_ => blob.as_file_name().to_string(),
_ => msg
.param
.get(Param::Filename)
.unwrap_or_else(|| blob.as_file_name())
.to_string(),
};

/* check mimetype */
Expand Down
1 change: 1 addition & 0 deletions src/mimeparser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1268,6 +1268,7 @@ impl MimeMessage {
part.mimetype = Some(mime_type);
part.bytes = decoded_data.len();
part.param.set(Param::File, blob.as_name());
part.param.set(Param::Filename, filename);
part.param.set(Param::MimeType, raw_mime);
part.is_related = is_related;

Expand Down
5 changes: 4 additions & 1 deletion src/param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ pub enum Param {
/// For messages and jobs
File = b'f',

/// For messages: original filename (as shown in chat)
Filename = b'v',

/// For messages: This name should be shown instead of contact.get_display_name()
/// (used if this is a mailinglist
/// or explicitly set using set_override_sender_name(), eg. by bots)
Expand Down Expand Up @@ -528,7 +531,7 @@ mod tests {

fs::write(fname, b"boo").await.unwrap();
let blob = p.get_blob(Param::File, &t, true).await.unwrap().unwrap();
assert_eq!(blob, BlobObject::from_name(&t, "foo".to_string()).unwrap());
assert!(blob.as_file_name().starts_with("foo"));

// Blob in blobdir, expect blob.
let bar_path = t.get_blobdir().join("bar");
Expand Down
25 changes: 12 additions & 13 deletions src/receive_imf/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1456,7 +1456,9 @@ async fn test_pdf_filename_simple() {
.await;
assert_eq!(msg.viewtype, Viewtype::File);
assert_eq!(msg.text, "mail body");
assert_eq!(msg.param.get(Param::File).unwrap(), "$BLOBDIR/simple.pdf");
let file_path = msg.param.get(Param::File).unwrap();
assert!(file_path.starts_with("$BLOBDIR/simple"));
assert!(file_path.ends_with(".pdf"));
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
Expand All @@ -1470,10 +1472,9 @@ async fn test_pdf_filename_continuation() {
.await;
assert_eq!(msg.viewtype, Viewtype::File);
assert_eq!(msg.text, "mail body");
assert_eq!(
msg.param.get(Param::File).unwrap(),
"$BLOBDIR/test pdf äöüß.pdf"
);
let file_path = msg.param.get(Param::File).unwrap();
assert!(file_path.starts_with("$BLOBDIR/test pdf äöüß"));
assert!(file_path.ends_with(".pdf"));
}

/// HTML-images may come with many embedded images, eg. tiny icons, corners for formatting,
Expand Down Expand Up @@ -2797,7 +2798,7 @@ Reply from different address
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_long_filenames() -> Result<()> {
async fn test_long_and_duplicated_filenames() -> Result<()> {
let mut tcm = TestContextManager::new();
let alice = tcm.alice().await;
let bob = tcm.bob().await;
Expand All @@ -2809,6 +2810,7 @@ async fn test_long_filenames() -> Result<()> {
"foo. .tar.gz",
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.tar.gz",
"a.tar.gz",
"a.tar.gz",
"a.a..a.a.a.a.tar.gz",
] {
let attachment = alice.blobdir.join(filename_sent);
Expand All @@ -2823,22 +2825,19 @@ async fn test_long_filenames() -> Result<()> {

let msg_bob = bob.recv_msg(&sent).await;

async fn check_message(msg: &Message, t: &TestContext, content: &str) {
async fn check_message(msg: &Message, t: &TestContext, filename: &str, content: &str) {
assert_eq!(msg.get_viewtype(), Viewtype::File);
let resulting_filename = msg.get_filename().unwrap();
assert_eq!(resulting_filename, filename);
let path = msg.get_file(t).unwrap();
assert!(
resulting_filename.ends_with(".tar.gz"),
"{resulting_filename:?} doesn't end with .tar.gz, path: {path:?}"
);
assert!(
path.to_str().unwrap().ends_with(".tar.gz"),
"path {path:?} doesn't end with .tar.gz"
);
assert_eq!(fs::read_to_string(path).await.unwrap(), content);
}
check_message(&msg_alice, &alice, &content).await;
check_message(&msg_bob, &bob, &content).await;
check_message(&msg_alice, &alice, filename_sent, &content).await;
check_message(&msg_bob, &bob, filename_sent, &content).await;
}

Ok(())
Expand Down
12 changes: 3 additions & 9 deletions src/summary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use crate::contact::{Contact, ContactId};
use crate::context::Context;
use crate::message::{Message, MessageState, Viewtype};
use crate::mimeparser::SystemMessage;
use crate::param::Param;
use crate::stock_str;
use crate::tools::truncate;

Expand Down Expand Up @@ -133,14 +132,8 @@ impl Message {
append_text = false;
stock_str::ac_setup_msg_subject(context).await
} else {
let file_name: String = self
.param
.get_path(Param::File, context)
.unwrap_or(None)
.and_then(|path| {
path.file_name()
.map(|fname| fname.to_string_lossy().into_owned())
})
let file_name = self
.get_filename()
.unwrap_or_else(|| String::from("ErrFileName"));
let label = if self.viewtype == Viewtype::Audio {
stock_str::audio(context).await
Expand Down Expand Up @@ -200,6 +193,7 @@ impl Message {
#[cfg(test)]
mod tests {
use super::*;
use crate::param::Param;
use crate::test_utils as test;

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
Expand Down
6 changes: 3 additions & 3 deletions src/webxdc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1088,7 +1088,7 @@ mod tests {
.await?;
let instance = t.get_last_msg().await;
assert_eq!(instance.viewtype, Viewtype::Webxdc);
assert_eq!(instance.get_filename(), Some("minimal.xdc".to_string()));
assert_eq!(instance.get_filename().unwrap(), "minimal.xdc");

receive_imf(
&t,
Expand All @@ -1098,7 +1098,7 @@ mod tests {
.await?;
let instance = t.get_last_msg().await;
assert_eq!(instance.viewtype, Viewtype::File); // we require the correct extension, only a mime type is not sufficient
assert_eq!(instance.get_filename(), Some("index.html".to_string()));
assert_eq!(instance.get_filename().unwrap(), "index.html");

Ok(())
}
Expand Down Expand Up @@ -1786,7 +1786,7 @@ mod tests {
// bob receives the instance together with the initial updates in a single message
let bob_instance = bob.recv_msg(&sent1).await;
assert_eq!(bob_instance.viewtype, Viewtype::Webxdc);
assert_eq!(bob_instance.get_filename(), Some("minimal.xdc".to_string()));
assert_eq!(bob_instance.get_filename().unwrap(), "minimal.xdc");
assert!(sent1.payload().contains("Content-Type: application/json"));
assert!(sent1.payload().contains("status-update.json"));
assert_eq!(
Expand Down