From 5589445d005e387730ed9aa70e27dd3bd5cbfefc Mon Sep 17 00:00:00 2001 From: Richard Newman Date: Thu, 23 Feb 2012 08:14:05 -0800 Subject: [PATCH] Bug 718238 - Part 2: parenting and ordering of bookmarks. r=nalexander --- mobile/android/base/sync/Utils.java | 40 +- .../AndroidBrowserBookmarksDataAccessor.java | 86 ++- ...roidBrowserBookmarksRepositorySession.java | 723 ++++++++++++++---- ...ndroidBrowserHistoryRepositorySession.java | 7 +- ...roidBrowserPasswordsRepositorySession.java | 7 +- .../AndroidBrowserRepositoryDataAccessor.java | 25 +- .../AndroidBrowserRepositorySession.java | 46 +- .../sync/repositories/android/RepoUtils.java | 180 ----- 8 files changed, 726 insertions(+), 388 deletions(-) diff --git a/mobile/android/base/sync/Utils.java b/mobile/android/base/sync/Utils.java index 12ae3f7d2a7..24ab713228c 100644 --- a/mobile/android/base/sync/Utils.java +++ b/mobile/android/base/sync/Utils.java @@ -44,8 +44,9 @@ import java.math.BigInteger; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.security.SecureRandom; -import java.util.HashMap; +import java.util.ArrayList; import java.util.Locale; +import java.util.TreeMap; import org.mozilla.apache.commons.codec.binary.Base32; import org.mozilla.apache.commons.codec.binary.Base64; @@ -234,37 +235,12 @@ public class Utils { return context.getSharedPreferences(prefsPath, SHARED_PREFERENCES_MODE); } - /** - * Populate null slots in the provided array from keys in the provided Map. - * Set values in the map to be the new indices. - * - * @param dest - * @param source - * @throws Exception - */ - public static void fillArraySpaces(String[] dest, HashMap source) throws Exception { - int i = 0; - int c = dest.length; - int needed = source.size(); - if (needed == 0) { - return; - } - if (needed > c) { - throw new Exception("Need " + needed + " array spaces, have no more than " + c); - } - for (String key : source.keySet()) { - while (i < c) { - if (dest[i] == null) { - // Great! - dest[i] = key; - source.put(key, (long) i); - break; - } - ++i; - } - } - if (i >= c) { - throw new Exception("Could not fill array spaces."); + public static void addToIndexBucketMap(TreeMap> map, long index, String value) { + ArrayList bucket = map.get(index); + if (bucket == null) { + bucket = new ArrayList(); } + bucket.add(value); + map.put(index, bucket); } } diff --git a/mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksDataAccessor.java b/mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksDataAccessor.java index 5e19baee877..8d61d979f4f 100644 --- a/mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksDataAccessor.java +++ b/mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksDataAccessor.java @@ -38,10 +38,13 @@ package org.mozilla.gecko.sync.repositories.android; +import java.util.ArrayList; import java.util.HashMap; import org.json.simple.JSONArray; +import org.mozilla.gecko.sync.Logger; import org.mozilla.gecko.sync.repositories.NullCursorException; +import org.mozilla.gecko.sync.repositories.android.BrowserContract.Bookmarks; import org.mozilla.gecko.sync.repositories.domain.BookmarkRecord; import org.mozilla.gecko.sync.repositories.domain.Record; @@ -49,7 +52,6 @@ import android.content.ContentValues; import android.content.Context; import android.database.Cursor; import android.net.Uri; -import android.util.Log; public class AndroidBrowserBookmarksDataAccessor extends AndroidBrowserRepositoryDataAccessor { @@ -78,16 +80,53 @@ public class AndroidBrowserBookmarksDataAccessor extends AndroidBrowserRepositor return BrowserContract.Bookmarks.CONTENT_URI; } + protected Uri getPositionsUri() { + return BrowserContract.Bookmarks.POSITIONS_CONTENT_URI; + } + protected Cursor getGuidsIDsForFolders() throws NullCursorException { // Exclude "places" and "tags", in case they've ended up in the DB. String where = BOOKMARK_IS_FOLDER + " AND " + GUID_NOT_TAGS_OR_PLACES; return queryHelper.safeQuery(".getGuidsIDsForFolders", null, where, null, null); } + /** + * Issue a request to the Content Provider to update the positions of the + * records named by the provided GUIDs to the index of their GUID in the + * provided array. + * + * @param childArray + * A sequence of GUID strings. + */ + public int updatePositions(ArrayList childArray) { + Logger.debug(LOG_TAG, "Updating positions for " + childArray.size() + " items."); + String[] args = childArray.toArray(new String[childArray.size()]); + return context.getContentResolver().update(getPositionsUri(), new ContentValues(), null, args); + } + + /** + * Bump the modified time of a record by ID. + * + * @param id + * @param modified + * @return + */ + public int bumpModified(long id, long modified) { + Logger.debug(LOG_TAG, "Bumping modified for " + id + " to " + modified); + String where = Bookmarks._ID + " = ?"; + String[] selectionArgs = new String[] { String.valueOf(id) }; + ContentValues values = new ContentValues(); + values.put(Bookmarks.DATE_MODIFIED, modified); + + return context.getContentResolver().update(getUri(), values, where, selectionArgs); + } + protected void updateParentAndPosition(String guid, long newParentId, long position) { ContentValues cv = new ContentValues(); cv.put(BrowserContract.Bookmarks.PARENT, newParentId); - cv.put(BrowserContract.Bookmarks.POSITION, position); + if (position >= 0) { + cv.put(BrowserContract.Bookmarks.POSITION, position); + } updateByGuid(guid, cv); } @@ -96,12 +135,13 @@ public class AndroidBrowserBookmarksDataAccessor extends AndroidBrowserRepositor * Insert them if they aren't there. */ public void checkAndBuildSpecialGuids() throws NullCursorException { - Cursor cur = fetch(RepoUtils.SPECIAL_GUIDS); + final String[] specialGUIDs = AndroidBrowserBookmarksRepositorySession.SPECIAL_GUIDS; + Cursor cur = fetch(specialGUIDs); long mobileRoot = 0; long desktopRoot = 0; // Map from GUID to whether deleted. Non-presence implies just that. - HashMap statuses = new HashMap(RepoUtils.SPECIAL_GUIDS.length); + HashMap statuses = new HashMap(specialGUIDs.length); try { if (cur.moveToFirst()) { while (!cur.isAfterLast()) { @@ -123,11 +163,11 @@ public class AndroidBrowserBookmarksDataAccessor extends AndroidBrowserRepositor } // Insert or undelete them if missing. - for (String guid : RepoUtils.SPECIAL_GUIDS) { + for (String guid : specialGUIDs) { if (statuses.containsKey(guid)) { if (statuses.get(guid)) { // Undelete. - Log.i(LOG_TAG, "Undeleting special GUID " + guid); + Logger.info(LOG_TAG, "Undeleting special GUID " + guid); ContentValues cv = new ContentValues(); cv.put(BrowserContract.SyncColumns.IS_DELETED, 0); updateByGuid(guid, cv); @@ -135,12 +175,15 @@ public class AndroidBrowserBookmarksDataAccessor extends AndroidBrowserRepositor } else { // Insert. if (guid.equals("mobile")) { - Log.i(LOG_TAG, "No mobile folder. Inserting one."); + Logger.info(LOG_TAG, "No mobile folder. Inserting one."); mobileRoot = insertSpecialFolder("mobile", 0); } else if (guid.equals("places")) { + // This is awkward. + Logger.info(LOG_TAG, "No places root. Inserting one under mobile (" + mobileRoot + ")."); desktopRoot = insertSpecialFolder("places", mobileRoot); } else { // unfiled, menu, toolbar. + Logger.info(LOG_TAG, "No " + guid + " root. Inserting one under places (" + desktopRoot + ")."); insertSpecialFolder(guid, desktopRoot); } } @@ -149,7 +192,7 @@ public class AndroidBrowserBookmarksDataAccessor extends AndroidBrowserRepositor private long insertSpecialFolder(String guid, long parentId) { BookmarkRecord record = new BookmarkRecord(guid); - record.title = RepoUtils.SPECIAL_GUIDS_MAP.get(guid); + record.title = AndroidBrowserBookmarksRepositorySession.SPECIAL_GUIDS_MAP.get(guid); record.type = "folder"; record.androidParentID = parentId; return(RepoUtils.getAndroidIdFromUri(insert(record))); @@ -180,12 +223,31 @@ public class AndroidBrowserBookmarksDataAccessor extends AndroidBrowserRepositor return cv; } - // Returns a cursor with any records that list the given androidID as a parent + /** + * Returns a cursor over non-deleted records that list the given androidID as a parent. + */ public Cursor getChildren(long androidID) throws NullCursorException { - String where = BrowserContract.Bookmarks.PARENT + " = ?"; - String[] args = new String[] { String.valueOf(androidID) }; - return queryHelper.safeQuery(".getChildren", getAllColumns(), where, args, null); + return getChildren(androidID, false); } + + /** + * Returns a cursor with any records that list the given androidID as a parent. + * Excludes 'places', and optionally any deleted records. + */ + public Cursor getChildren(long androidID, boolean includeDeleted) throws NullCursorException { + final String where = BrowserContract.Bookmarks.PARENT + " = ? AND " + + BrowserContract.SyncColumns.GUID + " <> ? " + + (!includeDeleted ? ("AND " + BrowserContract.SyncColumns.IS_DELETED + " = 0") : ""); + + final String[] args = new String[] { String.valueOf(androidID), "places" }; + + // Order by position, falling back on creation date and ID. + final String order = BrowserContract.Bookmarks.POSITION + ", " + + BrowserContract.SyncColumns.DATE_CREATED + ", " + + BrowserContract.Bookmarks._ID; + return queryHelper.safeQuery(".getChildren", getAllColumns(), where, args, order); + } + @Override protected String[] getAllColumns() { diff --git a/mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java b/mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java index 7df6cee5f6f..f397ff4cdcb 100644 --- a/mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java +++ b/mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java @@ -1,46 +1,18 @@ -/* ***** BEGIN LICENSE BLOCK ***** - * Version: MPL 1.1/GPL 2.0/LGPL 2.1 - * - * The contents of this file are subject to the Mozilla Public License Version - * 1.1 (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * http://www.mozilla.org/MPL/ - * - * Software distributed under the License is distributed on an "AS IS" basis, - * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License - * for the specific language governing rights and limitations under the - * License. - * - * The Original Code is Android Sync Client. - * - * The Initial Developer of the Original Code is - * the Mozilla Foundation. - * Portions created by the Initial Developer are Copyright (C) 2011 - * the Initial Developer. All Rights Reserved. - * - * Contributor(s): - * Jason Voll - * - * Alternatively, the contents of this file may be used under the terms of - * either the GNU General Public License Version 2 or later (the "GPL"), or - * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), - * in which case the provisions of the GPL or the LGPL are applicable instead - * of those above. If you wish to allow use of your version of this file only - * under the terms of either the GPL or the LGPL, and not to allow others to - * use your version of this file under the terms of the MPL, indicate your - * decision by deleting the provisions above and replace them with the notice - * and other provisions required by the GPL or the LGPL. If you do not delete - * the provisions above, a recipient may use your version of this file under - * the terms of any one of the MPL, the GPL or the LGPL. - * - * ***** END LICENSE BLOCK ***** */ +/* 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/. */ package org.mozilla.gecko.sync.repositories.android; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; +import java.util.Map; +import java.util.Map.Entry; +import java.util.TreeMap; import org.json.simple.JSONArray; +import org.mozilla.gecko.R; import org.mozilla.gecko.sync.Logger; import org.mozilla.gecko.sync.Utils; import org.mozilla.gecko.sync.repositories.NoGuidForIdException; @@ -61,11 +33,139 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo private HashMap guidToID = new HashMap(); private HashMap idToGuid = new HashMap(); + /** + * Some notes on reparenting/reordering. + * + * Fennec stores new items with a high-negative position, because it doesn't care. + * On the other hand, it also doesn't give us any help managing positions. + * + * We can process records and folders in any order, though we'll usually see folders + * first because their sortindex is larger. + * + * We can also see folders that refer to children we haven't seen, and children we + * won't see (perhaps due to a TTL, perhaps due to a limit on our fetch). + * + * And of course folders can refer to local children (including ones that might + * be reconciled into oblivion!), or local children in other folders. And the local + * version of a folder -- which might be a reconciling target, or might not -- can + * have local additions or removals. (That causes complications with on-the-fly + * reordering: we don't know in advance which records will even exist by the end + * of the sync.) + * + * We opt to leave records in a reasonable state as we go, applying reordering/ + * reparenting operations whenever possible. A final sequence is applied after all + * incoming records have been handled. + * + * As such, we need to track a bunch of stuff as we go: + * + * • For each downloaded folder, the array of children. These will be server GUIDs, + * but not necessarily identical to the remote list: if we download a record and + * it's been locally moved, it must be removed from this child array. + * + * This mapping can be discarded when final reordering has occurred, either on + * store completion or when every child has been seen within this session. + * + * • A list of orphans: records whose parent folder does not yet exist. This can be + * trimmed as orphans are reparented. + * + * • Mappings from folder GUIDs to folder IDs, so that we can parent items without + * having to look in the DB. Of course, this must be kept up-to-date as we + * reconcile. + * + * Reordering also needs to occur during fetch. That is, a folder might have been + * created locally, or modified locally without any remote changes. An order must + * be generated for the folder's children array, and it must be persisted into the + * database to act as a starting point for future changes. But of course we don't + * want to incur a database write if the children already have a satisfactory order. + * + * Do we also need a list of "adopters", parents that are still waiting for children? + * As items get picked out of the orphans list, we can do on-the-fly ordering, until + * we're left with lonely records at the end. + * + * As we modify local folders, perhaps by moving children out of their purview, we + * must bump their modification time so as to cause them to be uploaded on the next + * stage of syncing. The same applies to simple reordering. + */ + + // TODO: can we guarantee serial access to these? private HashMap> missingParentToChildren = new HashMap>(); - private HashMap parentToChildArray = new HashMap(); - private AndroidBrowserBookmarksDataAccessor dataAccessor; + private HashMap parentToChildArray = new HashMap(); private int needsReparenting = 0; + private AndroidBrowserBookmarksDataAccessor dataAccessor; + + /** + * An array of known-special GUIDs. + */ + public static String[] SPECIAL_GUIDS = new String[] { + // Mobile and desktop places roots have to come first. + "mobile", + "places", + "toolbar", + "menu", + "unfiled" + }; + + /** + * = A note about folder mapping = + * + * Note that _none_ of Places's folders actually have a special GUID. They're all + * randomly generated. Special folders are indicated by membership in the + * moz_bookmarks_roots table, and by having the parent `1`. + * + * Additionally, the mobile root is annotated. In Firefox Sync, PlacesUtils is + * used to find the IDs of these special folders. + * + * Sync skips over `places` and `tags` when finding IDs. + * + * We need to consume records with these various guids, producing a local + * representation which we are able to stably map upstream. + * + * That is: + * + * * We should not upload a `places` record or a `tags` record. + * * We can stably _store_ menu/toolbar/unfiled/mobile as special GUIDs, and set + * their parent ID as appropriate on upload. + * + * + * = Places folders = + * + * guid root_name folder_id parent + * ---------- ---------- ---------- ---------- + * ? places 1 0 + * ? menu 2 1 + * ? toolbar 3 1 + * ? tags 4 1 + * ? unfiled 5 1 + * + * ? mobile* 474 1 + * + * + * = Fennec folders = + * + * guid folder_id parent + * ---------- ---------- ---------- + * mobile ? 0 + * + */ + public static final Map SPECIAL_GUID_PARENTS; + static { + HashMap m = new HashMap(); + m.put("places", null); + m.put("menu", "places"); + m.put("toolbar", "places"); + m.put("tags", "places"); + m.put("unfiled", "places"); + m.put("mobile", "places"); + SPECIAL_GUID_PARENTS = Collections.unmodifiableMap(m); + } + + /** + * A map of guids to their localized name strings. + */ + // Oh, if only we could make this final and initialize it in the static initializer. + public static Map SPECIAL_GUIDS_MAP; + /** * Return true if the provided record GUID should be skipped * in child lists or fetch results. @@ -81,7 +181,17 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo public AndroidBrowserBookmarksRepositorySession(Repository repository, Context context) { super(repository); - RepoUtils.initialize(context); + + if (SPECIAL_GUIDS_MAP == null) { + HashMap m = new HashMap(); + m.put("menu", context.getString(R.string.bookmarks_folder_menu)); + m.put("places", context.getString(R.string.bookmarks_folder_places)); + m.put("toolbar", context.getString(R.string.bookmarks_folder_toolbar)); + m.put("unfiled", context.getString(R.string.bookmarks_folder_unfiled)); + m.put("mobile", context.getString(R.string.bookmarks_folder_mobile)); + SPECIAL_GUIDS_MAP = Collections.unmodifiableMap(m); + } + dbHelper = new AndroidBrowserBookmarksDataAccessor(context); dataAccessor = (AndroidBrowserBookmarksDataAccessor) dbHelper; } @@ -96,6 +206,15 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo return guid; } + private long getIDForGUID(String guid) { + Long id = guidToID.get(guid); + if (id == null) { + Logger.warn(LOG_TAG, "Couldn't find local ID for GUID " + guid); + return -1; + } + return id.longValue(); + } + private String getGUID(Cursor cur) { return RepoUtils.getStringFromCursor(cur, "guid"); } @@ -104,12 +223,20 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo return RepoUtils.getLongFromCursor(cur, BrowserContract.Bookmarks.PARENT); } + // More efficient for bulk operations. + private long getPosition(Cursor cur, int positionIndex) { + return cur.getLong(positionIndex); + } + private long getPosition(Cursor cur) { + return RepoUtils.getLongFromCursor(cur, BrowserContract.Bookmarks.POSITION); + } + private String getParentName(String parentGUID) throws ParentNotFoundException, NullCursorException { if (parentGUID == null) { return ""; } - if (RepoUtils.SPECIAL_GUIDS_MAP.containsKey(parentGUID)) { - return RepoUtils.SPECIAL_GUIDS_MAP.get(parentGUID); + if (SPECIAL_GUIDS_MAP.containsKey(parentGUID)) { + return SPECIAL_GUIDS_MAP.get(parentGUID); } // Get parent name from database. @@ -130,74 +257,116 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo return parentName; } + /** + * Retrieve the child array for a record, repositioning and updating the database as necessary. + * + * @param folderID + * The database ID of the folder. + * @param persist + * True if generated positions should be written to the database. The modified + * time of the parent folder is only bumped if this is true. + * @return + * An array of GUIDs. + * @throws NullCursorException + */ @SuppressWarnings("unchecked") - private JSONArray getChildren(long androidID) throws NullCursorException { - trace("Calling getChildren for androidID " + androidID); + private JSONArray getChildrenArray(long folderID, boolean persist) throws NullCursorException { + trace("Calling getChildren for androidID " + folderID); JSONArray childArray = new JSONArray(); - Cursor children = dataAccessor.getChildren(androidID); + Cursor children = dataAccessor.getChildren(folderID); try { if (!children.moveToFirst()) { trace("No children: empty cursor."); return childArray; } + final int positionIndex = children.getColumnIndex(BrowserContract.Bookmarks.POSITION); + final int count = children.getCount(); + Logger.debug(LOG_TAG, "Expecting " + count + " children."); - int count = children.getCount(); - String[] kids = new String[count]; - trace("Expecting " + count + " children."); + // Sorted by requested position. + TreeMap> guids = new TreeMap>(); - // Track badly positioned records. - // TODO: use a mechanism here that preserves ordering. - HashMap broken = new HashMap(); - - // Get children into array in correct order. while (!children.isAfterLast()) { - String childGuid = getGUID(children); + final String childGuid = getGUID(children); + final long childPosition = getPosition(children, positionIndex); trace(" Child GUID: " + childGuid); - int childPosition = (int) RepoUtils.getLongFromCursor(children, BrowserContract.Bookmarks.POSITION); trace(" Child position: " + childPosition); - if (childPosition >= count) { - Logger.warn(LOG_TAG, "Child position " + childPosition + " greater than expected children " + count); - broken.put(childGuid, 0L); - } else { - String existing = kids[childPosition]; - if (existing != null) { - Logger.warn(LOG_TAG, "Child position " + childPosition + " already occupied! (" + - childGuid + ", " + existing + ")"); - broken.put(childGuid, 0L); - } else { - kids[childPosition] = childGuid; - } - } + Utils.addToIndexBucketMap(guids, Math.abs(childPosition), childGuid); children.moveToNext(); } - try { - Utils.fillArraySpaces(kids, broken); - } catch (Exception e) { - Logger.error(LOG_TAG, "Unable to reposition children to yield a valid sequence. Data loss may result.", e); - } - // TODO: now use 'broken' to edit the records on disk. + // This will suffice for taking a jumble of records and indices and + // producing a sorted sequence that preserves some kind of order -- + // from the abs of the position, falling back on cursor order (that + // is, creation time and ID). + // Note that this code is not intended to merge values from two sources! + boolean changed = false; + int i = 0; + for (Entry> entry : guids.entrySet()) { + long pos = entry.getKey().longValue(); + int atPos = entry.getValue().size(); - // Collect into a more friendly data structure. - for (int i = 0; i < count; ++i) { - String kid = kids[i]; - if (forbiddenGUID(kid)) { - continue; + // If every element has a different index, and the indices are + // in strict natural order, then changed will be false. + if (atPos > 1 || pos != i) { + changed = true; + } + for (String guid : entry.getValue()) { + if (!forbiddenGUID(guid)) { + childArray.add(guid); + } } - childArray.add(kid); } + if (Logger.logVerbose(LOG_TAG)) { // Don't JSON-encode unless we're logging. Logger.trace(LOG_TAG, "Output child array: " + childArray.toJSONString()); } + + if (!changed) { + Logger.debug(LOG_TAG, "Nothing moved! Database reflects child array."); + return childArray; + } + + if (!persist) { + return childArray; + } + + Logger.debug(LOG_TAG, "Generating child array required moving records. Updating DB."); + final long time = now(); + if (0 < dataAccessor.updatePositions(childArray)) { + Logger.debug(LOG_TAG, "Bumping parent time to " + time + "."); + dataAccessor.bumpModified(folderID, time); + } } finally { children.close(); } + return childArray; } + protected static boolean isDeleted(Cursor cur) { + return RepoUtils.getLongFromCursor(cur, BrowserContract.SyncColumns.IS_DELETED) != 0; + } + @Override - protected Record recordFromMirrorCursor(Cursor cur) throws NoGuidForIdException, NullCursorException, ParentNotFoundException { + protected Record retrieveDuringStore(Cursor cur) throws NoGuidForIdException, NullCursorException, ParentNotFoundException { + // During storing of a retrieved record, we never care about the children + // array that's already present in the database -- we don't use it for + // reconciling. Skip all that effort for now. + return retrieveRecord(cur, false); + } + + @Override + protected Record retrieveDuringFetch(Cursor cur) throws NoGuidForIdException, NullCursorException, ParentNotFoundException { + return retrieveRecord(cur, true); + } + + /** + * Build a record from a cursor, with a flag to dictate whether the + * children array should be computed and written back into the database. + */ + protected BookmarkRecord retrieveRecord(Cursor cur, boolean computeAndPersistChildren) throws NoGuidForIdException, NullCursorException, ParentNotFoundException { String recordGUID = getGUID(cur); Logger.trace(LOG_TAG, "Record from mirror cursor: " + recordGUID); @@ -206,8 +375,20 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo return null; } - long androidParentID = getParentID(cur); - String androidParentGUID = getGUIDForID(androidParentID); + // Short-cut for deleted items. + if (isDeleted(cur)) { + return AndroidBrowserBookmarksRepositorySession.bookmarkFromMirrorCursor(cur, null, null, null); + } + + long androidParentID = getParentID(cur); + + // Ensure special folders stay in the right place. + String androidParentGUID = SPECIAL_GUID_PARENTS.get(recordGUID); + if (androidParentGUID == null) { + androidParentGUID = getGUIDForID(androidParentID); + } + + boolean needsReparenting = false; if (androidParentGUID == null) { Logger.debug(LOG_TAG, "No parent GUID for record " + recordGUID + " with parent " + androidParentID); @@ -216,25 +397,64 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo Logger.error(LOG_TAG, "Have the parent android ID for the record but the parent's GUID wasn't found."); throw new NoGuidForIdException(null); } + + // We have a parent ID but it's wrong. If the record is deleted, + // we'll just say that it was in the Unsorted Bookmarks folder. + // If not, we'll move it into Mobile Bookmarks. + needsReparenting = true; } - // If record is a folder, build out the children array. - JSONArray childArray = getChildArrayForCursor(cur, recordGUID); + // If record is a folder, and we want to see children at this time, then build out the children array. + final JSONArray childArray; + if (computeAndPersistChildren) { + childArray = getChildrenArrayForRecordCursor(cur, recordGUID, true); + } else { + childArray = null; + } String parentName = getParentName(androidParentGUID); - return RepoUtils.bookmarkFromMirrorCursor(cur, androidParentGUID, parentName, childArray); + BookmarkRecord bookmark = AndroidBrowserBookmarksRepositorySession.bookmarkFromMirrorCursor(cur, androidParentGUID, parentName, childArray); + + if (needsReparenting) { + Logger.warn(LOG_TAG, "Bookmark record " + recordGUID + " has a bad parent pointer. Reparenting now."); + + String destination = bookmark.deleted ? "unfiled" : "mobile"; + bookmark.androidParentID = getIDForGUID(destination); + bookmark.androidPosition = getPosition(cur); + bookmark.parentID = destination; + bookmark.parentName = getParentName(destination); + if (!bookmark.deleted) { + // Actually move it. + // TODO: compute position. Persist. + relocateBookmark(bookmark); + } + } + + return bookmark; } - protected JSONArray getChildArrayForCursor(Cursor cur, String recordGUID) throws NullCursorException { - JSONArray childArray = null; + /** + * Ensure that the local database row for the provided bookmark + * reflects this record's parent information. + * + * @param bookmark + */ + private void relocateBookmark(BookmarkRecord bookmark) { + dataAccessor.updateParentAndPosition(bookmark.guid, bookmark.androidParentID, bookmark.androidPosition); + } + + protected JSONArray getChildrenArrayForRecordCursor(Cursor cur, String recordGUID, boolean persist) throws NullCursorException { boolean isFolder = rowIsFolder(cur); - Logger.debug(LOG_TAG, "Record " + recordGUID + " is a " + (isFolder ? "folder." : "bookmark.")); - if (isFolder) { - long androidID = guidToID.get(recordGUID); - childArray = getChildren(androidID); + if (!isFolder) { + return null; } - if (childArray != null) { - Logger.debug(LOG_TAG, "Fetched " + childArray.size() + " children for " + recordGUID); + + long androidID = guidToID.get(recordGUID); + JSONArray childArray = getChildrenArray(androidID, persist); + if (childArray == null) { + return null; } + + Logger.debug(LOG_TAG, "Fetched " + childArray.size() + " children for " + recordGUID); return childArray; } @@ -275,7 +495,11 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo // hairy stuff. Here's the setup for it. Logger.debug(LOG_TAG, "Preparing folder ID mappings."); - idToGuid.put(0L, "places"); // Fake our root. + + // Fake our root. + Logger.debug(LOG_TAG, "Tracking places root as ID 0."); + idToGuid.put(0L, "places"); + guidToID.put("places", 0L); try { cur.moveToFirst(); while (!cur.isAfterLast()) { @@ -308,33 +532,27 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo }; @Override - @SuppressWarnings("unchecked") + protected Record reconcileRecords(Record remoteRecord, Record localRecord, + long lastRemoteRetrieval, + long lastLocalRetrieval) { + + BookmarkRecord reconciled = (BookmarkRecord) super.reconcileRecords(remoteRecord, localRecord, + lastRemoteRetrieval, + lastLocalRetrieval); + + // For now we *always* use the remote record's children array as a starting point. + // We won't write it into the database yet; we'll record it and process as we go. + reconciled.children = ((BookmarkRecord) remoteRecord).children; + return reconciled; + } + + @Override protected Record prepareRecord(Record record) { BookmarkRecord bmk = (BookmarkRecord) record; - - // Check if parent exists - if (guidToID.containsKey(bmk.parentID)) { - bmk.androidParentID = guidToID.get(bmk.parentID); - JSONArray children = parentToChildArray.get(bmk.parentID); - if (children != null) { - if (!children.contains(bmk.guid)) { - children.add(bmk.guid); - parentToChildArray.put(bmk.parentID, children); - } - bmk.androidPosition = children.indexOf(bmk.guid); - } - } - else { - bmk.androidParentID = guidToID.get("unfiled"); - ArrayList children; - if (missingParentToChildren.containsKey(bmk.parentID)) { - children = missingParentToChildren.get(bmk.parentID); - } else { - children = new ArrayList(); - } - children.add(bmk.guid); - needsReparenting++; - missingParentToChildren.put(bmk.parentID, children); + + if (!isSpecialRecord(record)) { + // We never want to reparent special records. + handleParenting(bmk); } if (Logger.LOG_PERSONAL_INFORMATION) { @@ -363,36 +581,162 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo return bmk; } + /** + * If the provided record doesn't have correct parent information, + * update appropriate bookkeeping to improve the situation. + * + * @param bmk + */ + private void handleParenting(BookmarkRecord bmk) { + if (guidToID.containsKey(bmk.parentID)) { + bmk.androidParentID = guidToID.get(bmk.parentID); + + // Might as well set a basic position from the downloaded children array. + JSONArray children = parentToChildArray.get(bmk.parentID); + if (children != null) { + int index = children.indexOf(bmk.guid); + if (index >= 0) { + bmk.androidPosition = index; + } + } + } + else { + bmk.androidParentID = guidToID.get("unfiled"); + ArrayList children; + if (missingParentToChildren.containsKey(bmk.parentID)) { + children = missingParentToChildren.get(bmk.parentID); + } else { + children = new ArrayList(); + } + children.add(bmk.guid); + needsReparenting++; + missingParentToChildren.put(bmk.parentID, children); + } + } + + private boolean isSpecialRecord(Record record) { + return SPECIAL_GUID_PARENTS.containsKey(record.guid); + } + @Override - @SuppressWarnings("unchecked") protected void updateBookkeeping(Record record) throws NoGuidForIdException, NullCursorException, ParentNotFoundException { super.updateBookkeeping(record); BookmarkRecord bmk = (BookmarkRecord) record; - // If record is folder, update maps and re-parent children if necessary - if (bmk.type.equalsIgnoreCase(AndroidBrowserBookmarksDataAccessor.TYPE_FOLDER)) { - guidToID.put(bmk.guid, bmk.androidID); - idToGuid.put(bmk.androidID, bmk.guid); - - JSONArray childArray = bmk.children; - - // Re-parent. - if (missingParentToChildren.containsKey(bmk.guid)) { - for (String child : missingParentToChildren.get(bmk.guid)) { - long position; - if (!bmk.children.contains(child)) { - childArray.add(child); - } - position = childArray.indexOf(child); - dataAccessor.updateParentAndPosition(child, bmk.androidID, position); - needsReparenting--; - } - missingParentToChildren.remove(bmk.guid); - } - parentToChildArray.put(bmk.guid, childArray); + // If record is folder, update maps and re-parent children if necessary. + if (!bmk.isFolder()) { + Logger.debug(LOG_TAG, "Not a folder. No bookkeeping."); + return; } + + Logger.debug(LOG_TAG, "Updating bookkeeping for folder " + record.guid); + + // Mappings between ID and GUID. + // TODO: update our persisted children arrays! + // TODO: if our Android ID just changed, replace parents for all of our children. + guidToID.put(bmk.guid, bmk.androidID); + idToGuid.put(bmk.androidID, bmk.guid); + + JSONArray childArray = bmk.children; + + if (Logger.logVerbose(LOG_TAG)) { + Logger.trace(LOG_TAG, bmk.guid + " has children " + childArray.toJSONString()); + } + parentToChildArray.put(bmk.guid, childArray); + + // Re-parent. + if (missingParentToChildren.containsKey(bmk.guid)) { + for (String child : missingParentToChildren.get(bmk.guid)) { + // This might return -1; that's OK, the bookmark will + // be properly repositioned later. + long position = childArray.indexOf(child); + dataAccessor.updateParentAndPosition(child, bmk.androidID, position); + needsReparenting--; + } + missingParentToChildren.remove(bmk.guid); + } + } + + @Override + protected void storeRecordDeletion(final Record record) { + if (SPECIAL_GUIDS_MAP.containsKey(record.guid)) { + Logger.debug(LOG_TAG, "Told to delete record " + record.guid + ". Ignoring."); + return; + } + final BookmarkRecord bookmarkRecord = (BookmarkRecord) record; + if (bookmarkRecord.isFolder()) { + Logger.debug(LOG_TAG, "Deleting folder. Ensuring consistency of children."); + handleFolderDeletion(bookmarkRecord); + return; + } + super.storeRecordDeletion(record); + } + + /** + * When a folder deletion is received, we must ensure -- for database + * consistency -- that its children are placed somewhere sane. + * + * Note that its children might also be deleted, but we'll process + * folders first. For that reason we might want to queue up these + * folder deletions and handle them in onStoreDone. + * + * See Bug 724739. + * + * @param folder + */ + protected void handleFolderDeletion(final BookmarkRecord folder) { + // TODO: reparent children. Bug 724740. + // For now we'll trust that we'll process the item deletions, too. + super.storeRecordDeletion(folder); + } + + @SuppressWarnings("unchecked") + private void finishUp() { + try { + Logger.debug(LOG_TAG, "Have " + parentToChildArray.size() + " folders whose children might need repositioning."); + for (Entry entry : parentToChildArray.entrySet()) { + String guid = entry.getKey(); + JSONArray onServer = entry.getValue(); + try { + final long folderID = getIDForGUID(guid); + JSONArray inDB = getChildrenArray(folderID, false); + int added = 0; + for (Object o : inDB) { + if (!onServer.contains(o)) { + onServer.add(o); + added++; + } + } + Logger.debug(LOG_TAG, "Added " + added + " items locally."); + dataAccessor.updatePositions(new ArrayList(onServer)); + dataAccessor.bumpModified(folderID, now()); + // Wow, this is spectacularly wasteful. + Logger.debug(LOG_TAG, "Untracking " + guid); + final Record record = retrieveByGUIDDuringStore(guid); + if (record == null) { + return; + } + untrackRecord(record); + } catch (Exception e) { + Logger.warn(LOG_TAG, "Error repositioning children for " + guid, e); + } + } + } finally { + super.storeDone(); + } + } + + @Override + public void storeDone() { + Runnable command = new Runnable() { + @Override + public void run() { + finishUp(); + } + }; + storeWorkQueue.execute(command); } @Override @@ -400,4 +744,101 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo BookmarkRecord bmk = (BookmarkRecord) record; return bmk.title + bmk.bookmarkURI + bmk.type + bmk.parentName; } + + public static BookmarkRecord computeParentFields(BookmarkRecord rec, String suggestedParentGUID, String suggestedParentName) { + final String guid = rec.guid; + if (guid == null) { + // Oh dear. + Logger.error(LOG_TAG, "No guid in computeParentFields!"); + return null; + } + + String realParent = SPECIAL_GUID_PARENTS.get(guid); + if (realParent == null) { + // No magic parent. Use whatever the caller suggests. + realParent = suggestedParentGUID; + } else { + Logger.debug(LOG_TAG, "Ignoring suggested parent ID " + suggestedParentGUID + + " for " + guid + "; using " + realParent); + } + + if (realParent == null) { + // Oh dear. + Logger.error(LOG_TAG, "No parent for record " + guid); + return null; + } + + // Always set the parent name for special folders back to default. + String parentName = SPECIAL_GUIDS_MAP.get(realParent); + if (parentName == null) { + parentName = suggestedParentName; + } + + rec.parentID = realParent; + rec.parentName = parentName; + return rec; + } + + private static BookmarkRecord logBookmark(BookmarkRecord rec) { + try { + Logger.debug(LOG_TAG, "Returning " + (rec.deleted ? "deleted " : "") + + "bookmark record " + rec.guid + " (" + rec.androidID + + ", parent " + rec.parentID + ")"); + if (!rec.deleted && Logger.LOG_PERSONAL_INFORMATION) { + Logger.pii(LOG_TAG, "> Parent name: " + rec.parentName); + Logger.pii(LOG_TAG, "> Title: " + rec.title); + Logger.pii(LOG_TAG, "> Type: " + rec.type); + Logger.pii(LOG_TAG, "> URI: " + rec.bookmarkURI); + Logger.pii(LOG_TAG, "> Android position: " + rec.androidPosition); + Logger.pii(LOG_TAG, "> Position: " + rec.pos); + if (rec.isFolder()) { + Logger.pii(LOG_TAG, "FOLDER: Children are " + + (rec.children == null ? + "null" : + rec.children.toJSONString())); + } + } + } catch (Exception e) { + Logger.debug(LOG_TAG, "Exception logging bookmark record " + rec, e); + } + return rec; + } + + // Create a BookmarkRecord object from a cursor on a row containing a Fennec bookmark. + public static BookmarkRecord bookmarkFromMirrorCursor(Cursor cur, String parentGUID, String parentName, JSONArray children) { + final String collection = "bookmarks"; + final String guid = RepoUtils.getStringFromCursor(cur, BrowserContract.SyncColumns.GUID); + final long lastModified = RepoUtils.getLongFromCursor(cur, BrowserContract.SyncColumns.DATE_MODIFIED); + final boolean deleted = isDeleted(cur); + BookmarkRecord rec = new BookmarkRecord(guid, collection, lastModified, deleted); + + // No point in populating it. + if (deleted) { + return logBookmark(rec); + } + + boolean isFolder = RepoUtils.getIntFromCursor(cur, BrowserContract.Bookmarks.IS_FOLDER) == 1; + + rec.title = RepoUtils.getStringFromCursor(cur, BrowserContract.Bookmarks.TITLE); + rec.bookmarkURI = RepoUtils.getStringFromCursor(cur, BrowserContract.Bookmarks.URL); + rec.description = RepoUtils.getStringFromCursor(cur, BrowserContract.Bookmarks.DESCRIPTION); + rec.tags = RepoUtils.getJSONArrayFromCursor(cur, BrowserContract.Bookmarks.TAGS); + rec.keyword = RepoUtils.getStringFromCursor(cur, BrowserContract.Bookmarks.KEYWORD); + rec.type = isFolder ? AndroidBrowserBookmarksDataAccessor.TYPE_FOLDER : + AndroidBrowserBookmarksDataAccessor.TYPE_BOOKMARK; + + rec.androidID = RepoUtils.getLongFromCursor(cur, BrowserContract.Bookmarks._ID); + rec.androidPosition = RepoUtils.getLongFromCursor(cur, BrowserContract.Bookmarks.POSITION); + rec.children = children; + + // Need to restore the parentId since it isn't stored in content provider. + // We also take this opportunity to fix up parents for special folders, + // allowing us to map between the hierarchies used by Fennec and Places. + BookmarkRecord withParentFields = computeParentFields(rec, parentGUID, parentName); + if (withParentFields == null) { + // Oh dear. Something went wrong. + return null; + } + return logBookmark(withParentFields); + } } diff --git a/mobile/android/base/sync/repositories/android/AndroidBrowserHistoryRepositorySession.java b/mobile/android/base/sync/repositories/android/AndroidBrowserHistoryRepositorySession.java index f9db74f01cd..19282ffc882 100644 --- a/mobile/android/base/sync/repositories/android/AndroidBrowserHistoryRepositorySession.java +++ b/mobile/android/base/sync/repositories/android/AndroidBrowserHistoryRepositorySession.java @@ -27,7 +27,12 @@ public class AndroidBrowserHistoryRepositorySession extends AndroidBrowserReposi } @Override - protected Record recordFromMirrorCursor(Cursor cur) { + protected Record retrieveDuringStore(Cursor cur) { + return RepoUtils.historyFromMirrorCursor(cur); + } + + @Override + protected Record retrieveDuringFetch(Cursor cur) { return RepoUtils.historyFromMirrorCursor(cur); } diff --git a/mobile/android/base/sync/repositories/android/AndroidBrowserPasswordsRepositorySession.java b/mobile/android/base/sync/repositories/android/AndroidBrowserPasswordsRepositorySession.java index 4843eb520f0..effc697c53f 100644 --- a/mobile/android/base/sync/repositories/android/AndroidBrowserPasswordsRepositorySession.java +++ b/mobile/android/base/sync/repositories/android/AndroidBrowserPasswordsRepositorySession.java @@ -20,7 +20,12 @@ public class AndroidBrowserPasswordsRepositorySession extends } @Override - protected Record recordFromMirrorCursor(Cursor cur) { + protected Record retrieveDuringStore(Cursor cur) { + return RepoUtils.passwordFromMirrorCursor(cur); + } + + @Override + protected Record retrieveDuringFetch(Cursor cur) { return RepoUtils.passwordFromMirrorCursor(cur); } diff --git a/mobile/android/base/sync/repositories/android/AndroidBrowserRepositoryDataAccessor.java b/mobile/android/base/sync/repositories/android/AndroidBrowserRepositoryDataAccessor.java index 2459f91e9a6..ce7c171541a 100644 --- a/mobile/android/base/sync/repositories/android/AndroidBrowserRepositoryDataAccessor.java +++ b/mobile/android/base/sync/repositories/android/AndroidBrowserRepositoryDataAccessor.java @@ -38,6 +38,7 @@ package org.mozilla.gecko.sync.repositories.android; +import org.mozilla.gecko.sync.Logger; import org.mozilla.gecko.sync.repositories.NullCursorException; import org.mozilla.gecko.sync.repositories.domain.Record; @@ -45,7 +46,6 @@ import android.content.ContentValues; import android.content.Context; import android.database.Cursor; import android.net.Uri; -import android.util.Log; public abstract class AndroidBrowserRepositoryDataAccessor { @@ -68,7 +68,7 @@ public abstract class AndroidBrowserRepositoryDataAccessor { } public void wipe() { - Log.i(LOG_TAG, "wiping: " + getUri()); + Logger.info(LOG_TAG, "wiping: " + getUri()); String where = BrowserContract.SyncColumns.GUID + " NOT IN ('mobile')"; context.getContentResolver().delete(getUri(), where, null); } @@ -98,7 +98,7 @@ public abstract class AndroidBrowserRepositoryDataAccessor { if (deleted == 1) { return; } - Log.w(LOG_TAG, "Unexpectedly deleted " + deleted + " rows for guid " + guid); + Logger.warn(LOG_TAG, "Unexpectedly deleted " + deleted + " rows for guid " + guid); } public void update(String guid, Record newRecord) { @@ -107,13 +107,12 @@ public abstract class AndroidBrowserRepositoryDataAccessor { ContentValues cv = getContentValues(newRecord); int updated = context.getContentResolver().update(getUri(), cv, where, args); if (updated != 1) { - Log.w(LOG_TAG, "Unexpectedly updated " + updated + " rows for guid " + guid); + Logger.warn(LOG_TAG, "Unexpectedly updated " + updated + " rows for guid " + guid); } } public Uri insert(Record record) { ContentValues cv = getContentValues(record); - Log.d(LOG_TAG, "INSERTING: " + cv.getAsString("guid")); return context.getContentResolver().insert(getUri(), cv); } @@ -138,9 +137,9 @@ public abstract class AndroidBrowserRepositoryDataAccessor { */ public Cursor getGUIDsSince(long timestamp) throws NullCursorException { return queryHelper.safeQuery(".getGUIDsSince", - GUID_COLUMNS, - dateModifiedWhere(timestamp), - null, null); + GUID_COLUMNS, + dateModifiedWhere(timestamp), + null, null); } /** @@ -153,9 +152,9 @@ public abstract class AndroidBrowserRepositoryDataAccessor { */ public Cursor fetchSince(long timestamp) throws NullCursorException { return queryHelper.safeQuery(".fetchSince", - getAllColumns(), - dateModifiedWhere(timestamp), - null, null); + getAllColumns(), + dateModifiedWhere(timestamp), + null, null); } /** @@ -193,7 +192,7 @@ public abstract class AndroidBrowserRepositoryDataAccessor { if (deleted == 1) { return; } - Log.w(LOG_TAG, "Unexpectedly deleted " + deleted + " rows for guid " + record.guid); + Logger.warn(LOG_TAG, "Unexpectedly deleted " + deleted + " rows for guid " + record.guid); } public void updateByGuid(String guid, ContentValues cv) { @@ -204,6 +203,6 @@ public abstract class AndroidBrowserRepositoryDataAccessor { if (updated == 1) { return; } - Log.w(LOG_TAG, "Unexpectedly updated " + updated + " rows for guid " + guid); + Logger.warn(LOG_TAG, "Unexpectedly updated " + updated + " rows for guid " + guid); } } diff --git a/mobile/android/base/sync/repositories/android/AndroidBrowserRepositorySession.java b/mobile/android/base/sync/repositories/android/AndroidBrowserRepositorySession.java index 928c255b8f7..566dfc0474b 100644 --- a/mobile/android/base/sync/repositories/android/AndroidBrowserRepositorySession.java +++ b/mobile/android/base/sync/repositories/android/AndroidBrowserRepositorySession.java @@ -96,7 +96,9 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos } /** - * Override this. + * Retrieve a record from a cursor. Act as if we don't know the final contents of + * the record: for example, a folder's child array might change. + * * Return null if this record should not be processed. * * @param cur @@ -105,7 +107,20 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos * @throws NullCursorException * @throws ParentNotFoundException */ - protected abstract Record recordFromMirrorCursor(Cursor cur) throws NoGuidForIdException, NullCursorException, ParentNotFoundException; + protected abstract Record retrieveDuringStore(Cursor cur) throws NoGuidForIdException, NullCursorException, ParentNotFoundException; + + /** + * Retrieve a record from a cursor. Ensure that the contents of the database are + * updated to match the record that we're constructing: for example, the children + * of a folder might be repositioned as we generate the folder's record. + * + * @param cur + * @return + * @throws NoGuidForIdException + * @throws NullCursorException + * @throws ParentNotFoundException + */ + protected abstract Record retrieveDuringFetch(Cursor cur) throws NoGuidForIdException, NullCursorException, ParentNotFoundException; // Must be overriden by AndroidBookmarkRepositorySession. protected boolean checkRecordType(Record record) { @@ -247,7 +262,7 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos return; } while (!cursor.isAfterLast()) { - Record r = recordFromMirrorCursor(cursor); + Record r = retrieveDuringFetch(cursor); if (r != null) { if (filter == null || !filter.excludeRecord(r)) { Logger.trace(LOG_TAG, "Processing record " + r.guid); @@ -408,7 +423,7 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos Record existingRecord; try { // GUID matching only: deleted records don't have a payload with which to search. - existingRecord = recordForGUID(record.guid); + existingRecord = retrieveByGUIDDuringStore(record.guid); if (record.deleted) { if (existingRecord == null) { // We're done. Don't bother with a callback. That can change later @@ -540,7 +555,18 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos return toStore; } - protected Record recordForGUID(String guid) throws + /** + * Retrieve a record from the store by GUID, without writing unnecessarily to the + * database. + * + * @param guid + * @return + * @throws NoGuidForIdException + * @throws NullCursorException + * @throws ParentNotFoundException + * @throws MultipleRecordsForGuidException + */ + protected Record retrieveByGUIDDuringStore(String guid) throws NoGuidForIdException, NullCursorException, ParentNotFoundException, @@ -551,7 +577,7 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos return null; } - Record r = recordFromMirrorCursor(cursor); + Record r = retrieveDuringStore(cursor); cursor.moveToNext(); if (cursor.isAfterLast()) { @@ -588,7 +614,7 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos String guid = getRecordToGuidMap().get(recordString); if (guid != null) { Logger.debug(LOG_TAG, "Found one. Returning computed record."); - return recordForGUID(guid); + return retrieveByGUIDDuringStore(guid); } Logger.debug(LOG_TAG, "findExistingRecord failed to find one for " + record.guid); return null; @@ -604,13 +630,17 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos private void createRecordToGuidMap() throws NoGuidForIdException, NullCursorException, ParentNotFoundException { Logger.info(LOG_TAG, "BEGIN: creating record -> GUID map."); recordToGuid = new HashMap(); + + // TODO: we should be able to do this entire thing with string concatenations within SQL. + // Also consider whether it's better to fetch and process every record in the DB into + // memory, or run a query per record to do the same thing. Cursor cur = dbHelper.fetchAll(); try { if (!cur.moveToFirst()) { return; } while (!cur.isAfterLast()) { - Record record = recordFromMirrorCursor(cur); + Record record = retrieveDuringStore(cur); if (record != null) { recordToGuid.put(buildRecordString(record), record.guid); } diff --git a/mobile/android/base/sync/repositories/android/RepoUtils.java b/mobile/android/base/sync/repositories/android/RepoUtils.java index 55496c31d45..9cd08d71a92 100644 --- a/mobile/android/base/sync/repositories/android/RepoUtils.java +++ b/mobile/android/base/sync/repositories/android/RepoUtils.java @@ -4,17 +4,11 @@ package org.mozilla.gecko.sync.repositories.android; -import java.util.Collections; -import java.util.HashMap; -import java.util.Map; - import org.json.simple.JSONArray; import org.json.simple.parser.JSONParser; import org.json.simple.parser.ParseException; -import org.mozilla.gecko.R; import org.mozilla.gecko.sync.Logger; import org.mozilla.gecko.sync.repositories.NullCursorException; -import org.mozilla.gecko.sync.repositories.domain.BookmarkRecord; import org.mozilla.gecko.sync.repositories.domain.HistoryRecord; import org.mozilla.gecko.sync.repositories.domain.PasswordRecord; @@ -27,89 +21,6 @@ public class RepoUtils { private static final String LOG_TAG = "RepoUtils"; - /** - * An array of known-special GUIDs. - */ - public static String[] SPECIAL_GUIDS = new String[] { - // Mobile and desktop places roots have to come first. - "mobile", - "places", - "toolbar", - "menu", - "unfiled" - }; - - /** - * = A note about folder mapping = - * - * Note that _none_ of Places's folders actually have a special GUID. They're all - * randomly generated. Special folders are indicated by membership in the - * moz_bookmarks_roots table, and by having the parent `1`. - * - * Additionally, the mobile root is annotated. In Firefox Sync, PlacesUtils is - * used to find the IDs of these special folders. - * - * Sync skips over `places` and `tags` when finding IDs. - * - * We need to consume records with these various guids, producing a local - * representation which we are able to stably map upstream. - * - * That is: - * - * * We should not upload a `places` record or a `tags` record. - * * We can stably _store_ menu/toolbar/unfiled/mobile as special GUIDs, and set - * their parent ID as appropriate on upload. - * - * - * = Places folders = - * - * guid root_name folder_id parent - * ---------- ---------- ---------- ---------- - * ? places 1 0 - * ? menu 2 1 - * ? toolbar 3 1 - * ? tags 4 1 - * ? unfiled 5 1 - * - * ? mobile* 474 1 - * - * - * = Fennec folders = - * - * guid folder_id parent - * ---------- ---------- ---------- - * mobile ? 0 - * - */ - public static final Map SPECIAL_GUID_PARENTS; - static { - HashMap m = new HashMap(); - m.put("places", null); - m.put("menu", "places"); - m.put("toolbar", "places"); - m.put("tags", "places"); - m.put("unfiled", "places"); - m.put("mobile", "places"); - SPECIAL_GUID_PARENTS = Collections.unmodifiableMap(m); - } - - /** - * A map of guids to their localized name strings. - */ - // Oh, if only we could make this final and initialize it in the static initializer. - public static Map SPECIAL_GUIDS_MAP; - public static void initialize(Context context) { - if (SPECIAL_GUIDS_MAP == null) { - HashMap m = new HashMap(); - m.put("menu", context.getString(R.string.bookmarks_folder_menu)); - m.put("places", context.getString(R.string.bookmarks_folder_places)); - m.put("toolbar", context.getString(R.string.bookmarks_folder_toolbar)); - m.put("unfiled", context.getString(R.string.bookmarks_folder_unfiled)); - m.put("mobile", context.getString(R.string.bookmarks_folder_mobile)); - SPECIAL_GUIDS_MAP = Collections.unmodifiableMap(m); - } - } - /** * A helper class for monotonous SQL querying. Does timing and logging, * offers a utility to throw on a null cursor. @@ -205,97 +116,6 @@ public class RepoUtils { return Long.parseLong(path.substring(lastSlash + 1)); } - public static BookmarkRecord computeParentFields(BookmarkRecord rec, String suggestedParentGUID, String suggestedParentName) { - final String guid = rec.guid; - if (guid == null) { - // Oh dear. - Logger.error(LOG_TAG, "No guid in computeParentFields!"); - return null; - } - - String realParent = SPECIAL_GUID_PARENTS.get(guid); - if (realParent == null) { - // No magic parent. Use whatever the caller suggests. - realParent = suggestedParentGUID; - } else { - Logger.debug(LOG_TAG, "Ignoring suggested parent ID " + suggestedParentGUID + - " for " + guid + "; using " + realParent); - } - - if (realParent == null) { - // Oh dear. - Logger.error(LOG_TAG, "No parent for record " + guid); - return null; - } - - // Always set the parent name for special folders back to default. - String parentName = SPECIAL_GUIDS_MAP.get(realParent); - if (parentName == null) { - parentName = suggestedParentName; - } - - rec.parentID = realParent; - rec.parentName = parentName; - return rec; - } - - // Create a BookmarkRecord object from a cursor on a row containing a Fennec bookmark. - public static BookmarkRecord bookmarkFromMirrorCursor(Cursor cur, String parentGUID, String parentName, JSONArray children) { - - String guid = getStringFromCursor(cur, BrowserContract.SyncColumns.GUID); - String collection = "bookmarks"; - long lastModified = getLongFromCursor(cur, BrowserContract.SyncColumns.DATE_MODIFIED); - boolean deleted = getLongFromCursor(cur, BrowserContract.SyncColumns.IS_DELETED) == 1 ? true : false; - boolean isFolder = getIntFromCursor(cur, BrowserContract.Bookmarks.IS_FOLDER) == 1; - BookmarkRecord rec = new BookmarkRecord(guid, collection, lastModified, deleted); - - rec.title = getStringFromCursor(cur, BrowserContract.Bookmarks.TITLE); - rec.bookmarkURI = getStringFromCursor(cur, BrowserContract.Bookmarks.URL); - rec.description = getStringFromCursor(cur, BrowserContract.Bookmarks.DESCRIPTION); - rec.tags = getJSONArrayFromCursor(cur, BrowserContract.Bookmarks.TAGS); - rec.keyword = getStringFromCursor(cur, BrowserContract.Bookmarks.KEYWORD); - rec.type = isFolder ? AndroidBrowserBookmarksDataAccessor.TYPE_FOLDER : - AndroidBrowserBookmarksDataAccessor.TYPE_BOOKMARK; - - rec.androidID = getLongFromCursor(cur, BrowserContract.Bookmarks._ID); - rec.androidPosition = getLongFromCursor(cur, BrowserContract.Bookmarks.POSITION); - rec.children = children; - - // Need to restore the parentId since it isn't stored in content provider. - // We also take this opportunity to fix up parents for special folders, - // allowing us to map between the hierarchies used by Fennec and Places. - BookmarkRecord withParentFields = computeParentFields(rec, parentGUID, parentName); - if (withParentFields == null) { - // Oh dear. Something went wrong. - return null; - } - return logBookmark(withParentFields); - } - - private static BookmarkRecord logBookmark(BookmarkRecord rec) { - try { - Logger.debug(LOG_TAG, "Returning bookmark record " + rec.guid + " (" + rec.androidID + - ", parent " + rec.parentID + ")"); - if (Logger.LOG_PERSONAL_INFORMATION) { - Logger.pii(LOG_TAG, "> Parent name: " + rec.parentName); - Logger.pii(LOG_TAG, "> Title: " + rec.title); - Logger.pii(LOG_TAG, "> Type: " + rec.type); - Logger.pii(LOG_TAG, "> URI: " + rec.bookmarkURI); - Logger.pii(LOG_TAG, "> Android position: " + rec.androidPosition); - Logger.pii(LOG_TAG, "> Position: " + rec.pos); - if (rec.isFolder()) { - Logger.pii(LOG_TAG, "FOLDER: Children are " + - (rec.children == null ? - "null" : - rec.children.toJSONString())); - } - } - } catch (Exception e) { - Logger.debug(LOG_TAG, "Exception logging bookmark record " + rec, e); - } - return rec; - } - //Create a HistoryRecord object from a cursor on a row with a Moz History record in it public static HistoryRecord historyFromMirrorCursor(Cursor cur) {