Bug 1396897 - Don't use query parameters getter in sql related code. r=MakeMyDay
authorPhilipp Kewisch <mozilla@kewis.ch>
Fri, 06 Oct 2017 01:52:17 +0200
changeset 29114 91eec4d4f89b73547050d1e6bfbde7e40c90ba4a
parent 29113 55159bf4efc9b3960577e5c6f55e334a061a3723
child 29115 66b336c0c7f7165ed34ca5aecfe243acf2ba8f4b
push id2068
push userclokep@gmail.com
push dateMon, 13 Nov 2017 19:02:14 +0000
treeherdercomm-beta@9c7e7ce8672b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMakeMyDay
bugs1396897, 1392554
Bug 1396897 - Don't use query parameters getter in sql related code. r=MakeMyDay Over in bug 1392554, some internal changes to the sql statement parameters implementation were made which removed the getters for the properties. They can be set, but always return undefined if retrieved. This patch works around by not using the parmeters, and also aligns the memory calendar behavior with the storage calendar. MozReview-Commit-ID: 8NGZY7BODgf
calendar/base/modules/calAsyncUtils.jsm
calendar/providers/memory/calMemoryCalendar.js
calendar/providers/storage/calStorageCalendar.js
calendar/test/unit/test_gdata_provider.js
calendar/test/unit/test_providers.js
--- a/calendar/base/modules/calAsyncUtils.jsm
+++ b/calendar/base/modules/calAsyncUtils.jsm
@@ -19,23 +19,35 @@ var promisifyProxyHandler = {
         let deferred = PromiseUtils.defer();
         let listener = cal.async.promiseOperationListener(deferred);
         args.push(listener);
         target[name](...args);
         return deferred.promise;
     },
     get: function(target, name) {
         switch (name) {
+            // calICalendar methods
             case "adoptItem":
             case "addItem":
             case "modifyItem":
             case "deleteItem":
             case "getItem":
             case "getItems":
                 return (...args) => this.promiseOperation(target, name, args);
+            // calIOfflineStorage methods
+            case "addOfflineItem":
+            case "modifyOfflineItem":
+            case "deleteOfflineItem":
+            case "getOfflineItemFlag":
+            case "resetItemOfflineFlag": {
+                let offline = target.QueryInterface(Components.interfaces.calIOfflineStorage);
+                return (...args) => this.promiseOperation(offline, name, args);
+            }
+
+            // Special getAllItems shortcut
             case "getAllItems":
                 return () => this.promiseOperation(target, "getItems", [cIC.ITEM_FILTER_ALL_ITEMS, 0, null, null]);
             default:
                 return target[name];
         }
     }
 };
 
