Skip to content

Merging Master into Multi-Tab #1038

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 3 commits into from
Jul 25, 2018
Merged

Merging Master into Multi-Tab #1038

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eecb5ab
Clean up merge conflicts
schmidt-sebastian Jul 24, 2018
a9b1fca
Move schemaVersion 0 block
schmidt-sebastian Jul 25, 2018
dbb95ef
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Jul 25, 2018
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
12 changes: 1 addition & 11 deletions packages/firestore/src/core/sync_engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import { assert, fail } from '../util/assert';
import { FirestoreError } from '../util/error';
import * as log from '../util/log';
import { AnyJs, primitiveComparator } from '../util/misc';
import * as objUtils from '../util/obj';
import { ObjectMap } from '../util/obj_map';
import { Deferred } from '../util/promise';
import { SortedMap } from '../util/sorted_map';
Expand Down Expand Up @@ -194,13 +193,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
this.syncEngineListener === null,
'SyncEngine already has a subscriber.'
);
<<<<<<< HEAD
this.syncEngineListener = syncEngineListener;
this.limboCollector.addGarbageSource(this.limboDocumentRefs);
=======
this.viewHandler = viewHandler;
this.errorHandler = errorHandler;
>>>>>>> master
}

/**
Expand Down Expand Up @@ -501,16 +494,13 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {

async rejectListen(targetId: TargetId, err: FirestoreError): Promise<void> {
this.assertSubscribed('rejectListens()');
<<<<<<< HEAD

// PORTING NOTE: Multi-tab only.
this.sharedClientState.trackQueryUpdate(targetId, 'rejected', err);

const limboKey = this.limboKeysByTarget[targetId];
=======
const limboResolution = this.limboResolutionsByTarget[targetId];
const limboKey = limboResolution && limboResolution.key;
>>>>>>> master

if (limboKey) {
// Since this query failed, we won't want to manually unlisten to it.
// So go ahead and remove it from bookkeeping.
Expand Down
62 changes: 24 additions & 38 deletions packages/firestore/src/local/indexeddb_schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,25 +29,21 @@ import { SnapshotVersion } from '../core/snapshot_version';
* Schema Version for the Web client:
* 1. Initial version including Mutation Queue, Query Cache, and Remote Document
* Cache
<<<<<<< HEAD
* 2. Added targetCount to targetGlobal row.
* 3. Multi-Tab Support.
=======
* 2. Used to ensure a targetGlobal object exists and add targetCount to it. No
* longer required because migration 3 unconditionally clears it.
* 3. Dropped and re-created Query Cache to deal with cache corruption related
* to limbo resolution. Addresses
* https://github.com/firebase/firebase-ios-sdk/issues/1548
>>>>>>> master
* 4. Multi-Tab Support.
*/
export const SCHEMA_VERSION = 3;
export const SCHEMA_VERSION = 4;

