586742 - Add an attachmentInfos noun to gloda messages, including, but not limited to, attachment url, name, size, and content-type

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Jonathan Protzenko [:protz]]

This is a highly wanted feature for: Thunderbird Air, the Conversation View, and possibly other extensions or experiments out there. Following discussion with Andrew, what will be added is an extra noun that does duplicate other information available (name and content-type), but that is not included in the faceted search results nor in the full text index (bottom line is: no one's really motivated to add a faceting on the attachment's sizes).

So something like glodaMsg.attachmentInfos = [{ name: ..., size: ..., url: ..., contentType: ..., ...}, ...]

The bug this depends on should be fixed shortly.

Comment 1

[Jonathan Protzenko [:protz]]

... and assigning to me.

Comment 2

[Jonathan Protzenko [:protz]]

So the plan is basically:
- add a new class (GlodaAttachment) inside datamodel.js and a corresponding NOUN_ATTACHMENT constant
- add the corresponding NOUN_ATTACHMENT inside gloda.js
- call defineNoun inside gloda.js
- call defineAttribute from fundattr.js
- fill the attribute's value from fundattr.js

Did I forget something?

Comment 3

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

- unit tests! :)

sounds reasonable.  you could also break out the noun definition out into its own noun_* file if you think the implementation is going to get complex.

Comment 4

[Jonathan Protzenko [:protz]]

Attachment: WIP (1)

Here's my first try at the patch. Please note that it applies on top of the patch from bug 561851 (I merely added extra tests -- sure, I could cancel the libmime patch and update it, but I think we both want to see that one checked in soon). Anyway, this patch adds super-duper tests, and yes, it does do as advertised: basically, it adds a attachmentInfos property (just like attachmentTypes and attachmentNames) that's not used for indexing but that simply exposes more information about that gloda message's attachments.

glodaMsg.attachmentsInfos[i] has the following properties: name, size, contentType and url so that you can stream the attachment into some iframe should you want to.

If there's anything else you want me to cram in, please speak now.

Andrew, should you find this patch to your taste, I'll remove the debug and ask for review.

Comment 5

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

Comment on attachment 467181 [details] [diff] [review]
WIP (1)

Looking good!

From http://reviews.visophyte.org/r/467181/:

on file: mailnews/db/gloda/modules/datamodel.js line 128
>       if (nounDef.toParamAndValue) {

strict mode can get unhappy about this.  it's better to do if
("toParamAndValue" in nounDef) unless you are going to add the attribute to
your implementation but set it to null or undefined or what not.


on file: mailnews/db/gloda/modules/datamodel.js line 846
>  * @class An attachment, with as much information as we can gather on it

you can lose @class; I originally was autogenerating documentation nightly
with a tool that required that syntax to produce useful results, but that is
no longer happening.


on file: mailnews/db/gloda/modules/datamodel.js line 858
>   // set by GlodaDatastore
>   _datastore: null,

I feel like this should not be needed since no managed persistence happens for
this class. (The bit in datastore.js that sets it could also go too.)


on file: mailnews/db/gloda/modules/datamodel.js line 860
>   get id() { return 42 },

Is this still needed?  I think we should drop this as a requirement since
we're also dropping the toParamAndValue case.

In the event we can't drop it, a comment explaining why and perhaps having the
getter throw if accessed might be reasonable.


on file: mailnews/db/gloda/modules/fundattr.js line 372
>       emptySetIsSignificant: true,

This should probably be false since we don't care about emitting anything
queriable for this attribute.  (Setting this to true makes a special sentinel
attribute be emitted when the array on the object is empty, which is not
desired.)


on file: mailnews/db/gloda/test/unit/base_index_messages.js line 452
>       for (k in expectedInfos[i])

where is k scoped?

Comment 6

[Jonathan Protzenko [:protz]]

Attachment: WIP (2)

Thanks for the quick review. I've taken the comments into account and it all seems to work even without an ID (which I guess was just used for the table storing the attributes pairings).

Comment 7

[Jonathan Protzenko [:protz]]

Oh and about the message/rfc822 thing, you were right, I wasn't building the attached message properly. I'd done the same mistake before on another patch, which I'm fixing as a side-effect at the very end of the patch.

Comment 8

[Jonathan Protzenko [:protz]]

Also, if you could just run the xpcshell test under your windows box, that'd be great. I've tried not to reproduce the same mistake and I'm allowing a small delta for the attachment size, but we're never sure (and thunderbird-try doesn't run xpcshell tests).

Comment 9

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

TEST-UNEXPECTED-FAIL | c:/rev_control/hg/comm-central/objdir-thunderbird-debug/mozilla/_tests/xpcshell/test_mailnewsglob
aldb/unit/test_mime_attachments_size.js | false == true - See following stack:
JS frame :: c:\rev_control\hg\comm-central\mozilla\testing\xpcshell\head.js :: do_throw :: line 317
JS frame :: c:\rev_control\hg\comm-central\mozilla\testing\xpcshell\head.js :: do_check_eq :: line 347
JS frame :: c:\rev_control\hg\comm-central\mozilla\testing\xpcshell\head.js :: do_check_true :: line 359
JS frame :: c:/rev_control/hg/comm-central/objdir-thunderbird-debug/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/t
est_mime_attachments_size.js :: check_attachments :: line 213
JS frame :: c:/rev_control/hg/comm-central/objdir-thunderbird-debug/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/t
est_mime_attachments_size.js :: anonymous :: line 232
JS frame :: resource:///modules/gloda/mimemsg.js :: anonymous :: line 134
TEST-INFO | (xpcshell/head.js) | exiting test
TEST-UNEXPECTED-FAIL | c:/rev_control/hg/comm-central/objdir-thunderbird-debug/mozilla/_tests/xpcshell/test_mailnewsglob
aldb/unit/test_mime_attachments_size.js | 2147500036 - See following stack:
JS frame :: c:\rev_control\hg\comm-central\mozilla\testing\xpcshell\head.js :: do_throw :: line 317
JS frame :: c:/rev_control/hg/comm-central/objdir-thunderbird-debug/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/t
est_mime_attachments_size.js :: anonymous :: line 234
JS frame :: resource:///modules/gloda/mimemsg.js :: anonymous :: line 134

Comment 10

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

(that was windows; linux xpcshell tests pass)

Comment 11

[Jonathan Protzenko [:protz]]

Attachment: WIP (3)

This patch passes all tests. Looks good to me :).

Comment 12

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

Comment on attachment 467575 [details] [diff] [review]
WIP (3)

As briefly discussed, we will need to bump the schema revision for this to take full effect (or people will need to blow away their databases).  We aren't going to bump the schema revision for this since nothing in core cares right now, but presumably something else will bump it sooner or later.

Comment 13

[Jonathan Protzenko [:protz]]

Yes, by the time 3.2 is released, I guess someone will probably bump the schema revision. Otherwise, I can run a query and get all messages that have attachments. From then on, can I request Gloda to re-index all of these?

Marking as checkin-needed, thx for the review.

Comment 14

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

(In reply to comment #13)
> From then on, can I request Gloda to re-index all of these?

yes.

Comment 15

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/78ea3006c4c5