Bug 731024 - Part 4: Handle additional types. Test for livemarks. r=nalexander

mobile/android/base/sync/Utils.java

@@ -41,11 +41,14 @@ package org.mozilla.gecko.sync;
 import java.io.UnsupportedEncodingException;
 import java.math.BigDecimal;
 import java.math.BigInteger;
+import java.net.URLDecoder;
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 import java.security.SecureRandom;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.Locale;
+import java.util.Map;
 import java.util.TreeMap;
 
 import org.json.simple.JSONArray;
@@ -267,4 +270,38 @@ public class Utils {
     }
     return true;
   }
+
+  /**
+   * Takes a URI, extracting URI components.
+   * @param scheme the URI scheme on which to match.
+   */
+  public static Map<String, String> extractURIComponents(String scheme, String uri) {
+    if (uri.indexOf(scheme) != 0) {
+      throw new IllegalArgumentException("URI scheme does not match: " + scheme);
+    }
+
+    // Do this the hard way to avoid taking a large dependency on
+    // HttpClient or getting all regex-tastic.
+    String components = uri.substring(scheme.length());
+    HashMap<String, String> out = new HashMap<String, String>();
+    String[] parts = components.split("&");
+    for (int i = 0; i < parts.length; ++i) {
+      String part = parts[i];
+      if (part.length() == 0) {
+        continue;
+      }
+      String[] pair = part.split("=", 2);
+      switch (pair.length) {
+      case 0:
+        continue;
+      case 1:
+        out.put(URLDecoder.decode(pair[0]), null);
+        break;
+      case 2:
+        out.put(URLDecoder.decode(pair[0]), URLDecoder.decode(pair[1]));
+        break;
+      }
+    }
+    return out;
+  }
 }

mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksDataAccessor.java

@@ -187,9 +187,16 @@ public class AndroidBrowserBookmarksDataAccessor extends AndroidBrowserRepositor
 
   @Override
   protected ContentValues getContentValues(Record record) {
-    ContentValues cv = new ContentValues();
     BookmarkRecord rec = (BookmarkRecord) record;
+
+    final int recordType = BrowserContractHelpers.typeCodeForString(rec.type);
+    if (recordType == -1) {
+      throw new IllegalStateException("Unexpected record type " + rec.type);
+    }
+
+    ContentValues cv = new ContentValues();
     cv.put(BrowserContract.SyncColumns.GUID,      rec.guid);
+    cv.put(BrowserContract.Bookmarks.TYPE,        recordType);
     cv.put(BrowserContract.Bookmarks.TITLE,       rec.title);
     cv.put(BrowserContract.Bookmarks.URL,         rec.bookmarkURI);
     cv.put(BrowserContract.Bookmarks.DESCRIPTION, rec.description);
@@ -201,12 +208,6 @@ public class AndroidBrowserBookmarksDataAccessor extends AndroidBrowserRepositor
     cv.put(BrowserContract.Bookmarks.PARENT,      rec.androidParentID);
     cv.put(BrowserContract.Bookmarks.POSITION,    rec.androidPosition);
 
-    // Only bookmark and folder types should make it this far.
-    // Other types should be filtered out and dropped.
-    cv.put(BrowserContract.Bookmarks.TYPE, rec.type.equalsIgnoreCase(TYPE_FOLDER) ?
-                                           BrowserContract.Bookmarks.TYPE_FOLDER :
-                                           BrowserContract.Bookmarks.TYPE_BOOKMARK);
-
     // Note that we don't set the modified timestamp: we allow the
     // content provider to do that for us.
     return cv;

mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java

@@ -198,8 +198,8 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
     dataAccessor = (AndroidBrowserBookmarksDataAccessor) dbHelper;
   }
 
