capacitor-community / contacts

Contacts Plugin for Capacitor
https://capacitor-community.github.io/contacts/
119 stars 52 forks source link

projection is returning more fields than expected #94

Open RRGT19 opened 1 year ago

RRGT19 commented 1 year ago

Describe the bug According to the docs here, if we use:

const {contacts} = await Contacts.getContacts({
      projection: {
        name: true // <======
      }
    });

It will only return two properties: contactId and name. This is not what is happening, I am receiving more fields, phones.

Example:

[
    {
        "contactId": "1",
        "name": {
            "display": "Juan Perez",
            "given": "Juan",
            "family": "Perez"
        },
        "phones": [
            {
                "type": "mobile",
                "number": "(809) 333-2222"
            },
            {
                "type": "custom",
                "label": "Flote",
                "number": "(849) 333-2222"
            }
        ]
    },
]

The same thing is happening with the pickContact() method. Example:

const result = await Contacts.pickContact({
      projection: {
        name: true
      }
});

Platforms

iOS seems to be fine, but maybe you can re-validate the issue on iOS to be sure is not happening.

To Reproduce Steps to reproduce the behavior:

  1. Install the latest version of the plugin
  2. Call getContacts() method with a projection with only name
  3. Call pickContact() method with a projection with only name

Expected behavior Contacts should have only a name object, no more than that.

Example:

[
    {
        "contactId": "1",
        "name": {
            "display": "Juan Perez",
            "given": "Juan",
            "family": "Perez"
        },
    },
]

Screenshots None.

Smartphone (please complete the following information):

Additional context None.

tafelnl commented 1 year ago

This is indeed only the case in Android. It's kind of expected behavior which I am currently looking into if it can be worked around. Allow me to explain:

On iOS one can simply indicate the projection with unique keys, and the iOS system will only retrieve and return those fields specified in the projection.

In Android this works pretty similar. However, as it turns out, these projection keys are not necessarily unique. For example DISPLAY_NAME resolves to the string data1. But so do COMPANY and NUMBER for example.

That means that if you query the name of a contact, Android will also retrieve and return the organization and phones fields for that contact.

This can be worked around of course. There are a few ways:

1. Provide a selection matching the projection. This means that we will indicate to Android inside the query that we only want to retrieve specific types of MIMETYPES. I actually already tried this before and it's now commented out in the code: https://github.com/capacitor-community/contacts/blob/main/android/src/main/java/getcapacitor/community/contacts/Contacts.java#L144

The disadvantage of this, is that it will only retrieve contacts that match your projection. For example if your projection only specifies organization, Android will only return contacts that actually have an organization attached. I think this may be unexpected to a developer. Moreover, it's different from the iOS behavior, which will always return all contacts. Even if those contacts won't have any other field than the contactId, because they do not have an organization attached for example.

2. We could do two separate queries. One where we follow the solution mentioned above. And then also one where we query all the remaining contacts, but then only request the contactId of those.

This solution seems pretty neat to me. However, I'm not sure if it's actually faster than just doing it the way we're doing it now. If it's not faster, it kinda defeats the purpose of projection. So it wouldn't make sense to implement it anyways. But that's something I need the investigate still.


That being said however, I don't think for most use-cases this imperfection in the plugin will actually effect the performance significantly. But please let me know if you have experienced otherwise.

tafelnl commented 1 year ago

I have a PR for this: #97

Ran a bunch of benchmarks, but couldn't really find a significant performance difference between the current implementation and the two alternatives. So I'll probably just end up implementing it like this because of consistency with the docs and the iOS counterpart.

If you would like to share your opinion about this; please do!