1497822 - nsAbQueryStringToExpression::CreateBooleanConditionString does not support Exists and DoesNotExist provided by nsIAbBooleanExpression

Status: RESOLVED FIXED

(Thunderbird :: Address Book, enhancement)

Description

[John Bieling (:TbSync)]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.140 Safari/537.36 Edge/17.17134

Steps to reproduce:

I want to search for contacts/Cards not having a Attribute, like cards without the field "categories". Searching for an empty field using this search query does not work:

let searchstring = abURI + "?(or(categories,=,))";

Probably this does not work, because the field is not empty, but does not exists at all. In
 
https://searchfox.org/comm-central/source/mailnews/addrbook/public/nsIAbBooleanExpression.idl

there is the condition type "DoesNotExist" which is however not avail in 


https://dxr.mozilla.org/comm-central/source/comm/mailnews/addrbook/src/nsAbQueryStringToExpression.cpp#258

I think I need the condition DoesNotExist to solve my issue, but it is not avail.


Actual results:

Search does not return anything.


Expected results:

Search should return all cards without any value in the querried field and without having that field at all.

Comment 1

[Jorg K (CEST = GMT+2)]

Can you provide a patch?

Comment 2

[John Bieling (:TbSync)]

I can try, but it would be my first ever patch for Thunderbird. Currently I fail to build TB60 from sources, because of some issue with rust...

Comment 3

[Jorg K (CEST = GMT+2)]

Developers only build trunk from sources on our machines. For TB60 you need "old" versions of everything, so we just push patches to the try server for building.

Comment 4

[John Bieling (:TbSync)]

Attachment: bug1497822.patch

Comment 5

[John Bieling (:TbSync)]

It turns out to be a very small patch. The actual logic is already implemented here:
https://searchfox.org/comm-central/source/mailnews/addrbook/src/nsAbDirectoryQuery.cpp#464

but until now it was not possible to actually use DoesNotExist.

The attached patch is adding the "!ex" command.

How to proceed? Can I just add "review? Jorg" to this bug?

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 9056419 [details] [diff] [review]
bug1497822.patch

I'm not a great connoisseur of that address book stuff. So maybe on of the other guys can take a more qualified look. That said, can you add a test for this stuff?

Comment 7

[John Bieling (:TbSync)]

Is there a tutorial on how to create tests? This is my second patch I have send in and my first actual bugfix, this is all new to me.

I tested this patch by using the new !ex command from an AddOn.

Comment 8

[Jorg K (CEST = GMT+2)]

Usually you can just extend an existing test. I had a look, but only found test_searchAddressInAb.js which is doing something else. The code you're touching here is parsing some stuff from some AB URI. I guess we'll just accept it "as is". Let's see what the reviewers say.

Comment 9

[Geoff Lankow (:darktrojan)]

Comment on attachment 9056419 [details] [diff] [review]
bug1497822.patch

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

I'm happy with this, but while you're here, please add Exists as well as DoesNotExist. I can't find any tests for this stuff (surprise, surprise) so as long as it works, we'll have it…

Comment 10

[John Bieling (:TbSync)]

Attachment: bug1497822.patch

Updated the patch to also include nsIAbBooleanConditionTypes::Exists.

If it is extra work, I do not need this backported to TB60. Having it in TB68 will be enough for me. Thanks for accepting this.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 9056464 [details] [diff] [review]
bug1497822.patch

I'll land it with r=darktrojan :-)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a4a9ee36c20d
Support Exists and DoesNotExist nsAbQueryStringToExpression::CreateBooleanConditionString(). r=darktrojan

Comment 13

[Jorg K (CEST = GMT+2)]

Can you please provide a complete HG header (with author's e-mail address) and a decent commit message next time.

Anyway, first patch, right? So thank you for your contribution.

This can easily be backported, just let me know.

Comment 14

[Geoff Lankow (:darktrojan)]

I was hoping for the nsAbDirectoryQuery.cpp part too. Never mind.

Comment 15

[Geoff Lankow (:darktrojan)]

Oh, scratch that, it's done further down.

Comment 16

[:aceman]

I would welcome a test here as it seems we have none for the various query keywords.
I can look into that.

As noticed in comment 0, checking empty fields with '=' or actually any other operator does not work.
It is due to the code at https://searchfox.org/comm-central/source/mailnews/addrbook/src/nsAbDirectoryQuery.cpp#462, where an empty value in AbCard (not query) returns a match only with the operator DoesNotExist (which wasn't accessible until now). Otherwise the result is false (no match) and no further operators are tested.

It could be argued (field,=,string) [Is] or (field,!=,string) [Isnt] could return a useful result too. E.g. if a card doesn't have 'field' defined, why would it NOT match (field,!=,string) ? Right now you have to always use (or(field,!=,string),(field,!ex,)) ???

Also, if we just created the 'ex' and '!ex' query keywords right now, I wonder where all the keywords are documented and we should add the new ones to the list.

Comment 17

[Geoff Lankow (:darktrojan)]

The documentation points to this part of the source code.

Comment 18

[John Bieling (:TbSync)]

As noticed in comment 0, checking empty fields with '=' or actually any other operator does not work.
It is due to the code at https://searchfox.org/comm-central/source/mailnews/addrbook/src/nsAbDirectoryQuery.cpp#462, where an empty value in AbCard (not query) returns a match only with the operator DoesNotExist (which wasn't accessible until now). Otherwise the result is false (no match) and no further operators are tested.

That is correct.

It could be argued (field,=,string) [Is] or (field,!=,string) [Isnt] could return a useful result too. E.g. if a card doesn't have 'field' defined, why would it NOT match (field,!=,string) ? Right now you have to always use (or(field,!=,string),(field,!ex,)) ???

Hm, that looks also correct. I will test that. But under the assumption, that "!=" only checks existing strings, having to test for (or(field,!=,string),(field,!ex,)) is straight forward.

It must be documented of course. When I started to work with this search syntax, I had to look it up in the source. Can I contribute to the doc?

Anyhow, I did not want to modify the standard behaviour of "=" and "!=" because it might break stuff. Since there where special implementations for Exists and DoesNotExists, I just wanted to makle those accessible.

Comment 19

[John Bieling (:TbSync)]

Right now you have to always use (or(field,!=,string),(field,!ex,))

Since the code returns early at line #462 if the field is empty, this is true. I also run a few tests to verify that. For me, this is not an issue, if it gets documented. Ping me, if I can contribute to the docs.