Closed Bug 449401 Opened 15 years ago Closed 15 years ago
storage provider does cleanly separate items of the same id across different calendars
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; fr-FR; rv:18.104.22.168) 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.
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
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+
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
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.
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 #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.
Comment on attachment 332872 [details] [diff] [review] final patch I revalidate this patch as final.
Attachment #332872 - Flags: review?(philipp)
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+
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+
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
Resolution: --- → FIXED
Whiteboard: [patch in hand]
Target Milestone: --- → 0.9
You need to log in before you can comment on or make changes to this bug.