1495678 - Address Book WebExtensions API: allow setting contact UID during creation

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, enhancement)

Description

[Magnus Melin [:mkmelin]]

+++ This bug was initially created as a clone of Bug #1469238 +++

From bug 1469238 comment 49 and around:

For contacts that already have a UID from elsewhere (another Thunderbird user's contacts, or what not) allow setting UID during creation of the contact copy in the creation phase.

It should not be allowed to change later. Only for the creation step.

Comment 1

[John Bieling (:TbSync)]

I want to request to be able to Change the UID even after creation. The EAS protocol actually requires this. When a contact is created locally and send to the Server, it will return a new UID which I have to adapt.

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1495678-abcard-setuid-1.diff

Here's the binary changes required to do this.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9063396 [details] [diff] [review]
1495678-abcard-setuid-1.diff

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

LGTM, thx!! r=mkmelin

::: mailnews/addrbook/src/nsAbCardProperty.cpp
@@ +346,5 @@
> +  nsresult rv = GetPropertyAsAString(kUIDProperty, aString);
> +  if (NS_SUCCEEDED(rv)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +

I wonder if it makes sense not to fail if the UID is already the same as what it's getting set to?

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: 1495678-abcard-setuid-2.diff

Better?

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: 1495678-contacts-withuid-1.diff

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9063684 [details] [diff] [review]
1495678-abcard-setuid-2.diff

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

Great! r=mkmelin

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9063686 [details] [diff] [review]
1495678-contacts-withuid-1.diff

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

::: mail/components/extensions/parent/ext-addressBook.js
@@ +503,5 @@
>          },
>          get(id) {
>            return addressBookCache.convert(addressBookCache.findContactById(id), false);
>          },
> +        create(parentId, id, properties) {

it would seem more logical to have the id as last, optional parameter so people don't have to call this with the middle parameter null

::: mail/components/extensions/schemas/addressBook.json
@@ +319,5 @@
>            },
>            {
> +            "name": "id",
> +            "type": "string",
> +            "description": "Assigns the contact an id. It is the caller's responsibility to ensure ids are unique.",

true, but can you still add a check that the card with that id doesn't exist already (and throw if it does?)
maybe also document that we're expecting something like a uuid, though other forms would be acceptable, only so that people don't mis-use this to have like id=2 or something like that.
Basically, what I think should be allowed is things that can reasonable be considered to be unique: uuids, and uri based ids.

Comment 8

[Geoff Lankow (:darktrojan)]

(In reply to Magnus Melin [:mkmelin] from comment #7)

it would seem more logical to have the id as last, optional parameter so
people don't have to call this with the middle parameter null

It is optional. The WebExtensions interface checks the argument count and types and assigns null to id before calling this function.

true, but can you still add a check that the card with that id doesn't exist
already (and throw if it does?)

Reasonable, in fact I considered it but didn't do it for performance reasons when adding multiple contacts, but I guess the whole address book only needs to be checked the first time.

maybe also document that we're expecting something like a uuid, though other
forms would be acceptable, only so that people don't mis-use this to have
like id=2 or something like that.
Basically, what I think should be allowed is things that can reasonable be
considered to be unique: uuids, and uri based ids.

How do you propose to enforce that?

Comment 9

[Magnus Melin [:mkmelin]]

Open to suggestions, but maybe we want to ensure the length is at least 16 (which is already stretching unique quite far)

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 1495678-contacts-withuid-2.diff

I don't think we can do a lot. Having a minimum length is tempting though.

This would all be a lot easier if we had address books that could only be written to by the extension that created them. Something to consider in the future.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/254c026047c8
Allow setting nsIAbCard UID from script if it is not already set; r=mkmelin
https://hg.mozilla.org/comm-central/rev/1eaba2d214b2
Address Book WebExtensions API: allow setting contact UID during creation; r=mkmelin

Comment 12

[[e-mail]]

(In reply to Geoff Lankow from comment #8)

(In reply to Magnus Melin from comment #7)

it would seem more logical to have the id as last, optional parameter so people don't have to call this with the middle parameter null

It is optional. The WebExtensions interface checks the argument count and types and assigns null to id before calling this function.

In what way is it optional? I can't call create with two parameters and get WebExtensions to assign null to id; I'm forced to pass three parameters, aren't I?

Comment 13

[Geoff Lankow (:darktrojan)]

No you're not. If you call the function with the second parameter as an object, id is automatically null when it gets to the actual code.

As seen here: https://hg.mozilla.org/comm-central/file/d05d70ad9d05bde2e19cab53929c98c1d3058fe9/mail/components/extensions/test/xpcshell/test_ext_addressBook.js#l154

The function your extension code calls isn't the function in the API code. There's a layer in between which checks you're passing in usable arguments as specified in the schema.

Comment 14

[[e-mail]]

(In reply to Geoff Lankow from comment #13)

No you're not. If you call the function with the second parameter as an object, id is automatically null when it gets to the actual code.

As seen here: https://hg.mozilla.org/comm-central/file/d05d70ad9d05bde2e19cab53929c98c1d3058fe9/mail/components/extensions/test/xpcshell/test_ext_addressBook.js#l154

The function your extension code calls isn't the function in the API code. There's a layer in between which checks you're passing in usable arguments as specified in the schema.

The problem is that the extension manager is using our Thunderbird 60 compatibility schema, so it isn't checking our arguments, but it's calling the Thunderbird 68 implementation, so the call throws an exception. Anyway, that's easily worked around on our end, now we know what the problem is.