diff --git a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksDataAccessor.java b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksDataAccessor.java index 061bf42137b8..660f0967ce79 100644 --- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksDataAccessor.java +++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksDataAccessor.java @@ -292,6 +292,11 @@ public class AndroidBrowserBookmarksDataAccessor extends AndroidBrowserRepositor cv.put(BrowserContract.Bookmarks.TITLE, rec.title); cv.put(BrowserContract.Bookmarks.URL, rec.bookmarkURI); cv.put(BrowserContract.Bookmarks.DESCRIPTION, rec.description); + + if (rec.dateAdded != null) { + cv.put(BrowserContract.Bookmarks.DATE_CREATED, rec.dateAdded); + } + if (rec.tags == null) { rec.tags = new JSONArray(); } diff --git a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java index d18809c0b8af..cd47669cc245 100644 --- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java +++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java @@ -699,6 +699,34 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo } } + /** + * Incoming bookmark records might be missing dateAdded field, because clients started to sync it + * only in version 55. However, record's lastModified value is a good upper bound. In order to + * encourage modern clients to re-upload the record with an earlier local dateAdded value, + * we bump record's lastModified value if we perform any substitutions. + * + * This function is only called while inserting a record which doesn't exist locally. + */ + @Override + protected Record processBeforeInsertion(Record toProcess) { + // If incoming record is missing dateAdded, use its lastModified value instead. + if (((BookmarkRecord) toProcess).dateAdded == null) { + ((BookmarkRecord) toProcess).dateAdded = toProcess.lastModified; + toProcess.lastModified = now(); + return toProcess; + } + + // If both are present, use the lowest value. We trust server's monotonously increasing timestamps + // more than clients' potentially bogus ones. + if (toProcess.lastModified < ((BookmarkRecord) toProcess).dateAdded) { + ((BookmarkRecord) toProcess).dateAdded = toProcess.lastModified; + toProcess.lastModified = now(); + return toProcess; + } + + return toProcess; + } + @Override protected Record reconcileRecords(Record remoteRecord, Record localRecord, long lastRemoteRetrieval, @@ -708,15 +736,40 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo lastRemoteRetrieval, lastLocalRetrieval); + final BookmarkRecord remote = (BookmarkRecord) remoteRecord; + final BookmarkRecord local = (BookmarkRecord) localRecord; + // 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; + reconciled.children = remote.children; // *Always* track folders, though: if we decide we need to reposition items, we'll // untrack later. if (reconciled.isFolder()) { trackRecord(reconciled); } + + // We should always have: + // - local dateAdded + // - lastModified values for both records + // We might not have the remote dateAdded. + // We always pick the lowest value out of what is available. + long lowest = remote.lastModified; + + // During a similar operation, desktop clients consider dates before Jan 23, 1993 to be invalid. + // We do the same here out of a desire to be consistent. + final long releaseOfNCSAMosaicMillis = 727747200000L; + + if (local.dateAdded != null && local.dateAdded < lowest && local.dateAdded > releaseOfNCSAMosaicMillis) { + lowest = local.dateAdded; + } + + if (remote.dateAdded != null && remote.dateAdded < lowest && remote.dateAdded > releaseOfNCSAMosaicMillis) { + lowest = remote.dateAdded; + } + + reconciled.dateAdded = lowest; + return reconciled; } @@ -1089,6 +1142,7 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo 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.dateAdded = RepoUtils.getLongFromCursor(cur, BrowserContract.SyncColumns.DATE_CREATED); rec.androidID = RepoUtils.getLongFromCursor(cur, BrowserContract.Bookmarks._ID); rec.androidPosition = RepoUtils.getLongFromCursor(cur, BrowserContract.Bookmarks.POSITION); diff --git a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserRepositorySession.java b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserRepositorySession.java index d440fc138839..42a664199edd 100644 --- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserRepositorySession.java +++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserRepositorySession.java @@ -484,7 +484,7 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos if (existingRecord == null) { // The record is new. trace("No match. Inserting."); - insert(record); + insert(processBeforeInsertion(record)); return; } @@ -552,6 +552,13 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos storeDelegate.onRecordStoreSucceeded(record.guid); } + /** + * Identity function by default. Override in subclasses if necessary. + */ + protected Record processBeforeInsertion(Record toProcess) { + return toProcess; + } + protected void insert(Record record) throws NoGuidForIdException, NullCursorException, ParentNotFoundException { Record toStore = prepareRecord(record); Uri recordURI = dbHelper.insert(toStore); diff --git a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/domain/BookmarkRecord.java b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/domain/BookmarkRecord.java index 27b8e7151aef..2a94bee2c2f3 100644 --- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/domain/BookmarkRecord.java +++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/domain/BookmarkRecord.java @@ -4,6 +4,8 @@ package org.mozilla.gecko.sync.repositories.domain; +import android.support.annotation.Nullable; + import java.io.UnsupportedEncodingException; import java.net.URLEncoder; import java.util.Map; @@ -57,6 +59,10 @@ public class BookmarkRecord extends Record { public String type; public long androidPosition; + // This field started to be synced by clients in v55, so there's a chance it will be `null` + // if this instance is initialized from a remote payload. + @Nullable public Long dateAdded; + public JSONArray children; public JSONArray tags; @@ -104,6 +110,7 @@ public class BookmarkRecord extends Record { out.androidParentID = this.androidParentID; out.type = this.type; out.androidPosition = this.androidPosition; + out.dateAdded = this.dateAdded; out.children = this.copyChildren(); out.tags = this.copyTags(); @@ -174,6 +181,21 @@ public class BookmarkRecord extends Record { this.parentID = payload.getString("parentid"); this.parentName = payload.getString("parentName"); + if (!payload.containsKey("dateAdded")) { + // Missing data will be filled later on, during record insertion/reconciliation. + this.dateAdded = null; + } else { + // Oddly enough, in local testing both of these exception paths turned out to be necessary. + try { + this.dateAdded = payload.getLong("dateAdded"); + } catch (ClassCastException e) { + this.dateAdded = Long.parseLong(payload.getString("dateAdded")); + } catch (Exception e) { + Logger.error(LOG_TAG, "Cannot parse bookmark's 'dateAdded' value. Setting it to null.", e); + this.dateAdded = null; + } + } + if (isFolder()) { try { this.children = payload.getArray("children"); @@ -256,6 +278,16 @@ public class BookmarkRecord extends Record { putPayload(payload, "parentName", this.parentName); putPayload(payload, "keyword", this.keyword); + // This field is marked as nullable, so we're careful when unboxing it. However, we do not expect + // it to be null at this point, since it was constructed from our DB cursor which will always have + // a `created` timestamp. + // However, very old bookmarks that didn't have a corresponding history item might not have a + // creation date. `created` is not a NOT NULL field in the bookmarks table. Either way, it pays + // to be cautious here. + if (this.dateAdded != null) { + putPayload(payload, "dateAdded", Long.toString(this.dateAdded)); + } + if (isFolder()) { payload.put("children", this.children); return; @@ -451,6 +483,7 @@ public class BookmarkRecord extends Record { type: "bookmark", title: "Your Flight Status", parentName: "mobile", + dateAdded: 1490997969974, bmkUri: "http: //www.flightstats.com/go/Mobile/flightStatusByFlightProcess.do;jsessionid=13A6C8DCC9592AF141A43349040262CE.web3: 8009?utm_medium=cpc&utm_campaign=co-op&utm_source=airlineInformationAndStatus&id=212492593", tags: [], keyword: null, @@ -469,6 +502,7 @@ public class BookmarkRecord extends Record { parentName: "", title: "mobile", description: null, + dateAdded: 1490997969974, children: ["1ROdlTuIoddD", "3Z_bMIHPSZQ8", "4mSDUuOo2iVB", "8aEdE9IIrJVr", "9DzPTmkkZRDb", "Qwwb99HtVKsD", "s8tM36aGPKbq", "JMTi61hOO3JV", "JQUDk0wSvYip", "LmVH-J1r3HLz", "NhgQlC5ykYGW", "OVanevUUaqO2",