diff --git a/.changeset/spotty-trainers-lay.md b/.changeset/spotty-trainers-lay.md new file mode 100644 index 00000000000..64c30a73e76 --- /dev/null +++ b/.changeset/spotty-trainers-lay.md @@ -0,0 +1,6 @@ +--- +'@firebase/firestore': patch +'firebase': patch +--- + +Fixed a server and sdk mismatch in unicode string sorting. diff --git a/packages/firestore/src/local/indexeddb_remote_document_cache.ts b/packages/firestore/src/local/indexeddb_remote_document_cache.ts index b3d4658d53d..9b23c64fcf5 100644 --- a/packages/firestore/src/local/indexeddb_remote_document_cache.ts +++ b/packages/firestore/src/local/indexeddb_remote_document_cache.ts @@ -655,5 +655,9 @@ 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 64cb0376a0e..e7aeb6f61cc 100644 --- a/packages/firestore/src/model/path.ts +++ b/packages/firestore/src/model/path.ts @@ -19,6 +19,7 @@ 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__'; @@ -181,7 +182,7 @@ abstract class BasePath> { return comparison; } } - return Math.sign(p1.length - p2.length); + return primitiveComparator(p1.length, p2.length); } private static compareSegments(lhs: string, rhs: string): number { @@ -201,13 +202,7 @@ abstract class BasePath> { ); } else { // both non-numeric - if (lhs < rhs) { - return -1; - } - if (lhs > rhs) { - return 1; - } - return 0; + return compareUtf8Strings(lhs, rhs); } } diff --git a/packages/firestore/src/model/values.ts b/packages/firestore/src/model/values.ts index 1977767515e..2e3417e674f 100644 --- a/packages/firestore/src/model/values.ts +++ b/packages/firestore/src/model/values.ts @@ -25,7 +25,11 @@ import { Value } from '../protos/firestore_proto_api'; import { fail } from '../util/assert'; -import { arrayEquals, primitiveComparator } from '../util/misc'; +import { + arrayEquals, + primitiveComparator, + compareUtf8Strings +} from '../util/misc'; import { forEach, objectSize } from '../util/obj'; import { isNegativeZero } from '../util/types'; @@ -251,7 +255,7 @@ export function valueCompare(left: Value, right: Value): number { getLocalWriteTime(right) ); case TypeOrder.StringValue: - return primitiveComparator(left.stringValue!, right.stringValue!); + return compareUtf8Strings(left.stringValue!, right.stringValue!); case TypeOrder.BlobValue: return compareBlobs(left.bytesValue!, right.bytesValue!); case TypeOrder.RefValue: @@ -400,7 +404,7 @@ function compareMaps(left: MapValue, right: MapValue): number { rightKeys.sort(); for (let i = 0; i < leftKeys.length && i < rightKeys.length; ++i) { - const keyCompare = primitiveComparator(leftKeys[i], rightKeys[i]); + const keyCompare = compareUtf8Strings(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 acaff77abb6..4a508553e75 100644 --- a/packages/firestore/src/util/misc.ts +++ b/packages/firestore/src/util/misc.ts @@ -74,6 +74,22 @@ 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 = new TextEncoder().encode(left); + const encodedRight = new TextEncoder().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 1cda49d9229..1304b7b4aba 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -2424,4 +2424,199 @@ 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(); + } + ); + } + ); + }); });