295775 - calICalendar.modifyItem should require caller to pass in the old item

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

In our intermittently-connected world, there's no guarantee that the old item
that's being modified is the same version of a given item that is currently live
on the server, in which case there may be a conflict between this change and
whatever changes have taken place in the intervening time on the server. 
Without the caller passing in the old item, there's no way to detect this conflict.

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

Is this really a job for the provider? I think the caceh or whatever should
handle conficts. (and in my tree, it does, although it doesn't tell the user yet)

Comment 2

[Mike Shaver (:shaver emeritus)]

You can (pessimistically) detect the conflict on the basis of the generation
number, or what-have-you, no?  You can't really analyze the conflict on that
basis, though, to see if it's a conflict that requires user interaction.

mvl: how does your tree handle the conflict in the cache?  (Not that I think use
of a local cache should be required for correctness, but it sounds interesting.)
 If we auto-sync while the user is causing a clone-and-modify cycle to occur, it
seems to me that the cache will no longer necessarily recall the original object.

Would it be better to track the "source" of cloned objects?  I fear leaks or
otherwise over-large object graphs, but it would make this sort of thing more
automatic for all uses, and not just modifyItem.  (I'm thinking of undo here, I
guess, though I'm a little travel-damaged and might therefore not be thinking
very _well_ of undo.)

Vlad: does the infrastructure for your new exception stuff help us here, by
letting us keep an "overlay" object with only the changes?

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #2)
> Vlad: does the infrastructure for your new exception stuff help us here, by
> letting us keep an "overlay" object with only the changes?

It can; we can change the model to be to get a proxy and then modify that,
instead of cloning.  I can't think of any issues we'd run into in that case
though.. what would the generation number be?  And it would be up to the
provider's modifyItem call to deal with moving the modified properties to the
base item?

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

The idea of my cache is not only to be a cache for faster access (it might not
even always be faster, the old way of storing everythin in memory might be
better) but also to work with intermittent connections.

Currently, it works like this:
The cache-engine is a wrapper around the storage provider. Because it is a
wrapper, it knows about what items the user changed. It keeps a list of those.
The ics-part of the cache has a list of last-modified dates of all the items. It
downloads the ics files on a timer. If the last-modified of an item changed, it
knows the items changed by some external party. If the item was also changed by
the user, a conflict is detected (but not currently handled, i still need to do
that).

So the cache doesn't know about the original object, but it doesn't need to
know. (Or maybe it does, for the user interface when trying to handle a
conflict, need to figure that part out) The cache depends on the lastmodified to
change, but that part is in the ics-provider. Some other provider might do some
other check, and can store the original item if it really needs to. Keeping a
copy of the cache in a seperate storage file might be easier in that case.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #2)
> You can (pessimistically) detect the conflict on the basis of the generation
> number, or what-have-you, no?  You can't really analyze the conflict on that
> basis, though, to see if it's a conflict that requires user interaction.

Yes, exactly correct.

> mvl: how does your tree handle the conflict in the cache?  (Not that I think
> use of a local cache should be required for correctness, but it sounds 
> interesting.)  If we auto-sync while the user is causing a clone-and-modify 
> cycle to occur, it seems to me that the cache will no longer necessarily 
> recall the original object.

In fact, it's not just the cache that's an issue here.  The thing that
originally got me thinking about this stuff is that right now, the provider has
to return the old item.  However, in the case of the CalDAV (as the provider
code is currently structured, anyway), it doesn't keep a local cache of what's
on the server.  This means it has to query the server just to get the old item,
and the old item could have potentially changed since the caller last refreshed
itself, in which case we've got a problem.

> 
> Would it be better to track the "source" of cloned objects?  I fear leaks or
> otherwise over-large object graphs, but it would make this sort of thing more
> automatic for all uses, and not just modifyItem.  (I'm thinking of undo here,
> I guess, though I'm a little travel-damaged and might therefore not be 
> thinking very _well_ of undo.)

I can't speak to whether this will be more effective, but I have to admit that
it sounds little scary to me too.

Comment 6

[Mike Shaver (:shaver emeritus)]

If you don't have the old item (because the server's store has been mutated by
another caller), I think you have two choices:

- throw NS_ERROR_MODIFY_CONFLICT, and let the upper levels deal with the error
(likely by just starting the edit over)

- look at some snapshot state that corresponds to the "basis" of the
modification, analyze the local and remote changes, and then either
automatically resolve (spooky!) or toss a conflict exception.

For fine-grained conflict resolution that copes with intermittent connection, I
think we need some form of local record of the current basis for modification. 
I think a caching calendar can provide that local record, given some
communication between the "real" calendar and the cache.

I think that this communication requirement is mostly handled by having the
caching calendar wrapper ask the authoritative calendar to make sure that the
cache is updated, though we do need a 'cached modifications' calendar as well,
so that we can keep the cache "pure" while still letting the views and such see
the effects of local changes.  The synchronization operation would then
basically be applying the cached modifications, with whatever conflict
resolution story we choose from the above, and then refreshing the cache from
the authoritative server.  A non-optimized version of this (full cache refresh,
pessimistic conflict resolution) is probably not extremely difficult.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

*** Bug 297109 has been marked as a duplicate of this bug. ***

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

So after a conversation on IRC last week, we tentatively agreed to temporarily
make this change just to make life easier for implementers of networked
calICalendars.  Once everybody supports caching calendars, the need for this may
go away.  Patch forthcoming.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: modifyItems changes + CalDAV tweaks from the Interop

Here are the modifyItems changes.  For existing provider code that gets its own
copy of the old item internally, I left that alone and just had them ignore the
new parameter.

Also in this patch are a bunch of CalDAV fixes for bugs discovered at the
Interop which are unrelated to this bug.  I'd like to check this stuff in all
at once.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 185702 [details] [diff] [review]
modifyItems changes + CalDAV tweaks from the Interop

This looks ok to me, with one nit -- can you switch the order of params to be
(newitem, olditem), to match calIObserver?

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: calICalendar + CalDAV interop changes, v3

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Checked in.