[NOT FOR SUBMISSION] Audit major components where Document* is now used.

packages/firestore/src/api/database.ts

@@ -1242,6 +1242,9 @@ export interface SnapshotOptions extends firestore.SnapshotOptions {}
 export class DocumentSnapshot implements firestore.DocumentSnapshot {
   constructor(
     private _firestore: Firestore,
+    // DC: Type too broad. This should only ever be EXISTS or MISSING. Passing
+    // UNKNOWN would be a bug (we should never raise snapshots if we don't
+    // definitively know the document exists or not).
     public _document: Document,
     private _fromCache: boolean,
     private _hasPendingWrites: boolean
@@ -1363,6 +1366,8 @@ export class QueryDocumentSnapshot extends DocumentSnapshot
   implements firestore.QueryDocumentSnapshot {
   constructor(
     firestore: Firestore,
+    // DC: Type too broad. Should only accept DOCUMENT. Passing MISSING or
+    // UNKNOWN would be a bug.
     document: Document,
     fromCache: boolean,
     hasPendingWrites: boolean

packages/firestore/src/core/firestore_client.ts

@@ -544,6 +544,12 @@ export class FirestoreClient {
     });
   }
 
+  // DC: Type may be too broad. This only returns EXISTS / MISSING at present
+  // which allows the return value to be passed directly to DocumentSnapshot,
+  // but you'd need to read the code to verify that UNKNOWN is never returned
+  // since the return value is now too broad.
+  // Sidenote: It might be cleaner to go ahead and return UNKNOWN and have the
+  // calling code generate the exception instead of throwing from this method.
   getDocumentFromLocalCache(docKey: DocumentKey): Promise<Document> {
     this.verifyNotShutdown();
     return this.asyncQueue

packages/firestore/src/core/query.ts

@@ -306,6 +306,10 @@ export class Query {
       : other.endAt === null;
   }
 
+  // DC: Type benignly too broad. Only EXISTS entries can be in a query, so it
+  // is probably a bug to call this with anything else, but as written it'll
+  // silently sort UNKNOWN or MISSING documents first (I think?) which is
+  // probably not really harmful.
   docComparator(d1: Document, d2: Document): number {
     let comparedOnKeyField = false;
     for (const orderBy of this.orderBy) {
@@ -321,6 +325,9 @@ export class Query {
     return 0;
   }
 
+  // DC: Better type! This used to only allow EXISTS, but it's probably fine
+  // (more convenient even) to allow matches() to accept any type of Document
+  // entry.
   matches(doc: Document): boolean {
     return (
       doc.exists &&
@@ -741,6 +748,9 @@ export class Bound {
    * Returns true if a document sorts before a bound using the provided sort
    * order.
    */
+  // DC: Type benignly too broad. Only EXISTS document entries can be in a
+  // query, so it is probably a bug to call this on anything else, but we could
+  // allow it to "work" on MISSING / UNKNOWN entries with little harm.
   sortsBeforeDocument(orderBy: OrderBy[], doc: Document): boolean {
     assert(
       this.position.length <= orderBy.length,

packages/firestore/src/core/transaction.ts

@@ -37,12 +37,19 @@ export class Transaction {
 
   constructor(private datastore: Datastore) {}
 
+  // DC: Type too broad. This should only ever be called with DOCUMENT or
+  // MISSING entries, since UNKNOWN would indicate we don't know if the document
+  // exists, in which case we cannot record a valid precondition version here.
   private recordVersion(doc: Document): void {
     let docVersion: SnapshotVersion;
 
     if (doc.exists) {
       docVersion = doc.version;
     } else {
+      // DC: This is a bug waiting to happen. Doing this for UNKNOWN documents
+      // is wrong. If we want to handle UNKNOWN in this method, we should rename
+      // it to tryRecordVersion() and make UNKNOWN be a no-op.
+
       // For deleted docs, we must use baseVersion 0 when we overwrite them.
       docVersion = SnapshotVersion.forMissingDoc();
     }
@@ -61,6 +68,9 @@ export class Transaction {
     }
   }
 
+  // DC: Type may be too broad. This should (and does) only ever return EXISTS
+  // or MISSING documents, but you would need to read the code to verify that
+  // since the return type is too broad.
   lookup(keys: DocumentKey[]): Promise<Document[]> {
     if (this.committed) {
       return Promise.reject<Document[]>('Transaction has already completed.');

packages/firestore/src/core/view.ts

@@ -47,6 +47,8 @@ export class RemovedLimboDocument {
 /** The result of applying a set of doc changes to a view. */
 export interface ViewDocumentChanges {
   /** The new set of docs that should be in the view. */
+  // DC: Type too broad. Views can only contain EXISTS documents, but this now
+  // accepts UNKNOWN and MISSING document entries which would be a bug.
   documentSet: DocumentSet;
   /** The diff of these docs with the previous set of docs. */
   changeSet: DocumentChangeSet;
@@ -112,6 +114,10 @@ export class View {
    * @return a new set of docs, changes, and refill flag.
    */
   computeDocChanges(
+    // DC: Type may be too broad. Per the old code, this will not contain
+    // UNKNOWN documents. I'm not sure what it would mean to pass UNKNOWN
+    // documents or whether this code would handle them in a sane way. It would
+    // be better if we didn't have to think about it.
     docChanges: DocumentMap,
     previousChanges?: ViewDocumentChanges
   ): ViewDocumentChanges {
@@ -443,6 +449,9 @@ export class View {
    */
   // PORTING NOTE: Multi-tab only.
   synchronizeWithPersistedState(
+    // DC: Type too broad. THis can now contain UNKNOWN documents which would be
+    // invalid. Though FWIW the old code was already broader than needed
+    // (accepting MaybeDocumentMap) I think. :-/
     localDocs: DocumentMap,
     remoteKeys: DocumentKeySet
   ): ViewChange {

packages/firestore/src/core/view_snapshot.ts

@@ -33,6 +33,13 @@ export enum ChangeType {
 
 export interface DocumentViewChange {
   type: ChangeType;
+  // DC: Type too broad. Due to the weak naming and lack of comments it's hard
+  // to figure out what DocumentViewChange represents and how "doc" is used.
+  // From the old code I can tell that doc is always an EXISTS document, but in
+  // the new code it could be UNKNOWN or MISSING as well, making the code even
+  // harder to reason about. And I think it's pretty likely that the code would
+  // not actually behave in a sane way if MISSING or UNKNOWN documents were
+  // provided. So this seems harmful.
   doc: Document;
 }
 
@@ -142,6 +149,14 @@ export class DocumentChangeSet {
 export class ViewSnapshot {
   constructor(
     readonly query: Query,
+    // DC: Type too broad. docs and oldDocs should only have EXISTS entries
+    // since only existing documents can match a query and therefore show up in
+    // a View. But now that DocumentSet can contain UNKNOWN and MISSING
+    // documents as well, it is possible that a ViewSnapshot could be
+    // constructed containing invalid document entries (which would be a bug)
+    // and I'm pretty sure code consuming ViewSnapshot would *not* handle it
+    // gracefully.
+    // This seems harmful.
     readonly docs: DocumentSet,
     readonly oldDocs: DocumentSet,
     readonly docChanges: DocumentViewChange[],
@@ -154,12 +169,16 @@ export class ViewSnapshot {
   /** Returns a view snapshot as if all documents in the snapshot were added. */
   static fromInitialDocuments(
     query: Query,
+    // DC: Type too broad. It would be a bug to pass MISSING or UNKNOWN
+    // documents here
     documents: DocumentSet,
     mutatedKeys: DocumentKeySet,
     fromCache: boolean
   ): ViewSnapshot {
     const changes: DocumentViewChange[] = [];
     documents.forEach(doc => {
+      // DC: This is a bug waiting to happen. If we were passed MISSING or
+      // UNKNOWN documents, we'd generate "added" events for them.
       changes.push({ type: ChangeType.Added, doc });
     });
 

packages/firestore/src/local/local_documents_view.ts

@@ -53,6 +53,11 @@ export class LocalDocumentsView {
    * @return Local view of the document or null if we don't have any cached
    * state for it.
    */
+  // DC: Simpler type! This return type is now simpler (Document instead of
+  // MaybeDocument|null). Nice. Though the fact that we use "Document" for
+  // everything now means I can't necessarily assume that we really do use the
+  // full breadth of the return type (e.g. does the method really return UNKNOWN
+  // for an uncached document or might it reject the promise?).
   getDocument(
     transaction: PersistenceTransaction,
     key: DocumentKey
@@ -99,6 +104,11 @@ export class LocalDocumentsView {
    * If we don't have cached state for a document in `keys`, a missing Document
    * will be stored for that key in the resulting set.
    */
+  // DC: Type too broad. This could now return UNKNOWN document entries whereas
+  // it didn't before. And (at least as documented), we return MISSING documents
+  // for uncached documents. Since getDocuments() and getDocument() share a
+  // similar return type but represent uncached documents differently, this
+  // seems actively confusing.
   getDocuments(
     transaction: PersistenceTransaction,
     keys: DocumentKeySet
@@ -112,6 +122,7 @@ export class LocalDocumentsView {
    * Similar to `getDocuments`, but creates the local view from the given
    * `baseDocs` without retrieving documents from the local store.
    */
+  // DC: Type too broad. Same feedback as above.
   getLocalViewOfDocuments(
     transaction: PersistenceTransaction,
     baseDocs: DocumentMap
@@ -134,6 +145,9 @@ export class LocalDocumentsView {
   }
 
   /** Performs a query against the local view of all documents. */
+  // DC: Type too broad. This will only return EXISTS documents since documents
+  // must exist to match the query, and the broader return type could cause
+  // confusion.
   getDocumentsMatchingQuery(
     transaction: PersistenceTransaction,
     query: Query

packages/firestore/src/local/local_serializer.ts

@@ -43,6 +43,8 @@ export class LocalSerializer {
   constructor(private remoteSerializer: JsonProtoSerializer) {}
 
   /** Decodes a remote document from storage locally to a Document. */
+  // DC: Simpler type! Can return EXISTS, MISSING, or UNKNOWN (once we merge
+  // CONTENTS_UNKNOWN and UNKNOWN) so the return type is appropriate.
   fromDbRemoteDocument(remoteDoc: DbRemoteDocument): Document {
     if (remoteDoc.document) {
       return this.remoteSerializer.fromDocument(
@@ -65,6 +67,8 @@ export class LocalSerializer {
   }
 
   /** Encodes a document for storage locally. */
+  // DC: Simpler type! Can encode EXISTS, MISSING, or UNKNOWN (once we merge
+  // CONTENTS_UNKNOWN and UNKNOWN) so the parameter type is appropriate.
   toDbRemoteDocument(maybeDoc: Document): DbRemoteDocument {
     switch (maybeDoc.type) {
       case DocumentType.EXISTS: {

packages/firestore/src/local/local_store.ts

@@ -310,6 +310,11 @@ export class LocalStore {
 
   /** Returns the local view of the documents affected by a mutation batch. */
   // PORTING NOTE: Multi-tab only.
+  // DC: Type may be too broad. Can now return UNKNOWN documents while it didn't
+  // before. It might make sense to return UNKNOWN documents for documents that
+  // aren't in cache and have a patch applied (presumably we just omitted them
+  // from the map before), but I don't know if we do or not. So this should be
+  // documented in the method doc comment at minimum.
   lookupMutationDocuments(batchId: BatchId): Promise<DocumentMap | null> {
     return this.persistence.runTransaction(
       'Lookup mutation documents',
@@ -345,6 +350,10 @@ export class LocalStore {
    *
    * @returns The resulting (modified) documents.
    */
+  // DC: Type may be too broad. This code can now return UNKNOWN documents where
+  // it didn't before. I don't know if it does, so it should be documented, but
+  // it's probably not too harmful as long as the calling code properly ignores
+  // them (doesn't send them to the View, etc.)
   acknowledgeBatch(batchResult: MutationBatchResult): Promise<DocumentMap> {
     return this.persistence.runTransaction(
       'Acknowledge batch',
@@ -370,6 +379,7 @@ export class LocalStore {
    *
    * @returns The resulting modified documents.
    */
+  // DC: Type may be too broad. Same feedback as above.
   rejectBatch(batchId: BatchId): Promise<DocumentMap> {
     return this.persistence.runTransaction(
       'Reject batch',
@@ -439,6 +449,9 @@ export class LocalStore {
    * LocalDocuments are re-calculated if there are remaining mutations in the
    * queue.
    */
+  // DC: Type may be too broad. This can now return UNKNOWN documents where it
+  // didn't before. I don't think this ever makes sense, but it probably isn't
+  // too harmful as long as they don't make it to the View, etc.
   applyRemoteEvent(remoteEvent: RemoteEvent): Promise<DocumentMap> {
     const documentBuffer = this.remoteDocuments.newChangeBuffer();
     return this.persistence.runTransaction(
@@ -688,6 +701,7 @@ export class LocalStore {
    * Read the current value of a Document with a given key or null if not
    * found - used for testing.
    */
+  // DC: Simpler type! (but comment needs to be updated)
   readDocument(key: DocumentKey): Promise<Document> {
     return this.persistence.runTransaction('read document', 'readonly', txn => {
       return this.localDocuments.getDocument(txn, key);
@@ -785,6 +799,10 @@ export class LocalStore {
    * Runs the specified query against all the documents in the local store and
    * returns the results.
    */
+  // DC: Type too broad. This should only ever return EXISTS document entries,
+  // since only existing documents can match a query. It could be harmful if
+  // this returned non-existing documents, since they could get sent to the View
+  // which isn't currently designed to deal with MISSING / UNKNOWN.
   executeQuery(query: Query): Promise<DocumentMap> {
     return this.persistence.runTransaction('Execute query', 'readonly', txn => {
       return this.localDocuments.getDocumentsMatchingQuery(txn, query);
@@ -888,6 +906,8 @@ export class LocalStore {
   }
 
   // PORTING NOTE: Multi-tab only.
+  // DC: Type may be too broad. Can now return UNKNOWN documents. Not sure what
+  // that would mean or if it's harmful.
   getNewDocumentChanges(): Promise<DocumentMap> {
     return this.persistence.runTransaction(
       'Get new document changes',

packages/firestore/src/local/remote_document_cache.ts

@@ -43,6 +43,7 @@ export interface RemoteDocumentCache {
    * @return The cached Document (existing or missing), or unknown if we have
    *     nothing cached.
    */
+  // DC: Simpler type!
   getEntry(
     transaction: PersistenceTransaction,
     documentKey: DocumentKey
@@ -55,6 +56,7 @@ export interface RemoteDocumentCache {
    * @return The cached Document entries indexed by key. If an entry is not
    *     cached, the corresponding key will be mapped to an unknown document.
    */
+  // DC: Simpler type!
   getEntries(
     transaction: PersistenceTransaction,
     documentKeys: DocumentKeySet
@@ -71,6 +73,9 @@ export interface RemoteDocumentCache {
    * @param query The query to match documents against.
    * @return The set of matching documents.
    */
+  // DC: Type too broad. This could now return MISSING and UNKNOWN documents
+  // which doesn't make sense and could be harmful (calling code has to filter
+  // them out).
   getDocumentsMatchingQuery(
     transaction: PersistenceTransaction,
     query: Query
@@ -86,6 +91,8 @@ export interface RemoteDocumentCache {
    * invocations will return document changes since the point of rejection.
    */
   // PORTING NOTE: This is only used for multi-tab synchronization.
+  // DC: Type may be too broad. Can now return UNKNOWN documents where it didn't
+  // before. Not sure if it's harmful.
   getNewDocumentChanges(
     transaction: PersistenceTransaction
   ): PersistencePromise<DocumentMap>;

packages/firestore/src/model/collections.ts

@@ -26,6 +26,13 @@ import { DocumentKey } from './document_key';
 
 /** Miscellaneous collection types / constants. */
 
+// DC: NOTE: Prior to this refactor, the broadest type typically used was
+// MaybeDocumentMap which would contain EXISTS and MISSING documents (ignoring
+// the unfortunate UnknownDocument), and UNKNOWN documents would just be omitted
+// from the map. NOW DocumentMap is too broad for most usages and we have two
+// ways to represent an uncached document: UNKNOWN or missing from the map. We
+// end up compensating for this with the getDocument() helper below, which helps
+// to re-unifies the two cases.
 export type DocumentMap = SortedMap<DocumentKey, Document>;
 
 const EMPTY_DOCUMENT_MAP = new SortedMap<DocumentKey, Document>(

packages/firestore/src/model/document_comparator.ts

@@ -20,6 +20,7 @@ import { DocumentKey } from './document_key';
 
 export type DocumentComparator = (doc1: Document, doc2: Document) => number;
 
+// DC: Better type! Comparing UNKNOWN and MISSING documents could be handy.
 export function compareByKey(doc1: Document, doc2: Document): number {
   return DocumentKey.comparator(doc1.key, doc2.key);
 }

packages/firestore/src/model/document_set.ts

@@ -28,6 +28,8 @@ import { DocumentKey } from './document_key';
  * comparator on top of what is provided to guarantee document equality based on
  * the key.
  */
+// DC: NOTE: This is now a much broader type (contains UNKNOWN / MISSING
+// documents) which seems likely to be harmful in at least some uses.
 export class DocumentSet {
   /**
    * Returns an empty copy of the existing DocumentSet, using the same