Bug in etag handling results in deleted calendar events

pgoerzen 2008-03-31 21:41:49 UTC #1

Greetings,

In calendar/inc/class.bocalupdate.inc.php::save(), we have:

$check_etag = ($event[‘etag’] ? $event[‘etag’]:1);

When coming from
icalsrv/inc/class.bocalupdate_vevents.inc.php::import_vevent(), $event may
represent an existing event, but import_vevent() doesn’t set ‘etag’.
$check_etag then gets the value 1 instead of its appropriate value for the
event. This ultimately causes import_vevent to fail, and the event gets
deleted. Luckily, I didn’t push trunk out to our users yet

I think the problem is lack of clarity as to where ‘etag’ is expected to be
set. Should it be done in icalsrv, or bocalupdate::update(), or
bocalupdate::save(), or as late as socal::save() ? Or we could just set
$check_etag appropriately in bocalupdate::save():

if ($event[‘id’]) {
$event2update = $this->read($event[‘id’]);
$check_etag = ($event2update[‘etag’] ? $ event2update[‘etag’] : 1);
}

Suggestions?

Thanks!

–
Peter Goerzen
I.S. Development
Hustler Turf Equipment/Excel Industries, Inc.

Check out the new SourceForge.net Marketplace.
It’s the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

eGroupWare-developers mailing list
eGroupWare-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/egroupware-developers

Leithoff_Klaus 2008-04-01 07:26:01 UTC #2

Hi Peter,
I did commit a fix for that behavior, in class.socal.inc.php
if ($cal_id && $check_etag && $check_modified)
in line 491
Somehow, someone took that out
if ($cal_id && $check_etag /&& $check_modified/)

the check_modified is a new optional parameter by the save function, if this
parameter is set, it is a save action caused by the ui, if not, …
If the parameter is set the saving of an event is done, regardless of the
etag.

Please test, and if it resolves the problem, please tell, and I will
recommit.
Best regards
Klaus

“Peter Goerzen” schrieb:

Greetings,

In calendar/inc/class.bocalupdate.inc.php::save(), we have:

$check_etag = ($event[‘etag’] ? $event[‘etag’]:1);

When coming from
icalsrv/inc/class.bocalupdate_vevents.inc.php::import_vevent(), $event may

represent an existing event, but import_vevent() doesn’t set ‘etag’.
$check_etag then gets the value 1 instead of its appropriate value for the

event. This ultimately causes import_vevent to fail, and the event gets
deleted. Luckily, I didn’t push trunk out to our users yet

I think the problem is lack of clarity as to where ‘etag’ is expected to
be
set. Should it be done in icalsrv, or bocalupdate::update(), or
bocalupdate::save(), or as late as socal::save() ? Or we could just set
$check_etag appropriately in bocalupdate::save():

if ($event[‘id’]) {
$event2update = $this->read($event[‘id’]);
$check_etag = ($event2update[‘etag’] ? $ event2update[‘etag’] : 1);
}

This is related to the following thread:

http://www.nabble.com/Termineintrag-in-Lightning---icalsrv_resourcehandler%3A-INTERNAL-ERROR-to15907760s3741.html

Suggestions?

Thanks!

–
Peter Goerzen
I.S. Development
Hustler Turf Equipment/Excel Industries, Inc.

Check out the new SourceForge.net Marketplace.
It’s the best place to buy or sell services for
just about anything Open Source.

http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

eGroupWare-developers mailing list
eGroupWare-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/egroupware-developers

eGroupWare-developers mailing list
eGroupWare-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/egroupware-developers

pgoerzen 2008-04-01 14:51:20 UTC #3

Hi Klaus,

Thanks for the information. I tested with the change, but unfortunately it did
not fix the problem. Here’s what’s happening as best as I can tell. My
apologies, as this isn’t very easy to communicate over email. . .

***** icalsrv.bocalupdate_vevents.import_vevent() *****
import_vevent() imports a VEVENT into an egw event and calls
calendar.bocalupdate.update():