-  private static long getTypeFromCursor(Cursor cur) {
+  private static int getTypeFromCursor(Cursor cur) {
-    return RepoUtils.getLongFromCursor(cur, BrowserContract.Bookmarks.TYPE);
+    return RepoUtils.getIntFromCursor(cur, BrowserContract.Bookmarks.TYPE);
   }
 
   private static boolean rowIsFolder(Cursor cur) {
@@ -479,10 +479,10 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
       return true;
     }
 
-    if (bmk.isBookmark() ||
+    if (BrowserContractHelpers.isSupportedType(bmk.type)) {
-        bmk.isFolder()) {
       return false;
     }
+
     Logger.debug(LOG_TAG, "Ignoring record with guid: " + bmk.guid + " and type: " + bmk.type);
     return true;
   }
@@ -578,22 +578,22 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
         Logger.pii(LOG_TAG, "Inserting folder " + bmk.guid + ", " + bmk.title +
                             " with parent " + bmk.androidParentID +
                             " (" + bmk.parentID + ", " + bmk.parentName +
-                            ", " + bmk.pos + ")");
+                            ", " + bmk.androidPosition + ")");
       } else {
         Logger.pii(LOG_TAG, "Inserting bookmark " + bmk.guid + ", " + bmk.title + ", " +
                             bmk.bookmarkURI + " with parent " + bmk.androidParentID +
                             " (" + bmk.parentID + ", " + bmk.parentName +
-                            ", " + bmk.pos + ")");
+                            ", " + bmk.androidPosition + ")");
       }
     } else {
       if (bmk.isFolder()) {
         Logger.debug(LOG_TAG, "Inserting folder " + bmk.guid +  ", parent " +
                               bmk.androidParentID +
-                              " (" + bmk.parentID + ", " + bmk.pos + ")");
+                              " (" + bmk.parentID + ", " + bmk.androidPosition + ")");
       } else {
         Logger.debug(LOG_TAG, "Inserting bookmark " + bmk.guid + " with parent " +
                               bmk.androidParentID +
-                              " (" + bmk.parentID + ", " + ", " + bmk.pos + ")");
+                              " (" + bmk.parentID + ", " + ", " + bmk.androidPosition + ")");
       }
     }
     return bmk;
@@ -685,7 +685,7 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
     }
     final BookmarkRecord bookmarkRecord = (BookmarkRecord) record;
     if (bookmarkRecord.isFolder()) {
-      Logger.debug(LOG_TAG, "Deleting folder. Ensuring consistency of children.");
+      Logger.debug(LOG_TAG, "Deleting folder. Ensuring consistency of children. TODO: Bug 724470.");
       handleFolderDeletion(bookmarkRecord);
       return;
     }
@@ -768,7 +768,20 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
   @Override
   protected String buildRecordString(Record record) {
     BookmarkRecord bmk = (BookmarkRecord) record;
-    return bmk.title + bmk.bookmarkURI + bmk.type + bmk.parentName;
+    String parent = bmk.parentName + "/";
+    if (bmk.isBookmark()) {
+      return "b" + parent + bmk.bookmarkURI + ":" + bmk.title;
+    }
+    if (bmk.isFolder()) {
+      return "f" + parent + bmk.title;
+    }
+    if (bmk.isSeparator()) {
+      return "s" + parent + bmk.androidPosition;
+    }
+    if (bmk.isQuery()) {
+      return "q" + parent + bmk.bookmarkURI;
+    }
+    return null;
   }
 
   public static BookmarkRecord computeParentFields(BookmarkRecord rec, String suggestedParentGUID, String suggestedParentName) {
@@ -815,8 +828,7 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
         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.androidPosition);
-        Logger.pii(LOG_TAG, "> Position:         " + rec.pos);
         if (rec.isFolder()) {
           Logger.pii(LOG_TAG, "FOLDER: Children are " +
                              (rec.children == null ?
@@ -843,15 +855,20 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
       return logBookmark(rec);
     }
 
-    boolean isFolder = rowIsFolder(cur);
+    int rowType = getTypeFromCursor(cur);
+    String typeString = BrowserContractHelpers.typeStringForCode(rowType);
 
+    if (typeString == null) {
+      Logger.warn(LOG_TAG, "Unsupported type code " + rowType);
+      return null;
+    }
+
+    rec.type = typeString;
     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);

mobile/android/base/sync/repositories/android/AndroidBrowserRepositorySession.java

@@ -456,7 +456,7 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos
           }
 
           // TODO: pass in timestamps?
