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

RESOLVED FIXED in Thunderbird 31.0

Status

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: marcoagpinto, 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)

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)
(In reply to Marco A.G.Pinto from comment #0)
In which fields are those names stored into (within the addressbook)?
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,
(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
(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)
Summary: Adress book quick search for strings with round brackets fails: (searchword) → Address Book quick search for strings with round brackets fails: (searchword)
(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...
(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: nobody → acelists
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Posted patch WIP patch (obsolete) — Splinter Review
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 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?
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)
Posted patch patch v2 (obsolete) — Splinter Review
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.
(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...
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?
(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.
It also looks like _searchWithinEmails() is going away in bug 558931.
Can you please review it now?
Depends on: 558931
Product: Thunderbird → MailNews Core
Depends on: 1000775
Blocks: 1000775
No longer depends on: 1000775
Flags: needinfo?(neil)
Posted patch patch v3 (obsolete) — Splinter Review
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)
Posted patch patch v4 (obsolete) — Splinter Review
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+
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.
Posted patch patch v4.1Splinter Review
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+
Of course, the added test passes.
For the other ones, Aryx could push it to try server.
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)
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
Closed: 5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
Blocks: 585483
You need to log in before you can comment on or make changes to this bug.