941008 - [API][Contacts] navigator.mozContacts.find is returning all contacts

Status: RESOLVED WORKSFORME

(Core Graveyard :: DOM: Contacts, defect)

Description

[Piotr Zalewa [:zalun]]

Running this code https://github.com/zalun/KitchenSink/blob/master/kitchensink-app/js/apis/contacts.js#L46-L60 shows 2 instead of 1 contact

First I'm cleaning everything which has "kitchensink" in note.
Then I'm adding 2 contacts (named "Tom" and "Jerry") one after another.
And Finally I'm searching for the contact named "Tom".
I'm finding both of them (as I checked).

I hope it is me doing some crazy stuff, but I looked to this from many sides. Found when writing the Cordova shim.

Comment 1

[Julien Wajsberg [:julienw]]

Piotr, which Firefox OS version are you using?

Comment 2

[Piotr Zalewa [:zalun]]

Right. It was 1.1
I've got other issues with 1.2/1.3, but this bug might be closed

Comment 3

[Julien Wajsberg [:julienw]]

We souldn't search by "name" in 1.1, only by givenName or lastName. But in theory this shouldn't have returned all contacts, this should have failed with an error...

I'm reopening, I'd like to try this more.

Piotr, it would help if you had an app that I could just install to try this.

Comment 4

[Piotr Zalewa [:zalun]]

This is an app (KitchenSink) 
The version which is not official one yet.
https://github.com/zalun/KitchenSink 
Just install it via App Manager please or create a zippy.

Comment 5

[Piotr Zalewa [:zalun]]

I modified the test so it now contains the find example from https://developer.mozilla.org/en-US/docs/Web/API/ContactManager.find
Right now Contacts API tests in KitchenSink are failing on 1.2 and 1.3 with:
"Contacts API.find example from documentation: Something goes wrong!"
"Contacts API.create and remove a contact: find failed in cleanUp"

Comment 6

[Julien Wajsberg [:julienw]]

assigning so that I don't forget, as I probably won't have the time today.

Comment 7

[Soledad Penades [:sole] [:spenades]]

I tried Piotr's code, and also added some "logging". Seeing as the request can return a DOMError in its error field, I tried logging that one out:

callback(false, test, 'Something goes wrong! err' + request.error.name + " BOOM");

Unfortunately I only got an "UnknownError" name, which wasn't very helpful in trying to trace the source of the error...

Comment 8

[Julien Wajsberg [:julienw]]

Yeah, we could use more explicit errors.

I'm quite sure we should actually get an error in this case in Firefox 1.1, but it should work in Firefox 1.2. The thing that should never happen is returning all contacts.

Comment 9

[Fred Wenzel [:wenzel]]

I used the Kitchen Sink app[1] to test this together with the App Manager / Simulator.
- 1.1 error is present
- 1.2 error is present
- 1.3 no error

Is that expected?

[1] https://github.com/zalun/KitchenSink/

Comment 10

[Piotr Zalewa [:zalun]]

1.3 - the error breaks KitchenSink for me - I will investigate it tomorrow.

Comment 11

[Fred Wenzel [:wenzel]]