-          Logger.debug(LOG_TAG, "Replacing " + existingRecord.guid + " with record " + toStore.guid);
+          Logger.debug(LOG_TAG, "Replacing existing " + existingRecord.guid + " with record " + toStore.guid);
           Record replaced = replace(toStore, existingRecord);
 
           // Note that we don't track records here; deciding that is the job
@@ -570,14 +570,20 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos
 
     Logger.debug(LOG_TAG, "Finding existing record for incoming record with GUID " + record.guid);
     String recordString = buildRecordString(record);
+    if (recordString == null) {
+      Logger.debug(LOG_TAG, "No record string for incoming record " + record.guid);
+      return null;
+    }
+
     Logger.debug(LOG_TAG, "Searching with record string " + recordString);
     String guid = getRecordToGuidMap().get(recordString);
-    if (guid != null) {
+    if (guid == null) {
-      Logger.debug(LOG_TAG, "Found one. Returning computed record.");
+      Logger.debug(LOG_TAG, "findExistingRecord failed to find one for " + record.guid);
-      return retrieveByGUIDDuringStore(guid);
+      return null;
     }
-    Logger.debug(LOG_TAG, "findExistingRecord failed to find one for " + record.guid);
+
-    return null;
+    Logger.debug(LOG_TAG, "Found one. Returning computed record.");
+    return retrieveByGUIDDuringStore(guid);
   }
 
   public HashMap<String, String> getRecordToGuidMap() throws NoGuidForIdException, NullCursorException, ParentNotFoundException {
@@ -602,7 +608,10 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos
       while (!cur.isAfterLast()) {
         Record record = retrieveDuringStore(cur);
         if (record != null) {
-          recordToGuid.put(buildRecordString(record), record.guid);
+          final String recordString = buildRecordString(record);
+          if (recordString != null) {
+            recordToGuid.put(recordString, record.guid);
+          }
         }
         cur.moveToNext();
       }
@@ -613,6 +622,10 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos
   }
 
   public void putRecordToGuidMap(String recordString, String guid) throws NoGuidForIdException, NullCursorException, ParentNotFoundException {
+    if (recordString == null) {
+      return;
+    }
+
     if (recordToGuid == null) {
       createRecordToGuidMap();
     }

mobile/android/base/sync/repositories/domain/BookmarkRecord.java

@@ -38,6 +38,10 @@
 
 package org.mozilla.gecko.sync.repositories.domain;
 
+import java.io.UnsupportedEncodingException;
+import java.net.URLEncoder;
+import java.util.Map;
+
 import org.json.simple.JSONArray;
 import org.mozilla.gecko.sync.ExtendedJSONObject;
 import org.mozilla.gecko.sync.Logger;
@@ -53,6 +57,8 @@ import android.util.Log;
  *
  */
 public class BookmarkRecord extends Record {
+  public static final String PLACES_URI_PREFIX = "places:";
+
   private static final String LOG_TAG = "BookmarkRecord";
 
   public static final String COLLECTION_NAME = "bookmarks";
@@ -83,7 +89,6 @@ public class BookmarkRecord extends Record {
   public String  parentName;
   public long    androidParentID;
   public String  type;
-  public String  pos;
   public long    androidPosition;
 
   public JSONArray children;
@@ -131,7 +136,6 @@ public class BookmarkRecord extends Record {
     out.parentName      = this.parentName;
     out.androidParentID = this.androidParentID;
     out.type            = this.type;
-    out.pos             = this.pos;
     out.androidPosition = this.androidPosition;
 
     out.children        = this.copyChildren();
@@ -197,30 +201,12 @@ public class BookmarkRecord extends Record {
 
   @Override
   protected void initFromPayload(ExtendedJSONObject payload) {
-    this.type        = (String) payload.get("type");
+    this.type        = payload.getString("type");
-    this.title       = (String) payload.get("title");
+    this.title       = payload.getString("title");
-    this.description = (String) payload.get("description");
+    this.description = payload.getString("description");
-    this.parentID    = (String) payload.get("parentid");
+    this.parentID    = payload.getString("parentid");
-    this.parentName  = (String) payload.get("parentName");
+    this.parentName  = payload.getString("parentName");
 
-    // bookmark, microsummary, query.
-    if (isBookmarkIsh()) {
-      this.keyword = (String) payload.get("keyword");
-    try {
-      this.tags = payload.getArray("tags");
-    } catch (NonArrayJSONException e) {
-      Logger.warn(LOG_TAG, "Got non-array tags in bookmark record " + this.guid, e);
-      this.tags = new JSONArray();
-    }
-    }
-
-    // bookmark.
-    if (isBookmark()) {
-      this.bookmarkURI = (String) payload.get("bmkUri");
-      return;
-    }
-
-    // folder.
     if (isFolder()) {
       try {
         this.children = payload.getArray("children");
@@ -232,20 +218,64 @@ public class BookmarkRecord extends Record {
       return;
     }
 
+    final String bmkUri = payload.getString("bmkUri");
+
+    // bookmark, microsummary, query.
+    if (isBookmarkIsh()) {
+      this.keyword = payload.getString("keyword");
+      try {
+        this.tags = payload.getArray("tags");
+      } catch (NonArrayJSONException e) {
+        Logger.warn(LOG_TAG, "Got non-array tags in bookmark record " + this.guid, e);
+        this.tags = new JSONArray();
+      }
+    }
+
+    if (isBookmark()) {
+      this.bookmarkURI = bmkUri;
+      return;
+    }
+
     if (isLivemark()) {
-      // TODO: siteUri, feedUri.
+      String siteUri = payload.getString("siteUri");
+      String feedUri = payload.getString("feedUri");
+      this.bookmarkURI = encodeUnsupportedTypeURI(bmkUri,
+                                                  "siteUri", siteUri,
+                                                  "feedUri", feedUri);
       return;
     }
     if (isQuery()) {
-      // TODO: queryId (optional), folderName.
+      String queryId = payload.getString("queryId");
+      String folderName = payload.getString("folderName");
+      this.bookmarkURI = encodeUnsupportedTypeURI(bmkUri,
+                                                  "queryId", queryId,
+                                                  "folderName", folderName);
       return;
     }
     if (isMicrosummary()) {
-      // TODO: generatorUri, staticTitle.
+      String generatorUri = payload.getString("generatorUri");
+      String staticTitle = payload.getString("staticTitle");
+      this.bookmarkURI = encodeUnsupportedTypeURI(bmkUri,
+                                                  "generatorUri", generatorUri,
+                                                  "staticTitle", staticTitle);
       return;
     }
     if (isSeparator()) {
-      this.pos = payload.getString("pos");
+      Object p = payload.get("pos");
+      if (p instanceof Long) {
+        this.androidPosition = (Long) p;
+      } else if (p instanceof String) {
+        try {
+          this.androidPosition = Long.parseLong((String) p, 10);
+        } catch (NumberFormatException e) {
+          return;
+        }
+      } else {
+        Logger.warn(LOG_TAG, "Unsupported position value " + p);
+        return;
+      }
+      String pos = String.valueOf(this.androidPosition);
+      this.bookmarkURI = encodeUnsupportedTypeURI(null, "pos", pos, null, null);
       return;
     }
   }
@@ -259,17 +289,57 @@ public class BookmarkRecord extends Record {
     putPayload(payload, "parentName", this.parentName);
     putPayload(payload, "keyword", this.keyword);
 
-    if (this.tags != null) {
+    if (isFolder()) {
-      payload.put("tags", this.tags);
-    }
-
-    if (isBookmark()) {
-      payload.put("bmkUri", bookmarkURI);
-    } else if (isFolder()) {
       payload.put("children", this.children);
+      return;
     }
 
-    // TODO: fields for other types.
+    // bookmark, microsummary, query.
+    if (isBookmarkIsh()) {
+      if (isBookmark()) {
+        payload.put("bmkUri", bookmarkURI);
+      }
+
+      if (isQuery()) {
+        Map<String, String> parts = Utils.extractURIComponents(PLACES_URI_PREFIX, this.bookmarkURI);
+        putPayload(payload, "queryId", parts.get("queryId"));
+        putPayload(payload, "folderName", parts.get("folderName"));
+        return;
+      }
+
+      if (this.tags != null) {
+        payload.put("tags", this.tags);
+      }
+
+      putPayload(payload, "keyword", this.keyword);
+      return;
+    }
+
+    if (isLivemark()) {
+      Map<String, String> parts = Utils.extractURIComponents(PLACES_URI_PREFIX, this.bookmarkURI);
+      putPayload(payload, "siteUri", parts.get("siteUri"));
+      putPayload(payload, "feedUri", parts.get("feedUri"));
+      return;
+    }
+    if (isMicrosummary()) {
+      Map<String, String> parts = Utils.extractURIComponents(PLACES_URI_PREFIX, this.bookmarkURI);
+      putPayload(payload, "generatorUri", parts.get("generatorUri"));
+      putPayload(payload, "staticTitle", parts.get("staticTitle"));
+      return;
+    }
+    if (isSeparator()) {
+      Map<String, String> parts = Utils.extractURIComponents(PLACES_URI_PREFIX, this.bookmarkURI);
+      String pos = parts.get("pos");
+      if (pos == null) {
+        return;
+      }
+      try {
+        payload.put("pos", Long.parseLong(pos, 10));
+      } catch (NumberFormatException e) {
+        return;
+      }
+      return;
+    }
   }
 
   private void trace(String s) {
@@ -348,6 +418,61 @@ public class BookmarkRecord extends Record {
     if (a != null && b == null) return false;
     return RepoUtils.stringsEqual(a.toJSONString(), b.toJSONString());
   }
+
+  /**
+   * URL-encode the provided string. If the input is null,
+   * the empty string is returned.
+   *
+   * @param in the string to encode.
+   * @return a URL-encoded version of the input.
+   */
+  protected static String encode(String in) {
+    if (in == null) {
+      return "";
+    }
+    try {
+      return URLEncoder.encode(in, "UTF-8");
+    } catch (UnsupportedEncodingException e) {
+      // Will never occur.
+      return null;
+    }
+  }
+
+  /**
+   * Take the provided URI and two parameters, constructing a URI like
+   *
+   *   places:uri=$uri&p1=$p1&p2=$p2
+   *
+   * null values in either parameter or value result in the parameter being omitted.
+   */
+  protected static String encodeUnsupportedTypeURI(String originalURI, String p1, String v1, String p2, String v2) {
+    StringBuilder b = new StringBuilder(PLACES_URI_PREFIX);
+    boolean previous = false;
+    if (originalURI != null) {
+      b.append("uri=");
+      b.append(encode(originalURI));
+      previous = true;
+    }
+    if (p1 != null) {
+      if (previous) {
+        b.append("&");
+      }
+      b.append(p1);
+      b.append("=");
+      b.append(encode(v1));
+      previous = true;
+    }
+    if (p2 != null) {
+      if (previous) {
+        b.append("&");
+      }
+      b.append(p2);
+      b.append("=");
+      b.append(encode(v2));
+      previous = true;
+    }
+    return b.toString();
+  }
 }