898337 - Contacts API does not allow to search by name field

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Contacts, defect)

Description

[Jose Manuel Cantera]

The Contacts API find function is not working when trying to find by name.

an error is raised with error code 'unknown error' 

Test case:

var reqName = navigator.mozContacts.find({
        filterValue: aContact.name[0].trim(),
        filterBy: ['name'],
        filterOp: 'equals'
      }

We need this function working in order to match reliably and consistently SIM duplicate contacts.

Comment 1

[Julien Wajsberg [:julienw]]

It's strange because it works well in the Sms app.

Comment 2

[Gregor Wagner [:gwagner]]

Yes we currently don't support it. Should be easy to add.

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

koi+ as it links to koi-comms bug

Comment 4

[Julien Wajsberg [:julienw]]

Thanks a lot Joe, I couldn't find this bug again !

Francisco, Jose, my understanding was that you didn't need this after all, bug 904623 would cover your needs.

Therefore should we close this bug ?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> Francisco, Jose, my understanding was that you didn't need this after all,
> bug 904623 would cover your needs.
> 
> Therefore should we close this bug ?

The documentation suggests this should work:

  https://developer.mozilla.org/en-US/docs/WebAPI/Contacts#Finding_a_contact

Also, while our apps may not use it, it does seem like something a 3rd party app might try to reasonably use.

Isn't think something we should fix or at least leave the bug open?

Comment 6

[Julien Wajsberg [:julienw]]

Problem is that, if I understood correctly, the "name" is not really the name of the contact, rather it's more an identifier, as it's used. But maybe that property has been abused and we should index it like the other, I'm not against it.

Comment 7

[Francisco Jordano [:arcturus] [:francisco]]

Hi Julien,

this is a different problem than bug 904623, in that bug we need to get the contacts from a list of ids.

Unfortunately name is a field used, we do have givenName and familyName, but the name is used as well, for example in the title of the contact when we go for the detailed view.

This field is used in several cases when we don't have given and family names, for example, importing from SIM.

Despite of that this change is for doing the matching and dedup functionality work better.

Thanks!

Comment 8

[Julien Wajsberg [:julienw]]

thanks Francisco, it seems I misunderstood :)

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: wip patch v1

WIP patch, not yet tested

asking feedback only

especially I do some not-so-unrelated changes here (ex: the debug function), and I don't know if I should rather do this in another bug and patch or if it's ok to do small not-so-unrelated changes in patches too.

Also, I don't see where you ensure the various properties are arrays. Can you tell me where it's done ? Or maybe it isn't... but that could be dangerous, right ?

Comment 10

[Julien Wajsberg [:julienw]]

I'd like to have WebIDL before landing this, because this will ensure we have arrays as input.

Comment 11

[Gregor Wagner [:gwagner]]

Comment on attachment 798581 [details] [diff] [review]
wip patch v1

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

We need more tests but it looks good to me!

::: dom/contacts/fallback/ContactDB.jsm
@@ +417,4 @@
>                           "givenNameLowerCase", "telLowerCase", "category", "email",
>                           "emailLowerCase"];
>          for (var i = 0; i < names.length; i++) {
> +          if (whiteList.indexOf(names[i]) < 0) {

no need to change this.

@@ +580,5 @@
> +              if (name && typeof name === 'string') {
> +                value.search.name.push(name.toLowerCase());
> +              }
> +            });
> +            cursor.update(value);

We could be smarter to only update the cursor if we actually push a name.

