Unknown VCard properties are lost



  • modifying an addressbook entry in davdroid 0.5.2-alpha removes all vcard data (be it properties or attributes) it does not recognize. for example, the x-evolution-ui-slot attribute on email properties is removed when the contact gets starred or unstarred.

    i did not find a particular mention of that carddav clients must preserve everything they don't recognize (apart from xml properties where rfc6350 is explicit ("Support for this property is OPTIONAL, but implementations of this specification MUST preserve instances of this property when propagating vCards."); rfc6352 deals with creating resources, but not with modifying them), but common practice seems to be to preserve them, and moreover, the server does not receive any indication of which attributes a client supports, so can't distinguish between a client not supporting something, and a client deleting an attribute/property.

    the behavior was tested in radicale by diff'ing the vcard files of a resource created in evolution with an email address before and after starring the contact via davdroid.



  • a note on the side i forgot to add: this seems to be known behavior (cf #79), nevertheless it drops data.


  • developer

    Thank you for your report/suggestion.

    However, this behaviour is by design. vCard/iCal files are newly generated when they're read from the Android content provider; the original vCard/iCal files are not preserved (they would have to be stored in an extra sync column which would add much redundancy). This would be very cumbersome and error-prone (although I have thought about it). So, I won't implement this except some standards require it.



  • So, I won't implement this except some standards require it.

    @evert do you know if this is required by any standard?



  • i searched several standards and failed, until i read rfc2518: "A PUT performed on an existing resource replaces the GET response entity of the resource. " -- it says replaces, not updates.



  • @bartv2 yea this is a terrible violation of the standard, and the fact that custom properties are not correctly preserved, would for me immediately put davdroid on a 'do not use' list.

    You are erasing people's information. @chrysn 's assertion is correct that a PUT is always a replace.

    Just maintaining a list of properties you don't support is not enough though, you must maintain:

    • groupnames (e.g.: groupname.TEL:12345678)
    • parameters
    • preferably even the order in which properties appear

    I've dedicated an entire chapter to this in the sabredav documentation. Check out this url:

    http://sabredav.org/dav/building-a-carddav-client/

    And scroll down to 'Retain full vCards!'. Note that this is the only chapter in the documentation that features an exclamation point.

    If you care about not destroying your users' data, treat this issue with the highest severity, nothing less.



  • Preserving unknown properties is very important.

    Citing from Davdroid feature list:

    Android favorite contacts (extended field X-DAVDROID-STARRED)

    So Davdroid expects other clients to preserve his field, but does not preserve theirs?

    This is critical bug.

    And when you fix it, please make sure you fix it also in events, tasks and all other types which Davdroid supports or will support in future.


  • developer

    Just to make something clear: I consider this as "nice-to-have" while I personally don't consider it as that important (for the typical use case of DAVdroid, not in general). However, I'll re-open this as high-quality pull requests are welcome (@jkufner, maybe?).

    To decide whether this is a bug or an enhancement:

    @bartv2 yea this is a terrible violation of the standard

    @evert: If wonder if you can back this statement up with some references for people who are refusing to take your manual as a standard? I guess you'll have good formal arguments because you consider the behaviour to be not only a violation, but a "terrible" violation.

    And scroll down to 'Retain full vCards!'. Note that this is the only chapter in the documentation that features an exclamation point.

    May I also ask why the only topic in your documentation "that features an exclamation point" is not even mentioned in any standard I could find? So please support it with references, or I'll consider it as a – well-prepared, but non-binding – recommendation.

    If you care about not destroying your users' data, treat this issue with the highest severity, nothing less.

    Exactly that's the point: When a user asks to "destroy" = overwrite the VCard by a new, "incomplete" one (by saving this "incomplete" one on the device), why should DAVdroid refuse to do so?

    But don't get me wrong: As I said above, this would be "nice-to-have". If someone can show me a standard that said clients MUST retain all properties, I'll even consider it as a bug.



  • what's wrong about the rfc2518 reference?

    @user asks to destroy: if you assume that a users's intention when he opens a contact for editing, adds a phone number, and saves it is that he wants to have the contact now in the very state he sees on his device, then yes, your point is valid -- but even in that case, afaict you also support a "add this number to a contact" interface in android, and at least then the user's intention is definitely not to clean up the vcard.


  • developer

    @chrysn Your reference is not related to the topic of the issue. It states that PUTing a resource to an existing path replaces the resource, which is the way how WebDAV works. Nobody has ever disputed this, and it is obvious. (To update resources, there's a HTTP method called PATCH, but it's not in the scope of CalDAV/CardDAV.)

    The question of this issue however is how DAVdroid should create the resource (= VCard, in case of CardDAV) before PUTting it onto the server: Is it required to store all properties in the local database and write it to the updated resource or do some people just think it is good practice?

    but even in that case, afaict you also support a "add this number to a contact" interface in android, and at least then the user's intention is definitely not to clean up the vcard.

    Mine is to do so (I don't want all the garbage entries of Evolution, Thunderbird, etc), and I develop DAVdroid for my personal use primarily. But I agree with you that it might lead to, lets say, unexpected behaviour for the average user and retaining all properties would be a good thing to have.



  • i've skimmed through the relevant standards, and did not find precise
    instructions on how to apply modifications to a vcard, so unless someone
    else finds a wording to that effect, i'll take a different route of
    argumentation:

    when a user presses save, it is up to the client to phrase the user's
    request in protocol terms. every other program in the field that i've
    tested preserves unknown attributes, so a user's expectation when he
    saves a contact is that the data he previously entered (eg in evolution)
    does not vanish. as it seems we agree that the server is within its
    rights to accept the PUT body as the new representation (which means
    dropping absent data), the user's intention can only be phrased by
    taking the original vcard, applying his modifications, and sending that.

    even outside of the field, programs that have much harsher (ie
    unspecified) environments do their best to preserve what they don't
    recognize (think libreoffice/word, any html editor). consider it a
    matter of doing what the user (who might not even know about vcards and
    their properties) expects.

    implementation-wise, this means either storing the full vcard (or the
    parts you can not 100% restore, but the former is probably easier), or
    fetching the vcard before sending an update, or using PATCH (of which i
    don't know if many servers support it). it comes with resource costs,
    but the benefit is interoperability between more than just a single
    client application. (were that the goal, things would be much easier
    anyway).


  • developer

    i've skimmed through the relevant standards, and did not find precise
    instructions on how to apply modifications to a vcard, so unless someone
    else finds a wording to that effect, i'll take a different route of
    argumentation: [...]

    Accepted. So I'll consider this as an enhancement request ("nice to have"), and not as a "critical bug" or "terrible violation of standards".

    implementation-wise, this means either storing the full vcard (or the
    parts you can not 100% restore, but the former is probably easier), or

    We also have to keep in mind the resources on mobile devices are very limited, and there are often out-of-memory exceptions as soon as there are photos etc. in the VCard. Storing the whole original VCard in a column would be the best approach, but I think at least attachments would have to be removed. Does this sound reasonable for you?

    or using PATCH (of which i don't know if many servers support it).

    This would be the best approach if the CalDAV/CardDAV specifications would have defined PATCH, but unfortunately they didn't (one might wonder whether they left this out on purpose and why).

    it comes with resource costs, but the benefit is interoperability between more than just a single
    client application.

    Sounds good.

    (were that the goal, things would be much easier anyway).

    For me (personally), it's a "second goal": the first and primary one is that I can store my contacts on my own server without depending on Google. Everything else is optional, but of course, it would be good to have.



  • I do not think it is neccessary to store whole original vcard. Storing unknown properties in some unprocessed form should be enough for their reconstruction. That way there will be only one copy of each property, so large chunks of data (like photo) will not cause troubles. And client-specific properties are usualy small, so there will be no significant performance drop.

    I would not remove attachments by default, just wait whether they cause problems first.



  • We also have to keep in mind the resources on mobile devices are
    very limited, and there are often out-of-memory exceptions as soon
    as there are photos etc. in the VCard. Storing the whole original
    VCard in a column would be the best approach, but I think at least
    attachments would have to be removed. Does this sound reasonable for
    you?

    i'm not really familiar with android development, so i don't know what
    you mean by column.

    from a generic software development point of view, i'd store it
    somewhere "far away" from the usual contact data, as most of the
    operations on contacts are reading, and the original card is only needed
    for the uploading operation. actually, i'd think of it as a cache -- you
    can safely drop that information, as long as you store the etag the
    internalized version is based on. if the original vcard is present,
    fine, if not, we have to download it before the editing process can be
    finished.

    in resource constrained environments, attachments are a problem anyway
    -- even if not stored, at least they get transferred in when the cards
    are downloaded initially. i'd be happy with attachments or overly large
    images being dropped if there is some kind of confirmation if it
    actually happens ("Uploading these changes will drop an attachment to
    this contact (pictures-of-my-cats.ppt, 20MB). [Save anyway] [Abort
    editing]").

    This would be the best approach if the CalDAV/CardDAV specifications
    would have defined PATCH, but unfortunately they didn't (one might
    wonder if they left this out on purpose and why).

    i haven't seen PATCH in practice so far at all, but sabredav seems to
    implement it[1] using its proprietary update format. looking into might
    be worth it, but i currently can't spare the time to implement it on
    calypso. anyway, i'm not sure the sabredav approach is suitable for
    vcards, we might as well draft something on our own, but as i said, it's
    time consuming.

    [1] http://sabre.io/dav/http-patch/

    For me (personally), it's a "second goal": the first and primary one
    is that I can store my contacts on my own server without depending on
    Google. Everything else is optional, but of course, it would be good
    to have.

    i think that has been achieved by now :-)

    best regards
    chrysn



  • rfc2822: of course you would rather see this as a feature request, or nice to have, as opposed to consider this a case of data-loss, because that would be bad.

    I'm not really interested to go into the small print of rfc's, but I can dig and perhaps I can find one or two lines that underline the main idea I'm trying to convey.

    That's not the point however. If you are a CardDAV client, and the first thing you do after connecting to a server is to DELETE every vcard that's already on the system... technically you are still working within the constraints of the protocol and are standards-compliant.

    @chrysn in this regard has it right. This all depends on a users' expectation. Based on how you choose to proceed in this matter, I would indeed at the very least always warn the user that some data may be lost after every save operation.

    As for PATCH. It's hard! And it will not solve your problem. Take this (part of a) hypothetical vcard:

    EMAIL;TYPE=WORK;PREF=1:foo@bar.com
    EMAIL;TYPE=HOME;PREF=2:foo@bar.com
    

    In a PATCH, we'd like to update one of these. How can we easily distinguish the two? The answer that really comes down to the following:

    The only way you can semantically patch one of those lines, is if you're updating exact byte indexes, the literal order of properties in a vcard (assuming they don't change), or you include the exact line you're patching, such as:

    -EMAIL;TYPE=WORK;PREF=1:foo@bar.com
    +EMAIL;TYPE=WORK;PREF=1:foo@example.com
    

    Semantic patching is, afaik, very hard... the way the format works is that you can't really say something like "update property X and set this value" or "find property X with parameter Y and update this value" accurately.

    So as far as I can tell, the only patch mechanisms actually possible, rely on the caller (you) to have a knowledge of the exact structure of the original, which defeats the point of what you're trying to achieve (because you discard the stuff you don't support).

    Note that your workaround of putting properties aside you don't support goes a long way, but only solves part of the problem, as it doesn't cover parameters, grouping and the other items on this list.

    So yea, I'd be happy to have a chat about the details of the rfc's and what their intended meaning is, but in the context of this ticket this is meaningless, because step 1 is to accept that it's not about being compliant to the letter of the specs, it's admitting you have a problem.


  • developer

    @jkufner:

    I would not remove attachments by default, just wait whether they cause problems first.

    They have caused problems already. Handling the images in the VCards requires special attention and still doesn't work with large images flawlessly.

    @chrysn:

    i'm not really familiar with android development, so i don't know what you mean by column.

    The contact information is not stored by DAVdroid itself, but by the Android contacts provider ("the address book") which itself uses an SQLite database to store the information. The tables and columns must be used exactly by the Android specification so the contacts information is exchangeable between all Android apps. There are some fields/record types that allow sync adapters to store "custom" data – I'd probably use such one for the original VCard.

    i'd be happy with attachments or overly large images being dropped if there is some kind of confirmation if it actually happens ("Uploading these changes will drop an attachment to this contact (pictures-of-my-cats.ppt, 20MB). [Save anyway] [Abort editing]").

    Normally, a sync adapter like DAVdroid (basically, just a service) should not need a (blocking) user interface (if that's even possible, haven't checked yet). For instance, if one app (let's say, Samsung Kies or some other remote control app) adds a contact to the contacts provider, DAVdroid should sync it to the server without user interaction.

    @bartv2

    I'm not really interested to go into the small print of rfc's, but I can dig and perhaps I can find one or two lines that underline the main idea I'm trying to convey.

    I have found this:

    »However, if a client needs to make a change to a vCard, it can only change the entire vCard data via a PUT request. There is no way to incrementally make a change to a set of properties within a vCard object resource. [Yes, we already know this. But now the interesting sentence:] As a result, the client will have to cache the entire set of properties on a resource that is being changed.« — RFC 6352 (CardDAV) 9.1. Restrict the Properties Returned

    This can be most reasonably interpreted as that it's expected behaviour to retain all properties (even if there's no MUST).

    As for PATCH. It's hard!

    As it's not within the scope of CardDAV, i won't use it until there's a) at least a draft specification for usage with CardDAV/VCards, and b) support from at least a few servers.

    So yea, I'd be happy to have a chat about the details of the rfc's and what their intended meaning is, but in the context of this ticket this is meaningless, because step 1 is to accept that it's not about being compliant to the letter of the specs, it's admitting you have a problem.

    I admit that you (not you @bartv2 but all who are affected by this issue) have a problem. Personally, I don't have one because I have enjoyed the removal of hundreds of useless X-Mozilla-*, X-Evolution-* etc. headers at every sync :) Maybe that's because I'm a minimalist — I honestly didn't have the idea that someone will be missing these headers. (And yes, I know that DAVdroid itself uses X-DAVdroid-Starred and this will be lost when updating the VCard by other, equally-behaving clients. I didn't think that somebody would be missing these headers, too.)

    @All:

    I want other users to have a good experience of DAVdroid. But it's not the best idea to tell me your feedback like "Look there! A f*cking critical bug, and it deletes all data! How dare you!1! Fix it NOW, you're my open-source slave and have to work when I say so! And by the way, you'll have to do X and Y and Z, too, otherwise DAVdroid is crap!" when the behaviour just works fine for me, because then I'll of course solve the conflict of your interests and my interests by asking an independent judge – namely the specs (even if I could simply ignore your feedback as well as the specs – there's no obligation that I'll code for you, and by the way, you could also do it yourself if its that important for you).

    So, after considering RFC 6352 9.1 (see above), I think DAVdroid really SHOULD retain all properties (meaning a higher priority than "nice to have" 😀).



  • Thank you for fixing this.



  • Thank you, now i don't need to worry that changing contacts on my phone will lose information.


  • developer

    You're welcome – but don't forget to clean your VCards from time to time… they can only grow and never shrink.



  • lol


Log in to reply
 

Looks like your connection to Bitfire App Forums was lost, please wait while we try to reconnect.