1048769 - Move calendar metadata from the provider to item

Status: NEW

(Calendar :: Internal Components, defect)

Description

[Reid Anderson]

Attachment: metadata

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8467603 [details] [diff] [review]
metadata

Review of attachment 8467603 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/base/public/calIChangeLog.idl
@@ +79,5 @@
>       *
>       * @param id    an item id
>       * @param value an arbitrary string
>       */
> +    void setCalendarMetadata(in AUTF8String id,

We will want to make these calls asynchronous for the new interface, i.e.

void setCalendarMetadata(in AUTF8String id, in calIGenericOperationListener listener);

::: calendar/base/public/calIItemBase.idl
@@ +285,5 @@
> +   * except for providers during serialization and deserialization. The same
> +   * goes for properties of the returned object, they should be considered
> +   * readonly.
> +   */
> +  /* readonly jsval allMetadata;*/

The previous form was correct. allMetadata should be a known member on the interface (jsval allMetadata), but I had added the /* readonly */ comment to stress that this shouldn't be used for normal item access but instead only for (de-)serialization.

/* readonly */ jsval allMetadata;

::: calendar/base/src/calItemBase.js
@@ +271,5 @@
> +        if (!this.mParentItem) {
> +            m.mMetadata = {};
> +            for each (let [name,value] in Iterator(this.mMetadata)) {
> +                //m.mMetadata.setProperty(name,value);
> +                Object.defineProperty(m.mMetadata,name,{value});

Isn't setting m.mMetadata[name] = value sufficient?

::: calendar/providers/caldav/calDavCalendar.js
@@ +187,1 @@
>                      delete this.mItemInfoCache[itemId];

Ah yes, resetLog will be problematic.

To really delete the metadata of an item, you need to do all of the following steps. Just calling clearMetadata will only remove it from the in-memory version of the item.
  - calendar.getItem();
  - item.clearMetadata();
  - calendar.modifyItem();

This means a lot of indirection and code called just to delete the metadata. Maybe we can find a solution that doesn't require us to delete the item metadata at all, or at least more efficiently. 

I guess an extra storage calendar method (thats on the calIOfflineStorage interface) that clears all metadata from all items with an sql DELETE statement would do it.

The alternative is to check if the caldav provider can live without clearing the metadata, I'm not yet sure how that would work though.

@@ +248,5 @@
> +                //var dataString = [etag,path,(isInboxItem ? "true" : "false")].join("\u001A");
> +                //this.mOfflineStorage.setMetaData(id, dataString);
> +                item.setMetadata('etag',etag);
> +                item.setMetadata('path',path);
> +                item.setMetadata('isInboxItem',isInboxItem);

You can probably inline this function, putting the item.setMetadata() calls into the caller functions. You do need to make sure that the calling code saves the item in the mOfflineStorage afterwards though, otherwise the changes will just be on the item instance and not be persisted.

Nevertheless, try to keep the modifyItem calls on the offline storage low: instead of calling modifyItem each time the metadata is changed, check if somewhere down the code path modifyItem is not called anyway. If it is, you don't have to do an extra modifyItem call inbetween.

@@ +302,5 @@
> +            cal.getFreeBusyService().addProvider(this);
> +        }
> +
> +        let pcal = cal.async.promisifyCalendar(this.mOfflineStorage);
> +        let foundItems = (yield pcal.getItems(calICalendar.ITEM_FILTER_ALL_ITEMS,0, null, null));

This won't work. You are making the function a generator without using Task.async or Task.spawn and would therefore need to make the calling functions asynchronous too. See next comment for an alternative.

@@ +306,5 @@
> +        let foundItems = (yield pcal.getItems(calICalendar.ITEM_FILTER_ALL_ITEMS,0, null, null));
> +        for (let eachItem of foundItems) {
> +            let itemId = eachItem.id;
> +            let meta = eachItem.allMetadata;
> +            this.mItemInfoCache[itemId] = meta;

I haven't checked if there are any roadblocks, but the final goal is to get rid of mItemInfoCache, since its really just another store for metadata. This means use item.get/setMetadata() instead of accessing mItemInfoCache.

The only problem you may have is if there are code paths that access mItemInfoCache without actually having an item, but instead just the item id. We could solve this by adding an extra method to calIOfflineStorage that would retrieve just the metadata object for a given item id.

If this is easy to do I'd suggest getting rid of mItemInfoCache, otherwise I'm fine with doing this in a followup patch.

::: calendar/providers/memory/calMemoryCalendar.js
@@ +548,5 @@
> +        out_ids.value = [];
> +        out_values.value = [];
> +        for (let id in this.mItems) {
> +            out_ids.push(id);
> +            out_values.push(this.mItems[id].allMetadata);

I believe getAllMetaData expects out_values to be an array of strings? On the other hand, I don't know if there is value in JSON.stringifying the metadata, possibly the method needs to be changed? Check the callers to find out more.

::: calendar/providers/storage/calStorageCalendar.js
@@ +1204,5 @@
>       * @param aID           The id of the item to delete.
>       * @param aIsModify     If true, then leave in metadata for the item
>       */
>      deleteItemById: function cSC_deleteItemById(aId) {
> +        let statements = [this.mDeleteItem, this.mDeleteCalMetaData];

You probably don't want to delete the calendar metadata when an item is being deleted?

Comment 2

[Reid Anderson]

Attachment: itemMetadata

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8472329 [details] [diff] [review]
itemMetadata

Review of attachment 8472329 [details] [diff] [review]:
-----------------------------------------------------------------

Still too many comments for r+, but most of this stuff is minor. I haven't given it a test run yet, will do so in the next iteration.

::: calendar/base/public/calIChangeLog.idl
@@ +60,5 @@
> +     * @param aListener   where to call back the results
> +     *
> +     *
> +     */
> +     void clearAllItemMetadata(in calIOperationListener aListener);

Can we use calIGenericOperationListener here? There are quite a few other methods that also use calIOperationListener where I have the same question.

Also, one too many lines with only a star in the comment.

@@ +67,5 @@
> +     * Remove metadata from all items in this calendar.
> +     *
> +     * @param aListener   where to call back the results
> +     *
> +     *

Same extra line here.

@@ +79,5 @@
>   * @note
>   *   This interface is used in conjunction with changelog-based synchronization
> + *   and stores calendar metadata for this purpose. The metadata is stored as
> + *   long as the corresponding properties persist in the calendar and automatically
> + *   cleaned up up once the calendar is deleted. 

Whitespace at the end here.

@@ +93,5 @@
>       * @param value an arbitrary string
>       */
> +     void setCalendarMetadata(in AUTF8String id,
> +                      in AUTF8String value,
> +                      in calIOperationListener aListener);

Please align the arguments here.

@@ +105,3 @@
>  
>      /**
> +     * Gets the metadata of a calendar or null if there isn't any or the item id is invalid.

This line looks quite long, please wrap at 80 chars if needed.

::: calendar/base/public/calIItemBase.idl
@@ +286,5 @@
> +   * goes for properties of the returned object, they should be considered
> +   * readonly.
> +   */
> +  /* readonly */
> +  attribute jsval allMetadata;

Go ahead and format these two lines like this, it makes it look more like its actually readonly:

/* readonly */ attribute jsval allMetadata,

::: calendar/base/src/calItemBase.js
@@ +48,5 @@
>          this.wrappedJSObject = this;
>          this.mProperties = new calPropertyBag();
>          this.mPropertyParams = {};
>          this.mProperties.setProperty("CREATED", cal.jsDateToDateTime(new Date()));
> +        this.mMetadata = {};

Please replace {} with Object.create(null); here and in other places that mMetadata is initialized. This will create an object without the Object prototype, which means it won't have predefined keys like "hasOwnProperty".

Eventually we should do this in other places too like with mPropertyParams, but not in this bug. I'm also not quite sure if a js Map() is better, but for a simple key-value store an object without prototype might be faster.

@@ +270,5 @@
>  
> +        if (!this.mParentItem) {
> +            m.mMetadata = {};
> +            if (this.mMetadata) {
> +                for each (let [name,value] in Iterator(this.mMetadata)) {

for..of should work here. Iterator() is deprecated, but I don't think there is an alternative yet so lets leave it.

@@ +514,5 @@
> +    getMetadata: function cIB_getMetadata(aName) {
> +        if (this.mParentItem) {
> +            return this.mParentItem.getMetadata(aName);
> +        } else {
> +            aName = aName.toUpperCase();

I know I initially added this, and the reason was for parity with ical property names always being uppercase, but I wonder, do we really want to force uppercase here?

If you don't find a compelling reason to keep it, I'd suggest removing any uppercasing and making sure callers don't rely on it.

@@ +515,5 @@
> +        if (this.mParentItem) {
> +            return this.mParentItem.getMetadata(aName);
> +        } else {
> +            aName = aName.toUpperCase();
> +            return this.mMetadata[aName] || null;

Maybe we should drop the || null so it returns undefined instead.

::: calendar/providers/caldav/calDavCalendar.js
@@ +215,5 @@
>              if (this[property] !== undefined) {
>                  properties[property] = this[property];
>              }
>          }
> +        this.mOfflineStorage.setCalendarMetadata("calendar-properties", JSON.stringify(properties),null);

Spaces after commas

@@ +289,5 @@
> +
> +        this.mOfflineStorage.getCalendarMetadata("calendar-properties",getCalMetaListener);
> +
> +        let getItemMetaListener = {
> +            onResult: function meta_onResult(aStatus, aItems) {

You should add code to calAsyncUtils so that you can get a promise for methods that use calIGenericOperationListener, it would simplify code a bit here when you use Task.async().

@@ +293,5 @@
> +            onResult: function meta_onResult(aStatus, aItems) {
> +                for (let id in aItems) {
> +                    let itemId = id;
> +                    let metadata = aItems[id];
> +                    let etag = metadata["ETAG"];

since its an object you can also use metadata.etag and avoid the string creation.

@@ +1073,5 @@
>              let needsAddModify = false;
>              if (self.isCached) {
> +                item.setMetadata('etag',etag);
> +                item.setMetadata('path',path);
> +                item.setMetadata('isInboxItem',isInboxItem);

spaces after commas.

::: calendar/providers/memory/calMemoryCalendar.js
@@ +548,4 @@
>      },
> +
> +    getAllItemMetadata: function memory_getAllItemMetadata(aListener) {
> +        let returnItems = {};

I'd suggest renaming this variable, as you are not returning items but instead metadata. You should also use Object.create(null) here. Same in a few other places.

::: calendar/providers/storage/calStorageCalendar.js
@@ +576,5 @@
> +
> +    getAllItemMetadata: function(aListener) {
> +        let self = this;
> +        let returnItems = {};
> +        let processItems = Task.async(function*(query,action) {

You are not using action any more, please remove the parameter and fix the caller. Given this isn't called multiple times you should also consider inlining this function, i.e using Task.spawn to execute it directly:

Task.spawn(function*() {
  let metaData = Object.create(null);
  let selectMeta = this.runCalendarStatement(...);
  while (!selectMeta.isComplete) {
    ...
  }

  return metaData;
}.bind(this)).then((results) => {
  aListener.onResult(...NS_OK...);
}, (aReason) => {
  ...
  aListener.onResult(...NS_ERROR_FAILURE...);
});

@@ +577,5 @@
> +    getAllItemMetadata: function(aListener) {
> +        let self = this;
> +        let returnItems = {};
> +        let processItems = Task.async(function*(query,action) {
> +            while (query.isComplete == false) {

while (!query.isComplete)

@@ +578,5 @@
> +        let self = this;
> +        let returnItems = {};
> +        let processItems = Task.async(function*(query,action) {
> +            while (query.isComplete == false) {
> +                let aResultSet;

The "a" prefix is meant for arguments of functions, but this is a simple variable. Use: let resultSet;

@@ +589,5 @@
> +        });
> +
> +        let selectMeta = this.runCalendarStatement(this.mSelectAllItemMetadata,"Error selecting all item metadata");
> +
> +        processItems(selectMeta,cal.createEvent.bind(cal)).then(() => {

Space after comma.

@@ +815,5 @@
>                  "        :todo_due, :todo_due_tz, :todo_completed," +
>                  "        :todo_completed_tz)"
>              );
>  
> +            this.mSetCalMetaData = this.mDB.createStatement(

No need to shorten this, mSetCalendarMetaData should be fine. Same for other statements. Also, if these are called async then you need createAsyncStatement.

@@ +1283,5 @@
> +        let selectMeta = this.runItemStatement(this.mSelectCalMetaData, aId);
> +        selectMeta.promiseOneRow().then((row) => {
> +           if (row) {
> +            aListener.onOperationComplete(this, Components.results.NS_OK,
> +                                         cIOL.GET, aId, row.getResultByName("value"));

Please check indent here.

::: calendar/providers/storage/calStorageUpgrade.jsm
@@ +1962,5 @@
>      }
>      return tbl;
>  };
>  
> +upgrade.v24 = function upgrade_v24(db, version) {

The upgrader should get a comment with the bug and reason, as for the others. I hope we did this for the async patch.

@@ +1970,5 @@
> +    try {
> +        addColumn(tbl, "cal_item_base", ["metadata"], "TEXT", db);
> +
> +        if (db) {
> +        createFunction(db, "migratecaldav", 1, {

Something is wrong with indent here.

@@ +1973,5 @@
> +        if (db) {
> +        createFunction(db, "migratecaldav", 1, {
> +            onFunctionCall: function migratecaldav(storArgs) {
> +                try {
> +                    let [mappedArgs] = mapStorageArgs(storArgs);

Go ahead and rename mappedArgs to meta, as its an array of mapped args but actually the metadata value.

@@ +1975,5 @@
> +            onFunctionCall: function migratecaldav(storArgs) {
> +                try {
> +                    let [mappedArgs] = mapStorageArgs(storArgs);
> +                    let resultString = null;
> +                    if (mappedArgs.contains("\u001A")) {

We should probably also handle the case where the metadata is empty. In this case, we don't need to set itemData but can set an empty object. To DRY this up you could also define the obj outside of the let and JSON.stringify afterwards.

@@ +2002,5 @@
> +
> +        let fillColumnSQL = "UPDATE cal_item_base SET metadata =" +
> +                            " migratecaldav((SELECT value FROM cal_metadata WHERE" +
> +                            " cal_metadata.cal_id = cal_item_base.cal_id" +
> +                            " AND cal_metadata.item_id = cal_item_base.item_id))";

The SQL here and below could use some alignment. I like this style:

let fillColumnSQL = "UPDATE cal_item_base " +
                    "   SET metadata = migratecaldav(" + 
                    "       (SELECT value" +
                    "          FROM cal_metadata" +
                    "         WHERE cal_metadata.cal_id = cal_item_base.cal_id" +
                    "           AND cal_metadata.item_id = cal_item_base.item_id)" +
                    "   )";

::: calendar/test/unit/test_providers.js
@@ +362,1 @@
>  function testMetaData() {

What about testing the other metadata functions, i.e. the functions for getting all item metadata, or those around calendar metadata?

@@ +395,2 @@
>  
> +        aCalendar.addItem(event2,{

The addItem call also needs do_test_pending/do_test_finished. Alternatively, I wouldn't mind converting this function into a task and using calAsyncUtils. If you need it, the test harness has add_task(), which requires using run_next test at some point.

Comment 4

[Reid Anderson]

Attachment: itemMetadata_v2

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8474443 [details] [diff] [review]
itemMetadata_v2

Review of attachment 8474443 [details] [diff] [review]:
-----------------------------------------------------------------

My build is broken and I need to do a full rebuild, but here are some more minor comments in the meanwhile. I'd appreciate if you could fix these comments in a new patch. I also had some failed chunks when applying the patch, what patches need to be applied for this one to apply cleanly?

::: calendar/base/modules/calAsyncUtils.jsm
@@ +137,5 @@
>              }
> +        },
> +        promiseGenericOperationListener: function(deferred) {
> +            return {
> +                onResult: function(aStatus, aDetail) {

The first argument in the onResult call is not aStatus but instead aOperation. You need to check for Components.isSuccessCode(aOperation.status) instead.

@@ +138,5 @@
> +        },
> +        promiseGenericOperationListener: function(deferred) {
> +            return {
> +                onResult: function(aStatus, aDetail) {
> +                    if (!Components.isSuccessCode(aStatus)) {

Go ahead and turn around the if block here so you don't need the extra ! operation.

::: calendar/base/public/calIChangeLog.idl
@@ +80,5 @@
>   *   Finally, keep in mind that the meta data is "calendar local" and not
>   *   automatically transferred when storing the item on another calISyncWriteCalendar.
>   */
>  [scriptable, uuid(651e137b-2f3a-4595-af89-da51b6a37f85)]
>  interface calISyncWriteCalendar : calICalendar {

You are changing methods here, therefore you need to change the uuid of the interface.

::: calendar/providers/caldav/calDavCalendar.js
@@ +184,2 @@
>                      delete this.mItemInfoCache[itemId];
>                  }

You can simplify this code even more by just resetting mItemInfoCache as a whole, no need to delete each member.

::: calendar/providers/storage/calStorageCalendar.js
@@ +77,5 @@
>      deleteCalendar: function cSC_deleteCalendar(aCalendar, listener) {
>          let self = aCalendar.wrappedJSObject;
>  
>          let deleteItems = self.runCalendarStatement(self.mDeleteAllItems,"Error deleting all events").completionPromise;
> +        let deleteMeta = self.runCalendarStatement(self.mdeleteAllCalendarMetaData,"Error deleting all metadata").completionPromise;

mDelete...

@@ +596,5 @@
> +            }
> +        }, (aReason) => {
> +            cal.ERROR("Failed to get all item metadata: " + aReason);
> +            if (aListener) {
> +                aListener.onResult({status: Components.results.NS_ERROR_FAILURE}, null);

Might as well put aReason into the listener detail

@@ +838,5 @@
>              this.mDeleteAllItems = this.mDB.createStatement(
>                  "DELETE FROM cal_item_base WHERE cal_id = :cal_id"
>              );
>  
> +            this.mdeleteAllCalendarMetaData = this.mDB.createAsyncStatement(

mDelete...

Also, make sure you finalize this statement later on.

::: calendar/providers/storage/calStorageUpgrade.jsm
@@ +2019,5 @@
> +            executeSimpleSQL(db, fillColumnSQL);
> +
> +            let deleteOldMetadataSQL = "DELETE FROM cal_metadata " +
> +                                       "    WHERE EXISTS" +
> +                                       "        (SELECT 1" + 

I spotted a whitespace at the end here :)

::: calendar/test/unit/test_providers.js
@@ +425,5 @@
> +                        aCalendar.deleteCalendarMetadata("property1", {
> +                            onResult: function (aStatus, aItems) {
> +                                // Check to make sure it was deleted.
> +                                aCalendar.getCalendarMetadata("property1", {
> +                                    onResult: function (aStatus, aItems) {

What a wonderful pyramid :-) Since we have support for the generic listener in calAsyncUtils now, maybe it would be a good idea to convert this into a task? I'd be fine with doing this in a followup though.

Comment 6

[Reid Anderson]

Attachment: Patchv2.1

I'll push converting test_providers to using calAsyncUtils to another bug if that's alright. I've got the patch for Bug 1042125 and the Async patch applied, and it applies cleanly. I haven't done a full checkout and rebuild in a while though, so I can do one and see if there's been some other change that would cause the failed hunks.

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8477418 [details] [diff] [review]
Patchv2.1

Review of attachment 8477418 [details] [diff] [review]:
-----------------------------------------------------------------

I've figured out why it doesn't apply for me, I was using a later version of the async patch that had a few nits fixed. I will attach a new version that is compatible to async patch v7.

There is one failing test for me, test_providers has errors. One issue is a missing do_test_pending/finished, the other is a common problem with defining closures inside for() loops. The closure function has the wrong scope in this case. The solution is either to move the code to create the listener out of the loop and into a helper function, or use some means of making the for body a function itself (self-calling anonymous function, or Array.forEach). I've went with the last method here since it was easy and this test is a candidate to rewrite with calAsyncUtils anyway. I suspect this was already failing with the async patch, but I don't feel like modifying that patch again.


I've also taken care of the following nits in the patch I will upload.

::: calendar/base/modules/calAsyncUtils.jsm
@@ +140,5 @@
> +            return {
> +                onResult: function(aOperation, aDetail) {
> +                    if (Components.isSuccessCode(aOperation.status)) {
> +                        // Success of an ADD MODIFY or DELETE operation, resolve
> +                        // with the one item that was processed.

The comment isn't quite correct here, I think you can just remove it.

@@ +144,5 @@
> +                        // with the one item that was processed.
> +                        deferred.resolve(aDetail);
> +                    } else {
> +                        // This function has failed, reject with the status
> +                        deferred.reject(aStatus);

This should be aOperation.status

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Patchv2.2