576282 - Expose a JS representation of attachments (as in "message reader attachments") for MimeMessages

Status: RESOLVED FIXED

(MailNews Core :: Database, enhancement)

Description

[Jonathan Protzenko [:protz]]

Attachment: Faulty message

This is not the first time I encounter this situation. If an email has .eml attachments, they do not appear as attachments in the MimeMessage representation.

STR:
Load the attached message in a local folder.
Find the msgHdr for the attached message.
Run MsgHdrToMimeMessage.
In the callback, dump(aMimeMsg.allAttachments.length)

Result:
0

Expected result:
1

Another similar situation is I have a message A that fowards another message B as .eml. Message B contains an image inside. The MimeMessage corresponding to message A shows Message B's image as the only attachment, instead of showing only message B as an attachment.

The message reader properly handles both cases.

Please note that I'm not talking about isRealAttachment as in the previous bug.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

The behaviour sounds consistent with the documentation.  MimeMessage's allAttachments getter doc is:

  /**
   * @return a list of all attachments contained in this message and all its
   *     sub-messages.  Only MimeMessageAttachment instances will be present in
   *     the list (no sub-messages).
   */

This is clearly biased towards the gloda indexer's needs.

I would just manually traverse the resulting hierarchy and extract what you want from it.  There is nothing privileged about the implementations of "allAttachments", they are just convenience helpers biased towards their originating consumer.

The prettyString helper method on the objects is a good way to reduce your headache for that since it will show you what the MimeMessage rep is.

Unless there's a bug I'm not seeing (in which case, please attach the prettyString output), please either change this to an enhancement request to have a more useful helper if you are likely to contribute such a patch or mark this invalid.

Comment 2

[Jonathan Protzenko [:protz]]

Ok, my bad, sorry for this awfully bad bug. I'm not closing since I'm very likely to provide a patch for this in the following weeks because I definitely need it.

I think what happened is I thought that MimeMessage was supposed to be a better, easier-to-use alternative to manipulating nsIMsgDbHdrs and stuff, and that "allAttachments" was *the* new standard way to manipulate attachments with a great JS api. Actually, attachments as in "gloda attachment" != attachments as in "message reader attachment".

So just for the record, if I am to provide a patch for this, the semantics are:
- if the mime part is a mime message attachment, it's an attachment (that was easy)
- if the mime part has content-type message/rfc822, return this part as an attachment, and don't dive into it searching for other attachments
- for all other mime parts, just recursively search into them for attachments

Is that correct?

Comment 3

[Jonathan Protzenko [:protz]]

Attachment: First try

Andrew, please tell me what you think. I've named the new property "allUserAttachments" because this is what we want to expose to user code. Constrast with allAttachments, that's mainly exposed for the needs of the indexer.

This is very basic, just hacked up jsmimeemitter in the right place. The cool thing is that the jsmimeemitter adds the right properties onto the MimeMessage so that it looks like a MimeAttachment. A little bit hackish though.

Comment 4

[Jonathan Protzenko [:protz]]

Ludo, you suggested adding a test. However, I have the feeling that the only tests for gloda are in mailnews/db/gloda/test/unit, and these don't test for the correctness of MimeMessage representation, so I'll wait for asuth to give me his opinion on that part.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 463322 [details] [diff] [review]
First try

Ludo is right, we should have a test.  test_mime_emitter.js is where related logic is tested, although since you are introducing a convenience representation rather than MIME tree equivalency, it should probably get its own test function and array of candidate messages.

Comment 6

[Jonathan Protzenko [:protz]]

Attachment: Updated patch

- Add the allUserAttachments property on MimeMessages
- Add a test for it
- Fix the rest of test_mime_emitter.js by adding the missing calls to yield false
- Add some error logging for the "I threw an exception in the callback for MsgHdrToMimeMessage but it silently failed" case.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 464176 [details] [diff] [review]
Updated patch

from http://reviews.visophyte.org/r/464176/

