Skip to content

Canonical ID Schema Migration #1182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 31, 2020
Merged

Canonical ID Schema Migration #1182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,10 @@ public SQLitePersistence(
DatabaseId databaseId,
LocalSerializer serializer,
LruGarbageCollector.Params params) {
this(serializer, params, new OpenHelper(context, databaseName(persistenceKey, databaseId)));
this(
serializer,
params,
new OpenHelper(context, serializer, databaseName(persistenceKey, databaseId)));
}

public SQLitePersistence(
Expand Down Expand Up @@ -274,10 +277,12 @@ private long getPageCount() {
*/
private static class OpenHelper extends SQLiteOpenHelper {

private final LocalSerializer serializer;
private boolean configured;

OpenHelper(Context context, String databaseName) {
OpenHelper(Context context, LocalSerializer serializer, String databaseName) {
super(context, databaseName, null, SQLiteSchema.VERSION);
this.serializer = serializer;
}

@Override
Expand All @@ -303,13 +308,13 @@ private void ensureConfigured(SQLiteDatabase db) {
@Override
public void onCreate(SQLiteDatabase db) {
ensureConfigured(db);
new SQLiteSchema(db).runMigrations(0);
new SQLiteSchema(db, serializer).runMigrations(0);
}

@Override
public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {
ensureConfigured(db);
new SQLiteSchema(db).runMigrations(oldVersion);
new SQLiteSchema(db, serializer).runMigrations(oldVersion);
}

@Override
Expand Down
33 changes: 30 additions & 3 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class SQLiteSchema {
* The version of the schema. Increase this by one for each migration added to runMigrations
* below.
*/
static final int VERSION = 10;
static final int VERSION = 11;

// Remove this constant and increment VERSION to enable indexing support
static final int INDEXING_SUPPORT_VERSION = VERSION + 1;
Expand All @@ -65,9 +65,11 @@ class SQLiteSchema {

private final SQLiteDatabase db;

// PORTING NOTE: The Android client doesn't need to use a serializer to remove held write acks.
SQLiteSchema(SQLiteDatabase db) {
private final LocalSerializer serializer;

SQLiteSchema(SQLiteDatabase db, LocalSerializer serializer) {
this.db = db;
this.serializer = serializer;
}

void runMigrations() {
Expand Down Expand Up @@ -151,6 +153,11 @@ void runMigrations(int fromVersion, int toVersion) {
dropLastLimboFreeSnapshotVersion();
}

if (fromVersion < 11 && toVersion >= 11) {
// Schema version 11 changed the format of canonical IDs in the target cache.
rewriteCanonicalIds();
}

/*
* Adding a new migration? READ THIS FIRST!
*
Expand Down Expand Up @@ -530,6 +537,26 @@ List<String> getTableColumns(String table) {
return columns;
}

private void rewriteCanonicalIds() {
new SQLitePersistence.Query(db, "SELECT target_id, target_proto FROM targets")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dumb question: this file doesn't appear to employ a transaction anywhere. Doesn't db.execSQL auto-commit if you don't? It seems like it would be bad if this partially committed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The transaction is opened here: https://cs.corp.google.com/aosp-master/frameworks/base/core/java/android/database/sqlite/SQLiteOpenHelper.java?q=f:SQLiteopenhelper.java&dr&l=409

It will only get committed if line 421 executes.

.forEach(
cursor -> {
int targetId = cursor.getInt(0);
byte[] targetProtoBytes = cursor.getBlob(1);

try {
Target targetProto = Target.parseFrom(targetProtoBytes);
TargetData targetData = serializer.decodeTargetData(targetProto);
String updatedCanonicalId = targetData.getTarget().getCanonicalId();
db.execSQL(
"UPDATE targets SET canonical_id = ? WHERE target_id = ?",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This migration can't be rolled back and has the potential to cause problems because target_id must be guaranteed unique (it's the primary key of the table).

If a user does roll back, will this actually cause a problem with duplicate target ids? I think probably not because the next target id is in target_globals and we're not changing that here, right?

One alternative that makes this slightly more rollback friendly would be to put the new target id in a new column and create an additional index on that column. I can't say I'm super excited about this prospect and likely would prefer to just document that this release includes a migration that can't be rolled-back from.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. The fix for this is actually pretty straightforward as we can always rewrite canonical IDs with the current format. The target IDs don't change and as as long the format in the schema matches the expected format at the current client version we should be safe. I modified the schema converter to support downgrades and updated the code to run this particular migration on both up- and downgrades.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I just realized that the new version of the schema converter is pretty dumb. It assumes that my shiny new code is shipped to old SDKs. What do you think about submitting the PR in its original form and letting LRU GC delete stale QueryData if the user chooses to downgrade?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Letting LRU GC do its thing SGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted the commit that pretended to add support for downgrades.

new Object[] {updatedCanonicalId, targetId});
} catch (InvalidProtocolBufferException e) {
throw fail("Failed to decode Query data for target %s", targetId);
}
});
};

private boolean tableExists(String table) {
return !new SQLitePersistence.Query(db, "SELECT 1=1 FROM sqlite_master WHERE tbl_name = ?")
.binding(table)
Expand Down
49 changes: 45 additions & 4 deletions firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import static com.google.firebase.firestore.local.EncodedPath.decodeResourcePath;
import static com.google.firebase.firestore.local.EncodedPath.encode;
import static com.google.firebase.firestore.testutil.TestUtil.filter;
import static com.google.firebase.firestore.testutil.TestUtil.key;
import static com.google.firebase.firestore.testutil.TestUtil.map;
import static com.google.firebase.firestore.testutil.TestUtil.path;
Expand All @@ -33,6 +34,7 @@
import android.database.sqlite.SQLiteOpenHelper;
import androidx.test.core.app.ApplicationProvider;
import com.google.firebase.database.collection.ImmutableSortedMap;
import com.google.firebase.firestore.core.Query;
import com.google.firebase.firestore.model.DatabaseId;
import com.google.firebase.firestore.model.DocumentKey;
import com.google.firebase.firestore.model.ResourcePath;
Expand Down Expand Up @@ -68,8 +70,13 @@ public class SQLiteSchemaTest {
private SQLiteSchema schema;
private SQLiteOpenHelper opener;

private DatabaseId databaseId;
private LocalSerializer serializer;

@Before
public void setUp() {
databaseId = DatabaseId.forProject("foo");
serializer = new LocalSerializer(new RemoteSerializer(databaseId));
opener =
new SQLiteOpenHelper(ApplicationProvider.getApplicationContext(), "foo", null, 1) {
@Override
Expand All @@ -79,7 +86,7 @@ public void onCreate(SQLiteDatabase db) {}
public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {}
};
db = opener.getWritableDatabase();
schema = new SQLiteSchema(db);
schema = new SQLiteSchema(db, serializer);
}

@After
Expand Down Expand Up @@ -521,14 +528,48 @@ public void keepsLastLimboFreeSnapshotIfNotDowngraded() {
Target targetProto = Target.parseFrom(targetProtoBytes);
assertTrue(targetProto.hasLastLimboFreeSnapshotVersion());
} catch (InvalidProtocolBufferException e) {
fail("Failed to decode Query data");
fail("Failed to decode Target data");
}
});
}

@Test
public void rewritesCanonicalIds() {
schema.runMigrations(0, 10);

Query filteredQuery = query("colletion").filter(filter("foo", "==", "bar"));
TargetData initialTargetData =
new TargetData(
filteredQuery.toTarget(),
/* targetId= */ 2,
/* sequenceNumber= */ 1,
QueryPurpose.LISTEN);

db.execSQL(
"INSERT INTO targets (target_id, canonical_id, target_proto) VALUES (?,?, ?)",
new Object[] {
2, "invalid_canonical_id", serializer.encodeTargetData(initialTargetData).toByteArray()
});

schema.runMigrations(10, 11);
new SQLitePersistence.Query(db, "SELECT canonical_id, target_proto, canonical_id FROM targets")
.forEach(
cursor -> {
String actualCanonicalId = cursor.getString(0);
byte[] targetProtoBytes = cursor.getBlob(1);

try {
Target targetProto = Target.parseFrom(targetProtoBytes);
TargetData targetData = serializer.decodeTargetData(targetProto);
String expectedCanonicalId = targetData.getTarget().getCanonicalId();
assertEquals(actualCanonicalId, expectedCanonicalId);
} catch (InvalidProtocolBufferException e) {
fail("Failed to decode Target data");
}
});
}

private SQLiteRemoteDocumentCache createRemoteDocumentCache() {
DatabaseId databaseId = DatabaseId.forProject("foo");
LocalSerializer serializer = new LocalSerializer(new RemoteSerializer(databaseId));
SQLitePersistence persistence =
new SQLitePersistence(serializer, LruGarbageCollector.Params.Default(), opener);
persistence.start();
Expand Down