@@ +717,5 @@
>                  contact.search[field].push(value.toLowerCase());
>                }
>              } else {
>                let val = aContact.properties[field][i];
> +              if (val && typeof val == "string") {

We already check this in the beginning of the loop.

Comment 12

[Julien Wajsberg [:julienw]]

Attachment: patch v1

Can't schedule a try because it's closes because of bug 914821, will try again tomorrow I guess.

Comment 13

[Reuben Morais [:reuben]]

Comment on attachment 802591 [details] [diff] [review]
patch v1

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +207,5 @@
>  
>          // Properties indexes
>          objectStore.createIndex("familyName", "properties.familyName", { multiEntry: true });
>          objectStore.createIndex("givenName",  "properties.givenName",  { multiEntry: true });
> +        objectStore.createIndex("name",       "properties.name",       { multiEntry: true });

I assume this is just leftover from an older version of the patch? You should only add the indexes on your new upgrade function.

@@ +212,4 @@
>  
>          objectStore.createIndex("familyNameLowerCase", "search.familyName", { multiEntry: true });
>          objectStore.createIndex("givenNameLowerCase",  "search.givenName",  { multiEntry: true });
> +        objectStore.createIndex("nameLowerCase",       "search.name",       { multiEntry: true });

Ditto.

@@ +404,5 @@
>          if (!objectStore) {
>            objectStore = aTransaction.objectStore(STORE_NAME);
>          }
>          let names = objectStore.indexNames;
> +        let whiteList = ["tel", "familyName", "givenName",  "familyNameLowerCase",

Nice, I've been meaning to fix this forever and never get around to it :)

@@ +568,5 @@
> +          if (cursor) {
> +            let value = cursor.value;
> +            value.search.name = [];
> +            value.properties.name.forEach(function addNameIndex(name) {
> +              if (name && typeof name === 'string') {

|name| should only contain valid strings, did you run into |null| here when testing?

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 802591 [details] [diff] [review]
patch v1

will handle comments before asking for a new review

Comment 15

[Julien Wajsberg [:julienw]]

(In reply to Gregor Wagner [:gwagner] from comment #11)
> Comment on attachment 798581 [details] [diff] [review]
> wip patch v1
> 
> Review of attachment 798581 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We need more tests but it looks good to me!
> 
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +417,4 @@
> >                           "givenNameLowerCase", "telLowerCase", "category", "email",
> >                           "emailLowerCase"];
> >          for (var i = 0; i < names.length; i++) {
> > +          if (whiteList.indexOf(names[i]) < 0) {
> 
> no need to change this.

Discussed with Reuben over this, and we think the current code makes it difficult to read. I can move this to another patch if you want though.

> 
> @@ +580,5 @@
> > +              if (name && typeof name === 'string') {
> > +                value.search.name.push(name.toLowerCase());
> > +              }
> > +            });
> > +            cursor.update(value);
> 
> We could be smarter to only update the cursor if we actually push a name.

I'm still adding an empty array for value.search if there is nothing in value.properties.name. Should I keep it as null in that case ? I thought it was better to do this because we're doing exactly this in makeImport.

> 
> @@ +717,5 @@
> >                  contact.search[field].push(value.toLowerCase());
> >                }
> >              } else {
> >                let val = aContact.properties[field][i];
> > +              if (val && typeof val == "string") {
> 
> We already check this in the beginning of the loop.

removed.

(In reply to Reuben Morais [:reuben] from comment #13)
> Comment on attachment 802591 [details] [diff] [review]
> patch v1
> 
> Review of attachment 802591 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +207,5 @@
> >  
> >          // Properties indexes
> >          objectStore.createIndex("familyName", "properties.familyName", { multiEntry: true });
> >          objectStore.createIndex("givenName",  "properties.givenName",  { multiEntry: true });
> > +        objectStore.createIndex("name",       "properties.name",       { multiEntry: true });
> 
> I assume this is just leftover from an older version of the patch? You
> should only add the indexes on your new upgrade function.

Right, removed
 
> @@ +212,4 @@
> >  
> >          objectStore.createIndex("familyNameLowerCase", "search.familyName", { multiEntry: true });
> >          objectStore.createIndex("givenNameLowerCase",  "search.givenName",  { multiEntry: true });
> > +        objectStore.createIndex("nameLowerCase",       "search.name",       { multiEntry: true });
> 
> Ditto.

Removed.

> 
> @@ +404,5 @@
> >          if (!objectStore) {
> >            objectStore = aTransaction.objectStore(STORE_NAME);
> >          }
> >          let names = objectStore.indexNames;
> > +        let whiteList = ["tel", "familyName", "givenName",  "familyNameLowerCase",
> 
> Nice, I've been meaning to fix this forever and never get around to it :)

\o/

> @@ +568,5 @@
> > +          if (cursor) {
> > +            let value = cursor.value;
> > +            value.search.name = [];
> > +            value.properties.name.forEach(function addNameIndex(name) {
> > +              if (name && typeof name === 'string') {
> 
> |name| should only contain valid strings, did you run into |null| here when
> testing?

I was thinking: we're in the upgrade path, and we absolutly don't want that it breaks. So better be too safe here. Should I still remove it ?

Comment 16

[Julien Wajsberg [:julienw]]

Attachment: patch v2

try is https://tbpl.mozilla.org/?tree=Try&rev=59ef100a6b32

The new patch applies after WebIDL patches from bug 850430.

I looked at the android code and it seems to me that the android code already allows searching on "name". I don't know if the mochitests run on the android code.

Waiting for the try result before asking another review.

Comment 17

[Julien Wajsberg [:julienw]]

So, this try does not look so good, because some of the contact test files are timing out. But I really don't know what happens here, so I'd love to have some help here :)

Comment 18

[Reuben Morais [:reuben]]

I fixed the Android failures, but bug 915697 is preventing me from looking into the emulator failures.

Comment 19

[Gregor Wagner [:gwagner]]

(In reply to Julien Wajsberg [:julienw] from comment #17)
> So, this try does not look so good, because some of the contact test files
> are timing out. But I really don't know what happens here, so I'd love to
> have some help here :)

I noticed that the tests run pretty slow in debug mode and might trigger the max test run time. We should try to split them up into more files.

Comment 20

[Julien Wajsberg [:julienw]]

Reuben> do you mean you fixed the Android failures right now, or that you fixed them in another bug ?

Gregoe> thanks, I'll split them :)

Comment 21

[Preeti Raghunath(:Preeti)]

Julien,

What are the next steps here?

Comment 22

[Julien Wajsberg [:julienw]]

Next step is me splitting the mochitests so that they don't time out in TBPL.

However I don't think this bug should be a blocker.

Francisco, do you think the same ?

Comment 23

[Gregor Wagner [:gwagner]]

(In reply to Julien Wajsberg [:julienw] from comment #22)
> Next step is me splitting the mochitests so that they don't time out in TBPL.
> 
> However I don't think this bug should be a blocker.
> 
> Francisco, do you think the same ?

Yep not a blocker and not a regression. We shipped earlier versions without it.

Comment 24

[Noemí Freire (:noemi)]

Hi,

Just adding that this issue is blocking a blocker for v1.2 (bug 917674, notice that handle duplicate contacts is a new feature for v1.2) so this issue should remain as a blocker.

Comment 25

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Gregor Wagner [:gwagner] from comment #23)
> 
> Yep not a blocker and not a regression. We shipped earlier versions without
> it.

IMHO, this is blocking a blocker. We ship before without this feature, but now we have version 1.2 (that is not shipped), that needs this feature for the contacts dedup committed feature (see bug 917674 ).

I understand, as we were talking in Oslo, to not to add extra things to the API, but this is needed for a feature that we compromised to deliver, and without it, we can see several inconsistencies.

Cheers,
F.

Comment 26

[Julien Wajsberg [:julienw]]

Oki, that's exactly what I wanted to know, whether you managed to work around.

Will do my best to move this forward quickly then.

Comment 27

[Julien Wajsberg [:julienw]]

Francisco, Gregor told me that bug 850430 won't likely be uplifted to koi. I _really_ don't want todo the work twice here, as I based my patch on the work in bug 850430.

Therefore, is it ok to shift bug 917674 to 1.3 (ie: do it as soon as possible in master/central, but let it ride the train) ?

Comment 28

[Francisco Jordano [:arcturus] [:francisco]]

Hi Julien,

I totally understand your point, but if we don't fix the search by name, unfortunately a whole new feature, like contacts dedup, will be broken mainly cause we won't be able to dedup contacts imported from sim card.

Reasking Jose, that has more insights about the dedup to see if we could workaround this or not.

Cheers,
F.

Comment 29

[Julien Wajsberg [:julienw]]

No it's ok, if you need to work around, it's not better than doing another patch here.

Will do but I'll need to think a lot about DB versions...

Comment 30

[Francisco Jordano [:arcturus] [:francisco]]

Thanks Julien :)

Comment 31

[Jose Manuel Cantera]

(In reply to Julien Wajsberg [:julienw] from comment #29)
> No it's ok, if you need to work around, it's not better than doing another
> patch here.
> 
> Will do but I'll need to think a lot about DB versions...

thanks Julien!

Comment 32

[Noemí Freire (:noemi)]

Hi Julien,

Could you please inform us under which milestone this bug will be taken?, it is just to align bug 917674 with it. Thanks!

Comment 33

[Julien Wajsberg [:julienw]]

I really intend to finish this patch today.

The code is ready, but I want to add more tests, because I changed how the migration works, and I'm not so confident about this.

Comment 34

[Noemí Freire (:noemi)]

(In reply to Julien Wajsberg [:julienw] from comment #33)
> I really intend to finish this patch today.
> 
> The code is ready, but I want to add more tests, because I changed how the
> migration works, and I'm not so confident about this.

Many thanks for your quick reply, Bug 917674 already updated accordingly.

Comment 35

[Julien Wajsberg [:julienw]]

Attachment: 1. add name index, patch v3

Latest patch

Comment 36

[Julien Wajsberg [:julienw]]

Attachment: 2. splitting tests patch v1

Splitting the big tests in smaller files.

Comment 37

[Julien Wajsberg [:julienw]]

Try is https://tbpl.mozilla.org/?tree=Try&rev=6aabd0945fb8

Comment 38

[Julien Wajsberg [:julienw]]

On the aurora patch, I'll include all upgrade paths that landed before this and are not on aurora (this means: the WebIDL patch's upgrade path). This was suggested by Ben Turner, instead of doing a complex upgrade mechanism I implemented previously.

I'll do an aurora patch tomorrow.

Comment 39

[Julien Wajsberg [:julienw]]

Attachment: aurora 1. add the name index

patch for aurora.

It includes all upgrade paths from the WebIDL patch. We should probably either wait for bug 929652 and include its changes too, or make bug 929652 koi+.

Comment 40

[Julien Wajsberg [:julienw]]

Attachment: aurora 2. tests splitting patch

Don't know if this patch is useful. Maybe useful so that tests uplifts are easier.

Comment 41

[Julien Wajsberg [:julienw]]

new central try for the b2g build only: https://tbpl.mozilla.org/?tree=Try&rev=45b284868b57
try for aurora: https://tbpl.mozilla.org/?tree=Try&rev=b69ef6fb6b52

Comment 42

[Julien Wajsberg [:julienw]]

Attachment: aurora 2. tests splitting patch v2

whitespace fix

Comment 43

[Julien Wajsberg [:julienw]]

Comment on attachment 823022 [details] [diff] [review]
1. add name index, patch v3

Gregor, please redirect to Reuben if you think he's a better fit.

Comment 44

[Julien Wajsberg [:julienw]]

ok, now I know my try line was wrong for the b2g mochitests.

specific b2g try for central: https://tbpl.mozilla.org/?tree=Try&rev=b772fefd77a0
specific b2g try for aurora: https://tbpl.mozilla.org/?tree=Try&rev=3ab5e104acf0

Comment 45

[Julien Wajsberg [:julienw]]

Attachment: aurora 2. tests splitting patch v3

New try is https://tbpl.mozilla.org/?tree=Try&rev=6aa43f463cb5

With working tests now...

Comment 46

[Reuben Morais [:reuben]]

Comment on attachment 823022 [details] [diff] [review]
1. add name index, patch v3

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +638,5 @@
> +          let cursor = event.target.result;
> +          if (cursor) {
> +            let value = cursor.value;
> +            value.search.name = [];
> +            value.properties.name.forEach(function addNameIndex(name) {

|value.properties.name| can be null, you need to check for that.

@@ +639,5 @@
> +          if (cursor) {
> +            let value = cursor.value;
> +            value.search.name = [];
> +            value.properties.name.forEach(function addNameIndex(name) {
> +              if (name && typeof name === 'string') {

Why are you checking this? Did you run into falsy values or something that isn't a string when testing?

Comment 47

[Julien Wajsberg [:julienw]]

(In reply to Reuben Morais [:reuben] from comment #46)
> Comment on attachment 823022 [details] [diff] [review]
> 1. add name index, patch v3
> 
> Review of attachment 823022 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +638,5 @@
> > +          let cursor = event.target.result;
> > +          if (cursor) {
> > +            let value = cursor.value;
> > +            value.search.name = [];
> > +            value.properties.name.forEach(function addNameIndex(name) {
> 
> |value.properties.name| can be null, you need to check for that.

Right, will do.

> 
> @@ +639,5 @@
> > +          if (cursor) {
> > +            let value = cursor.value;
> > +            value.search.name = [];
> > +            value.properties.name.forEach(function addNameIndex(name) {
> > +              if (name && typeof name === 'string') {
> 
> Why are you checking this? Did you run into falsy values or something that
> isn't a string when testing?

Heh, you already told me that in the first review ;) I answered: I was thinking: we're in the upgrade path, and we absolutly don't want that it breaks. So better be too safe here. But you're right, I can remove it, especially after bug 929652.

Comment 48

[Reuben Morais [:reuben]]

(In reply to Julien Wajsberg [:julienw] from comment #47)
> > @@ +639,5 @@
> > > +          if (cursor) {
> > > +            let value = cursor.value;
> > > +            value.search.name = [];
> > > +            value.properties.name.forEach(function addNameIndex(name) {
> > > +              if (name && typeof name === 'string') {
> > 
> > Why are you checking this? Did you run into falsy values or something that
> > isn't a string when testing?
> 
> Heh, you already told me that in the first review ;) I answered: I was
> thinking: we're in the upgrade path, and we absolutly don't want that it
> breaks. So better be too safe here. But you're right, I can remove it,
> especially after bug 929652.

Hah, my bad. I'm starting to think I suffered some minor brain damage from landing WebIDL Contacts and dealing with the aftermatch ;)

Comment 49

[Gregor Wagner [:gwagner]]

Comment on attachment 823022 [details] [diff] [review]
1. add name index, patch v3

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

Additional Comments to Reubens review.

::: dom/contacts/tests/test_contacts_basics.html
@@ +581,5 @@
> +      checkContacts(createResult1, findResult1);
> +      next();
> +    };
> +    req.onerror = onFailure;
> +  },

We should also test a search with the lowercase name property. So that we test the 'search' field.

::: dom/contacts/tests/test_contacts_upgrade.html
@@ +82,5 @@
>    let contact2 = dbContact2.properties;
>  
>    checkStrArray(contact1.name, contact2.name, "Same name");
>    checkStrArray(contact1.honorificPrefix, contact2.honorificPrefix, "Same honorificPrefix");
> +  checkStrArray(contact1.name, contact2.name, "Same name index");

Isn't that the same as 2 lines above?

Comment 50

[Noemí Freire (:noemi)]

Hi,

Due that bug 917674 has been deferred to v1.3 (bug 917674 comment 5), it is not necessary to uplift this one to v1.2 branch so re-nom to v1.3?.
Please re-nom as koi+ if for any other reason you think it should block v1.2. Thanks

Comment 51

[Julien Wajsberg [:julienw]]

\o/ no uplift makes my life a lot easier !

Comment 52

[Francisco Jordano [:arcturus] [:francisco]]

\o/ happiness for everyone then :)

Comment 53

[Julien Wajsberg [:julienw]]

Attachment: 1. add name index, patch v4

carrying r=gwagner,reuben

rebased on top of bug 929652

Comment 54

[Julien Wajsberg [:julienw]]

Attachment: 2. splitting tests patch v2

carrying r=gwagner,reuben

only updated the commit log

Comment 55

[Julien Wajsberg [:julienw]]

New try is https://tbpl.mozilla.org/?tree=Try&rev=6261215cd6d6

Comment 56

[Julien Wajsberg [:julienw]]

bug 929652 makes important sanitization we relies on.

Comment 57

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3+ so it gets into 1.3

Comment 58

[Julien Wajsberg [:julienw]]

Attachment: 1. add name index, patch v5

Rebased on latest central, carrying r=reuben,gwagner

Comment 59

[Julien Wajsberg [:julienw]]

Attachment: 2. splitting tests patch v3

rebased, retaining r=gwagner,reuben

I took care to keep the changes from bug 932763 and bug 929435

Comment 60

[Julien Wajsberg [:julienw]]

Attachment: WIP handle migration between branches (DON'T MERGE)

For posterity, my WIP patch to handle migration between branches.

The logic was to add an objectstore to keep the Firefox OS version. If the version changes, then we know we must do more migration than what the db version is saying. An object keeps the step index for the first migration to do, and then we execute all the next ones. This means all migrations can be executed several times, and therefore needs to be idempotent.

Luckily we don't need to do this right now, but maybe we'll have to do this at one point, thus I wanted to share this code somewhere.

Comment 61

[Julien Wajsberg [:julienw]]

bug 929652 is r+, so asking checkin-needed here too.

Please land after bug 929652, thanks!

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1752a5d1fd7a
https://hg.mozilla.org/integration/b2g-inbound/rev/5cc3612b6c04

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1752a5d1fd7a
https://hg.mozilla.org/mozilla-central/rev/5cc3612b6c04