449401 - storage provider does cleanly separate items of the same id across different calendars

Status: RESOLVED FIXED

(Calendar :: Provider: Local Storage, defect)

Description

[Wolfgang Sourdeau]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; fr-FR; rv:1.9.0.1) Gecko/2008071420 Iceweasel/3.0.1 (Debian-3.0.1-1)
Build Identifier: 0.9pre

The SQL queries in calStorageCalendar.js do not always specify the cal_id. This causes an event shared across different calendar to be tied to a specific calendar and tend to overwrite the other copies.

I am working on a patch.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Wolfgang Sourdeau]

Attachment: proposed solution

Here are the changes involved in the patch:
- an update to the schema (version 13):
  - since cal_metadata was defined with item_id as unique, we need to transfer it to a new table
  - addition of cal_id to all tables containing event data
- all relevant queries modified to take the cal_id into account
- modified current queries to use this.mCalId instead of ":cal_id", which 2 advantages:
  - one less parameter to bind, since it will be constant during all the calendar lifetime
  - avoid binding errors. While many queries needed to be modified and I encountered unexplainable error messages from the mozStorage API, I found it easier and faster to do it that way instead. This probably reduces the overall risk of error.

Comment 2

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 332645 [details] [diff] [review]
proposed solution

>-            this.mDeleteEventExtras[i].params.cal_id = cal.mCalId;
>-            this.mDeleteTodoExtras[i].params.cal_id = cal.mCalId;
>-        this.mDeleteAllEvents.params.cal_id = cal.mCalId;
>-        this.mDeleteAllTodos.params.cal_id = cal.mCalId;

Why are these lines taken out? I'd assume the calId should be taken into account here?


r=philipp with explanation

Comment 3

[Daniel Boelzle [:dbo]]

Comment on attachment 332645 [details] [diff] [review]
proposed solution

Philipp, the calId is now hard-coded into the queries, which is fine. The calId must not change during a calendar object's life cycle.

>+                var updTables = [ "cal_attendees", "cal_recurrence",
>+                                  "cal_properties", "cal_attachments" ];
>+                for each (var updTable in updTables)
>+                    addColumn(this.mDB, updTable, "cal_id", "INTEGER");
>+
>+                // update schema
>+                this.mDB.executeSimpleSQL("DROP INDEX IF EXISTS" +
>+                                          " idx_cal_properies_item_id");
>+                this.mDB.executeSimpleSQL("CREATE INDEX IF NOT EXISTS " + 
>+                                          "idx_cal_properies_item_id " +
>+                                          "ON cal_properties(cal_id, item_id);");
>+

I am missing code actually filling the new cal_id columns.

Comment 4

[Daniel Boelzle [:dbo]]

Nice work BTW, we should really fix that.

Comment 5

[Wolfgang Sourdeau]

Attachment: final patch

This should be the final implementation, along with some bug fixes and the comment of Daniel taken into account:
- fixed a remaining call to a statement with a calId passed as parameter
- adding the cal_id fields to some table would generate an exception, because the correct schema would already have been created for those tables during previous upgrade steps
- the cal_id is populated in the tables where the column has been added with the item_id/cal_id pairs from cal_events and cal_todos

Comment 6

[Wolfgang Sourdeau]

For whatever reason, whenever a row is inserted into cal_metadata, the previous rows are deleted... I am looking into this and will probably provide yet another patch.

Comment 7

[Wolfgang Sourdeau]

Attachment: final patch

I found the problem... in deleteItemById, mDeleteMetaData was called. At first sight this seemed fine, so it might reveal a bug in the caldav provider instead. Since the caldav provider handles the metadata on its own (as the API is meant to), I have removed that call and that solved the problem.

Here is the latest patch, with this only fix. Unless something else is revealed, I consider this as the final patch for this bug.

Comment 8

[Daniel Boelzle [:dbo]]

(In reply to comment #7)
> I found the problem... in deleteItemById, mDeleteMetaData was called. At first
> sight this seemed fine, so it might reveal a bug in the caldav provider
> instead. Since the caldav provider handles the metadata on its own (as the API
> is meant to), I have removed that call and that solved the problem.

I think we shouldn't remove that, I added and documented that on purpose. Deleting an item should clean up its meta data as well. Sounds like the caldav improvement code needs fixing then, right?

Comment 9

[Wolfgang Sourdeau]

Comment on attachment 332884 [details] [diff] [review]
final patch

Patch cancelled. The previous one is correct.

Comment 10

[Wolfgang Sourdeau]

Comment on attachment 332872 [details] [diff] [review]
final patch

I revalidate this patch as final.

Comment 11

[Daniel Boelzle [:dbo]]

Comment on attachment 332872 [details] [diff] [review]
final patch

Thanks a lot, the patch looks good; r=dbo

One minor nit: I think we should condense the upgrade code (12->13) as much as possible, removing the _Db functions.

Comment 12

[Daniel Boelzle [:dbo]]

Attachment: final patch - v2

Done that, condensed the upgrade code a bit.
I'd request some more testing on this patch to avoid we run into further trouble (and further schema changes ;-) ). Otherwise I'll check this patch in beginning of the coming week.

Comment 13

[Daniel Boelzle [:dbo]]

Comment on attachment 332954 [details] [diff] [review]
final patch - v2

Requesting second review from Philipp.

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 332954 [details] [diff] [review]
final patch - v2

r=philipp.

Comment 15

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.

Comment 16

[Stefan Sitter [:ssitter]]

This checkin regressed Bug 450285.