/**
* Performs database creation and schema upgrades.
*
* Note that in production, this method is only ever used to upgrade the schema
* to SCHEMA_VERSION. Different versions are only used for testing and
* local feature development.
* to SCHEMA_VERSION. Different values of toVersion are only used for testing
* and local feature development.
*/
export function createOrUpgradeDb(
db: IDBDatabase,
Expand All @@ -56,14 +52,9 @@ export function createOrUpgradeDb(
toVersion: number
): PersistencePromise<void> {
assert(
<<<<<<< HEAD
fromVersion < toVersion && fromVersion >= 0 && toVersion <= 3,
=======
fromVersion < toVersion && fromVersion >= 0 && toVersion <= SCHEMA_VERSION,
>>>>>>> master
'Unexpected schema upgrade from v${fromVersion} to v{toVersion}.'
);
let p = PersistencePromise.resolve();

if (fromVersion < 1 && toVersion >= 1) {
createOwnerStore(db);
Expand All @@ -72,14 +63,21 @@ export function createOrUpgradeDb(
createRemoteDocumentCache(db);
}

<<<<<<< HEAD
if (fromVersion < 2 && toVersion >= 2) {
p = p
.next(() => ensureTargetGlobalExists(txn))
.next(targetGlobal => saveTargetCount(txn, targetGlobal));
// Migration 2 to populate the targetGlobal object no longer needed since
// migration 3 unconditionally clears it.

let p = PersistencePromise.resolve();
if (fromVersion < 3 && toVersion >= 3) {
// Brand new clients don't need to drop and recreate--only clients that
// potentially have corrupt data.
if (fromVersion !== 0) {
dropQueryCache(db);
createQueryCache(db);
}
p = p.next(() => writeEmptyTargetGlobalEntry(txn));
}

if (fromVersion > 0 && fromVersion < 3 && toVersion >= 3) {
if (fromVersion > 0 && fromVersion < 4 && toVersion >= 4) {
// Schema version 3 uses auto-generated keys to generate globally unique
// mutation batch IDs (this was previously ensured internally by the
// client). To migrate to the new schema, we have to read all mutations
Expand All @@ -89,25 +87,11 @@ export function createOrUpgradeDb(
p = p.next(() => upgradeMutationBatchSchemaAndMigrateData(db, txn));
}

if (fromVersion < 3 && toVersion >= 3) {
if (fromVersion < 4 && toVersion >= 4) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm. So it doesn't have to be in this PR, but I would be a fan of having a single block per migration, and then the fromVersion==0 special case will also end up looking more like the above migration. i.e.:

if (fromVersion < 4 && toVersion >= 4) {
  // Brand new clients don't need to rewrite the mutations.
  if (fromVersion !== 0) {
    // mutation queue migration
  }

  // create client metadata store and remote document changes store
}

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'm gonna pretend this is part of the merge and take care of it in this PR. It does clean this up a bit.

p = p.next(() => {
createClientMetadataStore(db);
createRemoteDocumentChangesStore(db);
});
=======
// Migration 2 to populate the targetGlobal object no longer needed since
// migration 3 unconditionally clears it.

let p = PersistencePromise.resolve();
if (fromVersion < 3 && toVersion >= 3) {
// Brand new clients don't need to drop and recreate--only clients that
// potentially have corrupt data.
if (fromVersion !== 0) {
dropQueryCache(db);
createQueryCache(db);
}
p = p.next(() => writeEmptyTargetGlobalEntry(txn));
>>>>>>> master
}

return p;
Expand Down Expand Up @@ -711,12 +695,14 @@ export const V1_STORES = [
DbTargetDocument.store
];

// V2 is no longer usable (see comment at top of file)

// Visible for testing
export const V2_STORES = V1_STORES;
export const V3_STORES = V1_STORES;

// Visible for testing
export const V3_STORES = [
...V2_STORES,
export const V4_STORES = [
...V3_STORES,
DbClientMetadata.store,
DbRemoteDocumentChanges.store
];
Expand All @@ -726,4 +712,4 @@ export const V3_STORES = [
* used when creating transactions so that access across all stores is done
* atomically.
*/
export const ALL_STORES = V3_STORES;
export const ALL_STORES = V4_STORES;
70 changes: 11 additions & 59 deletions packages/firestore/test/unit/local/indexeddb_schema.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,17 @@ import {
createOrUpgradeDb,
DbMutationBatch,
DbMutationBatchKey,
<<<<<<< HEAD
DbOwner,
DbOwnerKey,
DbTarget,
DbTargetGlobal,
DbTargetGlobalKey,
DbTargetKey,
DbTimestamp,
SCHEMA_VERSION,
V1_STORES,
V2_STORES,
V3_STORES
V3_STORES,
V4_STORES
} from '../../../src/local/indexeddb_schema';
import { SimpleDb, SimpleDbTransaction } from '../../../src/local/simple_db';
import { PersistencePromise } from '../../../src/local/persistence_promise';
Expand All @@ -41,16 +42,7 @@ import { JsonProtoSerializer } from '../../../src/remote/serializer';
import { PlatformSupport } from '../../../src/platform/platform';
import { AsyncQueue } from '../../../src/util/async_queue';
import { SharedFakeWebStorage, TestPlatform } from '../../util/test_platform';
=======
DbTarget,
DbTargetGlobal,
DbTargetGlobalKey,
DbTargetKey,
DbTimestamp
} from '../../../src/local/indexeddb_schema';
import { SimpleDb, SimpleDbTransaction } from '../../../src/local/simple_db';
import { SnapshotVersion } from '../../../src/core/snapshot_version';
>>>>>>> master

const INDEXEDDB_TEST_DATABASE_PREFIX = 'schemaTest/';
const INDEXEDDB_TEST_DATABASE =
Expand Down Expand Up @@ -144,31 +136,6 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
});
});

<<<<<<< HEAD
it('can install schema version 2', () => {
return withDb(2, db => {
expect(db.version).to.equal(2);
// We should have all of the stores, we should have the target global row
// and we should not have any targets counted, because there are none.
expect(getAllObjectStores(db)).to.have.members(V2_STORES);
// Check the target count. We haven't added any targets, so we expect 0.
return getTargetCount(db).then(targetCount => {
expect(targetCount).to.equal(0);
});
});
});

it('can install schema version 3', () => {
return withDb(3, async db => {
expect(db.version).to.be.equal(3);
expect(getAllObjectStores(db)).to.have.members(V3_STORES);
});
});

it('can upgrade from schema version 1 to 2', () => {
const expectedTargetCount = 5;
return withDb(1, db => {
=======
it('drops the query cache from 2 to 3', () => {
const userId = 'user';
const batchId = 1;
Expand All @@ -189,7 +156,6 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
);

return withDb(2, db => {
>>>>>>> master
const sdb = new SimpleDb(db);
return sdb.runTransaction(
'readwrite',
Expand All @@ -213,24 +179,11 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
.next(() => mutations.put(expectedMutation))
);
}
<<<<<<< HEAD
return p;
});
}).then(() =>
withDb(2, db => {
expect(db.version).to.equal(2);
expect(getAllObjectStores(db)).to.have.members(V2_STORES);
return getTargetCount(db).then(targetCount => {
expect(targetCount).to.equal(expectedTargetCount);
});
})
);
=======
);
}).then(() => {
return withDb(3, db => {
expect(db.version).to.equal(3);
expect(getAllObjectStores(db)).to.have.members(ALL_STORES);
expect(getAllObjectStores(db)).to.have.members(V3_STORES);

const sdb = new SimpleDb(db);
return sdb.runTransaction(
Expand Down Expand Up @@ -259,7 +212,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
const expected = JSON.parse(JSON.stringify(resetTargetGlobal));
expect(targetGlobalEntry).to.deep.equal(expected);
})
.next(() => mutations.get([userId, batchId]))
.next(() => mutations.get(batchId))
.next(mutation => {
// Mutations should be unaffected.
expect(mutation.userId).to.equal(userId);
Expand All @@ -269,10 +222,9 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
);
});
});
>>>>>>> master
});

it('can upgrade from schema version 2 to 3', () => {
it('can upgrade from schema version 3 to 4', () => {
const testWrite = { delete: 'foo' };
const testMutations = [
{
Expand All @@ -295,7 +247,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
}
];

return withDb(2, db => {
return withDb(3, db => {
const sdb = new SimpleDb(db);
return sdb.runTransaction('readwrite', [DbMutationBatch.store], txn => {
const store = txn.store(DbMutationBatch.store);
Expand All @@ -306,9 +258,9 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
return p;
});
}).then(() =>
withDb(3, db => {
expect(db.version).to.be.equal(3);
expect(getAllObjectStores(db)).to.have.members(V3_STORES);
withDb(4, db => {
expect(db.version).to.be.equal(4);
expect(getAllObjectStores(db)).to.have.members(V4_STORES);

const sdb = new SimpleDb(db);
return sdb.runTransaction('readwrite', [DbMutationBatch.store], txn => {
Expand Down
Loading