Skip to content

Checking for UnknownDocument #1174

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 3 commits into from
Aug 31, 2018
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 5 additions & 1 deletion packages/firestore/src/api/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -597,7 +597,11 @@ export class Transaction implements firestore.Transaction {
} else if (doc instanceof Document) {
return new DocumentSnapshot(this._firestore, ref._key, doc, false);
} else {
throw fail('MaybeDocument is neither Document nor NoDocument');
throw fail(
`BatchGetDocumentsRequest returned unexpected document type: ${
doc.constructor.name
}`
);
}
});
}
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 {
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
7 changes: 2 additions & 5 deletions packages/firestore/src/local/local_documents_view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import { Document, MaybeDocument, NoDocument } from '../model/document';
import { DocumentKey } from '../model/document_key';
import { MutationBatch } from '../model/mutation_batch';
import { ResourcePath } from '../model/path';
import { fail } from '../util/assert';

import { MutationQueue } from './mutation_queue';
import { PersistenceTransaction } from './persistence';
Expand Down Expand Up @@ -168,12 +167,10 @@ export class LocalDocumentsView {
baseDoc,
batch.localWriteTime
);
if (!mutatedDoc || mutatedDoc instanceof NoDocument) {
results = results.remove(key);
} else if (mutatedDoc instanceof Document) {
if (mutatedDoc instanceof Document) {
results = results.insert(key, mutatedDoc);
} else {
fail('Unknown MaybeDocument: ' + mutatedDoc);
results = results.remove(key);
}
}
}
Expand Down
6 changes: 1 addition & 5 deletions packages/firestore/src/model/mutation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,11 +156,7 @@ export class Precondition {
maybeDoc.version.isEqual(this.updateTime)
);
} else if (this.exists !== undefined) {
if (this.exists) {
return maybeDoc instanceof Document;
} else {
return maybeDoc === null || maybeDoc instanceof NoDocument;
}
return this.exists === maybeDoc instanceof Document;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that prettier ate the parenthesis around maybeDoc instanceof Document.

} else {
assert(this.isNone, 'Precondition should be empty');
return true;
Expand Down