Closed Bug 327974 Opened 14 years ago Closed 11 years ago

schema-*.sql has fallen out of sync with the provider


(Calendar :: Provider: Local Storage, defect)

Not set


(Not tracked)



(Reporter: dmose, Unassigned)




(6 files)

We should update it and start using it to generate the js file again, rather than the other way around.
This would be good to get done once we finalize v6 of the schema.

Vlad, do you mind if I take this bug off your hands?
Depends on: 333688
No response from Vlad in 7 months. Moving to Matt per comment 1.
Assignee: vladimir → lilmatt
Attached file schema-7.sql
Updated schema to currently used version 7 and created new schema-7.sql file that belongs to mozilla/calendar/providers/storage.
Assignee: lilmatt → ssitter
Attachment #265835 - Flags: review?(mvl)
Attached file schema-6.sql
schema-6.sql for documentation purpose only, comments omitted
Attached file schema-5.sql
schema-5.sql for documentation purpose only, comments omitted
Attached file schema-4.sql
schema-4.sql for documentation purpose only, comments omitted
Comment on attachment 265835 [details]

but this doesn't fix the second part of the bug-as-filed: using the file to generate the javascript. So either file a new bug, or don't close this one.
Attachment #265835 - Flags: review?(mvl) → review+
(In reply to comment #7)
One can create the javascript (var sqlTables in calStorageCalendar.js) from schema-7.sql via Currently there is no update to the schema required, the new file just documents the current schema.
Whiteboard: [checkin needed after 0.5]
Schema files checked in on branch and trunk. 
Stefan, mvl, can you please sort out whether this bug can be closed or not.
Whiteboard: [checkin needed after 0.5]
This is the result of the "perl schema-7.sql" run inserted into calStorageCalendar.js.
Attachment #270589 - Flags: review?(mvl)
This is the same patch as patch 270589 but with option -w for better readability.
Target Milestone: --- → 0.7
Comment on attachment 270589 [details] [diff] [review]
update generated javascript part

updating to reality: I won't be able to review this any time soon
Attachment #270589 - Flags: review?(mvl)
Comment on attachment 270592 [details] [diff] [review]
update generated javascript part (ignoring white space)

Moving review request to the last available peer for storage provider
Attachment #270592 - Flags: review?(daniel.boelzle)
IMO the coupling of calendar management and local calendar schmema is a flaw (bug 377845). My view on things is that calendar management needs to be separated out into the prefs (bug 378754, comment #1, had some conversation with mvl who agreed).
I know calStorageCalendar's tables e.g.

    /* 	REFERENCES, */
    "	cal_id		INTEGER, " +

currently effectively references tables maintained in the calendar management code, but this need not be the case (calStorageCalendar's mCalId is passed via URI).
So after all, I don't see any need for calendar management's tables in calStorageCalendar (i.e. schema-7.sql).
(In reply to comment #14)
This patch just updates/syncs the storage provider code to the current database schema as requested by mvl.

If the sql schema will be changed in the future the changes to the storage provider would be easier because you can just use the again with the new sql file.
I know, but IMO it's wrong to keep base/src/calCalendarManager.js' internal schema definitions in providers/storage. Apart from sharing the same database file, calendar management and storage provider should not have anything in common. That's what I wanted to say in comment #14.
If anybody convinces me, I have no problem with updating calStorageCalendar. But for the time being, I think it's undermining any decoupling of those code.
Comment on attachment 270592 [details] [diff] [review]
update generated javascript part (ignoring white space)

r- per comment #16; schema-?.sql files should IMO also not contain tables that are managed by base/src/calCalendarManager.js.
Attachment #270592 - Flags: review?(daniel.boelzle) → review-
The schema files are now up-to-date. Reassign back to default. If there is anything left to do feel free to take the bug. Otherwise it can be resolved fixed I guess. For new issues like what tables should be handled by what component new bugs should be filed.
Assignee: ssitter → nobody
Stefan, your comment reads like you are not convinced. Why do you think the "CREATE TABLE" statements for cal_calendars resp. cal_calendars_prefs should stay in the schema-?.sql files?
I don't think that anybody disagrees with the idea that those statements should move. But I think (and from what I read, stefan also) that moving that should be handled in a different bug. This bug is just to get the schema and js file in sync. Any other fixes should be in a different bug, because they require much more work and thought. It would not be good to make the simple parts of this bug be blocked by that.
Maybe I am missing something or you get me wrong:
The files are currently not in sync, because the schema-?.sql checkins (comment #9) contain "CREATE TABLE" statements for cal_calendars/cal_calendar_prefs that are not in calStorageCalendar.js. Thus this bug is not fixed.
Apart from that, the question I am repeatedly asking is why calStorageCalendar.js resp. schema-?.sql should contain those statements at all, because IMO those tables should be solely governed by base/src/calCalendarManager.js.
However, if somebody has valid reasons, I am fine and patch "update generated javascript part (ignoring white space)" should be r+. But if not, we should back out those statements from the schema-?.sql files.
From my understanding the sql files describe/comment the database schema used in the database file. Therefore it includes all elements of the database and not only parts of it. This could be useful e.g. for extension authors that need an outline of the schema used.
Target Milestone: 0.7 → ---
Version: Trunk → unspecified
Resolving as INVALID because the schema-*.sql files no longer exists. If we want a new sql file as reference for (add-on) developers, this should be done in a new bug.
Closed: 11 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.