(In reply to Fred Wenzel [:wenzel] from comment #9)
> - 1.3 no error

Yeah I think I misinterpreted the output.

Comment 12

[Julien Wajsberg [:julienw]]

(In reply to Fred Wenzel [:wenzel] from comment #9)
> I used the Kitchen Sink app[1] to test this together with the App Manager /
> Simulator.
> - 1.1 error is present
> - 1.2 error is present
> - 1.3 no error
> 
> Is that expected?

Yep sorry, 1.2 does not have the name index. (for reference, this was bug 898337).
This is exactly what's expected.

(still found no time to look at this, sorry :( )

Comment 13

[Piotr Zalewa [:zalun]]

1.3 was throwing because new mozContacts().init is no longer a function. After changing it - the situation is the same as it was on 1.2

I removed 'name' from the filterBy as suggested. No difference.
The test looks like that at the moment: https://github.com/zalun/KitchenSink/blob/c74018968a9caf00a6c2012b5bab395336a0e43e/kitchensink-app/js/apis/contacts.js#L82-L103

And the response: "ERROR: Contacts API.find example from documentation: Something goes wrong!"

The other find which is failing is in cleanUp function where it searches by givenName only.
I also checked with the filterOp: 'startsWith', but no difference as well

Comment 14

[Julien Wajsberg [:julienw]]

So, several things are wrong in your code:

* firstName is not an index, you want "givenName" [1] [2]
* nickname is not an index [1]
* filterValue needs to be an array [3]

[1] https://github.com/zalun/KitchenSink/blob/master/kitchensink-app/js/apis/contacts.js#L88
[2] https://github.com/zalun/KitchenSink/blob/master/kitchensink-app/js/apis/contacts.js#L122
[3] https://github.com/zalun/KitchenSink/blob/master/kitchensink-app/js/apis/contacts.js#L25

I'll file a bug to provide better errors, because currently it's really guessing which indices are available...

Comment 15

[Julien Wajsberg [:julienw]]

Also Piotr, I'd really want to know if your comment 0 was accurate: did you get all contacts? Can you share a code that would still do this in 1.1 (and maybe on other versions too?)

Comment 16

[Piotr Zalewa [:zalun]]

Thanks. 
Tests fixed - https://github.com/zalun/KitchenSink/commit/c543aab3d097cb6be578c80fcf476fc53a130199
This is working fine in 1.3 (**+)
1.2 reports an issue (*F+) "Contacts API.create, search and remove a contact: find test failed. Expected: 1, Found: 0"

Comment 17

[Piotr Zalewa [:zalun]]

in 1.1 This is providing 2 results (expecting one)
https://github.com/zalun/KitchenSink/blob/4c5b45c498225696c176545b4f5f99249847b011/kitchensink-app/js/apis/contacts.js#L41-L61

Be careful as cleanUp is deleting everything (after running KitchenSink there is only Tom contact left, Jerry is deleted inside test)

CleanUp:
https://github.com/zalun/KitchenSink/blob/4c5b45c498225696c176545b4f5f99249847b011/kitchensink-app/js/apis/contacts.js#L14-L39

Comment 18

[Julien Wajsberg [:julienw]]

(In reply to Piotr Zalewa [:zalun] from comment #16)
> Thanks. 
> Tests fixed -
> https://github.com/zalun/KitchenSink/commit/
> c543aab3d097cb6be578c80fcf476fc53a130199
> This is working fine in 1.3 (**+)
> 1.2 reports an issue (*F+) "Contacts API.create, search and remove a
> contact: find test failed. Expected: 1, Found: 0"

I think that in 1.2 and 1.1 you need to use the old "init" method, and that would be the reason of the failure for 1.2 at least.
I'm not sure how you can feature-test this... Maybe "if (mozContact.prototype.init)" ?

Reuben, how can we make a code that works with both the old and the new mozContact API ?

Comment 19

[Piotr Zalewa [:zalun]]

Good idea - I will work on Contacts API for Cordova so it will work with both versions

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Julien Wajsberg [:julienw] from comment #18)
> I think that in 1.2 and 1.1 you need to use the old "init" method, and that
> would be the reason of the failure for 1.2 at least.
> I'm not sure how you can feature-test this... Maybe "if
> (mozContact.prototype.init)" ?
> 
> Reuben, how can we make a code that works with both the old and the new
> mozContact API ?

It seems we should have a dummy init() method on the new WebIDL mozContact object.  We broke backward compatibility by removing it completely.

Comment 21

[Reuben Morais [:reuben]]

(In reply to Julien Wajsberg [:julienw] from comment #18)
> I think that in 1.2 and 1.1 you need to use the old "init" method, and that
> would be the reason of the failure for 1.2 at least.
> I'm not sure how you can feature-test this... Maybe "if
> (mozContact.prototype.init)" ?
> 
> Reuben, how can we make a code that works with both the old and the new
> mozContact API ?

Due to the way the mozContact ctor was registered before WebIDL, you have to do something like:

  if ("init" in (new mozContact())) {

Alternatively, you can use that fact to do an indirect check that doesn't create an object:

  if (window.mozContact.prototype) {

Which of course doesn't work if someone sets the prototype.

Comment 22

[Julien Wajsberg [:julienw]]

Reuben, would something like this work:

  var data = {
    givenName: "Mark",
    familyName: "Smith
  };

  var contact = new mozContact(data);
  if ("init" in contact) {
    contact.init(data);
  }

?

Comment 23

[Reuben Morais [:reuben]]

(In reply to Julien Wajsberg [:julienw] from comment #22)
>     givenName: "Mark",
>     familyName: "Smith

Surely you mean ["Mark"] and ["Smith"] ;)

>   var contact = new mozContact(data);
>   if ("init" in contact) {
>     contact.init(data);
>   }

Yes, that would work.

Comment 24

[Julien Wajsberg [:julienw]]

Yeah, I forgot that givenName and familyName are arrays too ;)

Comment 25

[Piotr Zalewa [:zalun]]

Cool - this code is working fine in 1.2 and 1.3
https://github.com/zalun/KitchenSink/blob/c129f09b6b7458f53a918dbbae33c52f310dc9f1/kitchensink-app/js/apis/contacts.js#L89-L96

I'm now investigating search issues - is there a place where I can find on which fields one may and on which may not search?

Thanks

Comment 26

[Julien Wajsberg [:julienw]]

The best place right now is directly at the Source:

https://mxr.mozilla.org/mozilla-central/source/dom/contacts/fallback/ContactDB.jsm#166

The "tel" index is special cased.
The "LowerCase" indices are used for the "startsWith" search.

The same location for v1.1 is: https://mxr.mozilla.org/mozilla-b2g18/source/dom/contacts/fallback/ContactDB.jsm#168

v1.2 is currently https://mxr.mozilla.org/mozilla-beta/source/dom/contacts/fallback/ContactDB.jsm#165

Comment 27

[Julien Wajsberg [:julienw]]

I updated MDN with what we said here:

https://developer.mozilla.org/en-US/docs/Web/API/mozContact
https://developer.mozilla.org/en-US/docs/WebAPI/Contacts

Hopefully we'll find soon why this doesn't work correctly in v1.1 :)

Comment 28

[Julien Wajsberg [:julienw]]

I think this works now.