on file: mailnews/db/gloda/test/unit/test_mime_emitter.js line 398
> var partTachMessages2 = [

Please give this a better name; just attaching a 2 to things is generally an
indicator that something is not structured well...

For example, partTachMultipleMessages seems reasonable to me.


on file: mailnews/db/gloda/test/unit/test_mime_emitter.js line 408
> var messageInfos2 = [

rename this guy too...


on file: mailnews/db/gloda/test/unit/test_mime_emitter.js line 427
> var expectedAttachmentsInfo = [

This fellow can stay named the same, but for future test things, my personal
preference is to try and avoid having two parallel arrays... as additional
test cases get added it gets harder to visually/mentally match them up.  I
would instead (in the future) merge the two arrays.


on file: mailnews/db/gloda/test/unit/test_mime_emitter.js line 454
>     let msgHdr = synSet.getMsgHdr(0);

dead code?


Please post an updated patch with the name changes made and mark the bug checkin-needed.  Please also set the approval 3.1.x flag if you want this to go out in 3.1.x.  We probably want to do that since 3.2 will only get the changes if we do that...

Comment 8

[Jonathan Protzenko [:protz]]

Attachment: Take 3

(In reply to comment #7)
> Comment on attachment 464176 [details] [diff] [review]
> Updated patch
> 
> from http://reviews.visophyte.org/r/464176/
> 
> on file: mailnews/db/gloda/test/unit/test_mime_emitter.js line 398
> > var partTachMessages2 = [
> 
> Please give this a better name; just attaching a 2 to things is generally an
> indicator that something is not structured well...
> rename this guy too...
Done.
> 
> 
> on file: mailnews/db/gloda/test/unit/test_mime_emitter.js line 427
> > var expectedAttachmentsInfo = [
> 
> This fellow can stay named the same, but for future test things, my personal
> preference is to try and avoid having two parallel arrays... as additional
> test cases get added it gets harder to visually/mentally match them up.  I
> would instead (in the future) merge the two arrays.
Will keep that in mind.
> 
> 
> on file: mailnews/db/gloda/test/unit/test_mime_emitter.js line 454
> >     let msgHdr = synSet.getMsgHdr(0);
> 
> dead code?
No. Next non-empty line is:
    MsgHdrToMimeMessage(msgHdr, null, function(aMsgHdr, aMimeMsg) {
which I think uses msgHdr =)

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 464192 [details] [diff] [review]
Take 3

> No. Next non-empty line is:
>    MsgHdrToMimeMessage(msgHdr, null, function(aMsgHdr, aMimeMsg) {
> which I think uses msgHdr =)

Hah.  I misread the idiom and then forgot what web browser I was using to inspect the code; was assuming control-f would highlight all matches without further action on my part...

Since I r+'d the previous patch, you could have just set the r+ bit on this one too unless you lack privileges to do so, in which case let me know and I'll make sure you get hooked up.  (Then when you post the patch you would say 'carrying forward r=asuth'.  Ideally you would also include that in the patch message using 'qrefresh -e' so that whoever commits the patch can save some effort...)

Comment 10

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/8e9ab7796793

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #7)
> Please post an updated patch with the name changes made and mark the bug
> checkin-needed.  Please also set the approval 3.1.x flag if you want this to go
> out in 3.1.x.  We probably want to do that since 3.2 will only get the changes
> if we do that...

This looks to me like a change of API, i.e. an addition. In which case we wouldn't take that in 3.1.x. 3.2 will be getting their own flags soon (I'm working out what we need there).

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Why wouldn't we take it?  I understand why we don't take changes to XPCOM interfaces on point-releases, but this is JS code in an unsupported API whose changes would be useful to both protz's extension and the Thunderbird Air/hometab extension.

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #12)
> Why wouldn't we take it?  I understand why we don't take changes to XPCOM
> interfaces on point-releases, but this is JS code in an unsupported API whose
> changes would be useful to both protz's extension and the Thunderbird
> Air/hometab extension.

How are you going to publish the extension(s) and declare compatibility with 3.1 to 3.1.2? For all I can tell is that if we add this API, then some users will have broken functionality if they haven't updated Thunderbird. I can have a chat to some of the Firefox guys, but I suspect supported or not they don't take API changes unless they have to for security issues.

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

It seems like it's the extension's problem if it doesn't work.

AMO appears to in some cases support point release version numbers, but I assume it requires some human to do something.  For example, we have 3.0 and 3.0.1 versions allowed:
https://addons.mozilla.org/en-US/firefox/pages/appversions

I personally don't care either way, but since 3.2 is not yet defined and protz made the effort to upstream usable functionality, it seems nice to let him bear the fruits of those labours sooner rather than later.  Otherwise, it probably would have been a better strategy for him to just walk the attachment tree and not bother upstreaming since that's what his extension would need to do for 3.1 until 3.2 is released otherwise.

I'm fine with sticking to 3.1.x as only 'security & stability'; I recognize I'm not carrying any of the burdens of suggesting random code be taken onto a stable release branch.

Comment 15

[Jonathan Protzenko [:protz]]

It all depends on how early 3.2a1 is out. In the past I already did that for other extensions, i.e. marking them compatible from a certain point release onwards, and AMO is fine with it. And while I'm ok with requiring 3.1.3, having to choose between requiring 3.2a1 and having missing features is a much harder call, and I'm not sure I would make it.

As Andrew says, this is an unsupported API, which should allow us to change it in a point release. From a strategy point of view, I think the Thunderbird Air folks would me much more ok with requiring 3.1.3 than having to choose between broken functionality (i.e. not showing .eml attachments in their message reader) and requiring 3.2 which will be out sometime in the distant future...

Comment 16

[Jonathan Protzenko [:protz]]

Comment on attachment 464192 [details] [diff] [review]
Take 3

Adding approval-3.2a1 anyway since we want it there as well.

Comment 17

[Jonathan Protzenko [:protz]]

And for the record, every point release of Firefox, Thunderbird, SeaMonkey or Fennec is made available for the "minVersion" field a few days before its release on AMO, so I can perfectly start requiring 3.1.3 from the next version onwards.

Comment 18

[Mark Banner (:standard8)]

(In reply to comment #17)
> And for the record, every point release of Firefox, Thunderbird, SeaMonkey or
> Fennec is made available for the "minVersion" field a few days before its
> release on AMO, so I can perfectly start requiring 3.1.3 from the next version
> onwards.

Err, no it isn't. n.0 of a n.0.x range is the only minVersion made available.

minVersions for each alpha/beta release are made available, but not for each stable release - see https://addons.mozilla.org/en-US/thunderbird/pages/appversions for the current list of valid versions (3.0.1 was an unusual exception because we'd totally messed up on a binary compatibility issue, 3.0.4 was a request I didn't know about, but also totally surprised at requiring a specific version of sqlite, however generally these are unusual exceptions to the rules I believe).

Comment 19

[Jonathan Protzenko [:protz]]

You're right, I had in the past required beta3, beta4, and then 3.0.x, 3.0.1 because of a bug in the folder pane, and I just assumed every version was available on AMO.

How hard is it to ask AMO to support a given version number?

Comment 20

[Jonathan Protzenko [:protz]]

One more point, and then I think I'll run out of arguments :-) : extensions don't need to enforce any minimum Thunderbird version, as they can simply fallback to the regular implementation by doing:

let attachments = aMimeMsg.allUserAttachments || aMimeMsg.allAttachments;

which kind of solves the problem.

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 464192 [details] [diff] [review]
Take 3

I've had a few discussions about this and the conclusion is that we feel it falls outside of the range of security and stability releases - (note that it is felt that TbAir won't get much benefit from it either) - so sorry, but we're not going to accept it on the 3.1.x releases.