--- a/calendar/providers/memory/calMemoryCalendar.js
+++ b/calendar/providers/memory/calMemoryCalendar.js
@@ -421,33 +421,56 @@ calMemoryCalendar.prototype = {
         aRangeEnd = cal.ensureDateTime(aRangeEnd);
 
 
         let offline_filter = aItemFilter &
             (calICalendar.ITEM_FILTER_OFFLINE_DELETED |
              calICalendar.ITEM_FILTER_OFFLINE_CREATED |
              calICalendar.ITEM_FILTER_OFFLINE_MODIFIED);
 
+        let requestedFlag = 0;
+        if ((aItemFilter & calICalendar.ITEM_FILTER_OFFLINE_CREATED) != 0) {
+            requestedFlag = cICL.OFFLINE_FLAG_CREATED_RECORD;
+        } else if ((aItemFilter & calICalendar.ITEM_FILTER_OFFLINE_MODIFIED) != 0) {
+            requestedFlag = cICL.OFFLINE_FLAG_MODIFIED_RECORD;
+        } else if ((aItemFilter & calICalendar.ITEM_FILTER_OFFLINE_DELETED) != 0) {
+            requestedFlag = cICL.OFFLINE_FLAG_DELETED_RECORD;
+        }
+
+        let matchOffline = function(itemFlag, requestedFlag) {
+            // Same as storage calendar sql query. For comparison:
+            // requestedFlag is :offline_journal (parameter),
+            // itemFlag is offline_journal (field value)
+            // ...
+            // AND (:offline_journal IS NULL
+            // AND  (offline_journal IS NULL
+            //  OR   offline_journal != ${cICL.OFFLINE_FLAG_DELETED_RECORD}))
+            //  OR offline_journal == :offline_journal
+
+            return (!requestedFlag &&
+                    (!itemFlag ||
+                     itemFlag != cICL.OFFLINE_FLAG_DELETED_RECORD)) ||
+                   itemFlag == requestedFlag;
+        };
+
         cal.forEach(this.mItems, ([id, item]) => {
             let isEvent_ = cal.isEvent(item);
             if (isEvent_) {
                 if (!wantEvents) {
                     return cal.forEach.CONTINUE;
                 }
             } else if (!wantTodos) {
                 return cal.forEach.CONTINUE;
             }
 
             let hasItemFlag = (item.id in this.mOfflineFlags);
-            let offlineFlag = (hasItemFlag ? this.mOfflineFlags[item.id] : null);
+            let itemFlag = (hasItemFlag ? this.mOfflineFlags[item.id] : 0);
 
             // If the offline flag doesn't match, skip the item
-            if ((hasItemFlag ||
-                    (offline_filter != 0 && offlineFlag == cICL.OFFLINE_FLAG_DELETED_RECORD)) &&
-                (offlineFlag != offline_filter)) {
+            if (!matchOffline(itemFlag, requestedFlag)) {
                 return cal.forEach.CONTINUE;
             }
 
             if (itemReturnOccurrences && item.recurrenceInfo) {
                 let startDate = aRangeStart;
                 if (!aRangeStart && cal.isToDo(item)) {
                     startDate = item.entryDate;
                 }
--- a/calendar/providers/storage/calStorageCalendar.js
+++ b/calendar/providers/storage/calStorageCalendar.js
@@ -804,35 +804,36 @@ calStorageCalendar.prototype = {
 
             return false;
         }
 
         // First fetch all the events
         if (wantEvents) {
             let params;             // stmt params
             let resultItems = [];
+            let requestedOfflineJournal = null;
+
+            if (wantOfflineDeletedItems) {
+                requestedOfflineJournal = cICL.OFFLINE_FLAG_DELETED_RECORD;
+            } else if (wantOfflineCreatedItems) {
+                requestedOfflineJournal = cICL.OFFLINE_FLAG_CREATED_RECORD;
+            } else if (wantOfflineModifiedItems) {
+                requestedOfflineJournal = cICL.OFFLINE_FLAG_MODIFIED_RECORD;
+            }
 
             // first get non-recurring events that happen to fall within the range
             //
             try {
                 this.prepareStatement(this.mSelectNonRecurringEventsByRange);
                 params = this.mSelectNonRecurringEventsByRange.params;
                 params.range_start = startTime;
                 params.range_end = endTime;
                 params.start_offset = aRangeStart ? aRangeStart.timezoneOffset * USECS_PER_SECOND : 0;
                 params.end_offset = aRangeEnd ? aRangeEnd.timezoneOffset * USECS_PER_SECOND : 0;
-                params.offline_journal = null;
-
-                if (wantOfflineDeletedItems) {
-                    params.offline_journal = cICL.OFFLINE_FLAG_DELETED_RECORD;
-                } else if (wantOfflineCreatedItems) {
-                    params.offline_journal = cICL.OFFLINE_FLAG_CREATED_RECORD;
-                } else if (wantOfflineModifiedItems) {
-                    params.offline_journal = cICL.OFFLINE_FLAG_MODIFIED_RECORD;
-                }
+                params.offline_journal = requestedOfflineJournal;
 
                 while (this.mSelectNonRecurringEventsByRange.executeStep()) {
                     let row = this.mSelectNonRecurringEventsByRange.row;
                     resultItems.push(this.getEventFromRow(row, {}));
                 }
             } catch (e) {
                 this.logError("Error selecting non recurring events by range!\n", e);
             } finally {
@@ -845,52 +846,52 @@ calStorageCalendar.prototype = {
                 if (checkCount()) {
                     return;
                 }
             }
 
             // Process the recurring events from the cache
             for (let id in this.mRecEventCache) {
                 let evitem = this.mRecEventCache[id];
-                let offline_journal_flag = this.mRecEventCacheOfflineFlags[evitem.id] || null;
-                // No need to return flagged unless asked i.e. params.offline_journal == offline_journal_flag
-                // Return created and modified offline records if params.offline_journal is null alongwith events that have no flag
-                if ((params.offline_journal == null && offline_journal_flag != cICL.OFFLINE_FLAG_DELETED_RECORD) ||
-                    (params.offline_journal != null && offline_journal_flag == params.offline_journal)) {
+                let cachedJournalFlag = this.mRecEventCacheOfflineFlags[evitem.id] || null;
+                // No need to return flagged unless asked i.e. requestedOfflineJournal == cachedJournalFlag
+                // Return created and modified offline records if requestedOfflineJournal is null alongwith events that have no flag
+                if ((requestedOfflineJournal == null && cachedJournalFlag != cICL.OFFLINE_FLAG_DELETED_RECORD) ||
+                    (requestedOfflineJournal != null && cachedJournalFlag == requestedOfflineJournal)) {
                     count += handleResultItem(evitem, Components.interfaces.calIEvent);
                     if (checkCount()) {
                         return;
                     }
                 }
             }
         }
 
         // if todos are wanted, do them next
         if (wantTodos) {
             let params;             // stmt params
             let resultItems = [];
+            let requestedOfflineJournal = null;
+
+            if (wantOfflineCreatedItems) {
+                requestedOfflineJournal = cICL.OFFLINE_FLAG_CREATED_RECORD;
+            } else if (wantOfflineDeletedItems) {
+                requestedOfflineJournal = cICL.OFFLINE_FLAG_DELETED_RECORD;
+            } else if (wantOfflineModifiedItems) {
+                requestedOfflineJournal = cICL.OFFLINE_FLAG_MODIFIED_RECORD;
+            }
 
             // first get non-recurring todos that happen to fall within the range
             try {
                 this.prepareStatement(this.mSelectNonRecurringTodosByRange);
                 params = this.mSelectNonRecurringTodosByRange.params;
                 params.range_start = startTime;
                 params.range_end = endTime;
                 params.start_offset = aRangeStart ? aRangeStart.timezoneOffset * USECS_PER_SECOND : 0;
                 params.end_offset = aRangeEnd ? aRangeEnd.timezoneOffset * USECS_PER_SECOND : 0;
-                params.offline_journal = null;
-                if (wantOfflineCreatedItems) {
-                    params.offline_journal = cICL.OFFLINE_FLAG_CREATED_RECORD;
-                }
-                if (wantOfflineDeletedItems) {
-                    params.offline_journal = cICL.OFFLINE_FLAG_DELETED_RECORD;
-                }
-                if (wantOfflineModifiedItems) {
-                    params.offline_journal = cICL.OFFLINE_FLAG_MODIFIED_RECORD;
-                }
+                params.offline_journal = requestedOfflineJournal;
 
                 while (this.mSelectNonRecurringTodosByRange.executeStep()) {
                     let row = this.mSelectNonRecurringTodosByRange.row;
                     resultItems.push(this.getTodoFromRow(row, {}));
                 }
             } catch (e) {
                 this.logError("Error selecting non recurring todos by range", e);
             } finally {
@@ -907,23 +908,23 @@ calStorageCalendar.prototype = {
 
             // Note: Reading the code, completed *occurrences* seems to be broken, because
             //       only the parent item has been filtered; I fixed that.
             //       Moreover item.todo_complete etc seems to be a leftover...
 
             // process the recurring todos from the cache
             for (let id in this.mRecTodoCache) {
                 let todoitem = this.mRecTodoCache[id];
-                let offline_journal_flag = this.mRecTodoCacheOfflineFlags[todoitem.id] || null;
-                if ((params.offline_journal == null &&
-                     (offline_journal_flag == cICL.OFFLINE_FLAG_MODIFIED_RECORD ||
-                      offline_journal_flag == cICL.OFFLINE_FLAG_CREATED_RECORD ||
-                      offline_journal_flag == null)) ||
-                    (params.offline_journal != null &&
-                     (offline_journal_flag == params.offline_journal))) {
+                let cachedJournalFlag = this.mRecTodoCacheOfflineFlags[todoitem.id] || null;
+                if ((requestedOfflineJournal == null &&
+                     (cachedJournalFlag == cICL.OFFLINE_FLAG_MODIFIED_RECORD ||
+                      cachedJournalFlag == cICL.OFFLINE_FLAG_CREATED_RECORD ||
+                      cachedJournalFlag == null)) ||
+                    (requestedOfflineJournal != null &&
+                     (cachedJournalFlag == requestedOfflineJournal))) {
                     count += handleResultItem(todoitem,
                                               Components.interfaces.calITodo,
                                               checkCompleted);
                     if (checkCount()) {
                         return;
                     }
                 }
             }
--- a/calendar/test/unit/test_gdata_provider.js
+++ b/calendar/test/unit/test_gdata_provider.js
@@ -1175,21 +1175,20 @@ add_task(function* test_recurring_except
 
     let ex = items[0].recurrenceInfo.getExceptionFor(exIds[0]);
     equal(ex.title, "New Event changed");
 
     client.refresh();
     yield gServer.waitForLoad(client);
 
     items = yield pclient.getAllItems();
-    // Disabled for bug 1393704: Failed on the next line with 0 == 1:
-    // equal(items.length, 1);
+    equal(items.length, 1);
 
-    // exIds = items[0].recurrenceInfo.getExceptionIds({});
-    // equal(exIds.length, 0);
+    exIds = items[0].recurrenceInfo.getExceptionIds({});
+    equal(exIds.length, 0);
 
     gServer.resetClient(client);
 });
 
 add_task(function* test_recurring_cancelled_exception() {
     gServer.syncs = [{
         token: "1",
         events: [{
--- a/calendar/test/unit/test_providers.js
+++ b/calendar/test/unit/test_providers.js
@@ -1,12 +1,16 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+Components.utils.import("resource://calendar/modules/calAsyncUtils.jsm");
+
+const cIC = Components.interfaces.calICalendar;
+
 var icalStringArray = [
     // Comments refer to the range defined in testGetItems().
     // 1: one-hour event
     "BEGIN:VEVENT\n" +
     "DTSTART:20020402T114500Z\n" +
     "DTEND:20020402T124500Z\n" +
     "END:VEVENT\n",
     // 2: Test a zero-length event with DTSTART and DTEND
@@ -189,16 +193,23 @@ var icalStringArray = [
     // 36: one-day event with duration (after the range). See bug 390492.
     "BEGIN:VEVENT\n" +
     "DTSTART:20020403T000000Z\n" +
     "DURATION:P1D\n" +
     "END:VEVENT\n"
 ];
 
 function run_test() {
+    testIcalData();
+    testMetaData();
+
+    run_next_test();
+}
+
+function testIcalData() {
     // First entry is test number, second item is expected result for testGetItems().
     let wantedArray = [[1, 1],
                        [2, 1],
                        [3, 1],
                        [5, 0],
                        [6, 1],
                        [7, 1],
                        [8, 0],
@@ -351,18 +362,16 @@ function run_test() {
                         returnedItem = aItems[0];
                     }
                 }
             };
             // add item to calendar
             calendar.addItem(aItem, listener);
         }
     }
-
-    testMetaData();
 }
 
 function testMetaData() {
     function testMetaData_(aCalendar) {
         dump("testMetaData_() calendar type: " + aCalendar.type + "\n");
         let event1 = createEventFromIcalString("BEGIN:VEVENT\n" +
                                                "DTSTART;VALUE=DATE:20020402\n" +
                                                "END:VEVENT\n");
@@ -412,8 +421,91 @@ function testMetaData() {
         equal(count.value, 0);
 
         aCalendar.deleteMetaData("unknown"); // check graceful return
     }
 
     testMetaData_(getMemoryCal());
     testMetaData_(getStorageCal());
 }
+
+async function testOfflineStorage(storageGetter, isRecurring) {
+    let storage = storageGetter();
+    print(`Running offline storage test for ${storage.type} calendar for ${isRecurring ? "recurring" : "normal"} item`);
+    let pcal = cal.async.promisifyCalendar(storage);
+
+    let event1 = createEventFromIcalString("BEGIN:VEVENT\n" +
+                                           "DTSTART;VALUE=DATE:20020402\n" +
+                                           "DTEND;VALUE=DATE:20020403\n" +
+                                           "SUMMARY:event1\n" +
+                                           (isRecurring ? "RRULE:FREQ=DAILY;INTERVAL=1;COUNT=10\n" : "") +
+                                           "END:VEVENT\n");
+
+    event1 = await pcal.addItem(event1);
+
+    // Make sure the event is really in the calendar
+    let result = await pcal.getAllItems();
+    equal(result.length, 1);
+
+    // When searching for offline added items, there are none
+    let filter = cIC.ITEM_FILTER_ALL_ITEMS | cIC.ITEM_FILTER_OFFLINE_CREATED;
+    result = await pcal.getItems(filter, 0, null, null);
+    equal(result.length, 0);
+
+    // Mark the item as offline added
+    await pcal.addOfflineItem(event1);
+
+    // Now there should be an offline item
+    result = await pcal.getItems(filter, 0, null, null);
+    equal(result.length, 1);
+
+    let event2 = event1.clone();
+    event2.title = "event2";
+
+    event2 = await pcal.modifyItem(event2, event1);
+
+    await pcal.modifyOfflineItem(event2);
+
+    // The flag should still be offline added, as it was already marked as such
+    filter = cIC.ITEM_FILTER_ALL_ITEMS | cIC.ITEM_FILTER_OFFLINE_CREATED;
+    result = await pcal.getItems(filter, 0, null, null);
+    equal(result.length, 1);
+
+    // Reset the flag
+    await pcal.resetItemOfflineFlag(event2);
+
+    // No more offline items after resetting the flag
+    filter = cIC.ITEM_FILTER_ALL_ITEMS | cIC.ITEM_FILTER_OFFLINE_CREATED;
+    result = await pcal.getItems(filter, 0, null, null);
+    equal(result.length, 0);
+
+    // Setting modify flag without one set should actually set that flag
+    await pcal.modifyOfflineItem(event2);
+    filter = cIC.ITEM_FILTER_ALL_ITEMS | cIC.ITEM_FILTER_OFFLINE_CREATED;
+    result = await pcal.getItems(filter, 0, null, null);
+    equal(result.length, 0);
+
+    filter = cIC.ITEM_FILTER_ALL_ITEMS | cIC.ITEM_FILTER_OFFLINE_MODIFIED;
+    result = await pcal.getItems(filter, 0, null, null);
+    equal(result.length, 1);
+
+    // Setting the delete flag should modify the flag accordingly
+    await pcal.deleteOfflineItem(event2);
+    filter = cIC.ITEM_FILTER_ALL_ITEMS | cIC.ITEM_FILTER_OFFLINE_MODIFIED;
+    result = await pcal.getItems(filter, 0, null, null);
+    equal(result.length, 0);
+
+    filter = cIC.ITEM_FILTER_ALL_ITEMS | cIC.ITEM_FILTER_OFFLINE_DELETED;
+    result = await pcal.getItems(filter, 0, null, null);
+    equal(result.length, 1);
+
+    // Setting the delete flag on an offline added item should remove it
+    await pcal.resetItemOfflineFlag(event2);
+    await pcal.addOfflineItem(event2);
+    await pcal.deleteOfflineItem(event2);
+    result = await pcal.getAllItems();
+    equal(result.length, 0);
+}
+
+add_task(testOfflineStorage.bind(null, () => getMemoryCal(), false));
+add_task(testOfflineStorage.bind(null, () => getStorageCal(), false));
+add_task(testOfflineStorage.bind(null, () => getMemoryCal(), true));
+add_task(testOfflineStorage.bind(null, () => getStorageCal(), true));