[Kde-pim] Review Request: iCal format fixes for CREATED and DTSTAMP

[David Jarvie]

> On Nov. 28, 2012, 12:21 p.m., David Jarvie wrote:
> > This looks good - things seem to have been a bit screwed up before. However, this will cause problems for KAlarm, which would purge alarms unintentionally when it reads a calendar created with the old code, with DTSTAMP and CREATED reversed. There needs to be backwards compatibility for calendars created using the old code, so that for them DTSTAMP is read into the Incidence's 'created' property, and CREATED is read into the 'dtstamp' property.
> 
> Christian Mollekopf wrote:
>     I don't quite follow. DTSTAMP is now the last-modification-date (except for iTip where we have a METHOD property), and CREATED is just not update on every write anymore. If you require the time the object has last been written you should probably use Akonadi::Item::modificationTime().
>     
>     If you can give me a pointer to your code I can take a look, compatibility code shouldn't be required I think.
> 
> David Jarvie wrote:
>     KAlarm uses Incidence::setCreated() and Incidence::created() to set and retrieve the date an archived event was created. It uses this date to determine whether the event should be purged from the calendar (the user can configure that events should be purged, i.e. deleted, a set number of days after they were archived). The old icalformat_p.cpp stored the date set by these methods in the DTSTAMP attribute, not the CREATED attribute. Your patch will change this so that the created value will be stored in the CREATED attribute. So if KAlarm is using your patched code to access a calendar which was written by the old software, when it uses Incidence::created() to fetch a value, the value of DTSTAMP will be returned instead, which will be set to some unrelated value. So the decision on whether to purge archived events will be based on a wrong date.
>     
>     Any other software which relies on either CREATED or DTSTAMP values will also have problems when reading calendars written with the old library, since their values are now swapped.
> 
> Christian Mollekopf wrote:
>     If you accessed the creation date before, you got back the timestamp of serialization. If you access the same object now, you still get back the very same date (on the created property only what is written out changed). What has changed is that if you write a new object out, it will not write a current timestamp, but instead use the created-date of the incidence. 
>     
>     So:
>     
>     Before:
>     * CREATED: write: current timestamp read: CREATED property
>     * DTSTAMP: write: created-date of incidence read: never read
>     Now:
>     * CREATED: write: created-date of incidence read: CREATED property
>     * DTSTAMP: write: last-modified-date (without METHOD) read: DTSTAMP property
>     
>     So I don't think it will affect your code in that respect. The only code which could potentially be affected if one relies on created containing the time of serialization, and I can't think of any situation where that would make any sense. So IMO this patch should be safe.

You are looking at it simply from the point of view of the data stored in the calendar, and you are quite correct as far as that goes. However, the problem I describe is due to the use of the access methods provided in the Incidence and Event classes, and how the data set and read by *them* was and will be stored in the calendar.

Before:
* Incidence::setCreated(): write DTSTAMP; Incidence::created(): read DTSTAMP.
* write CREATED: current timestamp; read: never read.
Now:
* Incidence::setCreated(): write CREATED; Incidence::created(): read CREATED.

As you can see, Incidence::created() called when accessing a calendar written by the old software will now return whatever was the current timestamp at the time the calendar was written, not what was set by Incidence::setCreated(). Backwards compatibility needs to be provided in the access methods (Incidence::created() and Incidence::setCreated()).


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107480/#review22692
-----------------------------------------------------------


On Nov. 26, 2012, 7:21 p.m., Christian Mollekopf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107480/
> -----------------------------------------------------------
> 
> (Updated Nov. 26, 2012, 7:21 p.m.)
> 
> 
> Review request for KDEPIM, KDEPIM-Libraries and David Jarvie.
> 
> 
> Description
> -------
> 
> This patch fixes the uses of CREATED and DTSTAMP in respect to RFC5545 (iCal) and RFC5546 (iTip).
> 
> CREATED was used as serialization timestamp, which is IMO wrong as it should preserve the creation date of the conceptual ical object. The dtstamp was written as creation-date but never read back, but it should be the last-modification-date without METHOD (iTip), and the time of serialization with a METHOD.
> 
> This patch fixes the following problems:
> * dtstamp and created exchanged when writing any ical object
> * created date is not preserved
> * last-modification-date is not preserved
> * vFreebusy iTip messages contains a created date (but must not)
> 
> As a sideeffect this patch also fixed the RecurNext* and RecurPrev* unittests.
> 
> This patch should be safe to apply to existing systems.
> 
> 
> This addresses bugs 310448 and 310469.
>     http://bugs.kde.org/show_bug.cgi?id=310448
>     http://bugs.kde.org/show_bug.cgi?id=310469
> 
> 
> Diffs
> -----
> 
>   kcalcore/icalformat_p.cpp 418a92a6e1f033d1110add754c4966436585dea2 
> 
> Diff: http://git.reviewboard.kde.org/r/107480/diff/
> 
> 
> Testing
> -------
> 
> checked existing unit tests (all pass). Running the patch on my productive system now.
> 
> 
> Thanks,
> 
> Christian Mollekopf
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/