Fix CSI timestamp issue (#8090)

firebase · Mar 28, 2024 · c6ecac8 · c6ecac8
1 parent c8a2568
commit c6ecac8
Show file tree

Hide file tree

Showing 8 changed files with 307 additions and 10 deletions.
diff --git a/.changeset/friendly-eyes-destroy.md b/.changeset/friendly-eyes-destroy.md
@@ -0,0 +1,5 @@
+---
+'@firebase/firestore': patch
+---
+
+Fixed the CSI issue where indexing on timestamp fields leads to incorrect query results.
diff --git a/packages/firestore/src/index/firestore_index_value_writer.ts b/packages/firestore/src/index/firestore_index_value_writer.ts
@@ -16,7 +16,11 @@
  */
 
 import { DocumentKey } from '../model/document_key';
-import { normalizeByteString, normalizeNumber } from '../model/normalize';
+import {
+  normalizeByteString,
+  normalizeNumber,
+  normalizeTimestamp
+} from '../model/normalize';
 import { isMaxValue } from '../model/values';
 import { ArrayValue, MapValue, Value } from '../protos/firestore_proto_api';
 import { fail } from '../util/assert';
@@ -93,14 +97,13 @@ export class FirestoreIndexValueWriter {
         }
       }
     } else if ('timestampValue' in indexValue) {
-      const timestamp = indexValue.timestampValue!;
+      let timestamp = indexValue.timestampValue!;
       this.writeValueTypeLabel(encoder, INDEX_TYPE_TIMESTAMP);
       if (typeof timestamp === 'string') {
-        encoder.writeString(timestamp);
-      } else {
-        encoder.writeString(`${timestamp.seconds || ''}`);
-        encoder.writeNumber(timestamp.nanos || 0);
+        timestamp = normalizeTimestamp(timestamp);
       }
+      encoder.writeString(`${timestamp.seconds || ''}`);
+      encoder.writeNumber(timestamp.nanos || 0);
     } else if ('stringValue' in indexValue) {
       this.writeIndexString(indexValue.stringValue!, encoder);
       this.writeTruncationMarker(encoder);

diff --git a/packages/firestore/src/local/indexeddb_schema.ts b/packages/firestore/src/local/indexeddb_schema.ts
@@ -51,9 +51,10 @@ import { DbTimestampKey } from './indexeddb_sentinels';
  *     document lookup via `getAll()`.
  * 14. Add overlays.
  * 15. Add indexing support.
+ * 16. Parse timestamp strings before creating index entries.
  */
 
-export const SCHEMA_VERSION = 15;
+export const SCHEMA_VERSION = 16;
 
 /**
  * Wrapper class to store timestamps (seconds and nanos) in IndexedDb objects.

diff --git a/packages/firestore/src/local/indexeddb_schema_converter.ts b/packages/firestore/src/local/indexeddb_schema_converter.ts
@@ -256,6 +256,19 @@ export class SchemaConverter implements SimpleDbSchemaConverter {
       p = p.next(() => createFieldIndex(db));
     }
 
+    if (fromVersion < 16 && toVersion >= 16) {
+      // Clear the object stores to remove possibly corrupted index entries
+      p = p
+        .next(() => {
+          const indexStateStore = txn.objectStore(DbIndexStateStore);
+          indexStateStore.clear();
+        })
+        .next(() => {
+          const indexEntryStore = txn.objectStore(DbIndexEntryStore);
+          indexEntryStore.clear();
+        });
+    }
+
     return p;
   }
 

diff --git a/packages/firestore/src/local/indexeddb_sentinels.ts b/packages/firestore/src/local/indexeddb_sentinels.ts
@@ -414,6 +414,7 @@ export const V15_STORES = [
   DbIndexStateStore,
   DbIndexEntryStore
 ];
+export const V16_STORES = V15_STORES;
 
 /**
  * The list of all default IndexedDB stores used throughout the SDK. This is
@@ -424,7 +425,9 @@ export const ALL_STORES = V12_STORES;
 
 /** Returns the object stores for the provided schema. */
 export function getObjectStores(schemaVersion: number): string[] {
-  if (schemaVersion === 15) {
+  if (schemaVersion === 16) {
+    return V16_STORES;
+  } else if (schemaVersion === 15) {
     return V15_STORES;
   } else if (schemaVersion === 14) {
     return V14_STORES;

diff --git a/packages/firestore/test/unit/index/firestore_index_value_writer.test.ts b/packages/firestore/test/unit/index/firestore_index_value_writer.test.ts
@@ -0,0 +1,173 @@
+/**
+ * @license
+ * Copyright 2024 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0x00 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0x00
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import { expect } from 'chai';
+
+import { FirestoreIndexValueWriter } from '../../../src/index/firestore_index_value_writer';
+import { IndexByteEncoder } from '../../../src/index/index_byte_encoder';
+import { Timestamp } from '../../../src/lite-api/timestamp';
+import { IndexKind } from '../../../src/model/field_index';
+import type { Value } from '../../../src/protos/firestore_proto_api';
+import { toTimestamp } from '../../../src/remote/serializer';
+import { JSON_SERIALIZER } from '../local/persistence_test_helpers';
+
+import { compare } from './ordered_code_writer.test';
+
+describe('Firestore Index Value Writer', () => {
+  function compareIndexEncodedValues(
+    value1: Value,
+    value2: Value,
+    direction: IndexKind
+  ): number {
+    const encoder1 = new IndexByteEncoder();
+    const encoder2 = new IndexByteEncoder();
+    FirestoreIndexValueWriter.INSTANCE.writeIndexValue(
+      value1,
+      encoder1.forKind(direction)
+    );
+
+    FirestoreIndexValueWriter.INSTANCE.writeIndexValue(
+      value2,
+      encoder2.forKind(direction)
+    );
+
+    return compare(encoder1.encodedBytes(), encoder2.encodedBytes());
+  }
+
+  describe('can gracefully handle different format of timestamp', () => {
+    it('can handle different format of timestamp', () => {
+      const value1 = { timestampValue: '2016-01-02T10:20:50.850Z' };
+      const value2 = { timestampValue: '2016-01-02T10:20:50.850000Z' };
+      const value3 = { timestampValue: '2016-01-02T10:20:50.850000000Z' };
+      const value4 = {
+        timestampValue: { seconds: 1451730050, nanos: 850000000 }
+      };
+      const value5 = {
+        timestampValue: toTimestamp(
+          JSON_SERIALIZER,
+          new Timestamp(1451730050, 850000000)
+        )
+      };
+      expect(
+        compareIndexEncodedValues(value1, value2, IndexKind.ASCENDING)
+      ).to.equal(0);
+      expect(
+        compareIndexEncodedValues(value1, value3, IndexKind.ASCENDING)
+      ).to.equal(0);
+      expect(
+        compareIndexEncodedValues(value1, value4, IndexKind.ASCENDING)
+      ).to.equal(0);
+      expect(
+        compareIndexEncodedValues(value1, value5, IndexKind.ASCENDING)
+      ).to.equal(0);
+
+      expect(
+        compareIndexEncodedValues(value1, value2, IndexKind.DESCENDING)
+      ).to.equal(0);
+      expect(
+        compareIndexEncodedValues(value1, value3, IndexKind.DESCENDING)
+      ).to.equal(0);
+      expect(
+        compareIndexEncodedValues(value1, value4, IndexKind.DESCENDING)
+      ).to.equal(0);
+      expect(
+        compareIndexEncodedValues(value1, value5, IndexKind.DESCENDING)
+      ).to.equal(0);
+    });
+
+    it('can handle timestamps with 0 nanoseconds', () => {
+      const value1 = { timestampValue: '2016-01-02T10:20:50Z' };
+      const value2 = { timestampValue: '2016-01-02T10:20:50.000000000Z' };
+      const value3 = { timestampValue: { seconds: 1451730050, nanos: 0 } };
+      const value4 = {
+        timestampValue: toTimestamp(
+          JSON_SERIALIZER,
+          new Timestamp(1451730050, 0)
+        )
+      };
+      expect(
+        compareIndexEncodedValues(value1, value2, IndexKind.ASCENDING)
+      ).to.equal(0);
+      expect(
+        compareIndexEncodedValues(value1, value3, IndexKind.ASCENDING)
+      ).to.equal(0);
+      expect(
+        compareIndexEncodedValues(value1, value4, IndexKind.ASCENDING)
+      ).to.equal(0);
+
+      expect(
+        compareIndexEncodedValues(value1, value2, IndexKind.DESCENDING)
+      ).to.equal(0);
+      expect(
+        compareIndexEncodedValues(value1, value3, IndexKind.DESCENDING)
+      ).to.equal(0);
+      expect(
+        compareIndexEncodedValues(value1, value4, IndexKind.DESCENDING)
+      ).to.equal(0);
+    });
+
+    it('can compare timestamps with  different formats', () => {
+      const value1 = { timestampValue: '2016-01-02T10:20:50Z' };
+      const value2 = { timestampValue: '2016-01-02T10:20:50.000001Z' };
+      const value3 = {
+        timestampValue: { seconds: 1451730050, nanos: 999999999 }
+      };
+      const value4 = {
+        timestampValue: toTimestamp(
+          JSON_SERIALIZER,
+          new Timestamp(1451730050, 1)
+        )
+      };
+      expect(
+        compareIndexEncodedValues(value1, value2, IndexKind.ASCENDING)
+      ).to.equal(-1);
+      expect(
+        compareIndexEncodedValues(value1, value3, IndexKind.ASCENDING)
+      ).to.equal(-1);
+      expect(
+        compareIndexEncodedValues(value1, value4, IndexKind.ASCENDING)
+      ).to.equal(-1);
+      expect(
+        compareIndexEncodedValues(value2, value3, IndexKind.ASCENDING)
+      ).to.equal(-1);
+      expect(
+        compareIndexEncodedValues(value2, value4, IndexKind.ASCENDING)
+      ).to.equal(1);
+      expect(
+        compareIndexEncodedValues(value3, value4, IndexKind.ASCENDING)
+      ).to.equal(1);
+
+      expect(
+        compareIndexEncodedValues(value1, value2, IndexKind.DESCENDING)
+      ).to.equal(1);
+      expect(
+        compareIndexEncodedValues(value1, value3, IndexKind.DESCENDING)
+      ).to.equal(1);
+      expect(
+        compareIndexEncodedValues(value1, value4, IndexKind.DESCENDING)
+      ).to.equal(1);
+      expect(
+        compareIndexEncodedValues(value2, value3, IndexKind.DESCENDING)
+      ).to.equal(1);
+      expect(
+        compareIndexEncodedValues(value2, value4, IndexKind.DESCENDING)
+      ).to.equal(-1);
+      expect(
+        compareIndexEncodedValues(value3, value4, IndexKind.DESCENDING)
+      ).to.equal(-1);
+    });
+  });
+});
diff --git a/packages/firestore/test/unit/index/ordered_code_writer.test.ts b/packages/firestore/test/unit/index/ordered_code_writer.test.ts
@@ -226,7 +226,7 @@ function fromHex(hexString: string): Uint8Array {
   return bytes;
 }
 
-function compare(left: Uint8Array, right: Uint8Array): number {
+export function compare(left: Uint8Array, right: Uint8Array): number {
   for (let i = 0; i < Math.min(left.length, right.length); ++i) {
     if (left[i] < right[i]) {
       return -1;