GNOME Bugzilla – Bug 686686
Preserve original contact UIDs in the addressbook.
Last modified: 2013-09-14 16:55:57 UTC
Currently EDS replaces each contact's unique identifier ("UID") with its own generated id. This behavior has drawbacks to synchronization performance and also violates the vCard specification. It would be nice to fix EDS to preserve the added contact UIDs where possible (or error out when trying to add a contact who's UID is already used by the addressbook). Perhaps this will require some ESourceExtension configuration in the case that it may break existing addressbooks.
I'm taking a look at this right now, after closer inspection here are some minor issues that come to mind: o When adding a new contact, what happens if the contact already exists ? a.) Should an error be reported ? b.) Should we overwrite the existing contact ? c.) Should a backup/duplicate be created for the existing contact bearing the same UID as the incoming new contact ? (or reverse ?) o For most backends this is straight forward, the webdav backend has this complication that it uses the contact's uid field to hold the actual uri where the vcard is stored. This can be a bit tricky to fix, the UIDs can be normalized for usage as filenames and then stored remotely as: "$webdav_base_uri/$normalized_uid" The migration bits might be tricky for this, though, as specially since it would be a bad idea to break/change existing UIDs stored via the webdav backend.
I'd expect that calling e_book_client_add_contact() with an already existing UID should result in a distinct error. If there is no error code yet, a new code should be added. The WebDAV problem should be solved by escaping UIDs via g_uri_escape_string() when building the URL. URI encoding is fully reversible via g_uri_unescape_string().
(In reply to comment #1) > I'm taking a look at this right now, after closer inspection here > are some minor issues that come to mind: > > o When adding a new contact, what happens if the contact already > exists ? > > a.) Should an error be reported ? Yes, with an error code that allows identifying this case. > b.) Should we overwrite the existing contact ? No. > c.) Should a backup/duplicate be created for the > existing contact bearing the same UID as the incoming > new contact ? (or reverse ?) > No. > o For most backends this is straight forward, the webdav backend > has this complication that it uses the contact's uid field to > hold the actual uri where the vcard is stored. At least for CalDAV, some servers use (and enforce) resource names (= uris) which are unrelated to the UID inside the item. I expect the same to be valid and used for CardDAV. Are you saying that the WebDAV backend overwrites the UID inside the vCard with the URI? I guess it has to because of the design defect in the EDS API which only supports one ID of an item (UID) instead of two (UID and some backend/database specific local ID). Without an API redesign, this issue therefore can only be solved for backends where local ID and UID can be made the same reliably.
Created attachment 233979 [details] [review] Allow EBookBackendSqliteDB to fail on UID conflicts EBookBackendSqliteDB: Added e_book_backend_new_contact[s]() APIs The new API allows control on whether or not to overwrite existing contacts with the same UID. Deprecated the older e_book_backend_add_contact[s]() apis which include the unused 'partial_contact' parameter and have no control on whether or not to overwrite existing contacts. Additionally this patch exposes the E_BOOK_SDB_ERROR domain and a couple of error codes to distinguish if an error occurred because of a constraint or other errors (UID conflicts at contact addition time are constraint errors).
Created attachment 233980 [details] [review] EBookBackendFile: Don't overwrite UIDs on incomming/new contacts EBookBackendFile: Don't overwrite UIDs on incomming/new contacts. o Only assign a generated UID if the new contact does not have any o If the UID already exists in the SQLite at creation time, then report an E_CLIENT_ERROR_QUERY_REFUSED error.
Created attachment 233981 [details] [review] Added test case ensuring that new contacts have thier UID preserved Asserts both that newly added contacts preserve the assigned UID and also that 2 contacts with the same UID cannot be added.
Created attachment 233982 [details] [review] EBookBackendVCF: Don't overwrite UIDs on incomming/new contacts. EBookBackendVCF: Don't overwrite UIDs on incomming/new contacts. o Only assign a generated UID if the new contact does not have any o If the UID already exists at creation time, then report the E_CLIENT_ERROR_QUERY_REFUSED error.
Created attachment 234001 [details] [review] Allow EBookBackendSqliteDB to fail on UID conflicts (rev 2) oops, the first version of this patch missed some details. This one brings in the missing lines of code to actually report the constraint error when SQLITE_CONSTRAINT is returned by sqlite3_exec() and also makes sure that '0' is not used as a reported error anywhere.
Review of attachment 233982 [details] [review]: Rejecting this, the file is gone in current git master.
Review of attachment 234001 [details] [review]: Looks good, just add the deprecated functions in .h file into #ifndef EDS_DISABLE_DEPRECATED #endif /* EDS_DISABLE_DEPRECATED */ The error codes are kinda cryptic, but I believe it's fine for now, it'll be enhanced with the offline cache when it'll be done.
Review of attachment 233980 [details] [review]: Please fix the one thing and commit. ::: addressbook/backends/file/e-book-backend-file.c @@ +758,3 @@ + E_BOOK_SDB_ERROR_CONSTRAINT)) { + g_set_error (perror, E_CLIENT_ERROR, + E_CLIENT_ERROR_QUERY_REFUSED, The backend should not return client-side errors, it should return E_DATA_BOOK_ERROR, which has a dedicated error code E_DATA_BOOK_STATUS_CONTACTID_ALREADY_EXISTS.
Review of attachment 233981 [details] [review]: The test file doesn't follow generic coding style being used in evolution, though the first file's line mentions the setup for whitespaces. Could you fix that too, before you'll commit, please? ::: tests/libebook/client/test-client-preserve-uid.c @@ +1,3 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */ +/* + * Copyright Copyright (C) 2013 Intel Corporation 3-times Copyright, while might be only twice, like "Copyright (C), right?
Comment on attachment 234001 [details] [review] Allow EBookBackendSqliteDB to fail on UID conflicts (rev 2) Thanks for review. (actually the deprecated functions were already in EDS_DISABLE_DEPRECATED guards, but not easy to tell by reading the patch ;-))
Comment on attachment 233980 [details] [review] EBookBackendFile: Don't overwrite UIDs on incomming/new contacts Thanks for your time reviewing this. Fixed to use E_DATA_BOOK_ERROR types.
Comment on attachment 233981 [details] [review] Added test case ensuring that new contacts have thier UID preserved Thanks for review. Fixed coding style and copyright.
For the remaining backends: o Google backend: Requires a specific ID be gdata_entry_get_id() o ldap: The contact UID is dynamic, and changes depending on contact information (i.e. changing the Full Name can change the UID) o webdav: The contact UID is the URI, and can change if ever the webdav URI is redirected to a new location (the code actually updates the UID based on the real location). For webdav & ldap, we're leaving this as is as there can be complications stemming from other versions of EDS or entirely different software accessing the same ldap/webdav repositories. Changing the way that UID is handled can potentially break other software accessing the same repositories.