Bug 1395703 - Make sure modifiedBySync CV field isn't passed to ContentProvider on updates r=rnewman
authorGrigory Kruglov <gkruglov@mozilla.com>
Thu, 31 Aug 2017 18:05:20 -0400
changeset 378112 b18cad4e3274cb2a6b92eb95779f099b79c4fd5d
parent 378111 001d66bec22f15c253e21306c72d04b518b94dd0
child 378113 aa111604eaa34be601b91b32aa077939b4252c28
push id50181
push usergkruglov@mozilla.com
push dateFri, 01 Sep 2017 01:40:00 +0000
treeherderautoland@b18cad4e3274 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1395703, 1392802
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1395703 - Make sure modifiedBySync CV field isn't passed to ContentProvider on updates r=rnewman Comment from bugzilla on this ugly hack: While processing bookmarks, we sometimes need to mark them for re-upload as we're inserting new ones or updating existing ones. For example, we might set or update a dateAdded field. We perform insertions "in-bulk", and so we might be inserting some bookmarks which need to be re-uploaded, and some bookmarks which don't. We compile an array of ContentValue objects, and make a single call to our ContentProvider. This means we can't use a URI param to indicate our intent, and so a non-column field in ContentValues objects - modifiedFromSync - is set for those bookmarks which need special treatment during insertion. Bug 1392802 added a similar mechanism for updating bookmarks. However, updates are done differently - currently, we perform a single call to our ContentProvider for each bookmark. Which means we _can_ use a URI field as a signaling mechanism, which is what that patch did. However, in haste I forgot to take into consideration existing signaling mechanism, which lead to update failures. And so we're left with an even clumsier interface to our data store, with two ways to signal the same thing in different circumstances... A quick solution is to just make sure 'modifiedBySync' field never makes its way to contentprovider on updates; a more refined fix would probably modify update logic to use a ContentValues field for consistency... Either way, there's going to be something ugly, somewhere in the code. I anticipate a lot of this code changing sometime soon in order to support better transactionality of bookmark syncing, and smarter merging, and so I'm inclined to just to the simple thing for now. MozReview-Commit-ID: H10LFsqjbFY
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksDataAccessor.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksDataAccessor.java
@@ -110,16 +110,21 @@ public class BookmarksDataAccessor exten
    * Update a record identified by GUID to new values, but only if assertion passes that localVersion
    * did not change. This is expected to be called during record reconciliation, and so we also request
    * that localVersion is incremented in the process.
   /* package-private */ boolean updateAssertingLocalVersion(String guid, int expectedLocalVersion, boolean shouldIncrementLocalVersion, Record newRecord) {
     final ContentValues cv = getContentValues(newRecord);
+    // A (hopefully) temporary hack, see https://bugzilla.mozilla.org/show_bug.cgi?id=1395703#c1
+    // We don't need this flag in CVs, since we're signaling the same thing via uri (see below).
+    cv.remove(BrowserContract.Bookmarks.PARAM_INSERT_FROM_SYNC_AS_MODIFIED);
     final Bundle data = new Bundle();
     data.putString(BrowserContract.SyncColumns.GUID, guid);
     data.putInt(BrowserContract.VersionColumns.LOCAL_VERSION, expectedLocalVersion);
     data.putParcelable(BrowserContract.METHOD_PARAM_DATA, cv);
     final Uri callUri;
     if (shouldIncrementLocalVersion) {
       callUri = withLocalVersionIncrement(getUri());