Closed
Bug 449401
Opened 16 years ago
Closed 16 years ago
storage provider does cleanly separate items of the same id across different calendars
Categories
(Calendar :: Provider: Local Storage, defect)
Calendar
Provider: Local Storage
Tracking
(Not tracked)
RESOLVED
FIXED
0.9
People
(Reporter: WSourdeau, Assigned: WSourdeau)
References
Details
Attachments
(2 files, 2 obsolete files)
22.10 KB,
patch
|
dbo
:
review+
|
Details | Diff | Splinter Review |
29.39 KB,
patch
|
dbo
:
review+
Fallen
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•16 years ago
|
Flags: blocking-calendar0.9?
Version: unspecified → Trunk
Updated•16 years ago
|
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
Assignee | ||
Comment 1•16 years ago
|
||
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 2•16 years ago
|
||
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 3•16 years ago
|
||
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•16 years ago
|
||
Nice work BTW, we should really fix that.
Assignee: nobody → WSourdeau
Flags: blocking-calendar0.9? → blocking-calendar0.9+
Assignee | ||
Comment 5•16 years ago
|
||
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?
Assignee | ||
Comment 6•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•16 years ago
|
||
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?
Assignee | ||
Updated•16 years ago
|
Attachment #332872 -
Attachment is obsolete: true
Attachment #332872 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #332884 -
Flags: review? → review?(daniel.boelzle)
Comment 8•16 years ago
|
||
(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?
Assignee | ||
Updated•16 years ago
|
Attachment #332872 -
Attachment is obsolete: false
Assignee | ||
Comment 9•16 years ago
|
||
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)
Assignee | ||
Comment 10•16 years ago
|
||
Comment on attachment 332872 [details] [diff] [review] final patch I revalidate this patch as final.
Attachment #332872 -
Flags: review?(philipp)
Assignee | ||
Updated•16 years ago
|
Attachment #332872 -
Flags: review?(philipp) → review?(daniel.boelzle)
Comment 11•16 years ago
|
||
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+
Comment 12•16 years ago
|
||
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+
Updated•16 years ago
|
Whiteboard: [patch in hand]
Comment 13•16 years ago
|
||
Comment on attachment 332954 [details] [diff] [review] final patch - v2 Requesting second review from Philipp.
Attachment #332954 -
Flags: review?(philipp)
Comment 14•16 years ago
|
||
Comment on attachment 332954 [details] [diff] [review] final patch - v2 r=philipp.
Attachment #332954 -
Flags: review?(philipp) → review+
Comment 15•16 years ago
|
||
Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: qawanted
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.
Description
•