diff --git a/.changeset/slimy-chicken-mix.md b/.changeset/slimy-chicken-mix.md new file mode 100644 index 00000000000..3b0de94b287 --- /dev/null +++ b/.changeset/slimy-chicken-mix.md @@ -0,0 +1,6 @@ +--- +'@firebase/firestore': patch +'firebase': patch +--- + +Reverted a change to use UTF-8 encoding in string comparisons which caused a performance issue. See [GitHub issue #8778](https://github.com/firebase/firebase-js-sdk/issues/8778) diff --git a/packages/firestore/src/local/indexeddb_remote_document_cache.ts b/packages/firestore/src/local/indexeddb_remote_document_cache.ts index 9b23c64fcf5..b3d4658d53d 100644 --- a/packages/firestore/src/local/indexeddb_remote_document_cache.ts +++ b/packages/firestore/src/local/indexeddb_remote_document_cache.ts @@ -655,9 +655,5 @@ export function dbKeyComparator(l: DocumentKey, r: DocumentKey): number { return cmp; } - // TODO(b/329441702): Document IDs should be sorted by UTF-8 encoded byte - // order, but IndexedDB sorts strings lexicographically. Document ID - // comparison here still relies on primitive comparison to avoid mismatches - // observed in snapshot listeners with Unicode characters in documentIds return primitiveComparator(left[left.length - 1], right[right.length - 1]); } diff --git a/packages/firestore/src/model/path.ts b/packages/firestore/src/model/path.ts index e7aeb6f61cc..64cb0376a0e 100644 --- a/packages/firestore/src/model/path.ts +++ b/packages/firestore/src/model/path.ts @@ -19,7 +19,6 @@ import { Integer } from '@firebase/webchannel-wrapper/bloom-blob'; import { debugAssert, fail } from '../util/assert'; import { Code, FirestoreError } from '../util/error'; -import { primitiveComparator, compareUtf8Strings } from '../util/misc'; export const DOCUMENT_KEY_NAME = '__name__'; @@ -182,7 +181,7 @@ abstract class BasePath> { return comparison; } } - return primitiveComparator(p1.length, p2.length); + return Math.sign(p1.length - p2.length); } private static compareSegments(lhs: string, rhs: string): number { @@ -202,7 +201,13 @@ abstract class BasePath> { ); } else { // both non-numeric - return compareUtf8Strings(lhs, rhs); + if (lhs < rhs) { + return -1; + } + if (lhs > rhs) { + return 1; + } + return 0; } } diff --git a/packages/firestore/src/model/values.ts b/packages/firestore/src/model/values.ts index 2e3417e674f..1977767515e 100644 --- a/packages/firestore/src/model/values.ts +++ b/packages/firestore/src/model/values.ts @@ -25,11 +25,7 @@ import { Value } from '../protos/firestore_proto_api'; import { fail } from '../util/assert'; -import { - arrayEquals, - primitiveComparator, - compareUtf8Strings -} from '../util/misc'; +import { arrayEquals, primitiveComparator } from '../util/misc'; import { forEach, objectSize } from '../util/obj'; import { isNegativeZero } from '../util/types'; @@ -255,7 +251,7 @@ export function valueCompare(left: Value, right: Value): number { getLocalWriteTime(right) ); case TypeOrder.StringValue: - return compareUtf8Strings(left.stringValue!, right.stringValue!); + return primitiveComparator(left.stringValue!, right.stringValue!); case TypeOrder.BlobValue: return compareBlobs(left.bytesValue!, right.bytesValue!); case TypeOrder.RefValue: @@ -404,7 +400,7 @@ function compareMaps(left: MapValue, right: MapValue): number { rightKeys.sort(); for (let i = 0; i < leftKeys.length && i < rightKeys.length; ++i) { - const keyCompare = compareUtf8Strings(leftKeys[i], rightKeys[i]); + const keyCompare = primitiveComparator(leftKeys[i], rightKeys[i]); if (keyCompare !== 0) { return keyCompare; } diff --git a/packages/firestore/src/util/misc.ts b/packages/firestore/src/util/misc.ts index 6af1238398e..acaff77abb6 100644 --- a/packages/firestore/src/util/misc.ts +++ b/packages/firestore/src/util/misc.ts @@ -16,7 +16,6 @@ */ import { randomBytes } from '../platform/random_bytes'; -import { newTextEncoder } from '../platform/text_serializer'; import { debugAssert } from './assert'; @@ -75,22 +74,6 @@ export interface Equatable { isEqual(other: T): boolean; } -/** Compare strings in UTF-8 encoded byte order */ -export function compareUtf8Strings(left: string, right: string): number { - // Convert the string to UTF-8 encoded bytes - const encodedLeft = newTextEncoder().encode(left); - const encodedRight = newTextEncoder().encode(right); - - for (let i = 0; i < Math.min(encodedLeft.length, encodedRight.length); i++) { - const comparison = primitiveComparator(encodedLeft[i], encodedRight[i]); - if (comparison !== 0) { - return comparison; - } - } - - return primitiveComparator(encodedLeft.length, encodedRight.length); -} - export interface Iterable { forEach: (cb: (v: V) => void) => void; } diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index 1304b7b4aba..1cda49d9229 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -2424,199 +2424,4 @@ apiDescribe('Database', persistence => { }); }); }); - - describe('Sort unicode strings', () => { - const expectedDocs = ['b', 'a', 'c', 'f', 'e', 'd', 'g']; - it('snapshot listener sorts unicode strings the same as server', async () => { - const testDocs = { - 'a': { value: 'Łukasiewicz' }, - 'b': { value: 'Sierpiński' }, - 'c': { value: '岩澤' }, - 'd': { value: '🄟' }, - 'e': { value: 'Ｐ' }, - 'f': { value: '︒' }, - 'g': { value: '🐵' } - }; - - return withTestCollection(persistence, testDocs, async collectionRef => { - const orderedQuery = query(collectionRef, orderBy('value')); - - const getSnapshot = await getDocsFromServer(orderedQuery); - expect(toIds(getSnapshot)).to.deep.equal(expectedDocs); - - const storeEvent = new EventsAccumulator(); - const unsubscribe = onSnapshot(orderedQuery, storeEvent.storeEvent); - const watchSnapshot = await storeEvent.awaitEvent(); - expect(toIds(watchSnapshot)).to.deep.equal(toIds(getSnapshot)); - - unsubscribe(); - - await checkOnlineAndOfflineResultsMatch(orderedQuery, ...expectedDocs); - }); - }); - - it('snapshot listener sorts unicode strings in array the same as server', async () => { - const testDocs = { - 'a': { value: ['Łukasiewicz'] }, - 'b': { value: ['Sierpiński'] }, - 'c': { value: ['岩澤'] }, - 'd': { value: ['🄟'] }, - 'e': { value: ['Ｐ'] }, - 'f': { value: ['︒'] }, - 'g': { value: ['🐵'] } - }; - - return withTestCollection(persistence, testDocs, async collectionRef => { - const orderedQuery = query(collectionRef, orderBy('value')); - - const getSnapshot = await getDocsFromServer(orderedQuery); - expect(toIds(getSnapshot)).to.deep.equal(expectedDocs); - - const storeEvent = new EventsAccumulator(); - const unsubscribe = onSnapshot(orderedQuery, storeEvent.storeEvent); - const watchSnapshot = await storeEvent.awaitEvent(); - expect(toIds(watchSnapshot)).to.deep.equal(toIds(getSnapshot)); - - unsubscribe(); - - await checkOnlineAndOfflineResultsMatch(orderedQuery, ...expectedDocs); - }); - }); - - it('snapshot listener sorts unicode strings in map the same as server', async () => { - const testDocs = { - 'a': { value: { foo: 'Łukasiewicz' } }, - 'b': { value: { foo: 'Sierpiński' } }, - 'c': { value: { foo: '岩澤' } }, - 'd': { value: { foo: '🄟' } }, - 'e': { value: { foo: 'Ｐ' } }, - 'f': { value: { foo: '︒' } }, - 'g': { value: { foo: '🐵' } } - }; - - return withTestCollection(persistence, testDocs, async collectionRef => { - const orderedQuery = query(collectionRef, orderBy('value')); - - const getSnapshot = await getDocsFromServer(orderedQuery); - expect(toIds(getSnapshot)).to.deep.equal(expectedDocs); - - const storeEvent = new EventsAccumulator(); - const unsubscribe = onSnapshot(orderedQuery, storeEvent.storeEvent); - const watchSnapshot = await storeEvent.awaitEvent(); - expect(toIds(watchSnapshot)).to.deep.equal(toIds(getSnapshot)); - - unsubscribe(); - - await checkOnlineAndOfflineResultsMatch(orderedQuery, ...expectedDocs); - }); - }); - - it('snapshot listener sorts unicode strings in map key the same as server', async () => { - const testDocs = { - 'a': { value: { 'Łukasiewicz': true } }, - 'b': { value: { 'Sierpiński': true } }, - 'c': { value: { '岩澤': true } }, - 'd': { value: { '🄟': true } }, - 'e': { value: { 'Ｐ': true } }, - 'f': { value: { '︒': true } }, - 'g': { value: { '🐵': true } } - }; - - return withTestCollection(persistence, testDocs, async collectionRef => { - const orderedQuery = query(collectionRef, orderBy('value')); - - const getSnapshot = await getDocsFromServer(orderedQuery); - expect(toIds(getSnapshot)).to.deep.equal(expectedDocs); - - const storeEvent = new EventsAccumulator(); - const unsubscribe = onSnapshot(orderedQuery, storeEvent.storeEvent); - const watchSnapshot = await storeEvent.awaitEvent(); - expect(toIds(watchSnapshot)).to.deep.equal(toIds(getSnapshot)); - - unsubscribe(); - - await checkOnlineAndOfflineResultsMatch(orderedQuery, ...expectedDocs); - }); - }); - - it('snapshot listener sorts unicode strings in document key the same as server', async () => { - const testDocs = { - 'Łukasiewicz': { value: true }, - 'Sierpiński': { value: true }, - '岩澤': { value: true }, - '🄟': { value: true }, - 'Ｐ': { value: true }, - '︒': { value: true }, - '🐵': { value: true } - }; - - return withTestCollection(persistence, testDocs, async collectionRef => { - const orderedQuery = query(collectionRef, orderBy(documentId())); - - const getSnapshot = await getDocsFromServer(orderedQuery); - const expectedDocs = [ - 'Sierpiński', - 'Łukasiewicz', - '岩澤', - '︒', - 'Ｐ', - '🄟', - '🐵' - ]; - expect(toIds(getSnapshot)).to.deep.equal(expectedDocs); - - const storeEvent = new EventsAccumulator(); - const unsubscribe = onSnapshot(orderedQuery, storeEvent.storeEvent); - const watchSnapshot = await storeEvent.awaitEvent(); - expect(toIds(watchSnapshot)).to.deep.equal(toIds(getSnapshot)); - - unsubscribe(); - - await checkOnlineAndOfflineResultsMatch(orderedQuery, ...expectedDocs); - }); - }); - - // eslint-disable-next-line no-restricted-properties - (persistence.storage === 'indexeddb' ? it.skip : it)( - 'snapshot listener sorts unicode strings in document key the same as server with persistence', - async () => { - const testDocs = { - 'Łukasiewicz': { value: true }, - 'Sierpiński': { value: true }, - '岩澤': { value: true }, - '🄟': { value: true }, - 'Ｐ': { value: true }, - '︒': { value: true }, - '🐵': { value: true } - }; - - return withTestCollection( - persistence, - testDocs, - async collectionRef => { - const orderedQuery = query(collectionRef, orderBy('value')); - - const getSnapshot = await getDocsFromServer(orderedQuery); - expect(toIds(getSnapshot)).to.deep.equal([ - 'Sierpiński', - 'Łukasiewicz', - '岩澤', - '︒', - 'Ｐ', - '🄟', - '🐵' - ]); - - const storeEvent = new EventsAccumulator(); - const unsubscribe = onSnapshot(orderedQuery, storeEvent.storeEvent); - const watchSnapshot = await storeEvent.awaitEvent(); - // TODO: IndexedDB sorts string lexicographically, and misses the document with ID '🄟','🐵' - expect(toIds(watchSnapshot)).to.deep.equal(toIds(getSnapshot)); - - unsubscribe(); - } - ); - } - ); - }); });