If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Thunderbird 31.0

Status

MailNews Core
Address Book
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Marco A.G.Pinto, Assigned: aceman)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 31.0
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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

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

Comment 2

6 years ago
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

6 years ago
(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

5 years ago
(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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Using brackets while searching in the address book → Adress book quick search for strings with round brackets fails: (searchword)

Updated

5 years ago
Summary: Adress book quick search for strings with round brackets fails: (searchword) → Address Book quick search for strings with round brackets fails: (searchword)

Comment 5

5 years ago
(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

5 years ago
(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.
(Assignee)

Updated

4 years ago
Assignee: nobody → acelists
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Version: unspecified → Trunk
(Assignee)

Comment 7

4 years ago
Created attachment 8409116 [details] [diff] [review]
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.
Attachment #8409116 - Flags: feedback?(standard8)
Attachment #8409116 - Flags: feedback?(neil)
Attachment #8409116 - Flags: feedback?(mconley)

Comment 8

4 years ago
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?
(Assignee)

Comment 9

4 years ago
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.
Flags: needinfo?(neil)
(Assignee)

Comment 10

4 years ago
Created attachment 8409647 [details] [diff] [review]
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.
Attachment #8409116 - Attachment is obsolete: true
Attachment #8409116 - Flags: feedback?(standard8)
Attachment #8409116 - Flags: feedback?(neil)
Attachment #8409116 - Flags: feedback?(mconley)
Attachment #8409647 - Flags: review?(standard8)
Attachment #8409647 - Flags: review?(neil)
Attachment #8409647 - Flags: feedback?(mconley)
Flags: needinfo?(neil)
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.
(Assignee)

Comment 12

4 years ago
(In reply to neil@parkwaycc.co.uk 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.
(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...
(Assignee)

Comment 14

4 years ago
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.
(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?
(Assignee)

Comment 16

4 years ago
(In reply to neil@parkwaycc.co.uk 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?
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.
(Assignee)

Comment 18

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

Updated

4 years ago
Component: Address Book → Address Book
Depends on: 558931
Product: Thunderbird → MailNews Core

Updated

4 years ago
Depends on: 1000775
(Assignee)

Updated

4 years ago
Blocks: 1000775
No longer depends on: 1000775
Flags: needinfo?(neil)
(Assignee)

Comment 19

4 years ago
Created attachment 8413292 [details] [diff] [review]
patch v3

Updated after landing of bug 558931. So this is now ready for review.
Attachment #8409647 - Attachment is obsolete: true
Attachment #8409647 - Flags: review?(standard8)
Attachment #8409647 - Flags: review?(neil)
Attachment #8409647 - Flags: feedback?(mconley)
Attachment #8413292 - Flags: review?(standard8)
Attachment #8413292 - Flags: review?(neil)
Attachment #8413292 - Flags: review?(mconley)
Flags: needinfo?(neil)
(Assignee)

Comment 20

4 years ago
Created attachment 8413301 [details] [diff] [review]
patch v4

Now with a test! :)
Attachment #8413292 - Attachment is obsolete: true
Attachment #8413292 - Flags: review?(standard8)
Attachment #8413292 - Flags: review?(neil)
Attachment #8413292 - Flags: review?(mconley)
Attachment #8413301 - Flags: review?(standard8)
Attachment #8413301 - Flags: review?(neil)
Attachment #8413301 - Flags: review?(mconley)
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
Attachment #8413301 - Flags: review?(neil) → review+

Comment 22

4 years ago
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)
(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.
(Assignee)

Comment 24

3 years ago
Created attachment 8413374 [details] [diff] [review]
patch v4.1

Thanks, fixed typos.
Attachment #8413301 - Attachment is obsolete: true
Attachment #8413301 - Flags: review?(standard8)
Attachment #8413301 - Flags: review?(mconley)
Attachment #8413374 - Flags: review?(standard8)
Attachment #8413374 - Flags: review?(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!
Attachment #8413374 - Flags: review?(mconley) → review+
(Assignee)

Comment 26

3 years ago
Of course, the added test passes.
For the other ones, Aryx could push it to try server.
Flags: needinfo?(archaeopteryx)
Pushed as https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=4243e3b3b26f
Flags: needinfo?(archaeopteryx)
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 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.
Attachment #8413374 - Flags: review?(standard8)
(Assignee)

Comment 30

3 years ago
OK, thanks. The try run has passed successfully with no new test failures.
Please land into TB31 if possible.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/699123240706
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0

Updated

3 years ago
Blocks: 585483
You need to log in before you can comment on or make changes to this bug.