798635 - Allow for case where Gmail label has space character when parsing Gmail labels

Status: NEW

(MailNews Core :: Networking: IMAP, defect)

Description

[David Lechner (:dlech)]

Follow up to item 4 in https://bugzilla.mozilla.org/show_bug.cgi?id=721316#c60

Need to patch nsImapMailFolder::GetOfflineMsgFolder in nsImapMailFolder.cpp to allow for the case where Gmail labels have a space. 

Current implementation
> +      ParseString(labels, ' ', labelNames);
will divide labels with spaces.

for example if the server returns the paren group
> ("\\Inbox" "My Folder" MyOtherFolder)
we need to ignore the space in "My Folder"

https://mxr.mozilla.org/comm-central/ident?i=ParseString&tree=comm-central&filter=nsImapMailFolder

Comment 1

[David Lechner (:dlech)]

Does Thunderbird already have a parse function that does "split strings on spaces that are not enclosed in quotation marks"?

Comment 2

[Atul Jangra [:atuljangra]]

(In reply to David Lechner (:dlech) from comment #1)
> Does Thunderbird already have a parse function that does "split strings on
> spaces that are not enclosed in quotation marks"?

I am not sure if TB already have such a parse function. But we can create one anytime. :-)
I guess we need to use a split regex like  " +(?=(?:[^\"]*\"[^\"]*\")*[^\"]*$)"  .

Comment 3

[David Lechner (:dlech)]

Check out the function I wrote here: https://bugzilla.mozilla.org/attachment.cgi?id=669557&action=diff#a/mailnews/test/fakeserver/imapd.js_sec7

It should be more standards compliant and as such could be used for other things as well.

It takes into account the possibility of imap literals (this is where the '{' comes in), and escaped quote characters ('\"'). 

Now that I am looking at it, it should also replace '\\' with '\' if it occurs between quotes.

Comment 4

[Atul Jangra [:atuljangra]]

(In reply to David Lechner (:dlech) from comment #3)
> Now that I am looking at it, it should also replace '\\' with '\' if it
> occurs between quotes.
Yes, this would be ideal solution, but if we do this then we need to change few if conditions here: https://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#9640
right?

Comment 5

[Atul Jangra [:atuljangra]]

Attachment: introductory patch

Dlech, please have a look at the patch :-)

I wasn't sure where to keep the function, thus I've included it in nsImapMailFolder only. Function is accessible to JavaScript which can be helpful and thus can be used elsewhere.
I've checked it with X-GM-LABELS. Working file with spaces between quotation marks, and also replacing '\\' with '\' if it occurs between quotes.
I've not checked it with IMAP literals( http://tools.ietf.org/html/rfc2088 ).

Merry Christmas :-)

Comment 6

[David Lechner (:dlech)]

(In reply to Atul Jangra [:atuljangra] from comment #5)
> Created attachment 695616 [details] [diff] [review]
> introductory patch
> 
> Dlech, please have a look at the patch :-)
> 
> I wasn't sure where to keep the function, thus I've included it in
> nsImapMailFolder only. Function is accessible to JavaScript which can be
> helpful and thus can be used elsewhere.
> I've checked it with X-GM-LABELS. Working file with spaces between quotation
> marks, and also replacing '\\' with '\' if it occurs between quotes.
> I've not checked it with IMAP literals( http://tools.ietf.org/html/rfc2088 ).
> 
> Merry Christmas :-)

Yea for Christmas!
------------------

The more I look at this, the less I like my original idea. The most appropriate place to put the function you created would be in nsImapServerResponseParser.cpp, but 1) it already does that and 2) it is not a public interface. But, forget all of that for a minute. Here is my new idea...

On second thought, I will create a patch with my new idea.

Comment 7

[Atul Jangra [:atuljangra]]

I will be waiting for the new idea :-)

Comment 8

[David Lechner (:dlech)]

Attachment: alt patch v1

This patch uses the existing parsing functions in nsImapServerResponseParser to further parse the paren group that contains the gmail labels. effectively, it strips the outer parentheses, strips quotes off of any quoted items in the list and also escapes \" and \\ from quoted items.

I think we were missing some matching of labels to folder names before because we were not stripping quotes and unescaping. The special use folder labels also now match the flags returned by XLIST, which has the potential to be useful in the future as well.

I made test_gmailAttributes.js more easily extensible for any future needs and fixed up test_gmailOfflineMsgStore.js too.

Comment 9

[Atul Jangra [:atuljangra]]

Comment on attachment 696642 [details] [diff] [review]
alt patch v1

Review of attachment 696642 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I took very long time on this one, was busy with my college :-/
This patch looks good to me, so feedback+ from my side.

Comment 10

[Atul Jangra [:atuljangra]]

Dlech, Should we land this?
Ccing Wayne.

