Skip to content

Remove Held Write Acks #1135

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 22 commits into from
Sep 5, 2018
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
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
50 changes: 35 additions & 15 deletions packages/firestore/src/api/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -593,11 +593,27 @@ export class Transaction implements firestore.Transaction {
}
const doc = docs[0];
if (doc instanceof NoDocument) {
return new DocumentSnapshot(this._firestore, ref._key, null, false);
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, false);
return new DocumentSnapshot(
this._firestore,
ref._key,
doc,
/* fromCache= */ false,
/* hasPendingWrites= */ false
);
} else {
throw fail('MaybeDocument is neither Document nor NoDocument');
throw fail(
`BatchGetDocumentsRequest returned unexpected document type: ${
doc.constructor.name
}`
);
}
});
}
Expand Down Expand Up @@ -1040,7 +1056,8 @@ export class DocumentReference implements firestore.DocumentReference {
this.firestore,
this._key,
doc,
snapshot.fromCache
snapshot.fromCache,
snapshot.hasPendingWrites
)
);
}
Expand Down Expand Up @@ -1082,7 +1099,8 @@ export class DocumentReference implements firestore.DocumentReference {
this.firestore,
this._key,
doc,
/*fromCache=*/ true
/*fromCache=*/ true,
doc instanceof Document ? doc.hasLocalMutations : false
)
);
}, reject);
Expand Down Expand Up @@ -1173,7 +1191,8 @@ export class DocumentSnapshot implements firestore.DocumentSnapshot {
private _firestore: Firestore,
private _key: DocumentKey,
public _document: Document | null,
private _fromCache: boolean
private _fromCache: boolean,
private _hasPendingWrites: boolean
) {}

data(
Expand Down Expand Up @@ -1228,10 +1247,7 @@ export class DocumentSnapshot implements firestore.DocumentSnapshot {
}

get metadata(): firestore.SnapshotMetadata {
return new SnapshotMetadata(
this._document !== null && this._document.hasLocalMutations,
this._fromCache
);
return new SnapshotMetadata(this._hasPendingWrites, this._fromCache);
}

isEqual(other: firestore.DocumentSnapshot): boolean {
Expand Down Expand Up @@ -1299,9 +1315,10 @@ export class QueryDocumentSnapshot extends DocumentSnapshot
firestore: Firestore,
key: DocumentKey,
document: Document,
fromCache: boolean
fromCache: boolean,
hasPendingWrites: boolean
) {
super(firestore, key, document, fromCache);
super(firestore, key, document, fromCache, hasPendingWrites);
}

data(options?: SnapshotOptions): firestore.DocumentData {
Expand Down Expand Up @@ -1945,7 +1962,8 @@ export class QuerySnapshot implements firestore.QuerySnapshot {
this._firestore,
doc.key,
doc,
this.metadata.fromCache
this.metadata.fromCache,
this._snapshot.mutatedKeys.has(doc.key)
);
}
}
Expand Down Expand Up @@ -2133,7 +2151,8 @@ export function changesFromSnapshot(
firestore,
change.doc.key,
change.doc,
snapshot.fromCache
snapshot.fromCache,
snapshot.mutatedKeys.has(change.doc.key)
);
assert(
change.type === ChangeType.Added,
Expand Down Expand Up @@ -2164,7 +2183,8 @@ export function changesFromSnapshot(
firestore,
change.doc.key,
change.doc,
snapshot.fromCache
snapshot.fromCache,
snapshot.mutatedKeys.has(change.doc.key)
);
let oldIndex = -1;
let newIndex = -1;
Expand Down
6 changes: 3 additions & 3 deletions packages/firestore/src/core/event_manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,8 @@ export class QueryListener {
snap.docs,
snap.oldDocs,
docChanges,
snap.mutatedKeys,
snap.fromCache,
snap.hasPendingWrites,
snap.syncStateChanged,
/* excludesMetadataChanges= */ true
);
Expand Down Expand Up @@ -288,8 +288,8 @@ export class QueryListener {
snap = ViewSnapshot.fromInitialDocuments(
snap.query,
snap.docs,
snap.fromCache,
snap.hasPendingWrites
snap.mutatedKeys,
snap.fromCache
);
this.raisedInitialEvent = true;
this.queryObserver.next(snap);
Expand Down
3 changes: 1 addition & 2 deletions packages/firestore/src/core/firestore_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -360,8 +360,7 @@ export class FirestoreClient {
this.localStore = new LocalStore(
this.persistence,
user,
this.garbageCollector,
this.sharedClientState
this.garbageCollector
);
const serializer = this.platform.newSerializer(
this.databaseInfo.databaseId
Expand Down
3 changes: 2 additions & 1 deletion packages/firestore/src/core/sync_engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
.remoteDocumentKeys(queryData.targetId)
.then(remoteKeys => {
const view = new View(query, remoteKeys);
const viewDocChanges = view.computeDocChanges(docs);
const viewDocChanges = view.computeInitialChanges(docs);
const synthesizedTargetChange = TargetChange.createSynthesizedTargetChangeForCurrentChange(
queryData.targetId,
current && this.onlineState !== OnlineState.Offline
Expand Down Expand Up @@ -606,6 +606,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
return this.localStore
.acknowledgeBatch(mutationBatchResult)
.then(changes => {
this.sharedClientState.updateMutationState(batchId, 'acknowledged');
return this.emitNewSnapsAndNotifyLocalStore(changes);
})
.catch(err => this.ignoreIfPrimaryLeaseLoss(err));
Expand Down
13 changes: 10 additions & 3 deletions packages/firestore/src/core/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,13 @@
import { ParsedSetData, ParsedUpdateData } from '../api/user_data_converter';
import { SnapshotVersion } from './snapshot_version';
import { documentVersionMap } from '../model/collections';
import { NoDocument } from '../model/document';
import { NoDocument, Document } from '../model/document';
import { MaybeDocument } from '../model/document';
import { DocumentKey } from '../model/document_key';
import { DeleteMutation, Mutation, Precondition } from '../model/mutation';
import { Datastore } from '../remote/datastore';
import { Code, FirestoreError } from '../util/error';
import { fail } from '../util/assert';

/**
* Internal transaction object responsible for accumulating the mutations to
Expand All @@ -36,7 +37,7 @@ export class Transaction {

constructor(private datastore: Datastore) {}

private recordVersion(doc: MaybeDocument): void {
private recordVersion(doc: NoDocument | Document): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

This construct is non-portable. Might as well take MaybeDocument and assert this is not an UnknownDocument (which is a portable construct).

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 already changed this when I did the initial Android port. This function takes a MaybeDocument and has an assert inside of it. The same is now true for the Web.

let docVersion = doc.version;
if (doc instanceof NoDocument) {
// For deleted docs, we must use baseVersion 0 when we overwrite them.
Expand Down Expand Up @@ -68,7 +69,13 @@ export class Transaction {
);
}
return this.datastore.lookup(keys).then(docs => {
docs.forEach(doc => this.recordVersion(doc));
docs.forEach(doc => {
if (doc instanceof NoDocument || doc instanceof Document) {
this.recordVersion(doc);
} else {
fail('Document in a transaction was a ' + doc.constructor.name);
}
});
return docs;
});
}
Expand Down
Loading