Skip to content

Overhaul Documents #1886

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

Closed
wants to merge 1 commit into from
Closed

Overhaul Documents #1886

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
100 changes: 35 additions & 65 deletions packages/firestore/src/api/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ import { Transaction as InternalTransaction } from '../core/transaction';
import { ChangeType, ViewSnapshot } from '../core/view_snapshot';
import { IndexedDbPersistence } from '../local/indexeddb_persistence';
import { LruParams } from '../local/lru_garbage_collector';
import { Document, MaybeDocument, NoDocument } from '../model/document';
import { getDocument } from '../model/collections';
import { Document } from '../model/document';
import { DocumentKey } from '../model/document_key';
import {
ArrayValue,
Expand Down Expand Up @@ -79,7 +80,7 @@ import { LogLevel } from '../util/log';
import { AutoId } from '../util/misc';
import * as objUtils from '../util/obj';
import { Rejecter, Resolver } from '../util/promise';
import { Deferred } from './../util/promise';
import { Deferred } from '../util/promise';
import { FieldPath as ExternalFieldPath } from './field_path';

import {
Expand Down Expand Up @@ -661,37 +662,18 @@ export class Transaction implements firestore.Transaction {
documentRef,
this._firestore
);
return this._transaction
.lookup([ref._key])
.then((docs: MaybeDocument[]) => {
if (!docs || docs.length !== 1) {
return fail('Mismatch in docs returned from document lookup.');
}
const doc = docs[0];
if (doc instanceof NoDocument) {
return new DocumentSnapshot(
this._firestore,
ref._key,
null,
/* fromCache= */ false,
/* hasPendingWrites= */ false
);
} else if (doc instanceof Document) {
return new DocumentSnapshot(
this._firestore,
ref._key,
doc,
/* fromCache= */ false,
/* hasPendingWrites= */ false
);
} else {
throw fail(
`BatchGetDocumentsRequest returned unexpected document type: ${
doc.constructor.name
}`
);
}
});
return this._transaction.lookup([ref._key]).then((docs: Document[]) => {
if (!docs || docs.length !== 1) {
return fail('Mismatch in docs returned from document lookup.');
}
const doc = docs[0];
return new DocumentSnapshot(
this._firestore,
doc,
/* fromCache= */ false,
/* hasPendingWrites= */ false
);
});
}

set(
Expand Down Expand Up @@ -1130,12 +1112,11 @@ export class DocumentReference implements firestore.DocumentReference {
snapshot.docs.size <= 1,
'Too many documents returned on a document query'
);
const doc = snapshot.docs.get(this._key);
const doc = getDocument(this._key, snapshot.docs);
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 if this helper pays for itself (I couldn't guess what it did, so I had to look it up only to find out it's a pretty trivial 2-liner). What about this?

  const doc = Document.fromNullable(snapshot.docs.get(this._key));


observer.next(
new DocumentSnapshot(
this.firestore,
this._key,
doc,
snapshot.fromCache,
snapshot.hasPendingWrites
Expand Down Expand Up @@ -1170,10 +1151,9 @@ export class DocumentReference implements firestore.DocumentReference {
resolve(
new DocumentSnapshot(
this.firestore,
this._key,
doc,
/*fromCache=*/ true,
doc instanceof Document ? doc.hasLocalMutations : false
doc.hasLocalMutations
)
);
}, reject);
Expand Down Expand Up @@ -1262,8 +1242,7 @@ export interface SnapshotOptions extends firestore.SnapshotOptions {}
export class DocumentSnapshot implements firestore.DocumentSnapshot {
constructor(
private _firestore: Firestore,
private _key: DocumentKey,
public _document: Document | null,
public _document: Document,
private _fromCache: boolean,
private _hasPendingWrites: boolean
) {}
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 occurs to me that this code (and a lot of this PR) now has an implicit assumption that _document is not a .noEntry document (if it is, we'll behave as if it's a .isMissingDocument document). This opens us up to potential bugs in the future where we e.g. accidentally create a DocumentSnapshot for a .noEntry DocumentEntry.

Should we add an assert to the constructor?

hardAssert(!_document.noEntry, 'DocumentSnapshot created with noEntry DocumentEntry')

Expand All @@ -1273,7 +1252,7 @@ export class DocumentSnapshot implements firestore.DocumentSnapshot {
): firestore.DocumentData | undefined {
validateBetweenNumberOfArgs('DocumentSnapshot.data', arguments, 0, 1);
options = validateSnapshotOptions('DocumentSnapshot.data', options);
return !this._document
return !this._document.exists
? undefined
: this.convertObject(
this._document.data,
Expand All @@ -1290,33 +1269,31 @@ export class DocumentSnapshot implements firestore.DocumentSnapshot {
): unknown {
validateBetweenNumberOfArgs('DocumentSnapshot.get', arguments, 1, 2);
options = validateSnapshotOptions('DocumentSnapshot.get', options);
if (this._document) {
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 super psyched about relying on document.data for nonexistent documents returning an empty ObjectValue instance. While convenient, it seems a bit non-obvious and so this code becomes less obviously-correct. I don't feel super strongly, but my preference would be to not rely on this and in fact to make .data throw for nonexistent documents.

const value = this._document.data.field(
fieldPathFromArgument('DocumentSnapshot.get', fieldPath)
const value = this._document.data.field(
fieldPathFromArgument('DocumentSnapshot.get', fieldPath)
);
if (value !== undefined) {
return this.convertValue(
value,
FieldValueOptions.fromSnapshotOptions(
options,
this._firestore._areTimestampsInSnapshotsEnabled()
)
);
if (value !== undefined) {
return this.convertValue(
value,
FieldValueOptions.fromSnapshotOptions(
options,
this._firestore._areTimestampsInSnapshotsEnabled()
)
);
}
}
return undefined;
}

get id(): string {
return this._key.path.lastSegment();
return this._document.key.path.lastSegment();
}

get ref(): firestore.DocumentReference {
return new DocumentReference(this._key, this._firestore);
return new DocumentReference(this._document.key, this._firestore);
}

get exists(): boolean {
return this._document !== null;
return this._document.exists;
}

get metadata(): firestore.SnapshotMetadata {
Expand All @@ -1330,10 +1307,7 @@ export class DocumentSnapshot implements firestore.DocumentSnapshot {
return (
this._firestore === other._firestore &&
this._fromCache === other._fromCache &&
this._key.isEqual(other._key) &&
(this._document === null
? other._document === null
: this._document.isEqual(other._document))
this._document.isEqual(other._document)
);
}

Expand All @@ -1359,7 +1333,7 @@ export class DocumentSnapshot implements firestore.DocumentSnapshot {
if (!value.databaseId.isEqual(database)) {
// TODO(b/64130202): Somehow support foreign references.
log.error(
`Document ${this._key.path} contains a document ` +
`Document ${this._document.key.path} contains a document ` +
`reference within a different database (` +
`${value.databaseId.projectId}/${
value.databaseId.database
Expand Down Expand Up @@ -1389,12 +1363,11 @@ export class QueryDocumentSnapshot extends DocumentSnapshot
implements firestore.QueryDocumentSnapshot {
constructor(
firestore: Firestore,
key: DocumentKey,
document: Document,
fromCache: boolean,
hasPendingWrites: boolean
) {
super(firestore, key, document, fromCache, hasPendingWrites);
super(firestore, document, fromCache, hasPendingWrites);
}

data(options?: SnapshotOptions): firestore.DocumentData {
Expand Down Expand Up @@ -2136,7 +2109,6 @@ export class QuerySnapshot implements firestore.QuerySnapshot {
private convertToDocumentImpl(doc: Document): QueryDocumentSnapshot {
return new QueryDocumentSnapshot(
this._firestore,
doc.key,
doc,
this.metadata.fromCache,
this._snapshot.mutatedKeys.has(doc.key)
Expand Down Expand Up @@ -2348,7 +2320,6 @@ export function changesFromSnapshot(
return snapshot.docChanges.map(change => {
const doc = new QueryDocumentSnapshot(
firestore,
change.doc.key,
change.doc,
snapshot.fromCache,
snapshot.mutatedKeys.has(change.doc.key)
Expand Down Expand Up @@ -2380,7 +2351,6 @@ export function changesFromSnapshot(
.map(change => {
const doc = new QueryDocumentSnapshot(
firestore,
change.doc.key,
change.doc,
snapshot.fromCache,
snapshot.mutatedKeys.has(change.doc.key)
Expand Down
13 changes: 6 additions & 7 deletions packages/firestore/src/core/firestore_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
documentKeySet,
DocumentMap
} from '../model/collections';
import { Document, MaybeDocument, NoDocument } from '../model/document';
import { Document } from '../model/document';
import { DocumentKey } from '../model/document_key';
import { Mutation } from '../model/mutation';
import { Platform } from '../platform/platform';
Expand Down Expand Up @@ -228,6 +228,7 @@ export class FirestoreClient {
* result of enabling offline persistence. This method will reject this if
* IndexedDB fails to start for any reason. If usePersistence is false
* this is unconditionally resolved.
* @param user The identity of the user that's currently active.
* @returns a Promise indicating whether or not initialization should
* continue, i.e. that one of the persistence implementations actually
* succeeded.
Expand Down Expand Up @@ -480,7 +481,7 @@ export class FirestoreClient {
}
});

// When a user calls clearPersistence() in one client, all other clientfs
// When a user calls clearPersistence() in one client, all other clients
// need to shut down to allow the delete to succeed.
await this.persistence.setDatabaseDeletedListener(async () => {
await this.shutdown();
Expand Down Expand Up @@ -543,17 +544,15 @@ export class FirestoreClient {
});
}

getDocumentFromLocalCache(docKey: DocumentKey): Promise<Document | null> {
getDocumentFromLocalCache(docKey: DocumentKey): Promise<Document> {
this.verifyNotShutdown();
return this.asyncQueue
.enqueue(() => {
return this.localStore.readDocument(docKey);
})
.then((maybeDoc: MaybeDocument | null) => {
if (maybeDoc instanceof Document) {
.then((maybeDoc: Document) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW in the fullness of time we should probably get rid of maybeDoc references, and (pending final rename decisions) perhaps move doc references toward docEntry etc... but I'm probably in favor of doing this cleanup in separate PRs. :-)

if (maybeDoc.definite) {
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 like definite (both the name itself and the mere fact that we're inventing an extra term as an alias for isDocument || isMissingDocument increasing the possibility of terminology confusion).

We could just inline doc.isDocument || doc.isMissingDocument everywhere... but that does seem a bit ugly, since it shows up a lot.

I'm not 100% sure this is a good idea, but I'm wondering if we could get rid of CONTENTS_UNKNOWN and instead just use UNKNOWN but with a real version number (instead of SnapshotVersion.MIN). I think that mostly "just works" and then this code can safely become if (!maybeDoc.noEntry) { ... } and all of our code can count on exactly one of isDocument, isMissingDocument, or noEntry being true.

return maybeDoc;
} else if (maybeDoc instanceof NoDocument) {
return null;
} else {
throw new FirestoreError(
Code.UNAVAILABLE,
Expand Down
1 change: 1 addition & 0 deletions packages/firestore/src/core/query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ export class Query {

matches(doc: Document): boolean {
return (
doc.exists &&
this.matchesPathAndCollectionGroup(doc) &&
this.matchesOrderBy(doc) &&
this.matchesFilters(doc) &&
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/src/core/snapshot_version.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export class SnapshotVersion {
return new SnapshotVersion(value);
}

static forDeletedDoc(): SnapshotVersion {
static forMissingDoc(): SnapshotVersion {
return SnapshotVersion.MIN;
}

Expand Down
10 changes: 5 additions & 5 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 @@ -23,9 +23,9 @@ import { ReferenceSet } from '../local/reference_set';
import {
documentKeySet,
DocumentKeySet,
MaybeDocumentMap
DocumentMap
} from '../model/collections';
import { MaybeDocument, NoDocument } from '../model/document';
import { Document } from '../model/document';
import { DocumentKey } from '../model/document_key';
import { Mutation } from '../model/mutation';
import { MutationBatchResult } from '../model/mutation_batch';
Expand Down Expand Up @@ -526,12 +526,12 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
// This is kind of a hack. Ideally, we would have a method in the local
// store to purge a document. However, it would be tricky to keep all of
// the local store's invariants with another method.
let documentUpdates = new SortedMap<DocumentKey, MaybeDocument>(
let documentUpdates = new SortedMap<DocumentKey, Document>(
DocumentKey.comparator
);
documentUpdates = documentUpdates.insert(
limboKey,
new NoDocument(limboKey, SnapshotVersion.forDeletedDoc())
Document.missing(limboKey, SnapshotVersion.forMissingDoc())
);
const resolvedLimboDocuments = documentKeySet().add(limboKey);
const event = new RemoteEvent(
Expand Down Expand Up @@ -758,7 +758,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
}

private async emitNewSnapsAndNotifyLocalStore(
changes: MaybeDocumentMap,
changes: DocumentMap,
remoteEvent?: RemoteEvent
): Promise<void> {
const newSnaps: ViewSnapshot[] = [];
Expand Down
29 changes: 12 additions & 17 deletions packages/firestore/src/core/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@

import { ParsedSetData, ParsedUpdateData } from '../api/user_data_converter';
import { documentVersionMap } from '../model/collections';
import { Document, NoDocument } from '../model/document';
import { MaybeDocument } from '../model/document';
import { Document } from '../model/document';
import { DocumentKey } from '../model/document_key';
import { DeleteMutation, Mutation, Precondition } from '../model/mutation';
import { Datastore } from '../remote/datastore';
Expand All @@ -38,16 +37,14 @@ export class Transaction {

constructor(private datastore: Datastore) {}

private recordVersion(doc: MaybeDocument): void {
private recordVersion(doc: Document): void {
let docVersion: SnapshotVersion;

if (doc instanceof Document) {
if (doc.exists) {
docVersion = doc.version;
} else if (doc instanceof NoDocument) {
// For deleted docs, we must use baseVersion 0 when we overwrite them.
docVersion = SnapshotVersion.forDeletedDoc();
} else {
throw fail('Document in a transaction was a ' + doc.constructor.name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm... while this case is no longer possible via the type system, we did introduce a new possibility which is that doc.noEntry could now be true. Should we throw an assert in to make sure (either for !doc.noEntry or for doc.isMissingDocument)?

// For deleted docs, we must use baseVersion 0 when we overwrite them.
Copy link
Contributor

Choose a reason for hiding this comment

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

Per my comment before, by switching from MaybeDocument to Document we now introduce the possibility that the caller has passed in a .noEntry Document. Should we add an assert here (either for .isMissingDocument or !.noEntry)?

docVersion = SnapshotVersion.forMissingDoc();
}

const existingVersion = this.readVersions.get(doc.key);
Expand All @@ -64,23 +61,21 @@ export class Transaction {
}
}

lookup(keys: DocumentKey[]): Promise<MaybeDocument[]> {
lookup(keys: DocumentKey[]): Promise<Document[]> {
if (this.committed) {
return Promise.reject<MaybeDocument[]>(
'Transaction has already completed.'
);
return Promise.reject<Document[]>('Transaction has already completed.');
}
if (this.mutations.length > 0) {
return Promise.reject<MaybeDocument[]>(
return Promise.reject<Document[]>(
'Transactions lookups are invalid after writes.'
);
}
return this.datastore.lookup(keys).then(docs => {
docs.forEach(doc => {
if (doc instanceof NoDocument || doc instanceof Document) {
if (doc.definite) {
this.recordVersion(doc);
} else {
fail('Document in a transaction was a ' + doc.constructor.name);
fail('Document in a transaction was a ' + doc.toString());
}
});
return docs;
Expand Down Expand Up @@ -115,7 +110,7 @@ export class Transaction {
*/
private preconditionForUpdate(key: DocumentKey): Precondition {
const version = this.readVersions.get(key);
if (version && version.isEqual(SnapshotVersion.forDeletedDoc())) {
if (version && version.isEqual(SnapshotVersion.forMissingDoc())) {
// The document doesn't exist, so fail the transaction.
throw new FirestoreError(
Code.FAILED_PRECONDITION,
Expand Down Expand Up @@ -145,7 +140,7 @@ export class Transaction {
// ensure that the precondition for the next write will be exists: false.
this.readVersions = this.readVersions.insert(
key,
SnapshotVersion.forDeletedDoc()
SnapshotVersion.forMissingDoc()
);
}

Expand Down
Loading