Comment 11

[David Lechner (:dlech)]

Attachment: proposed patch

removed trailing white space

Comment 12

[David Lechner (:dlech)]

(In reply to Atul Jangra [:atuljangra] from comment #10)
> Dlech, Should we land this?

Sure. We'll see if :bienvenu has time to review.

Comment 13

[David :Bienvenu]

sorry, guys, missed this in my review queue. I'll try to look at this weekend, though I don't currently have a build environment set up on my new desktop, and that might take a while to set up.

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 713416 [details] [diff] [review]
proposed patch

dlech, Bienvenu is no longer active in reviews or buzilla.  You'll want a different reviewer

Comment 15

[WADA:World Anti-bad-Duping Agency]

Attachment: A JavaScript sample to Split Space Separated Values to JavaScript associative Array.(==Object of JavaScript)

-GM-LABELS value is a "Space separated values", so, when space and some special chars is used, string is quoted by double-quote, and if double-quot is used in string, the doule-quote is escaped by back-slash, so, if back-slash is used in string, the back-slash is escaped by back-slash.
This is because X-GM-LABELS values are sent as imap response.
> X-GM-LABELS   = "INBOX/AA \" \\ BB" "INBOX/AA\"\\BB" "[A]" "[Imap]/Drafts" "[Imap]/Sent Mail" "\\Inbox" "\\Starred"
> Split to Array => [ INBOX/AA BB , INBOX/AA " \ BB , INBOX/AA"\BB , [A] , [Imap]/Drafts , [Imap]/Sent Mail , \Inbox , \Starred ]
So, following simple algorithm can be applied.
    if '"' is started, until closing '"' is detected, if \ is detected, eat a \ and use following one char, as value, and use other char as value
    if '"' is not started yet,  until space or next start of '"' is detected, use a char as value.
Attached is script code who split above actual "X-GM-LABELS string which is returned from Gmail then stored in StringPropertes of msgDBHdr" to array.

Because Delimiter-separated_values(CSV is an example) is widely used, I think many routines are already provided someone.
> http://en.wikipedia.org/wiki/Delimiter-separated_values
Why X-GM-LABELS specific code is needed?

Comment 16

[Atul Jangra [:atuljangra]]

X-GM-LABELS specific code is not needed. We just needed a code that would parse the labels easily. I think the above algorithm should work correctly.

Comment 17

[WADA:World Anti-bad-Duping Agency]

Following is actual response data passed from Gmail IMP for uid fetch nn ... X-GM-LABELS ...
> X-GM-LABELS ("INBOX/AA ( ) BB" "INBOX/AA \" \\ BB" "INBOX/AA\"\\BB" "[Imap]/Drafts" "\\Starred" INBOX/&ZeVnLIqe- INBOX/Inbox2)
Format is X-GM-LABELS (...), and "..." part enclosed by "(" and ")" is currently saved as-is in StringProperty of msgDBHdr.
Although nextToken() correctly processes quoted text and unit is a "quoted text", because of "list of quoted text", "splitting to each GM label upon each access" is slightly expensive.
In splitting, there is no need to process GMLABEL text itself such as "\\Starred"(string in imap response) or \Starred(quoting " is removed, \  and " is un-escaped). And, once split to array, there is no need to pay attention to  "space in Gmail Label".
X-GM-LABELS data is better saved in Array format with un-escaping \ and ", with removing ", for future access.
   Array[0] : INBOX/AA ( ) BB
   Array[1] : INBOX/AA " \ BB
   Array[m] : \Starred
   Array[n] : INBOX/&ZeVnLIqe-
Needless to say, data for \AllMail, \Trash, \Spam, should be added by Thunderbird by himself, because Gmail IMAP never returns these 3 GMLABELs.
This is perhaps hard if StringProperty is used. New msgDBHdr.GMLABEL_Array like property(nsIMutableArray) is perhaps needed.
If possible, msgDBHdr.GM_MSGID, msgDBHdr.GM_THRID is better created in addition to nsIMsgIncomingServer.isGmailServer.
If JavaScript Object, Object = { \Starred : actual Mbox URI for \Starred , \Important : actual Mbox URI for \Important  , ... } like one may be needed for special folders in Gmail and imap. JavaScript Object is always "associative array, it's always usable as primitive small Database.
Currently, attribute of Mbox(returned to XLIST) is saved in msgFolder.boxFlags, so conversion from folderURI -> GMLABEL is easy.
However, conversion from GMLABEL to actual msgFolder, folderURL, is not so easy. Scan of  all msgFolder of account is needed. And, Mbox name is changed according to Display Language setting change.
So, this kind of "mapping table of Gmail Label -> actulal Mbox name" is needed for efficient Gmail Label handling.
gFolderTree is best place to hold such data?

Comment 18

[WADA:World Anti-bad-Duping Agency]

(In reply to David Lechner (:dlech) from comment #11)
> proposed patch

Comment on following of code, not on change by you.
> if (!*aMsgFolder)
>    // Checking the existence of message in other folders in case of GMail Server

I think this is for next.
   When a new message is detected, if Gmail Label is alredy added to the mail, the mail is perhaps fetched at mbox relevant to the  GMLABEL.
   If the "mbox relevant to the  GMLABEL"(call \??? folder) is offline-use=on folder, the mail is perhaps auto-sync'ed at the \??? folder.
   If mail of the MSGID is already sync'ed at the \??? folder, skip auto-sync at this folder, and use mail data in the \??? folder.
If so, current logic is not apropriate, because \??? folder is not always "folder which has folderFlag such as Drafts, SentMail, which is obtained by GetFolderWithFlags,  and multiple folders can have folderFlag of Archive, Drafts, SentMail, Junk, and Templates(folder chosen by Copies&Folders and junk Settings. Other account's identity can use this Gmail imap account's folder as Drafts, Sent, ...).

If folder which is obtained by Get...WithFlags is used, GetFoldersWithFlags/getFoldersWithFlags should be used if other than \Inbox, \Trash("only one folder has this atribute" is guranteed is for Inbox and Trash only. If \Starred and \Important, Get...WithFlags ican do nothing).
And, if other folder is searched, it should be order in \AllMail folder, \Inbox folder, \Sent folder, \Drafts folder, \Spam folder, other user defined Gmail Label folder.

I think such logic is useless if \AllMail folder is Offline-Use=On. Following is better.
   When new mail is detected at a folder, if \AllMail folder is offline-use=on folder, check \AllMail folder for the MSGID.
   If not detected at \AllMail folder, issue "SELECT \AllMail folder, uid fetch NextUID:* Flags X-GM-MSGID X-GM-THRID X-GM-LABELS",
   and if the MSGID is not auto-sync'ed yet, "uid fetch UID for the MSGID BODY.PEEK[]". No need of "uid fetch xx BODY.PEEK[HEADERS]".
Current logic is effective only when \AllMail folder is offline-use=off folder.

Comment 19

[WADA:World Anti-bad-Duping Agency]

FYI.
If "top level \Important folder in Thunderbird" is created by user, Gmail looks to use "User defined Gmail Label named \Important". So, if a mail is copied to both [Gmail]/Important and top level \Important, funny phenomenon is observed.
                                                                    \AllMail folder          \Important folder                 Folder for user defined Gmail Label named \Important
                                                                    [Gmail]/All Mail        [Gmail]/Important                top level \Important
    MSGID                                                       X-GM-LABELS          X-GM-LABELS                        X-GM-LABELS
     n1 : held in [Gmail]/Important  only          \Important              not returned                          N/A 
     n2 : held in top level \Important  only       \Important              N/A                                        not returned
     n3 : held in both                                         \Important              \Important (for top level)     \Important (for \Important folder)

So, if GMLABEL of \Important is detected, both \Important folder([Gmail]/Important ) and top level \Important folder should be checked.
In this case, "a MSGID is is held in which folder" can not be known from "\Important GMLABEL returned at \\AllMail folder([Gmail]/All Mail)". Folder access via Gmail IMAP is needed at both \Important folder([Gmail]/Important ) and top level \Important folder.

I  don't know whether this phenomenon occurs on \Starred or not.
I think Gmail should reserve top level \Important and Gmail should map it to [Imap]/\Important as done on top level Important.
I don't know whether Gmail reserves(and rejects if creation is request) top level \Inbox, \Trash, \Spam, \AllMail or not.

By the way, if quoted-text in X-GM-LABELS is saved as-is(quoting double-quot is kept, escaping \ is not removed), when JavaScript is used, "de-composing of quoted-text" is done by JavaScript interpreter.
    When var GMLABEL contains string of "AB \" \\ CD" ,
    var CMD = "var X = " + GMLABEL; eval(CMD); => X contains AB " \ CD
If "de-composing of quoted-text" is done by Response Parser, I think "saving in nsIMutableArray" is better than "inserting 0xFF in StringProperty value".

Comment 20

[Wayne Mery (:wsmwk)]

Atul, are you able to finish up the patch?

Comment 21

[Atul Jangra [:atuljangra]]

Hey!

I won't be able to look at this patch.

Comment 22

[Wayne Mery (:wsmwk)]

Is this worth doing?

Comment 23

[gene smith]

Not sure I understand what this is about. It sounds like they are wanting to allow gmail labels (folders to TB) to have spaces. It looks like they are now allowed. As a test, I created a gmail folder/label named a b and it seems to work OK.
Maybe this was fixed as a side-effect of allowing UTF8 folder name encoding that I put in a few years ago?