if ($eidOk = $this->rsc->update($event, TRUE)){ // Line 907

***** calendar.bocalupdate *****
Note that $event[‘etag’] is not set at this point. Then update() calls
calendar.bocalupdate.save():

if (!($cal_id = $this->save($event))) // Line 274

$event[‘etag’] is still not set. In save(), $check_etag gets set:

$check_etag = ($event[‘etag’] ? $event[‘etag’]:1); // Line 706

Because $event[‘etag’] isn’t set, $check_etag = 1 always when coming from
icalsrv. Then, save() calls calendar.socal.save():

if (($cal_id = $this->so->save($event,$set_recurrences,NULL,$check_etag)) &&
$set_recurrences && $event[‘recur_type’] != MCAL_RECUR_NONE) // Line 724

***** calendar.socal.save() *****
The result here is that in calendar.socal.save(), $check_modified always gets
the value 1. Then, in calendar.socal.save(), we have:

$check_etag = ($check_modified ? $check_modified :
$event[‘cal_etag’]); // Line 490

$check_etag will always get set to 1, since $check_modified is always 1, then:

if ($cal_id && $check_etag && $check_modified) // Line 491

This will always be true as long as $cal_id is set, which it is for existing
events. Then:

$where = array(‘cal_id’ => $cal_id,’(cal_etag is NULL or cal_etag=’.
$check_etag.’)’); // Line 496

We always get ‘(cal_etag is NULL or cal_etag=1)’. The following update does
nothing for events that have an etag greater than 1, then we check affected
rows:

if ($check_etag && $this->db->affected_rows() < 1) // Line 506

Because affected_rows() returns 0, we return 0.

***** calendar.bocalupdate.save() *****
returns 0

***** calendar.bocalupdate.update() *****
returns 0

***** icalsrv.bocalupdate_vevents.import_vevent() *****
Returns an error because the update failed. Because the event wasn’t correctly
imported, it looks like it was removed by the client, and it gets deleted in
icalsrv_resourcehandler.

Hopefully this helps. I also hang out in #egroupware on FreeNode from 13:00 to
22:00 UTC if that would be easier

Thanks!

Hi Peter,
I did commit a fix for that behavior, in class.socal.inc.php
if ($cal_id && $check_etag && $check_modified)
in line 491
Somehow, someone took that out
if ($cal_id && $check_etag /&& $check_modified/)

the check_modified is a new optional parameter by the save function, if
this parameter is set, it is a save action caused by the ui, if not, … If
the parameter is set the saving of an event is done, regardless of the
etag.

Please test, and if it resolves the problem, please tell, and I will
recommit.
Best regards
Klaus

–
Peter Goerzen
I.S. Development
Hustler Turf Equipment/Excel Industries, Inc.

eGroupWare-developers mailing list
eGroupWare-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/egroupware-developers

Leithoff_Klaus 2008-04-01 15:29:40 UTC #4

I will have a look into that tomorrow.
I think this will help, on first sight it seems to be as detailed, as it can
be at this point.
Best regards
Klaus

“Peter Goerzen” schrieb:

Hi Klaus,

Thanks for the information. I tested with the change, but unfortunately it
did

not fix the problem. Here’s what’s happening as best as I can tell. My
apologies, as this isn’t very easy to communicate over email. . .

***** icalsrv.bocalupdate_vevents.import_vevent() *****
import_vevent() imports a VEVENT into an egw event and calls
calendar.bocalupdate.update():

if ($eidOk = $this->rsc->update($event, TRUE)){ // Line 907

***** calendar.bocalupdate *****
Note that $event[‘etag’] is not set at this point. Then update() calls
calendar.bocalupdate.save():

if (!($cal_id = $this->save($event))) // Line 274

$event[‘etag’] is still not set. In save(), $check_etag gets set:

$check_etag = ($event[‘etag’] ? $event[‘etag’]:1); // Line 706

Because $event[‘etag’] isn’t set, $check_etag = 1 always when coming
from
icalsrv. Then, save() calls calendar.socal.save():

if (($cal_id = $this->so->save($event,$set_recurrences,NULL,$check_etag))
&&

$set_recurrences && $event[‘recur_type’] != MCAL_RECUR_NONE) // Line
724

***** calendar.socal.save() *****
The result here is that in calendar.socal.save(), $check_modified always
gets
the value 1. Then, in calendar.socal.save(), we have:

$check_etag = ($check_modified ? $check_modified :
$event[‘cal_etag’]); // Line 490

$check_etag will always get set to 1, since $check_modified is always 1,
then:

if ($cal_id && $check_etag && $check_modified) // Line 491

This will always be true as long as $cal_id is set, which it is for
existing
events. Then:

$where = array(‘cal_id’ => $cal_id,’(cal_etag is NULL or cal_etag=’.
$check_etag.’)’); // Line 496

We always get ‘(cal_etag is NULL or cal_etag=1)’. The following update
does
nothing for events that have an etag greater than 1, then we check
affected
rows:

if ($check_etag && $this->db->affected_rows() < 1) // Line 506

Because affected_rows() returns 0, we return 0.

***** calendar.bocalupdate.save() *****
returns 0

***** calendar.bocalupdate.update() *****
returns 0

***** icalsrv.bocalupdate_vevents.import_vevent() *****
Returns an error because the update failed. Because the event wasn’t
correctly

imported, it looks like it was removed by the client, and it gets deleted
in
icalsrv_resourcehandler.

Hopefully this helps. I also hang out in #egroupware on FreeNode from
13:00 to

22:00 UTC if that would be easier

Thanks!

Hi Peter,
I did commit a fix for that behavior, in class.socal.inc.php
if ($cal_id && $check_etag && $check_modified)
in line 491
Somehow, someone took that out
if ($cal_id && $check_etag /&& $check_modified/)

the check_modified is a new optional parameter by the save function, if
this parameter is set, it is a save action caused by the ui, if not, …
If

the parameter is set the saving of an event is done, regardless of the
etag.

Please test, and if it resolves the problem, please tell, and I will
recommit.
Best regards
Klaus

–
Peter Goerzen
I.S. Development
Hustler Turf Equipment/Excel Industries, Inc.

Check out the new SourceForge.net Marketplace.
It’s the best place to buy or sell services for
just about anything Open Source.

http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

eGroupWare-developers mailing list
eGroupWare-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/egroupware-developers

eGroupWare-developers mailing list
eGroupWare-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/egroupware-developers

ChristophK 2008-04-03 10:07:50 UTC #5

Hello Klaus,

as I have the same problem, is there anything I can do, supplying you
with missing information or anything else?

Christoph

Klaus Leithoff schrieb:

Leithoff_Klaus 2008-04-03 12:39:16 UTC #6

Hi Christoph, Hi Peter,
please test again.
I commited a small patch to bocalupdate and the formerly noted patch to
socal.
Together, they should take care of the situation of a not set eTag. by
defaulting it to 0 in
bocalupdate, thus not triggering the etag update.
If the eTag is not set it will get set to 1 before the actual save of the
event.
If the event that is handed over to the save function is not carrying the
etag information,
the worst thing, that will happen, is that the etag counter will be reset to
one, by an update
that is caused by icalserver actions. this way, user, that work with the
event, will notice
that something has changed the event while they where working with it.
This is the theoretical part of it. Please test, and tell.
I will do the further testing on the UI Side of the issue.
Best regards
Klaus

“Christoph Kaulich” kaulich@gns-mbh.com schrieb:

Hello Klaus,

as I have the same problem, is there anything I can do, supplying you
with missing information or anything else?

Christoph

Klaus Leithoff schrieb:

I will have a look into that tomorrow.
I think this will help, on first sight it seems to be as detailed, as it
can

be at this point.
Best regards
Klaus

“Peter Goerzen” schrieb:

Hi Klaus,

Thanks for the information. I tested with the change, but unfortunately
it

did

not fix the problem. Here’s what’s happening as best as I can tell. My
apologies, as this isn’t very easy to communicate over email. . .

***** icalsrv.bocalupdate_vevents.import_vevent() *****
import_vevent() imports a VEVENT into an egw event and calls
calendar.bocalupdate.update():

if ($eidOk = $this->rsc->update($event, TRUE)){ // Line 907
***** calendar.bocalupdate *****
Note that $event[‘etag’] is not set at this point. Then update() calls
calendar.bocalupdate.save():

if (!($cal_id = $this->save($event))) // Line 274
$event[‘etag’] is still not set. In save(), $check_etag gets set:

$check_etag = ($event[‘etag’] ? $event[‘etag’]:1); // Line 706
Because $event[‘etag’] isn’t set, $check_etag = 1 always when coming
from
icalsrv. Then, save() calls calendar.socal.save():

if (($cal_id =
$this->so->save($event,$set_recurrences,NULL,$check_etag))

&&

$set_recurrences && $event[‘recur_type’] != MCAL_RECUR_NONE) // Line
724
***** calendar.socal.save() *****
The result here is that in calendar.socal.save(), $check_modified always
gets
the value 1. Then, in calendar.socal.save(), we have:

$check_etag = ($check_modified ? $check_modified :
$event[‘cal_etag’]); // Line 490

$check_etag will always get set to 1, since $check_modified is always 1,
then:

if ($cal_id && $check_etag && $check_modified) // Line 491
This will always be true as long as $cal_id is set, which it is for
existing
events. Then:

$where = array(‘cal_id’ => $cal_id,’(cal_etag is NULL or cal_etag=’.
$check_etag.’)’); // Line 496

We always get ‘(cal_etag is NULL or cal_etag=1)’. The following update
does
nothing for events that have an etag greater than 1, then we check
affected
rows:

if ($check_etag && $this->db->affected_rows() < 1) // Line 506
Because affected_rows() returns 0, we return 0.

***** calendar.bocalupdate.save() *****
returns 0

***** calendar.bocalupdate.update() *****
returns 0

***** icalsrv.bocalupdate_vevents.import_vevent() *****
Returns an error because the update failed. Because the event wasn’t
correctly
imported, it looks like it was removed by the client, and it gets
deleted

in

icalsrv_resourcehandler.

Hopefully this helps. I also hang out in #egroupware on FreeNode from
13:00 to
22:00 UTC if that would be easier

Thanks!

Hi Peter,
I did commit a fix for that behavior, in class.socal.inc.php
if ($cal_id && $check_etag && $check_modified)
in line 491
Somehow, someone took that out
if ($cal_id && $check_etag /&& $check_modified/)

the check_modified is a new optional parameter by the save function, if
this parameter is set, it is a save action caused by the ui, if not,
…

If

the parameter is set the saving of an event is done, regardless of the
etag.

Please test, and if it resolves the problem, please tell, and I will
recommit.
Best regards
Klaus
–
Peter Goerzen
I.S. Development
Hustler Turf Equipment/Excel Industries, Inc.

Check out the new SourceForge.net Marketplace.
It’s the best place to buy or sell services for
just about anything Open Source.

http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

eGroupWare-developers mailing list
eGroupWare-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/egroupware-developers

eGroupWare-developers mailing list
eGroupWare-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/egroupware-developers

ChristophK 2008-04-09 09:08:52 UTC #7

Hi Klaus,

it seems to be mor stable now. I use events.ics and tasks.ics as
splitted calendars and events didn’t get lost since the update. Butu
yesterday I suddenly lost some tasks, I can remember exactly what I was
doing before, but I will try to examine it.

Is the the possibility that something needsto be fixed in tasks as well?

Christoph

Klaus Leithoff schrieb:

Leithoff_Klaus 2008-04-09 09:32:55 UTC #8

Not sure about the task side.
Ralf did the handling of the “prevent concurring access/write” there.
He checks the/a timestamp (last change). He did not use extra fields for
that.
If the field used is not part of the protokoll, similar things as seen with
the
etag issue could happen I guess.
So we need more Info on your tests, to tell, if there is an issue.
Best Regards
Klaus

“Christoph Kaulich” kaulich@gns-mbh.com schrieb:

Hi Klaus,

it seems to be mor stable now. I use events.ics and tasks.ics as
splitted calendars and events didn’t get lost since the update. Butu
yesterday I suddenly lost some tasks, I can remember exactly what I was
doing before, but I will try to examine it.

Is the the possibility that something needsto be fixed in tasks as well?

Christoph

Klaus Leithoff schrieb:

Hi Christoph, Hi Peter,
please test again.
I commited a small patch to bocalupdate and the formerly noted patch to
socal.
Together, they should take care of the situation of a not set eTag. by
defaulting it to 0 in
bocalupdate, thus not triggering the etag update.
If the eTag is not set it will get set to 1 before the actual save of the
event.
If the event that is handed over to the save function is not carrying the
etag information,
the worst thing, that will happen, is that the etag counter will be reset
to

one, by an update
that is caused by icalserver actions. this way, user, that work with the
event, will notice
that something has changed the event while they where working with it.
This is the theoretical part of it. Please test, and tell.
I will do the further testing on the UI Side of the issue.
Best regards
Klaus

This SF.net email is sponsored by the 2008 JavaOne(SM) Conference
Don’t miss this year’s exciting event. There’s still time to save $100.
Use priority code J8TL2D2.

http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

eGroupWare-developers mailing list
eGroupWare-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/egroupware-developers

This SF.net email is sponsored by the 2008 JavaOne(SM) Conference
Don’t miss this year’s exciting event. There’s still time to save $100.
Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

eGroupWare-developers mailing list
eGroupWare-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/egroupware-developers