Review

Author: schmidt-sebastian

packages/firestore/src/core/sync_engine.ts

@@ -50,7 +50,7 @@ import { SortedSet } from '../util/sorted_set';
 import { ListenSequence } from './listen_sequence';
 import { Query, LimitType } from './query';
 import { SnapshotVersion } from './snapshot_version';
-import { Target } from './target';
+import { generateNextTargetId, Target, TARGET_ID_OFFSET_SYNC_ENGINE } from './target';
 import { Transaction } from './transaction';
 import {
   BatchId,
@@ -173,7 +173,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
   };
   /** Stores user callbacks waiting for all pending writes to be acknowledged. */
   private pendingWritesCallbacks = new Map<BatchId, Array<Deferred<void>>>();
-  private nextLimboTargetId = 1;
+  private nextLimboTargetId = TARGET_ID_OFFSET_SYNC_ENGINE;
 
   // The primary state is set to `true` or `false` immediately after Firestore
   // startup. In the interim, a client should only be considered primary if
@@ -808,17 +808,15 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
       this.activeLimboTargetsByKey.size < this.maxConcurrentLimboResolutions
     ) {
       const key = this.enqueuedLimboResolutions.shift()!;
-      // Target IDs in SyncEngine start at 1 and remain odd.
       const limboTargetId = this.nextLimboTargetId;
-      this.nextLimboTargetId += 2;
+      this.nextLimboTargetId = generateNextTargetId(limboTargetId);
       this.activeLimboResolutionsByTarget[limboTargetId] = new LimboResolution(
         key
       );
       this.activeLimboTargetsByKey = this.activeLimboTargetsByKey.insert(
         key,
         limboTargetId
       );
->>>>>>> master
       this.remoteStore.listen(
         new TargetData(
           Query.atPath(key.path).toTarget(),

packages/firestore/src/core/target.ts

@@ -19,6 +19,7 @@ import { DocumentKey } from '../model/document_key';
 import { ResourcePath } from '../model/path';
 import { isNullOrUndefined } from '../util/types';
 import { Bound, Filter, OrderBy } from './query';
+import { TargetId } from './types';
 
 /**
  * A Target represents the WatchTarget representation of a Query, which is used
@@ -153,3 +154,20 @@ export class Target {
     );
   }
 }
+
+/** Target IDs for Limbo resolution are odd. */
+export const TARGET_ID_OFFSET_SYNC_ENGINE: TargetId = 1;
+
+/** Target IDs for Queries start are even. */
+export const TARGET_ID_OFFSET_LOCAL_STORE: TargetId = 2;
+
+/**
+ * Generates the next target ID given the last target ID.
+ *
+ * If TARGET_ID_OFFSET_SYNC_ENGINE/TARGET_ID_OFFSET_LOCAL_STORE are used as
+ * initial target ID, then the IDs generated by `generateNextTargetId` never 
+ * collide.
+ */
+export function generateNextTargetId(lastTargetId: TargetId): number {
+  return lastTargetId + 2;
+}

packages/firestore/src/local/indexeddb_target_cache.ts

@@ -44,7 +44,7 @@ import { PersistencePromise } from './persistence_promise';
 import { TargetCache } from './target_cache';
 import { TargetData } from './target_data';
 import { SimpleDb, SimpleDbStore, SimpleDbTransaction } from './simple_db';
-import { Target } from '../core/target';
+import { generateNextTargetId, Target } from '../core/target';
 
 export class IndexedDbTargetCache implements TargetCache {
   constructor(
@@ -63,8 +63,7 @@ export class IndexedDbTargetCache implements TargetCache {
     transaction: PersistenceTransaction
   ): PersistencePromise<TargetId> {
     return this.retrieveMetadata(transaction).next(metadata => {
-      // Target IDs in persistence start at two and remain even.
-      metadata.highestTargetId += 2;
+      metadata.highestTargetId = generateNextTargetId(metadata.highestTargetId);
       return this.saveMetadata(transaction, metadata).next(
         () => metadata.highestTargetId
       );

packages/firestore/src/local/memory_target_cache.ts

@@ -29,7 +29,7 @@ import { PersistencePromise } from './persistence_promise';
 import { ReferenceSet } from './reference_set';
 import { TargetCache } from './target_cache';
 import { TargetData } from './target_data';
-import { Target } from '../core/target';
+import { generateNextTargetId, Target } from '../core/target';
 
 export class MemoryTargetCache implements TargetCache {
   /**
@@ -76,8 +76,7 @@ export class MemoryTargetCache implements TargetCache {
   allocateTargetId(
     transaction: PersistenceTransaction
   ): PersistencePromise<TargetId> {
-    // Target IDs in persistence start at two and remain even.
-    this.highestTargetId += 2;
+    this.highestTargetId = generateNextTargetId(this.highestTargetId);
     return PersistencePromise.resolve(this.highestTargetId);
   }
 

packages/firestore/test/unit/specs/spec_builder.ts

@@ -16,7 +16,12 @@
  */
 
 import { FieldFilter, Filter, Query } from '../../../src/core/query';
-import { Target } from '../../../src/core/target';
+import {
+  generateNextTargetId,
+  Target,
+  TARGET_ID_OFFSET_LOCAL_STORE,
+  TARGET_ID_OFFSET_SYNC_ENGINE
+} from '../../../src/core/target';
 import { TargetId } from '../../../src/core/types';
 import {
   Document,
@@ -81,7 +86,7 @@ export class ClientMemoryState {
   activeTargets: ActiveTargetMap = {};
   queryMapping = new ObjectMap<Target, TargetId>(t => t.canonicalId());
   limboMapping: LimboMap = {};
-  nextLimboTarget = 1;
+  nextLimboTargetId = TARGET_ID_OFFSET_SYNC_ENGINE;
 
   constructor() {
     this.reset();
@@ -92,12 +97,12 @@ export class ClientMemoryState {
     this.queryMapping = new ObjectMap<Target, TargetId>(t => t.canonicalId());
     this.limboMapping = {};
     this.activeTargets = {};
-    this.nextLimboTarget = 1;
+    this.nextLimboTargetId = TARGET_ID_OFFSET_SYNC_ENGINE;
   }
 
-  nextLimboTargetId(): TargetId {
-    const nextLimboTargetId = this.nextLimboTarget;
-    this.nextLimboTarget += 2;
+  generateLimboTargetId(): TargetId {
+    const nextLimboTargetId = this.nextLimboTargetId;
+    this.nextLimboTargetId = generateNextTargetId(this.nextLimboTargetId);
     return nextLimboTargetId;
   }
 
@@ -116,7 +121,7 @@ export class ClientMemoryState {
 class CachedTargetIdGenerator {
   // TODO(wuandy): rename this to targetMapping.
   private queryMapping = new ObjectMap<Target, TargetId>(t => t.canonicalId());
-  private nextTargetId = 2;
+  private nextTargetId = TARGET_ID_OFFSET_LOCAL_STORE;
 
   /**
    * Returns a cached target ID for the provided Target, or a new ID if no
@@ -127,7 +132,7 @@ class CachedTargetIdGenerator {
       return this.queryMapping.get(target)!;
     }
     const targetId = this.nextTargetId;
-    this.nextTargetId += 2;
+    this.nextTargetId = generateNextTargetId(this.nextTargetId);
     this.queryMapping.set(target, targetId);
     return targetId;
   }
@@ -175,8 +180,8 @@ export class SpecBuilder {
     return this.currentClientState;
   }
 
-  private nextLimboTargetId(): TargetId {
-    return this.clientState.nextLimboTargetId();
+  private generateLimboTargetId(): TargetId {
+    return this.clientState.generateLimboTargetId();
   }
 
   private get queryMapping(): ObjectMap<Target, TargetId> {
@@ -463,7 +468,7 @@ export class SpecBuilder {
       const path = key.path.canonicalString();
       // Create limbo target ID mapping if it was not in limbo yet
       if (!objUtils.contains(this.limboMapping, path)) {
-        this.limboMapping[path] = this.nextLimboTargetId();
+        this.limboMapping[path] = this.generateLimboTargetId();
       }
       // Limbo doc queries are always without resume token
       this.addQueryToActiveTargets(

packages/firestore/test/util/helpers.ts

@@ -512,7 +512,7 @@ export function byteStringFromString(value: string): ByteString {
   return ByteString.fromBase64String(base64);
 }
 
-/** 
+/**
  * Decodes a base 64 decoded string.
  *
  * Note that this is typed to accept Uint8Arrays to match the types used