Skip to content

Keep track of number of queries in the query cache #510

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.

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 61 commits into from
Feb 27, 2018
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
eb44929
Adding Schema Migration
schmidt-sebastian Feb 2, 2018
07e44e7
Pseudocode for Schema Migration
schmidt-sebastian Feb 2, 2018
3e1053f
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Feb 2, 2018
5e84782
IndexedDb Schema Migration
schmidt-sebastian Feb 5, 2018
fd50301
Lint cleanup
schmidt-sebastian Feb 6, 2018
be3b463
Removing unused import
schmidt-sebastian Feb 6, 2018
de237c5
Removing user ID from instance row
schmidt-sebastian Feb 6, 2018
53e56b5
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Feb 6, 2018
7b3bedb
Review comments
schmidt-sebastian Feb 7, 2018
7b97c0a
Merge branch 'master' into multitab-schemamigration
schmidt-sebastian Feb 7, 2018
c176eff
Lint fixes
schmidt-sebastian Feb 7, 2018
9154aad
Review
schmidt-sebastian Feb 7, 2018
6b072ff
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Feb 7, 2018
662365f
Fixing the tests
schmidt-sebastian Feb 7, 2018
dd21376
Closing the Database in the Schema tests
schmidt-sebastian Feb 8, 2018
5e54b3a
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Feb 8, 2018
cb81df8
Changing test helper to close the DB
schmidt-sebastian Feb 8, 2018
7991970
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Feb 8, 2018
2c6d6e1
Making v2 the default version
schmidt-sebastian Feb 9, 2018
f16bb4f
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Feb 9, 2018
038c159
Addressing comment
schmidt-sebastian Feb 9, 2018
b70303c
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Feb 9, 2018
34ab25a
Renamed to ALL_STORES
schmidt-sebastian Feb 9, 2018
f45be5a
Start work on adding query counts
Feb 12, 2018
44a2da8
Merge branch 'master' into add_query_count
Feb 12, 2018
fac7d17
Implement target count
Feb 13, 2018
55f5ff6
[AUTOMATED]: Prettier Code Styling
Feb 13, 2018
2d955e9
Separate out add and update for the query cache
Feb 13, 2018
d09153a
[AUTOMATED]: Prettier Code Styling
Feb 13, 2018
b1be9e9
Comments and formatting
Feb 13, 2018
9091d38
Merge branch 'master' into add_query_count
Feb 13, 2018
31119ec
Comments and restructuring
Feb 14, 2018
640ab7c
[AUTOMATED]: Prettier Code Styling
Feb 14, 2018
8527d67
Use SimpleDb, shorten iOS-y name
Feb 14, 2018
3ad42d8
[AUTOMATED]: Prettier Code Styling
Feb 14, 2018
9da5ecb
addTargetCount -> saveTargetCount
Feb 14, 2018
6d6a9d1
Fix lint warnings
Feb 14, 2018
80077ef
Comment fixes
Feb 15, 2018
dbf8a7d
Rename test file
Feb 15, 2018
62892cf
Add expectation for 0 targets if none were added
Feb 15, 2018
875595d
[AUTOMATED]: Prettier Code Styling
Feb 15, 2018
6156d25
Switch to PersistenceTransaction for schema upgrade
Feb 15, 2018
fbfa133
Fix the other tests
Feb 15, 2018
ba0a2d0
[AUTOMATED]: Prettier Code Styling
Feb 15, 2018
730fe57
Update comment
Feb 15, 2018
9e9a5d1
Add some asserts, revert to PersistencePromise
Feb 16, 2018
5d917f1
Add assert
Feb 16, 2018
c479800
[AUTOMATED]: Prettier Code Styling
Feb 16, 2018
28f7c48
helpers moved
Feb 16, 2018
8e452fa
Review feedback
Feb 16, 2018
edf8376
Switch to just using fail()
Feb 16, 2018
6d48526
Rename updateMetadata
Feb 16, 2018
4a85dfa
Renaming and thread metadata through schema upgrade
Feb 16, 2018
4a74e36
Merge branch 'master' into add_query_count
Feb 16, 2018
cc763dd
Use the proper assert
Feb 16, 2018
eb2b0a3
Review feedback
Feb 21, 2018
67fe837
[AUTOMATED]: Prettier Code Styling
Feb 21, 2018
860818e
Drop note re running time, initialize metadata to null
Feb 22, 2018
d9b551f
Fix tests that weren't calling start
Feb 23, 2018
231ef2e
Merge branch 'master' into add_query_count
Feb 27, 2018
097fdd0
[AUTOMATED]: Prettier Code Styling
Feb 27, 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
9 changes: 7 additions & 2 deletions packages/firestore/src/local/indexeddb_persistence.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,13 @@ import { AutoId } from '../util/misc';
import { IndexedDbMutationQueue } from './indexeddb_mutation_queue';
import { IndexedDbQueryCache } from './indexeddb_query_cache';
import { IndexedDbRemoteDocumentCache } from './indexeddb_remote_document_cache';
import { ALL_STORES, DbOwner, DbOwnerKey } from './indexeddb_schema';
import { createOrUpgradeDb, SCHEMA_VERSION } from './indexeddb_schema';
import {
ALL_STORES,
createOrUpgradeDb,
DbOwner,
DbOwnerKey,
SCHEMA_VERSION
} from './indexeddb_schema';
import { LocalSerializer } from './local_serializer';
import { MutationQueue } from './mutation_queue';
import { Persistence } from './persistence';
Expand Down
81 changes: 60 additions & 21 deletions packages/firestore/src/local/indexeddb_query_cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ import {
DbTargetDocumentKey,
DbTargetGlobal,
DbTargetGlobalKey,
DbTargetKey,
DbTimestamp
DbTargetKey
} from './indexeddb_schema';
import { LocalSerializer } from './local_serializer';
import { PersistenceTransaction } from './persistence';
Expand All @@ -56,7 +55,8 @@ export class IndexedDbQueryCache implements QueryCache {
private metadata = new DbTargetGlobal(
/*highestTargetId=*/ 0,
/*lastListenSequenceNumber=*/ 0,
SnapshotVersion.MIN.toTimestamp()
SnapshotVersion.MIN.toTimestamp(),
/*targetCount=*/ 0
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you've reworked this so that the DBTargetGlobal always exists, I think we don't need to initialize metadata here, and I'd prefer not have these defaults duplicated unnecessarily between here and indexeddb_schema.ts. So perhaps change this to just:

private metadata: DbTargetGlobal = null;

Copy link
Author

Choose a reason for hiding this comment

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

Done, and fixed the bug in the tests where we weren't calling start() but didn't know because we were initializing this to a dummy value.

So, good suggestion!


/** The garbage collector to notify about potential garbage keys. */
Expand Down Expand Up @@ -97,36 +97,75 @@ export class IndexedDbQueryCache implements QueryCache {
);
}

addQueryData(
private saveQueryData(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We usually put private helpers after the public methods that use them.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

transaction: PersistenceTransaction,
queryData: QueryData
): PersistencePromise<void> {
const targetId = queryData.targetId;
const addedQueryPromise = targetsStore(transaction).put(
this.serializer.toDbTarget(queryData)
);
if (targetId > this.metadata.highestTargetId) {
this.metadata.highestTargetId = targetId;
return addedQueryPromise.next(() =>
globalTargetStore(transaction).put(DbTargetGlobal.key, this.metadata)
);
} else {
return addedQueryPromise;
return targetsStore(transaction).put(this.serializer.toDbTarget(queryData));
}

// Updates the in-memory version of the metadata. Saving is done separately.
// Returns true if there were any changes to the metadata.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We usually use /** ... */ for method comments like this.

Copy link
Author

Choose a reason for hiding this comment

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

Updated.

private updateMetadata(queryData: QueryData): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this helper pays for itself since although it's used twice, we only use the return value once, so the other usage could just be "this.metadata.highestTargetId = Math.Max(...)"

If we keep it, can you rename to updateHighestTargetId() to better capture what it's doing and when it'll return true?

Copy link
Author

Choose a reason for hiding this comment

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

I updated the comment, but left the helper. It's being ported from here, which includes the sequence number bit: https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/Source/Local/FSTLevelDBQueryCache.mm#L139

let needsUpdate = false;
if (queryData.targetId > this.metadata.highestTargetId) {
this.metadata.highestTargetId = queryData.targetId;
needsUpdate = true;
}

// TODO(gsoltis): add sequence number check
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh. I guess maybe this is what motivates having this helper? I guess I'd still advocate renaming (eventually to updateTargetIdAndSequenceNumberGlobals or whatever)

Copy link
Author

Choose a reason for hiding this comment

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

It was originally updateMetadataForQueryData, but it was requested that I change it. I'd rather not spell out exactly what is getting updated in the method name, since it's conceivable that we might someday track other metadata as well. updateMetadataFromQueryData?

Copy link
Author

Choose a reason for hiding this comment

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

I went with updateMetadataFromQueryData to make it clear the updates are coming from the passed in QueryData. I'm open to other suggestions though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I think this is much clearer!

return needsUpdate;
}

removeQueryData(
private saveMetadata(
Copy link
Contributor

Choose a reason for hiding this comment

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

I was gonna claim ownership of the term 'metadata' (but Mike already shut me down). Can you make this more query-specific (updateQueryMetadata) ?

Copy link
Author

Choose a reason for hiding this comment

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

I think metadata is fine, since it's a private method referring specifically to the instance-scoped field called metadata. I don't think it lays claim to an sdk-wide notion of metadata.

Copy link
Contributor

Choose a reason for hiding this comment

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

Move this down with the other private helper methods? (probably next to saveQueryData)

Copy link
Author

Choose a reason for hiding this comment

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

Done.

transaction: PersistenceTransaction
): PersistencePromise<void> {
return globalTargetStore(transaction).put(
DbTargetGlobal.key,
this.metadata
);
}

addQueryData(
transaction: PersistenceTransaction,
queryData: QueryData
): PersistencePromise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it were me I'd add logic to verify that the query data doesn't already exist (asserting otherwise) as I suggested for the in-memory implementation. This is a perf hit, so I'd add a TODO to revisit for performance.

That said, having just the memory implementation enforce the contract is probably sufficient, so I'm okay with leaving it out here. I'd perhaps add a comment to make the expectation clear though, e.g.:

// Although we require that the target does not already exist, we don't enforce it here for performance (though MemoryQueryCache does).

[and similar for the updateQueryData() method]

Copy link
Author

Choose a reason for hiding this comment

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

I'd rather not add the read, as in practice we actually do the read before calling this anyways. I am intending on encapsulating this behavior though in the rethink of the interaction between LocalStore and the persistence layer. My current prototype calls for getOrCreateQueryData and various updateQuery methods that take a target ID and do a get first. The raw addQueryData and updateQueryData would no longer be exposed to the LocalStore.

return this.saveQueryData(transaction, queryData).next(() => {
this.metadata.targetCount += 1;
this.updateMetadata(queryData);
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned previously, it looks strange to call an updateMetadata() method after we've just manually updated metadata. It's not very clear what it actually does.

Copy link
Author

Choose a reason for hiding this comment

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

Renamed the method.

return this.saveMetadata(transaction);
});
}

updateQueryData(
transaction: PersistenceTransaction,
queryData: QueryData
): PersistencePromise<void> {
return this.removeMatchingKeysForTargetId(
transaction,
queryData.targetId
).next(() => {
targetsStore(transaction).delete(queryData.targetId);
return this.saveQueryData(transaction, queryData).next(() => {
if (this.updateMetadata(queryData)) {
return this.saveMetadata(transaction);
} else {
return PersistencePromise.resolve();
}
});
}

removeQueryData(
transaction: PersistenceTransaction,
queryData: QueryData
): PersistencePromise<void> {
return this.removeMatchingKeysForTargetId(transaction, queryData.targetId)
.next(() => targetsStore(transaction).delete(queryData.targetId))
.next(() => {
this.metadata.targetCount -= 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment for removeQueryData() says "no-op if no entry exists". We need to either change the comment (and add appropriate assertions / comments about existence requirement), or change the implementation to check for existence first...

As a sanity check, we may want to add a targetCount >= 0 assertion here.

Copy link
Author

Choose a reason for hiding this comment

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

Comment changed.

return this.saveMetadata(transaction);
});
}

count(): number {
return this.metadata.targetCount;
}

getQueryData(
transaction: PersistenceTransaction,
query: Query
Expand Down
194 changes: 149 additions & 45 deletions packages/firestore/src/local/indexeddb_schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,54 +21,55 @@ import { ResourcePath } from '../model/path';
import { assert } from '../util/assert';

import { encode, EncodedResourcePath } from './encoded_resource_path';
import { wrapRequest } from './simple_db';
import { PersistencePromise } from './persistence_promise';
import { SnapshotVersion } from '../core/snapshot_version';

export const SCHEMA_VERSION = 1;

/** Performs database creation and (in the future) upgrades between versions. */
export function createOrUpgradeDb(db: IDBDatabase, oldVersion: number): void {
assert(oldVersion === 0, 'Unexpected upgrade from version ' + oldVersion);

db.createObjectStore(DbMutationQueue.store, {
keyPath: DbMutationQueue.keyPath
});

// TODO(mikelehen): Get rid of "as any" if/when TypeScript fixes their
// types. https://github.com/Microsoft/TypeScript/issues/14322
db.createObjectStore(
DbMutationBatch.store,
// tslint:disable-next-line:no-any
{ keyPath: DbMutationBatch.keyPath as any }
);
/**
* Schema Version for the Web client (containing the Mutation Queue, the Query
* and the Remote Document Cache) and target count added to the target global.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment makes no sense to me. If you remove the parenthetical it says, "Schema version for the Web client and target count added to the target global." :-)

I'm not sure it's worth enumerating what the schema version represents here. I'd change it to just "Schema version for the Web client" or format it as a history or something:

/**
 * Schema Version for the Web client.
 *
 * Versions to date:
 * 1: Initial version including: Mutation Queue, Query Cache, Remote Document Cache
 * 2: Added 'targetCount' field to targetGlobal store.
 */

Copy link
Author

Choose a reason for hiding this comment

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

Done.

*/
export const SCHEMA_VERSION = 2;

const targetDocumentsStore = db.createObjectStore(
DbTargetDocument.store,
// tslint:disable-next-line:no-any
{ keyPath: DbTargetDocument.keyPath as any }
);
targetDocumentsStore.createIndex(
DbTargetDocument.documentTargetsIndex,
DbTargetDocument.documentTargetsKeyPath,
{ unique: true }
/**
* 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
Copy link
Contributor

Choose a reason for hiding this comment

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

"versions" => "toVersion values" ?

Copy link
Author

Choose a reason for hiding this comment

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

Done.

* local feature development.
*/
export function createOrUpgradeDb(
db: IDBDatabase,
txn: IDBTransaction,
Copy link
Contributor

Choose a reason for hiding this comment

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

The point of SimpleDb is to wrap IndexedDb in a nice async-friendly abstraction (by using PersistencePromise, and hiding the nasty onsuccess / onerror wrapping). We've been cheating a little bit by exposing IDBDatabase directly to this code (rather than adding the necessary wrapper functions to SimpleDb) since everything we do on IDBDatabase here is synchronous and so it's not bad to just use the raw IndexedDB API. But now that we're doing async stuff, I think we really want to wrap the upgrade transaction in a PersistenceTransaction and pass that into createOrUpgradeDb() rather than the raw IDBTransaction. This will remove the need for you to pull in wrapRequest() and let you just use the normal SimpleDb API like the rest of our code.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

fromVersion: number,
toVersion: number
): PersistencePromise<void> {
// This function currently supports migrating to schema version 1 (Mutation
// Queue, Query and Remote Document Cache) and schema version 2 (Query
// counting).
assert(
fromVersion < toVersion && fromVersion >= 0 && toVersion <= 2,
'Unexpected schema upgrade from v${fromVersion} to v{toVersion}.'
);

const targetStore = db.createObjectStore(DbTarget.store, {
keyPath: DbTarget.keyPath
});
// NOTE: This is unique only because the TargetId is the suffix.
targetStore.createIndex(
DbTarget.queryTargetsIndexName,
DbTarget.queryTargetsKeyPath,
{ unique: true }
);
if (fromVersion < 1 && toVersion >= 1) {
createOwnerStore(db);
createMutationQueue(db);
createQueryCache(db);
createRemoteDocumentCache(db);
}

// NOTE: keys for these stores are specified explicitly rather than using a
// keyPath.
db.createObjectStore(DbDocumentMutation.store);
db.createObjectStore(DbRemoteDocument.store);
db.createObjectStore(DbOwner.store);
db.createObjectStore(DbTargetGlobal.store);
let p = PersistencePromise.resolve();
if (fromVersion < 2 && toVersion >= 2) {
p = p.next(() => ensureTargetGlobal(txn)).next(() => saveTargetCount(txn));
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor but I'd rather combine these two methods and have saveTargetCount just gracefully handle if the row is missing. This 1) Avoids unnecessarily doing a get-put-get-put, and 2) Makes it clearer what is actually new in schema version 2 (the target count). If you want, you can keep ensureTargetGlobal() but make it return the TargetGlobal row, and then call it from within saveTargetCount().

Also, I suggest renaming saveTargetCount() to populateTargetCount() to make it clearer that it needs to do work to figure out what the target count needs to be.

Copy link
Author

Choose a reason for hiding this comment

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

Renamed to populateTargetCount. I was initially going to keep the return value of ensureTargetGlobal as void, since they are independent steps (making sure the global exists could be its own schema migration), but since both you and @schmidt-sebastian asked for it and I don't feel too strongly about it, I'll make the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the rename... I also still don't think there's a need to have two steps here. TargetGlobal was an existing part of the schema in v1. The only thing you added was a new field to it.

Another way to deal with this would be to get rid of ensureTargetGlobal() and have populateTargetCount() just do nothing if the TargetGlobal row doesn't exist yet (since it should get populated correctly as 0 when the TargetGlobal row gets created as a normal part of the client operating). And actually, I think I like this better since 1) it avoids us duplicating the default new DbTargetGlobal( ... ) code.

}
return p;
}

// TODO(mikelehen): Get rid of "as any" if/when TypeScript fixes their types.
// https://github.com/Microsoft/TypeScript/issues/14322
type KeyPath = any; // tslint:disable-line:no-any

/**
* Wrapper class to store timestamps (seconds and nanos) in IndexedDb objects.
*/
Expand All @@ -94,6 +95,10 @@ export class DbOwner {
constructor(public ownerId: string, public leaseTimestampMs: number) {}
}

function createOwnerStore(db: IDBDatabase): void {
db.createObjectStore(DbOwner.store);
}

/** Object keys in the 'mutationQueues' store are userId strings. */
export type DbMutationQueueKey = string;

Expand Down Expand Up @@ -183,6 +188,18 @@ export class DbMutationBatch {
*/
export type DbDocumentMutationKey = [string, EncodedResourcePath, BatchId];

function createMutationQueue(db: IDBDatabase): void {
db.createObjectStore(DbMutationQueue.store, {
keyPath: DbMutationQueue.keyPath
});

db.createObjectStore(DbMutationBatch.store, {
keyPath: DbMutationBatch.keyPath as KeyPath
});

db.createObjectStore(DbDocumentMutation.store);
}

/**
* An object to be stored in the 'documentMutations' store in IndexedDb.
*
Expand Down Expand Up @@ -241,6 +258,10 @@ export class DbDocumentMutation {
*/
export type DbRemoteDocumentKey = string[];

function createRemoteDocumentCache(db: IDBDatabase): void {
db.createObjectStore(DbRemoteDocument.store);
}

/**
* Represents the known absence of a document at a particular version.
* Stored in IndexedDb as part of a DbRemoteDocument object.
Expand Down Expand Up @@ -451,15 +472,91 @@ export class DbTargetGlobal {
* until the backend has caught up to this snapshot version again. This
* prevents our cache from ever going backwards in time.
*/
public lastRemoteSnapshotVersion: DbTimestamp
public lastRemoteSnapshotVersion: DbTimestamp,
/**
* The number of targets persisted.
*/
public targetCount: number
) {}
}

function createQueryCache(db: IDBDatabase): void {
const targetDocumentsStore = db.createObjectStore(DbTargetDocument.store, {
keyPath: DbTargetDocument.keyPath as KeyPath
});
targetDocumentsStore.createIndex(
DbTargetDocument.documentTargetsIndex,
DbTargetDocument.documentTargetsKeyPath,
{ unique: true }
);

const targetStore = db.createObjectStore(DbTarget.store, {
keyPath: DbTarget.keyPath
});

// NOTE: This is unique only because the TargetId is the suffix.
targetStore.createIndex(
DbTarget.queryTargetsIndexName,
DbTarget.queryTargetsKeyPath,
{ unique: true }
);
db.createObjectStore(DbTargetGlobal.store);
}

/**
* Counts the number of targets persisted and adds that value to the target
* global singleton.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The codebase usually puts a newline after the description, I think.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

* @param {IDBTransaction} txn The version upgrade transaction for indexeddb
Copy link
Contributor

Choose a reason for hiding this comment

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

jsdoc type annotation is unnecessary and no longer correct. :-)

Copy link
Author

Choose a reason for hiding this comment

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

Deleted.

* @return {PersistencePromise<void>} A promise of the operations to add the
* count to the metadata row.
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't generally put the param / return types in the doc comments since this is TypeScript... It's redundant / likely to become stale.

And I generally omit the "promise" part when documenting return values, so I'd treat this the same as a void function and just drop the @return comment entirely. The comments in our code where we do reference the promise are generally phrased as "A Promise that resolves once ..." so you could do that too.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

*/
function saveTargetCount(txn: IDBTransaction): PersistencePromise<void> {
const globalStore = txn.objectStore(DbTargetGlobal.store);
return wrapRequest<number>(txn.objectStore(DbTarget.store).count()).next(
(count: number) =>
wrapRequest<DbTargetGlobal>(globalStore.get(DbTargetGlobal.key)).next(
(metadata: DbTargetGlobal) => {
metadata.targetCount = count;
return wrapRequest<void>(
globalStore.put(metadata, DbTargetGlobal.key)
);
}
)
);
}

/**
* The list of all IndexedDB stored used by the SDK. This is used when creating
* transactions so that access across all stores is done atomically.
* Ensures that the target global singleton row exists by adding it if it's
* missing.
* @param {IDBTransaction} txn The version upgrade transaction for indexeddb
* @return {PersistencePromise<void>} A promise of operations to ensure that the
* metadata row exists
*/
export const ALL_STORES = [
function ensureTargetGlobal(txn: IDBTransaction): PersistencePromise<void> {
const globalStore = txn.objectStore(DbTargetGlobal.store);
return wrapRequest<DbTargetGlobal | null>(
globalStore.get(DbTargetGlobal.key)
).next((metadata: DbTargetGlobal | null) => {
if (metadata != null) {
return PersistencePromise.resolve();
} else {
return wrapRequest<void>(
globalStore.put(
new DbTargetGlobal(
/*highestTargetId=*/ 0,
/*lastListenSequenceNumber=*/ 0,
SnapshotVersion.MIN.toTimestamp(),
/*targetCount=*/ 0
),
DbTargetGlobal.key
)
);
}
});
}

// Visible for testing
export const V1_STORES = [
DbMutationQueue.store,
DbMutationBatch.store,
DbDocumentMutation.store,
Expand All @@ -469,3 +566,10 @@ export const ALL_STORES = [
DbTargetGlobal.store,
DbTargetDocument.store
];

/**
* The list of all default IndexedDB stores used throughout the SDK. This is
* used when creating transactions so that access across all stores is done
* atomically.
*/
export const ALL_STORES = [...V1_STORES];
Copy link
Contributor

Choose a reason for hiding this comment

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

This is weird... I wonder if we should get rid of the V1_STORES list or else do:

// No new stores added in V2.
const V2_STORES = V1_STORES;

...
export const ALL_STORES = V2_STORES;

(maybe V2_STORES is overkill, but I think we should at least have the comment explaining why we're using V1_STORES even though SCHEMA_VERSION is 2)

Copy link
Author

Choose a reason for hiding this comment

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

I got rid of V1_STORES, since there are not [yet] any new stores. Whoever adds new stores can decide how to split them up. Probably not with V2_STORES, since that won't be v2.

2 changes: 1 addition & 1 deletion packages/firestore/src/local/local_store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ export class LocalStore {
snapshotVersion: change.snapshotVersion
});
this.targetIds[targetId] = queryData;
promises.push(this.queryCache.addQueryData(txn, queryData));
promises.push(this.queryCache.updateQueryData(txn, queryData));
}
}
);
Expand Down
Loading