Closed Bug 449401 Opened 15 years ago Closed 15 years ago

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

Categories

(Calendar :: Provider: Local Storage, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: WSourdeau, Assigned: WSourdeau)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Flags: blocking-calendar0.9?
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
Summary: the cachedcalendar does not manage copies of the same event across calendars → storage provider does cleanly separate items of the same id across different calendars
Version: Trunk → unspecified
Attached patch proposed solution (obsolete) β€” β€” Splinter Review
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.
Attachment #332645 - Flags: review?
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
Attachment #332645 - Flags: review? → review+
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.
Nice work BTW, we should really fix that.
Assignee: nobody → WSourdeau
Flags: blocking-calendar0.9? → blocking-calendar0.9+
Attached patch final patch β€” β€” Splinter Review
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
Attachment #332645 - Attachment is obsolete: true
Attachment #332872 - Flags: review?
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.
Status: NEW → ASSIGNED
Attached patch final patch (obsolete) β€” β€” Splinter Review
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.
Attachment #332884 - Flags: review?
Attachment #332872 - Attachment is obsolete: true
Attachment #332872 - Flags: review?
Attachment #332884 - Flags: review? → review?(daniel.boelzle)
(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?
Attachment #332872 - Attachment is obsolete: false
Comment on attachment 332884 [details] [diff] [review]
final patch

Patch cancelled. The previous one is correct.
Attachment #332884 - Attachment is obsolete: true
Attachment #332884 - Flags: review?(daniel.boelzle)
Comment on attachment 332872 [details] [diff] [review]
final patch

I revalidate this patch as final.
Attachment #332872 - Flags: review?(philipp)
Blocks: 437318
Attachment #332872 - Flags: review?(philipp) → review?(daniel.boelzle)
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.
Attachment #332872 - Flags: review?(daniel.boelzle) → review+
Attached patch final patch - v2 β€” β€” Splinter Review
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.
Attachment #332954 - Flags: review+
Keywords: qawanted
Whiteboard: [patch in hand]
Comment on attachment 332954 [details] [diff] [review]
final patch - v2

Requesting second review from Philipp.
Attachment #332954 - Flags: review?(philipp)
Comment on attachment 332954 [details] [diff] [review]
final patch - v2

r=philipp.
Attachment #332954 - Flags: review?(philipp) → review+
Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: qawanted
Resolution: --- → FIXED
Whiteboard: [patch in hand]
Target Milestone: --- → 0.9
This checkin regressed Bug 450285.
Depends on: 450285
No longer blocks: 437318
You need to log in before you can comment on or make changes to this bug.