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

Fri Nov 30 19:52:06 GMT 2012

> 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.
> 
> David Jarvie wrote:
>     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()).
> 
> Christian Mollekopf wrote:
>     Ok, I get your point. So when reading and old incidence with the new code you'd expect the created date you set when you wrote the incidence out, but instead you'd get the timestamp when the incidence was written out. 
>     So the created-date is shifted forward to the timestamp the incidence was written out, from a user perspective.
>     
>     I don't see how we can provide backwards compatibility though, as we would need an implementation version number stored for that, as the fix would be to read DTSTAMP instead of CREATED for Incidence::created(), if the the file was written out using the old version.
>     
>     As far as I understand, in the specific case of the KAlarm archiving this shouldn't be a problem though, as you'd just read the timestamp of serialization when calling Incidence::created(), which is probably pretty much the same as what you set using Incidence::setCreated() before archiving the incidence. So it would in fact give you back the date-time when the incidence was archived. Worst case a little later, which would be safe.
>     
>     So while I understand the incompatibility you pointed out now, I still think it's safe and would rather not add the full backwards compatibility code since that would require introducing an implementation version number (or increasing the one encoded in PRODID), as far as I understand.
>
> 
> David Jarvie wrote:
>     I don't see why the PRODID version number can't be increased, and backwards compatibility can be based on it. It wouldn't be the first time libkcal has provided backwards compatibility, and I think it should since it actually IS an incompatibility.
>     
>     The Incidence class has been public API for a long time, and we guarantee that during KDE 4 existing methods shouldn't change. The fact that the change happens to be in a backend which the methods use isn't really relevant, IMHO. There could be third party applications which use the created()/setCreated() methods, and they could fall foul of this change. We can't know how serious such an incompatibility might be to them.

Fair enough, I'll see how I can get the code fully backwards compatible.

- Christian

-----------------------------------------------------------
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/