Bug 1384696 - Query folders iteratively to prevent exceeding maximum variable count in a clause. r?Grisha draft
authorJing-wei Wu <topwu.tw@gmail.com>
Thu, 27 Jul 2017 10:42:45 +0800
changeset 648104 7e2f343e12a3e14c4926fd6d4ae9920e42f55cae
parent 648103 7f85cfb0c77c952066f546294c37a8bd8d38b606
child 648105 b769a3428786890095e86f85160820fe4685e9cb
push id74628
push userbmo:topwu.tw@gmail.com
push dateThu, 17 Aug 2017 08:32:13 +0000
reviewersGrisha
bugs1384696
milestone57.0a1
Bug 1384696 - Query folders iteratively to prevent exceeding maximum variable count in a clause. r?Grisha MozReview-Commit-ID: AXAxJbp152l
mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderBookmarksTest.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ -2376,45 +2376,57 @@ public class BrowserProvider extends Sha
                 }
             }
         } finally {
             cursor.close();
         }
 
         // Keep finding descendant GUIDs from parent IDs.
         while (!folderQueue.isEmpty()) {
-            // Store all parent IDs in a in clause, and can query their children at once.
-            final String[] inClauseArgs = new String[folderQueue.size()];
-            int count = 0;
-            while (folderQueue.peek() != null) {
-                final long id = folderQueue.poll();
-                inClauseArgs[count++] = String.valueOf(id);
-            }
-
-            final String inClause = DBUtils.computeSQLInClause(count, Bookmarks.PARENT);
-            // We only select distinct parent IDs.
-            final Cursor c = db.query(true, TABLE_BOOKMARKS,
-                                      new String[] { Bookmarks._ID, Bookmarks.TYPE, Bookmarks.GUID },
-                                      inClause, inClauseArgs, null, null, null, null);
-            if (c == null) {
-                continue;
-            }
-            try {
-                while (c.moveToNext()) {
-                    final int type = c.getInt(c.getColumnIndexOrThrow(Bookmarks.TYPE));
-                    if (type == Bookmarks.TYPE_FOLDER) {
-                        final long id = c.getLong(c.getColumnIndexOrThrow(Bookmarks._ID));
-                        folderQueue.add(id);
+            final int folderCount = folderQueue.size();
+            final int chunkCount = folderCount / DBUtils.SQLITE_MAX_VARIABLE_NUMBER;
+            for (int chunk = 0; chunk <= chunkCount; chunk++) {
+                final int chunkStart = chunk * DBUtils.SQLITE_MAX_VARIABLE_NUMBER;
+                int chunkEnd = (chunk + 1) * DBUtils.SQLITE_MAX_VARIABLE_NUMBER;
+                if (chunkEnd > folderCount) {
+                    chunkEnd = folderCount;
+                }
+
+                // Store all parent IDs in a in clause, and can query their children at once.
+                final int inClauseSize = chunkEnd - chunkStart;
+                final String[] inClauseArgs = new String[inClauseSize];
+                int count = 0;
+
+                for (int i = 0; i < inClauseSize; i++) {
+                    final long id = folderQueue.poll();
+                    inClauseArgs[count++] = String.valueOf(id);
+                }
+
+                final String inClause = DBUtils.computeSQLInClause(count, Bookmarks.PARENT);
+                // We only select distinct parent IDs.
+                final Cursor c = db.query(true, TABLE_BOOKMARKS,
+                                          new String[] { Bookmarks._ID, Bookmarks.TYPE, Bookmarks.GUID },
+                                          inClause, inClauseArgs, null, null, null, null);
+                if (c == null) {
+                    continue;
+                }
+                try {
+                    while (c.moveToNext()) {
+                        final int type = c.getInt(c.getColumnIndexOrThrow(Bookmarks.TYPE));
+                        if (type == Bookmarks.TYPE_FOLDER) {
+                            final long id = c.getLong(c.getColumnIndexOrThrow(Bookmarks._ID));
+                            folderQueue.add(id);
+                        }
+
+                        final String guid = c.getString(c.getColumnIndexOrThrow(Bookmarks.GUID));
+                        guids.add(guid);
                     }
-
-                    final String guid = c.getString(c.getColumnIndexOrThrow(Bookmarks.GUID));
-                    guids.add(guid);
+                } finally {
+                    c.close();
                 }
-            } finally {
-                c.close();
             }
         }
         return guids;
     }
 
     private int deleteFavicons(Uri uri, String selection, String[] selectionArgs) {
         debug("Deleting favicons for URI: " + uri);
 
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderBookmarksTest.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderBookmarksTest.java
@@ -141,32 +141,32 @@ public class BrowserProviderBookmarksTes
         // Insert a bunch of folders, triggering chunking logic.
         for (int i = 0; i < 1500; i++) {
             String guid = UUID.randomUUID().toString();
             insertBookmark(guid, "https://www.mozilla-3.org", guid,
                     parentId, BrowserContract.Bookmarks.TYPE_FOLDER);
         }
 
         // Insert a folder with a lot of children, triggering chunking logic.
-        final Uri parentUri2 = insertBookmark("big folder", null, UUID.randomUUID().toString(), rootId,
+        final Uri parentUri2 = insertBookmark("big folder", null, UUID.randomUUID().toString(), parentId,
                 BrowserContract.Bookmarks.TYPE_FOLDER);
         final long parentId2 = Long.parseLong(parentUri2.getLastPathSegment());
         for (int i = 0; i < 2000; i++) {
             String guid = UUID.randomUUID().toString();
             insertBookmark(guid, "https://www.mozilla-3.org", guid,
                     parentId2, BrowserContract.Bookmarks.TYPE_BOOKMARK);
         }
 
         changed = bookmarksClient.delete(bookmarksTestUri,
                 BrowserContract.Bookmarks.GUID + " = ?",
                 new String[] { parentGuid });
 
-        // 3504 = guid=parent, mobile root ('parent's parent), 2 regular folders, 1500 empty folders,
-        // 1 folder with 2000 children
-        assertEquals(3504, changed);
+        // 3505 = `1` parent folder (guid=parent), `1` mobile root ('parent's parent), `2` regular folders,
+        //        `1500` empty folders, and `1` big folder with `2000` children
+        assertEquals(3505, changed);
 
         final String parentGuid2 = "parent-2";
         final Uri parentUri3 = insertBookmark(parentGuid2, null, parentGuid2, rootId,
                 BrowserContract.Bookmarks.TYPE_FOLDER);
         final long parentId3 = Long.parseLong(parentUri3.getLastPathSegment());
 
         // Test that we can correctly delete a lot of bookmarks, triggering our "chunking logic".
         for (int i = 0; i < 3000; i++) {