749097 - Address Book quick search for strings with round brackets fails: (searchword)

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Marco A.G.Pinto [:marcoagpinto]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

Imagine I had in my address book:
Marco Lee (CIA)
John Rambo (FBI)
Michael Loreto (CIA)

and I wanted to list all "(CIA)" ones.

I would type in the search box: (CIA)


Actual results:

It doesn't work because the brackets, "(" and ")", aren't recognized.


Expected results:

It should have listed:
Marco Lee (CIA)
Michael Loreto (CIA)

Comment 1

[Hashem Masoud]

(In reply to Marco A.G.Pinto from comment #0)
In which fields are those names stored into (within the addressbook)?

Comment 2

[Marco A.G.Pinto [:marcoagpinto]]

Hello!

I have the names stored as "Display:" and the e-mail addresses in "Email:", and I have the checkbox on for "always display name over message header".

I have no real CIA nor FBI contacts and this was just an example (I have other orgazinations there).

I have reported this issue in 2010 too if I can well remember but not so detailed.

Kind regards,

Comment 3

[Hashem Masoud]

(In reply to Marco A.G.Pinto from comment #2)
Confirmed. You cannot search for special characters in the Addressbook Display field.

User Agent: Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20120420 Thunderbird/12.0
Application Build ID: 20120420153403

Comment 4

[Thomas D. (:thomas8)]

(In reply to Hashem Masoud from comment #3)
> (In reply to Marco A.G.Pinto from comment #2)
> Confirmed. You cannot search for special characters in the Addressbook
> Display field.

Hashem, thanks for confirming (and all of your support here in bmo!).

For me, on a small sample of special characters like _ / - / . / $ / §, the only special character which doesn't work is ( and ). Did you find any other special characters that won't work for AB search?

Comment 5

[Thomas D. (:thomas8)]

(In reply to Marco A.G.Pinto from comment #2)
> I have no real CIA nor FBI contacts and this was just an example (I have
> other orgazinations there).

...like (al- Qae da)? *lol* just joking...

Comment 6

[Hashem Masoud]

(In reply to Thomas D. from comment #4)

> For me, on a small sample of special characters like _ / - / . / $ / §, the
> only special character which doesn't work is ( and ). Did you find any other
> special characters that won't work for AB search?

Those characters are searchable, as well as ' and ". ( and ) on the other hand are not searchable.

Comment 7

[:aceman]

Attachment: WIP patch

This is a quick and dirty proof of concept patch that some escaping can be done to make the ( and ) characters work. I also think the chosen 254 and 255 characters are not the best ones so I ask for feedback what escape characters we want to use. I just intentionally don't want to espace '(' to '\(' as that would need to touch the query splitting/tokenization code.

This converts the compose autocomplete search, the search box in addressbook and the Edit->Find->Search Addresses dialogs to properly escape ( and ). Did I miss any other occurrence of AB search? I can convert Seamonkey dialogs too when the escaping is agreed upon.

Comment 8

[[e-mail]]

Comment on attachment 8409116 [details] [diff] [review]
WIP patch

We already call encodeURIComponent on the search value, although this doesn't encode parentheses. Maybe we should just encode parentheses using URL encoding?

Comment 9

[:aceman]

You mean not changing encodeURIComponent but additionally encode '(' -> %xx at the callers? (Instead of current '(' - > 254). I have not yet found out at what stage we decodeURIComponent in the AB parser.

Comment 10

[:aceman]

Attachment: patch v2

Thanks, good idea. The chars are then automatically URL decoded somewhere down the line so there doesn't need to be any code change to nsAbQueryStringToExpression.cpp.
You can now test it in Seamonkey too.

Comment 11

[[e-mail]]

Comment on attachment 8409647 [details] [diff] [review]
patch v2

>-    var childCards =
>-      this._abManager.getDirectory(directory.URI + searchQuery).childCards;
>+    let childCards;
>+    try {
>+      childCards = this._abManager.getDirectory(directory.URI + searchQuery).childCards;
>+    } catch (e) {
>+      Components.utils.reportError("Error running addressbook query '" + searchQuery + "': " + e);
>+      return;
>+    }
Why this change?

>+function encodeABTermValue(aString) {
>+  return encodeURIComponent(aString).replace("(", "%28").replace(")", "%29");
Need to globally replace for safety, so use /\(/g etc.

Comment 12

[:aceman]

(In reply to [e-mail] from comment #11)
> >+      childCards = this._abManager.getDirectory(directory.URI + searchQuery).childCards;
> >+    } catch (e) {
> >+      Components.utils.reportError("Error running addressbook query '" + searchQuery + "': " + e);
> >+      return;
> >+    }
> Why this change?
Without this patch autocompleting a string with '(' in message recipient caused an exception. So I thought to catch it in case we get it again due to other reasons.
But I can remove the handling if you wish.

> >+function encodeABTermValue(aString) {
> >+  return encodeURIComponent(aString).replace("(", "%28").replace(")", "%29");
> Need to globally replace for safety, so use /\(/g etc.
Thanks. That is a very strange catch that replace does not replace all occurrences by default.

Comment 13

[[e-mail]]

(In reply to aceman from comment #12)
> Without this patch autocompleting a string with '(' in message recipient
> caused an exception. So I thought to catch it in case we get it again due to
> other reasons.
Sorry, it's not clear what the benefit of catching it is supposed to be. As far as I can tell, what will happen is that you'll now get the exception twice per address book...

Comment 14

[:aceman]

I expect to get the query string added to the exception as a further clue in the error console.
Why would it produce the exception twice?

Also, catching it and returning no results cleanly allows further code to run (where maybe some query passes). But it is questionable if we need this side effect.

Comment 15

[[e-mail]]

(In reply to aceman from comment #14)
> I expect to get the query string added to the exception as a further clue in
> the error console.
> Why would it produce the exception twice?
The names and emails are searched separately near the end of startSearch, so if searching the cards throws an exception you would catch it and try to search the emails. As it happens, bug 558931 changes this, in which case it's not so much of a problem.

> Also, catching it and returning no results cleanly allows further code to
> run (where maybe some query passes). But it is questionable if we need this
> side effect.

Yes, I was wondering that too; why would the query pass on some address books but not others?

Comment 16

[:aceman]

(In reply to [e-mail] from comment #15)
> > Also, catching it and returning no results cleanly allows further code to
> > run (where maybe some query passes). But it is questionable if we need this
> > side effect.
> Yes, I was wondering that too; why would the query pass on some address
> books but not others?
The emails query is much simpler. So if the problem doesn't lie in the term value (that is duplicated in all terms) but somewhere else, the emails query could succeed.

So should I remove the try-catch?

Comment 17

[[e-mail]]

Oops, I overlooked this, it looked like part of the paragraph at first.

(In reply to aceman from comment #14)
> I expect to get the query string added to the exception as a further clue in
> the error console.

OK, so the additional logging is useful for debugging, fair enough.

Comment 18

[:aceman]

It also looks like _searchWithinEmails() is going away in bug 558931.
Can you please review it now?

Comment 19

[:aceman]

Attachment: patch v3

Updated after landing of bug 558931. So this is now ready for review.

Comment 20

[:aceman]

Attachment: patch v4

Now with a test! :)

Comment 21

[[e-mail]]

Comment on attachment 8413301 [details] [diff] [review]
patch v4

(Not reviewed the test.)

I notice someone doing something similar in http://stackoverflow.com/a/814232

Comment 22

[Thomas D. (:thomas8)]

attachment 8413301 [details] [diff] [review]

+ * The espression takes the for of (BOOL1(FIELD1,OP1,VALUE1)..(FIELDn,OPn,VALUEn)(BOOL2(FIELD1,OP1,VALUE1)...)...)

Nit: some spelling errors in this comment (expression, form)

Comment 23

[[e-mail]]

(In reply to comment #21)
> I notice someone doing something similar in http://stackoverflow.com/a/814232

Sigh, that should be http://stackoverflow.com/a/8143232 of course.

Comment 24

[:aceman]

Attachment: patch v4.1

Thanks, fixed typos.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8413374 [details] [diff] [review]
patch v4.1

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

This works for me! I'll assume you've ensured that the tests pass.

::: mailnews/addrbook/src/nsAbQueryStringToExpression.cpp
@@ +27,5 @@
> + *         For possible values see CreateBooleanConditionString().
> + * VALUEn  The value to be matched in the FIELDn via the OPn operator.
> + *         The value must be URL encoded by the caller, if it contains any special
> + *         characters including '(' and ')'.
> + */

Thanks for the documentation!

Comment 26

[:aceman]

Of course, the added test passes.
For the other ones, Aryx could push it to try server.

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed as https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=4243e3b3b26f

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Hadn't pulled the latest c-c changes, so I canceled the previous job, pulled and pushed again: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=64d8da08fff2

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 8413374 [details] [diff] [review]
patch v4.1

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

As Mike and Neil have already reviewed this, I'm quite happy not to and defer to them.

Comment 30

[:aceman]

OK, thanks. The try run has passed successfully with no new test failures.
Please land into TB31 if possible.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/699123240706