From 79f3453aa85da44472b954c7c887aad59b9a3e48 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 30 Jan 2023 16:08:38 -0800 Subject: [PATCH 01/14] upload initial code --- packages/firestore/src/model/path.ts | 8 +-- packages/firestore/src/platform/base64.ts | 17 +++++- packages/firestore/src/remote/serializer.ts | 6 +- packages/firestore/src/remote/watch_change.ts | 59 +++++++++---------- .../test/unit/remote/remote_event.test.ts | 12 +++- .../test/unit/specs/spec_test_runner.ts | 6 +- packages/firestore/test/util/helpers.ts | 3 +- 7 files changed, 71 insertions(+), 40 deletions(-) diff --git a/packages/firestore/src/model/path.ts b/packages/firestore/src/model/path.ts index e5e55e1b12c..661ec71581c 100644 --- a/packages/firestore/src/model/path.ts +++ b/packages/firestore/src/model/path.ts @@ -163,10 +163,10 @@ abstract class BasePath> { return this.segments.slice(this.offset, this.limit()); } - // TODO(Mila): Use database info and toString() to get full path instead. - toFullPath(): string { - return this.segments.join('/'); - } + // // TODO(Mila): Use database info and toString() to get full path instead. + // toFullPath(): string { + // return this.segments.join('/'); + // } static comparator>( p1: BasePath, diff --git a/packages/firestore/src/platform/base64.ts b/packages/firestore/src/platform/base64.ts index 877a78c732b..ed8a070892d 100644 --- a/packages/firestore/src/platform/base64.ts +++ b/packages/firestore/src/platform/base64.ts @@ -15,13 +15,28 @@ * limitations under the License. */ +class Base64DecodeError extends Error { + readonly name = 'Base64DecodeError'; +} + // This file is only used under ts-node. // eslint-disable-next-line @typescript-eslint/no-require-imports const platform = require(`./${process.env.TEST_PLATFORM ?? 'node'}/base64`); /** Converts a Base64 encoded string to a binary string. */ export function decodeBase64(encoded: string): string { - return platform.decodeBase64(encoded); + const decoded = platform.decodeBase64(encoded); + const expectedEncodedLength = 4 * Math.ceil(decoded.length / 3); + + if (encoded.length !== expectedEncodedLength) { + throw new Base64DecodeError( + `Invalid base64 string ` + + `(decoded length: ${decoded.length}, ` + + `encoded length: ${encoded.length}, ` + + `expected encoded length: ${expectedEncodedLength})` + ); + } + return decoded; } /** Converts a binary string to a Base64 encoded string. */ diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index cbdcb3b2d88..efcad69e998 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -563,7 +563,11 @@ export function fromWatchChange( const { count = 0, unchangedNames } = filter; const existenceFilter = new ExistenceFilter(count, unchangedNames); const targetId = filter.targetId; - watchChange = new ExistenceFilterChange(targetId, existenceFilter); + watchChange = new ExistenceFilterChange( + targetId, + existenceFilter, + serializer + ); } else { return fail('Unknown change type ' + JSON.stringify(change)); } diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 596f5b93609..9f98a9c11b2 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -39,6 +39,7 @@ import { SortedSet } from '../util/sorted_set'; import { BloomFilter, BloomFilterError } from './bloom_filter'; import { ExistenceFilter } from './existence_filter'; import { RemoteEvent, TargetChange } from './remote_event'; +import { getEncodedDatabaseId, JsonProtoSerializer } from './serializer'; /** * Internal representation of the watcher API protocol buffers. @@ -73,7 +74,8 @@ export class DocumentWatchChange { export class ExistenceFilterChange { constructor( public targetId: TargetId, - public existenceFilter: ExistenceFilter + public existenceFilter: ExistenceFilter, + public serializer: JsonProtoSerializer ) {} } @@ -416,8 +418,7 @@ export class WatchChangeAggregator { if (currentSize !== expectedCount) { // Apply bloom filter to identify and mark removed documents. const bloomFilterApplied = this.applyBloomFilter( - watchChange.existenceFilter, - targetId, + watchChange, currentSize ); if (!bloomFilterApplied) { @@ -433,12 +434,11 @@ export class WatchChangeAggregator { /** Returns whether a bloom filter removed the deleted documents successfully. */ private applyBloomFilter( - existenceFilter: ExistenceFilter, - targetId: number, + watchChange: ExistenceFilterChange, currentCount: number ): boolean { - const unchangedNames = existenceFilter.unchangedNames; - const expectedCount = existenceFilter.count; + const unchangedNames = watchChange.existenceFilter.unchangedNames; + const expectedCount = watchChange.existenceFilter.count; if (!unchangedNames || !unchangedNames.bits) { return false; @@ -449,17 +449,14 @@ export class WatchChangeAggregator { hashCount = 0 } = unchangedNames; - // TODO(Mila): Remove this validation, add try catch to normalizeByteString. - if (typeof bitmap === 'string') { - const isValidBitmap = this.isValidBase64String(bitmap); - if (!isValidBitmap) { - logWarn('Invalid base64 string. Applying bloom filter failed.'); - return false; - } + let normalizedBitmap: Uint8Array; + try { + normalizedBitmap = normalizeByteString(bitmap).toUint8Array(); + } catch (err) { + logWarn('Base64 string error: ', err); + return false; } - const normalizedBitmap = normalizeByteString(bitmap).toUint8Array(); - let bloomFilter: BloomFilter; try { // BloomFilter throws error if the inputs are invalid. @@ -474,37 +471,39 @@ export class WatchChangeAggregator { } const removedDocumentCount = this.filterRemovedDocuments( - bloomFilter, - targetId + watchChange, + bloomFilter ); return expectedCount === currentCount - removedDocumentCount; } - // TODO(Mila): Move the validation into normalizeByteString. - private isValidBase64String(value: string): boolean { - const regExp = new RegExp( - '^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$' - ); - return regExp.test(value); - } - /** * Filter out removed documents based on bloom filter membership result and * return number of documents removed. */ private filterRemovedDocuments( - bloomFilter: BloomFilter, - targetId: number + watchChange: ExistenceFilterChange, + bloomFilter: BloomFilter ): number { - const existingKeys = this.metadataProvider.getRemoteKeysForTarget(targetId); + const targetId = watchChange.targetId; + const existingKeys = this.metadataProvider.getRemoteKeysForTarget( + watchChange.targetId + ); let removalCount = 0; + existingKeys.forEach(key => { - if (!bloomFilter.mightContain(key.path.toFullPath())) { + const documentPath = + getEncodedDatabaseId(watchChange.serializer!) + + '/documents/' + + key.path.toString(); + + if (!bloomFilter.mightContain(documentPath)) { this.removeDocumentFromTarget(targetId, key, /*updatedDocument=*/ null); removalCount++; } }); + return removalCount; } diff --git a/packages/firestore/test/unit/remote/remote_event.test.ts b/packages/firestore/test/unit/remote/remote_event.test.ts index cedfbe8c157..e9aaa672aa5 100644 --- a/packages/firestore/test/unit/remote/remote_event.test.ts +++ b/packages/firestore/test/unit/remote/remote_event.test.ts @@ -23,6 +23,7 @@ import { TargetData, TargetPurpose } from '../../../src/local/target_data'; import { DocumentKeySet, documentKeySet } from '../../../src/model/collections'; import { ExistenceFilter } from '../../../src/remote/existence_filter'; import { RemoteEvent, TargetChange } from '../../../src/remote/remote_event'; +import { JsonProtoSerializer } from '../../../src/remote/serializer'; import { DocumentWatchChange, ExistenceFilterChange, @@ -43,6 +44,7 @@ import { key, forEachNumber } from '../../util/helpers'; +import { TEST_DATABASE_ID } from '../local/persistence_test_helpers'; interface TargetMap { [targetId: string]: TargetData; @@ -155,6 +157,12 @@ describe('RemoteEvent', () => { version(options.snapshotVersion) ); } + + const serializer = new JsonProtoSerializer( + TEST_DATABASE_ID, + /* useProto3Json= */ true + ); + it('will accumulate document added and removed events', () => { const targets = listens(1, 2, 3, 4, 5, 6); @@ -451,7 +459,7 @@ describe('RemoteEvent', () => { // The existence filter mismatch will remove the document from target 1, // but not synthesize a document delete. aggregator.handleExistenceFilter( - new ExistenceFilterChange(1, new ExistenceFilter(0)) + new ExistenceFilterChange(1, new ExistenceFilter(0), serializer) ); event = aggregator.createRemoteEvent(version(3)); @@ -491,7 +499,7 @@ describe('RemoteEvent', () => { // The existence filter mismatch will clear the previous target mapping, // but not synthesize a document delete. aggregator.handleExistenceFilter( - new ExistenceFilterChange(1, new ExistenceFilter(0)) + new ExistenceFilterChange(1, new ExistenceFilter(0), serializer) ); const event = aggregator.createRemoteEvent(version(3)); diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 137f51d6d54..82ea0cd3c6f 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -700,7 +700,11 @@ abstract class TestRunner { 'ExistenceFilters currently support exactly one target only.' ); const filter = new ExistenceFilter(keys.length, bloomFilter); - const change = new ExistenceFilterChange(targetIds[0], filter); + const change = new ExistenceFilterChange( + targetIds[0], + filter, + this.serializer + ); return this.doWatchEvent(change); } diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index f4adb5d0434..eb6c41276fe 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -444,7 +444,8 @@ export function existenceFilterEvent( aggregator.handleExistenceFilter( new ExistenceFilterChange( targetId, - new ExistenceFilter(remoteCount, bloomFilter) + new ExistenceFilter(remoteCount, bloomFilter), + new JsonProtoSerializer(TEST_DATABASE_ID, /* useProto3Json= */ true) ) ); return aggregator.createRemoteEvent(version(snapshotVersion)); From d63e47fc6b3dec5b54d7a2eac2544e1272af392c Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 30 Jan 2023 16:14:32 -0800 Subject: [PATCH 02/14] format --- packages/firestore/src/model/path.ts | 5 ----- packages/firestore/src/platform/base64.ts | 4 ++++ 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/firestore/src/model/path.ts b/packages/firestore/src/model/path.ts index 661ec71581c..ecdb9ed086c 100644 --- a/packages/firestore/src/model/path.ts +++ b/packages/firestore/src/model/path.ts @@ -163,11 +163,6 @@ abstract class BasePath> { return this.segments.slice(this.offset, this.limit()); } - // // TODO(Mila): Use database info and toString() to get full path instead. - // toFullPath(): string { - // return this.segments.join('/'); - // } - static comparator>( p1: BasePath, p2: BasePath diff --git a/packages/firestore/src/platform/base64.ts b/packages/firestore/src/platform/base64.ts index ed8a070892d..4a5a7adb56d 100644 --- a/packages/firestore/src/platform/base64.ts +++ b/packages/firestore/src/platform/base64.ts @@ -15,6 +15,10 @@ * limitations under the License. */ + +/** + * An error encountered while decoding base64 string. + */ class Base64DecodeError extends Error { readonly name = 'Base64DecodeError'; } From 25ba8392e5f75493efbcab15c53557b1a6740ea6 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 30 Jan 2023 16:27:55 -0800 Subject: [PATCH 03/14] add toDocumentFullPath to serializer --- packages/firestore/src/platform/base64.ts | 1 - packages/firestore/src/remote/serializer.ts | 9 +++++++++ packages/firestore/src/remote/watch_change.ts | 8 ++------ 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/firestore/src/platform/base64.ts b/packages/firestore/src/platform/base64.ts index 4a5a7adb56d..9ad8a1b10c7 100644 --- a/packages/firestore/src/platform/base64.ts +++ b/packages/firestore/src/platform/base64.ts @@ -15,7 +15,6 @@ * limitations under the License. */ - /** * An error encountered while decoding base64 string. */ diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index efcad69e998..aaa66c0982f 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -362,6 +362,15 @@ export function getEncodedDatabaseId(serializer: JsonProtoSerializer): string { return path.canonicalString(); } +export function getDocumentFullPath( + serializer: JsonProtoSerializer, + key: DocumentKey +): string { + return ( + getEncodedDatabaseId(serializer!) + '/documents/' + key.path.toString() + ); +} + function fullyQualifiedPrefixPath(databaseId: DatabaseId): ResourcePath { return new ResourcePath([ 'projects', diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 9f98a9c11b2..6cc474cdd7f 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -39,7 +39,7 @@ import { SortedSet } from '../util/sorted_set'; import { BloomFilter, BloomFilterError } from './bloom_filter'; import { ExistenceFilter } from './existence_filter'; import { RemoteEvent, TargetChange } from './remote_event'; -import { getEncodedDatabaseId, JsonProtoSerializer } from './serializer'; +import { toDocumentFullPath, JsonProtoSerializer } from './serializer'; /** * Internal representation of the watcher API protocol buffers. @@ -493,11 +493,7 @@ export class WatchChangeAggregator { let removalCount = 0; existingKeys.forEach(key => { - const documentPath = - getEncodedDatabaseId(watchChange.serializer!) + - '/documents/' + - key.path.toString(); - + const documentPath = toDocumentFullPath(watchChange.serializer, key); if (!bloomFilter.mightContain(documentPath)) { this.removeDocumentFromTarget(targetId, key, /*updatedDocument=*/ null); removalCount++; From 185a3fadfb4d151509b02b92828b9968d35164ed Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 30 Jan 2023 16:34:58 -0800 Subject: [PATCH 04/14] correct a typo --- packages/firestore/src/remote/serializer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index aaa66c0982f..0815f2494d3 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -362,7 +362,7 @@ export function getEncodedDatabaseId(serializer: JsonProtoSerializer): string { return path.canonicalString(); } -export function getDocumentFullPath( +export function toDocumentFullPath( serializer: JsonProtoSerializer, key: DocumentKey ): string { From d4882eb004f9b3e3484e476d7f1e6c8a25e233c1 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 31 Jan 2023 10:11:18 -0800 Subject: [PATCH 05/14] use databaseId instead of serializer --- packages/firestore/src/core/database_info.ts | 4 ++++ packages/firestore/src/platform/base64.ts | 14 ++++++------- packages/firestore/src/remote/serializer.ts | 2 +- packages/firestore/src/remote/watch_change.ts | 20 ++++++++++--------- .../test/unit/remote/remote_event.test.ts | 10 ++-------- .../test/unit/specs/spec_test_runner.ts | 2 +- packages/firestore/test/util/helpers.ts | 2 +- 7 files changed, 27 insertions(+), 27 deletions(-) diff --git a/packages/firestore/src/core/database_info.ts b/packages/firestore/src/core/database_info.ts index 306a9920ea9..ca8bdb888e7 100644 --- a/packages/firestore/src/core/database_info.ts +++ b/packages/firestore/src/core/database_info.ts @@ -77,6 +77,10 @@ export class DatabaseId { other.database === this.database ); } + + canonicalString(): string { + return `projects/${this.projectId}/databases/${this.database}`; + } } export function databaseIdFromApp( diff --git a/packages/firestore/src/platform/base64.ts b/packages/firestore/src/platform/base64.ts index 9ad8a1b10c7..fbf48819243 100644 --- a/packages/firestore/src/platform/base64.ts +++ b/packages/firestore/src/platform/base64.ts @@ -28,16 +28,16 @@ const platform = require(`./${process.env.TEST_PLATFORM ?? 'node'}/base64`); /** Converts a Base64 encoded string to a binary string. */ export function decodeBase64(encoded: string): string { - const decoded = platform.decodeBase64(encoded); + let decoded: string; + try { + decoded = platform.decodeBase64(encoded); + } catch (e) { + throw new Base64DecodeError('Invalid base64 string'); + } const expectedEncodedLength = 4 * Math.ceil(decoded.length / 3); if (encoded.length !== expectedEncodedLength) { - throw new Base64DecodeError( - `Invalid base64 string ` + - `(decoded length: ${decoded.length}, ` + - `encoded length: ${encoded.length}, ` + - `expected encoded length: ${expectedEncodedLength})` - ); + throw new Base64DecodeError('Invalid base64 string'); } return decoded; } diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index 0815f2494d3..9597e3c1324 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -575,7 +575,7 @@ export function fromWatchChange( watchChange = new ExistenceFilterChange( targetId, existenceFilter, - serializer + serializer.databaseId ); } else { return fail('Unknown change type ' + JSON.stringify(change)); diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 6cc474cdd7f..a27017f97ff 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -15,6 +15,7 @@ * limitations under the License. */ +import { DatabaseId } from '../core/database_info'; import { SnapshotVersion } from '../core/snapshot_version'; import { targetIsDocumentTarget } from '../core/target'; import { TargetId } from '../core/types'; @@ -39,7 +40,6 @@ import { SortedSet } from '../util/sorted_set'; import { BloomFilter, BloomFilterError } from './bloom_filter'; import { ExistenceFilter } from './existence_filter'; import { RemoteEvent, TargetChange } from './remote_event'; -import { toDocumentFullPath, JsonProtoSerializer } from './serializer'; /** * Internal representation of the watcher API protocol buffers. @@ -75,7 +75,7 @@ export class ExistenceFilterChange { constructor( public targetId: TargetId, public existenceFilter: ExistenceFilter, - public serializer: JsonProtoSerializer + public databaseId: DatabaseId ) {} } @@ -437,8 +437,8 @@ export class WatchChangeAggregator { watchChange: ExistenceFilterChange, currentCount: number ): boolean { - const unchangedNames = watchChange.existenceFilter.unchangedNames; - const expectedCount = watchChange.existenceFilter.count; + const { unchangedNames, count: expectedCount } = + watchChange.existenceFilter; if (!unchangedNames || !unchangedNames.bits) { return false; @@ -486,14 +486,16 @@ export class WatchChangeAggregator { watchChange: ExistenceFilterChange, bloomFilter: BloomFilter ): number { - const targetId = watchChange.targetId; - const existingKeys = this.metadataProvider.getRemoteKeysForTarget( - watchChange.targetId - ); + const { targetId, databaseId } = watchChange; + const existingKeys = this.metadataProvider.getRemoteKeysForTarget(targetId); let removalCount = 0; existingKeys.forEach(key => { - const documentPath = toDocumentFullPath(watchChange.serializer, key); + const documentPath = + databaseId.canonicalString() + + '/documents/' + + key.path.canonicalString(); + if (!bloomFilter.mightContain(documentPath)) { this.removeDocumentFromTarget(targetId, key, /*updatedDocument=*/ null); removalCount++; diff --git a/packages/firestore/test/unit/remote/remote_event.test.ts b/packages/firestore/test/unit/remote/remote_event.test.ts index e9aaa672aa5..19e07c16f88 100644 --- a/packages/firestore/test/unit/remote/remote_event.test.ts +++ b/packages/firestore/test/unit/remote/remote_event.test.ts @@ -23,7 +23,6 @@ import { TargetData, TargetPurpose } from '../../../src/local/target_data'; import { DocumentKeySet, documentKeySet } from '../../../src/model/collections'; import { ExistenceFilter } from '../../../src/remote/existence_filter'; import { RemoteEvent, TargetChange } from '../../../src/remote/remote_event'; -import { JsonProtoSerializer } from '../../../src/remote/serializer'; import { DocumentWatchChange, ExistenceFilterChange, @@ -158,11 +157,6 @@ describe('RemoteEvent', () => { ); } - const serializer = new JsonProtoSerializer( - TEST_DATABASE_ID, - /* useProto3Json= */ true - ); - it('will accumulate document added and removed events', () => { const targets = listens(1, 2, 3, 4, 5, 6); @@ -459,7 +453,7 @@ describe('RemoteEvent', () => { // The existence filter mismatch will remove the document from target 1, // but not synthesize a document delete. aggregator.handleExistenceFilter( - new ExistenceFilterChange(1, new ExistenceFilter(0), serializer) + new ExistenceFilterChange(1, new ExistenceFilter(0), TEST_DATABASE_ID) ); event = aggregator.createRemoteEvent(version(3)); @@ -499,7 +493,7 @@ describe('RemoteEvent', () => { // The existence filter mismatch will clear the previous target mapping, // but not synthesize a document delete. aggregator.handleExistenceFilter( - new ExistenceFilterChange(1, new ExistenceFilter(0), serializer) + new ExistenceFilterChange(1, new ExistenceFilter(0), TEST_DATABASE_ID) ); const event = aggregator.createRemoteEvent(version(3)); diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 82ea0cd3c6f..c2aa8f82464 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -703,7 +703,7 @@ abstract class TestRunner { const change = new ExistenceFilterChange( targetIds[0], filter, - this.serializer + this.serializer.databaseId ); return this.doWatchEvent(change); } diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index eb6c41276fe..82c84d39316 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -445,7 +445,7 @@ export function existenceFilterEvent( new ExistenceFilterChange( targetId, new ExistenceFilter(remoteCount, bloomFilter), - new JsonProtoSerializer(TEST_DATABASE_ID, /* useProto3Json= */ true) + TEST_DATABASE_ID ) ); return aggregator.createRemoteEvent(version(snapshotVersion)); From 5160dbfce5e50e427981343d517f7ca336e6fadb Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 31 Jan 2023 10:18:55 -0800 Subject: [PATCH 06/14] remove unused toDocumentFullPath in serializer --- packages/firestore/src/remote/serializer.ts | 9 --------- 1 file changed, 9 deletions(-) diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index 9597e3c1324..7881b687d3c 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -362,15 +362,6 @@ export function getEncodedDatabaseId(serializer: JsonProtoSerializer): string { return path.canonicalString(); } -export function toDocumentFullPath( - serializer: JsonProtoSerializer, - key: DocumentKey -): string { - return ( - getEncodedDatabaseId(serializer!) + '/documents/' + key.path.toString() - ); -} - function fullyQualifiedPrefixPath(databaseId: DatabaseId): ResourcePath { return new ResourcePath([ 'projects', From 4081ead1438d3483707f13eb24dfb966374d06f4 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 31 Jan 2023 16:08:12 -0800 Subject: [PATCH 07/14] adjust the code to throw Base64DecodeError --- common/api-review/util.api.md | 8 ++++++ packages/firestore/src/platform/base64.ts | 19 +++++--------- .../src/platform/base64_decode_error.ts | 18 +++++++++++++ .../firestore/src/platform/browser/base64.ts | 12 ++++++++- .../platform/browser/base64_decode_error.ts | 23 ++++++++++++++++ .../browser_lite/base64_decode_error.ts | 18 +++++++++++++ .../src/platform/node/base64_decode_error.ts | 18 +++++++++++++ .../platform/node_lite/base64_decode_error.ts | 18 +++++++++++++ packages/firestore/src/platform/rn/base64.ts | 26 +++++++++++++------ .../src/platform/rn/base64_decode_error.ts | 18 +++++++++++++ .../platform/rn_lite/base64_decode_error.ts | 18 +++++++++++++ packages/firestore/src/remote/watch_change.ts | 7 ++++- packages/util/src/crypt.ts | 9 ++++++- 13 files changed, 188 insertions(+), 24 deletions(-) create mode 100644 packages/firestore/src/platform/base64_decode_error.ts create mode 100644 packages/firestore/src/platform/browser/base64_decode_error.ts create mode 100644 packages/firestore/src/platform/browser_lite/base64_decode_error.ts create mode 100644 packages/firestore/src/platform/node/base64_decode_error.ts create mode 100644 packages/firestore/src/platform/node_lite/base64_decode_error.ts create mode 100644 packages/firestore/src/platform/rn/base64_decode_error.ts create mode 100644 packages/firestore/src/platform/rn_lite/base64_decode_error.ts diff --git a/common/api-review/util.api.md b/common/api-review/util.api.md index be8703adf3f..75e484edd50 100644 --- a/common/api-review/util.api.md +++ b/common/api-review/util.api.md @@ -93,6 +93,14 @@ export function createSubscribe(executor: Executor, onNoObservers?: Execut // @public export const decode: (token: string) => DecodedToken; +// Warning: (ae-missing-release-tag) "DecodeBase64StringError" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public +export class DecodeBase64StringError extends Error { + // (undocumented) + readonly name = "DecodeBase64StringError"; +} + // Warning: (ae-missing-release-tag) "deepCopy" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public diff --git a/packages/firestore/src/platform/base64.ts b/packages/firestore/src/platform/base64.ts index fbf48819243..cac34ccc733 100644 --- a/packages/firestore/src/platform/base64.ts +++ b/packages/firestore/src/platform/base64.ts @@ -15,12 +15,7 @@ * limitations under the License. */ -/** - * An error encountered while decoding base64 string. - */ -class Base64DecodeError extends Error { - readonly name = 'Base64DecodeError'; -} +import { Base64DecodeError } from './base64_decode_error'; // This file is only used under ts-node. // eslint-disable-next-line @typescript-eslint/no-require-imports @@ -28,17 +23,15 @@ const platform = require(`./${process.env.TEST_PLATFORM ?? 'node'}/base64`); /** Converts a Base64 encoded string to a binary string. */ export function decodeBase64(encoded: string): string { - let decoded: string; - try { - decoded = platform.decodeBase64(encoded); - } catch (e) { - throw new Base64DecodeError('Invalid base64 string'); - } - const expectedEncodedLength = 4 * Math.ceil(decoded.length / 3); + const decoded = platform.decodeBase64(encoded); + // A quick sanity check as node and rn will not throw error if input is an + // invalid base64 string, ie, "A===". + const expectedEncodedLength = 4 * Math.ceil(decoded.length / 3); if (encoded.length !== expectedEncodedLength) { throw new Base64DecodeError('Invalid base64 string'); } + return decoded; } diff --git a/packages/firestore/src/platform/base64_decode_error.ts b/packages/firestore/src/platform/base64_decode_error.ts new file mode 100644 index 00000000000..368b65e819c --- /dev/null +++ b/packages/firestore/src/platform/base64_decode_error.ts @@ -0,0 +1,18 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (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.0 + * + * 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. + */ + +export * from './browser/base64_decode_error'; diff --git a/packages/firestore/src/platform/browser/base64.ts b/packages/firestore/src/platform/browser/base64.ts index 6c068b17244..74b8e693bf5 100644 --- a/packages/firestore/src/platform/browser/base64.ts +++ b/packages/firestore/src/platform/browser/base64.ts @@ -15,9 +15,19 @@ * limitations under the License. */ +import { Base64DecodeError } from './base64_decode_error'; + /** Converts a Base64 encoded string to a binary string. */ export function decodeBase64(encoded: string): string { - return atob(encoded); + try { + return atob(encoded); + } catch (e) { + if (e instanceof DOMException) { + throw new Base64DecodeError('Invalid base64 string'); + } else { + throw e; + } + } } /** Converts a binary string to a Base64 encoded string. */ diff --git a/packages/firestore/src/platform/browser/base64_decode_error.ts b/packages/firestore/src/platform/browser/base64_decode_error.ts new file mode 100644 index 00000000000..84a20c55d1d --- /dev/null +++ b/packages/firestore/src/platform/browser/base64_decode_error.ts @@ -0,0 +1,23 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (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.0 + * + * 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. + */ + +/** + * An error encountered while decoding base64 string. + */ +export class Base64DecodeError extends Error { + readonly name = 'Base64DecodeError'; +} diff --git a/packages/firestore/src/platform/browser_lite/base64_decode_error.ts b/packages/firestore/src/platform/browser_lite/base64_decode_error.ts new file mode 100644 index 00000000000..1cfec183d84 --- /dev/null +++ b/packages/firestore/src/platform/browser_lite/base64_decode_error.ts @@ -0,0 +1,18 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (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.0 + * + * 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. + */ + +export * from '../browser/base64_decode_error'; diff --git a/packages/firestore/src/platform/node/base64_decode_error.ts b/packages/firestore/src/platform/node/base64_decode_error.ts new file mode 100644 index 00000000000..1cfec183d84 --- /dev/null +++ b/packages/firestore/src/platform/node/base64_decode_error.ts @@ -0,0 +1,18 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (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.0 + * + * 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. + */ + +export * from '../browser/base64_decode_error'; diff --git a/packages/firestore/src/platform/node_lite/base64_decode_error.ts b/packages/firestore/src/platform/node_lite/base64_decode_error.ts new file mode 100644 index 00000000000..1cfec183d84 --- /dev/null +++ b/packages/firestore/src/platform/node_lite/base64_decode_error.ts @@ -0,0 +1,18 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (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.0 + * + * 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. + */ + +export * from '../browser/base64_decode_error'; diff --git a/packages/firestore/src/platform/rn/base64.ts b/packages/firestore/src/platform/rn/base64.ts index 4032190e5cd..85a970ae816 100644 --- a/packages/firestore/src/platform/rn/base64.ts +++ b/packages/firestore/src/platform/rn/base64.ts @@ -15,7 +15,9 @@ * limitations under the License. */ -import { base64 } from '@firebase/util'; +import { base64, DecodeBase64StringError } from '@firebase/util'; + +import { Base64DecodeError } from './base64_decode_error'; // WebSafe uses a different URL-encoding safe alphabet that doesn't match // the encoding used on the backend. @@ -23,13 +25,21 @@ const WEB_SAFE = false; /** Converts a Base64 encoded string to a binary string. */ export function decodeBase64(encoded: string): string { - return String.fromCharCode.apply( - null, - // We use `decodeStringToByteArray()` instead of `decodeString()` since - // `decodeString()` returns Unicode strings, which doesn't match the values - // returned by `atob()`'s Latin1 representation. - base64.decodeStringToByteArray(encoded, WEB_SAFE) - ); + try { + return String.fromCharCode.apply( + null, + // We use `decodeStringToByteArray()` instead of `decodeString()` since + // `decodeString()` returns Unicode strings, which doesn't match the values + // returned by `atob()`'s Latin1 representation. + base64.decodeStringToByteArray(encoded, WEB_SAFE) + ); + } catch (e) { + if (e instanceof DecodeBase64StringError) { + throw new Base64DecodeError('Invalid base64 string'); + } else { + throw e; + } + } } /** Converts a binary string to a Base64 encoded string. */ diff --git a/packages/firestore/src/platform/rn/base64_decode_error.ts b/packages/firestore/src/platform/rn/base64_decode_error.ts new file mode 100644 index 00000000000..1cfec183d84 --- /dev/null +++ b/packages/firestore/src/platform/rn/base64_decode_error.ts @@ -0,0 +1,18 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (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.0 + * + * 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. + */ + +export * from '../browser/base64_decode_error'; diff --git a/packages/firestore/src/platform/rn_lite/base64_decode_error.ts b/packages/firestore/src/platform/rn_lite/base64_decode_error.ts new file mode 100644 index 00000000000..1cfec183d84 --- /dev/null +++ b/packages/firestore/src/platform/rn_lite/base64_decode_error.ts @@ -0,0 +1,18 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (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.0 + * + * 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. + */ + +export * from '../browser/base64_decode_error'; diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index a27017f97ff..2361831471d 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -29,6 +29,7 @@ import { import { MutableDocument } from '../model/document'; import { DocumentKey } from '../model/document_key'; import { normalizeByteString } from '../model/normalize'; +import { Base64DecodeError } from '../platform/base64_decode_error'; import { debugAssert, fail, hardAssert } from '../util/assert'; import { ByteString } from '../util/byte_string'; import { FirestoreError } from '../util/error'; @@ -453,7 +454,11 @@ export class WatchChangeAggregator { try { normalizedBitmap = normalizeByteString(bitmap).toUint8Array(); } catch (err) { - logWarn('Base64 string error: ', err); + if (err instanceof Base64DecodeError) { + logWarn('Base64 string error: ', err); + } else { + logWarn('Normalizing bloom filter bitmap failed: ', err); + } return false; } diff --git a/packages/util/src/crypt.ts b/packages/util/src/crypt.ts index df1e058756f..7cd32125043 100644 --- a/packages/util/src/crypt.ts +++ b/packages/util/src/crypt.ts @@ -284,7 +284,7 @@ export const base64: Base64 = { ++i; if (byte1 == null || byte2 == null || byte3 == null || byte4 == null) { - throw Error(); + throw new DecodeBase64StringError(); } const outByte1 = (byte1 << 2) | (byte2 >> 4); @@ -333,6 +333,13 @@ export const base64: Base64 = { } }; +/** + * An error encountered while decoding base64 string. + */ +export class DecodeBase64StringError extends Error { + readonly name = 'DecodeBase64StringError'; +} + /** * URL-safe base64 encoding */ From 903536b77b76d0a30c93b20803c5ca03f4672472 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 1 Feb 2023 10:45:27 -0800 Subject: [PATCH 08/14] add databaseId to WatchChangeAggregator instead of ExistenceFilterChange --- packages/firestore/src/platform/base64.ts | 2 +- .../src/platform/base64_decode_error.ts | 18 ------------------ .../firestore/src/platform/browser/base64.ts | 3 ++- .../browser_lite/base64_decode_error.ts | 18 ------------------ .../src/platform/node/base64_decode_error.ts | 18 ------------------ .../platform/node_lite/base64_decode_error.ts | 18 ------------------ packages/firestore/src/platform/rn/base64.ts | 2 +- .../src/platform/rn/base64_decode_error.ts | 18 ------------------ .../platform/rn_lite/base64_decode_error.ts | 18 ------------------ packages/firestore/src/remote/datastore.ts | 2 ++ packages/firestore/src/remote/remote_store.ts | 4 +++- packages/firestore/src/remote/serializer.ts | 1 - packages/firestore/src/remote/watch_change.ts | 12 +++++------- .../browser => util}/base64_decode_error.ts | 0 .../test/unit/local/local_store.test.ts | 6 +++--- .../test/unit/remote/remote_event.test.ts | 7 ++++--- .../test/unit/specs/spec_test_runner.ts | 3 +-- packages/firestore/test/util/helpers.ts | 16 +++++++++++----- 18 files changed, 33 insertions(+), 133 deletions(-) delete mode 100644 packages/firestore/src/platform/base64_decode_error.ts delete mode 100644 packages/firestore/src/platform/browser_lite/base64_decode_error.ts delete mode 100644 packages/firestore/src/platform/node/base64_decode_error.ts delete mode 100644 packages/firestore/src/platform/node_lite/base64_decode_error.ts delete mode 100644 packages/firestore/src/platform/rn/base64_decode_error.ts delete mode 100644 packages/firestore/src/platform/rn_lite/base64_decode_error.ts rename packages/firestore/src/{platform/browser => util}/base64_decode_error.ts (100%) diff --git a/packages/firestore/src/platform/base64.ts b/packages/firestore/src/platform/base64.ts index cac34ccc733..3fb735ef299 100644 --- a/packages/firestore/src/platform/base64.ts +++ b/packages/firestore/src/platform/base64.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { Base64DecodeError } from './base64_decode_error'; +import { Base64DecodeError } from '../util/base64_decode_error'; // This file is only used under ts-node. // eslint-disable-next-line @typescript-eslint/no-require-imports diff --git a/packages/firestore/src/platform/base64_decode_error.ts b/packages/firestore/src/platform/base64_decode_error.ts deleted file mode 100644 index 368b65e819c..00000000000 --- a/packages/firestore/src/platform/base64_decode_error.ts +++ /dev/null @@ -1,18 +0,0 @@ -/** - * @license - * Copyright 2023 Google LLC - * - * Licensed under the Apache License, Version 2.0 (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.0 - * - * 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. - */ - -export * from './browser/base64_decode_error'; diff --git a/packages/firestore/src/platform/browser/base64.ts b/packages/firestore/src/platform/browser/base64.ts index 74b8e693bf5..8c79a7db7ef 100644 --- a/packages/firestore/src/platform/browser/base64.ts +++ b/packages/firestore/src/platform/browser/base64.ts @@ -15,7 +15,8 @@ * limitations under the License. */ -import { Base64DecodeError } from './base64_decode_error'; +import { Base64DecodeError } from "../../util/base64_decode_error"; + /** Converts a Base64 encoded string to a binary string. */ export function decodeBase64(encoded: string): string { diff --git a/packages/firestore/src/platform/browser_lite/base64_decode_error.ts b/packages/firestore/src/platform/browser_lite/base64_decode_error.ts deleted file mode 100644 index 1cfec183d84..00000000000 --- a/packages/firestore/src/platform/browser_lite/base64_decode_error.ts +++ /dev/null @@ -1,18 +0,0 @@ -/** - * @license - * Copyright 2023 Google LLC - * - * Licensed under the Apache License, Version 2.0 (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.0 - * - * 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. - */ - -export * from '../browser/base64_decode_error'; diff --git a/packages/firestore/src/platform/node/base64_decode_error.ts b/packages/firestore/src/platform/node/base64_decode_error.ts deleted file mode 100644 index 1cfec183d84..00000000000 --- a/packages/firestore/src/platform/node/base64_decode_error.ts +++ /dev/null @@ -1,18 +0,0 @@ -/** - * @license - * Copyright 2023 Google LLC - * - * Licensed under the Apache License, Version 2.0 (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.0 - * - * 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. - */ - -export * from '../browser/base64_decode_error'; diff --git a/packages/firestore/src/platform/node_lite/base64_decode_error.ts b/packages/firestore/src/platform/node_lite/base64_decode_error.ts deleted file mode 100644 index 1cfec183d84..00000000000 --- a/packages/firestore/src/platform/node_lite/base64_decode_error.ts +++ /dev/null @@ -1,18 +0,0 @@ -/** - * @license - * Copyright 2023 Google LLC - * - * Licensed under the Apache License, Version 2.0 (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.0 - * - * 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. - */ - -export * from '../browser/base64_decode_error'; diff --git a/packages/firestore/src/platform/rn/base64.ts b/packages/firestore/src/platform/rn/base64.ts index 85a970ae816..f64a4131629 100644 --- a/packages/firestore/src/platform/rn/base64.ts +++ b/packages/firestore/src/platform/rn/base64.ts @@ -16,8 +16,8 @@ */ import { base64, DecodeBase64StringError } from '@firebase/util'; +import { Base64DecodeError } from '../../util/base64_decode_error'; -import { Base64DecodeError } from './base64_decode_error'; // WebSafe uses a different URL-encoding safe alphabet that doesn't match // the encoding used on the backend. diff --git a/packages/firestore/src/platform/rn/base64_decode_error.ts b/packages/firestore/src/platform/rn/base64_decode_error.ts deleted file mode 100644 index 1cfec183d84..00000000000 --- a/packages/firestore/src/platform/rn/base64_decode_error.ts +++ /dev/null @@ -1,18 +0,0 @@ -/** - * @license - * Copyright 2023 Google LLC - * - * Licensed under the Apache License, Version 2.0 (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.0 - * - * 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. - */ - -export * from '../browser/base64_decode_error'; diff --git a/packages/firestore/src/platform/rn_lite/base64_decode_error.ts b/packages/firestore/src/platform/rn_lite/base64_decode_error.ts deleted file mode 100644 index 1cfec183d84..00000000000 --- a/packages/firestore/src/platform/rn_lite/base64_decode_error.ts +++ /dev/null @@ -1,18 +0,0 @@ -/** - * @license - * Copyright 2023 Google LLC - * - * Licensed under the Apache License, Version 2.0 (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.0 - * - * 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. - */ - -export * from '../browser/base64_decode_error'; diff --git a/packages/firestore/src/remote/datastore.ts b/packages/firestore/src/remote/datastore.ts index 64661485218..df126183c49 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/packages/firestore/src/remote/datastore.ts @@ -17,6 +17,7 @@ import { CredentialsProvider } from '../api/credentials'; import { User } from '../auth/user'; +import { DatabaseId } from '../core/database_info'; import { Query, queryToTarget } from '../core/query'; import { Document } from '../model/document'; import { DocumentKey } from '../model/document_key'; @@ -59,6 +60,7 @@ import { */ export abstract class Datastore { abstract terminate(): void; + abstract serializer: JsonProtoSerializer } /** diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index 769260e8d8d..b4149a13a25 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/packages/firestore/src/remote/remote_store.ts @@ -376,7 +376,9 @@ function startWatchStream(remoteStoreImpl: RemoteStoreImpl): void { remoteStoreImpl.remoteSyncer.getRemoteKeysForTarget!(targetId), getTargetDataForTarget: targetId => remoteStoreImpl.listenTargets.get(targetId) || null - }); + }, + remoteStoreImpl.datastore.serializer.databaseId + ); ensureWatchStream(remoteStoreImpl).start(); remoteStoreImpl.onlineStateTracker.handleWatchStreamStart(); } diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index 7881b687d3c..e83ff0700ba 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -566,7 +566,6 @@ export function fromWatchChange( watchChange = new ExistenceFilterChange( targetId, existenceFilter, - serializer.databaseId ); } else { return fail('Unknown change type ' + JSON.stringify(change)); diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 2361831471d..5a9318d83f1 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -29,7 +29,7 @@ import { import { MutableDocument } from '../model/document'; import { DocumentKey } from '../model/document_key'; import { normalizeByteString } from '../model/normalize'; -import { Base64DecodeError } from '../platform/base64_decode_error'; +import { Base64DecodeError } from '../util/base64_decode_error'; import { debugAssert, fail, hardAssert } from '../util/assert'; import { ByteString } from '../util/byte_string'; import { FirestoreError } from '../util/error'; @@ -76,7 +76,6 @@ export class ExistenceFilterChange { constructor( public targetId: TargetId, public existenceFilter: ExistenceFilter, - public databaseId: DatabaseId ) {} } @@ -264,7 +263,7 @@ const LOG_TAG = 'WatchChangeAggregator'; * A helper class to accumulate watch changes into a RemoteEvent. */ export class WatchChangeAggregator { - constructor(private metadataProvider: TargetMetadataProvider) {} + constructor(private metadataProvider: TargetMetadataProvider, private readonly databaseId: DatabaseId) {} /** The internal state of all tracked targets. */ private targetStates = new Map(); @@ -476,7 +475,7 @@ export class WatchChangeAggregator { } const removedDocumentCount = this.filterRemovedDocuments( - watchChange, + watchChange.targetId, bloomFilter ); @@ -488,16 +487,15 @@ export class WatchChangeAggregator { * return number of documents removed. */ private filterRemovedDocuments( - watchChange: ExistenceFilterChange, + targetId: number, bloomFilter: BloomFilter ): number { - const { targetId, databaseId } = watchChange; const existingKeys = this.metadataProvider.getRemoteKeysForTarget(targetId); let removalCount = 0; existingKeys.forEach(key => { const documentPath = - databaseId.canonicalString() + + this.databaseId.canonicalString() + '/documents/' + key.path.canonicalString(); diff --git a/packages/firestore/src/platform/browser/base64_decode_error.ts b/packages/firestore/src/util/base64_decode_error.ts similarity index 100% rename from packages/firestore/src/platform/browser/base64_decode_error.ts rename to packages/firestore/src/util/base64_decode_error.ts diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index c25d337dc13..e48f423003c 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -1274,7 +1274,7 @@ function genericLocalStoreTests( const aggregator = new WatchChangeAggregator({ getRemoteKeysForTarget: () => documentKeySet(), getTargetDataForTarget: () => targetData - }); + },persistenceHelpers.TEST_DATABASE_ID); aggregator.handleTargetChange(watchChange); const remoteEvent = aggregator.createRemoteEvent(version(1000)); await localStoreApplyRemoteEventToLocalCache(localStore, remoteEvent); @@ -1314,7 +1314,7 @@ function genericLocalStoreTests( const aggregator1 = new WatchChangeAggregator({ getRemoteKeysForTarget: () => documentKeySet(), getTargetDataForTarget: () => targetData - }); + },persistenceHelpers.TEST_DATABASE_ID); aggregator1.handleTargetChange(watchChange1); const remoteEvent1 = aggregator1.createRemoteEvent(version(1000)); await localStoreApplyRemoteEventToLocalCache(localStore, remoteEvent1); @@ -1327,7 +1327,7 @@ function genericLocalStoreTests( const aggregator2 = new WatchChangeAggregator({ getRemoteKeysForTarget: () => documentKeySet(), getTargetDataForTarget: () => targetData - }); + },persistenceHelpers.TEST_DATABASE_ID); aggregator2.handleTargetChange(watchChange2); const remoteEvent2 = aggregator2.createRemoteEvent(version(2000)); await localStoreApplyRemoteEventToLocalCache(localStore, remoteEvent2); diff --git a/packages/firestore/test/unit/remote/remote_event.test.ts b/packages/firestore/test/unit/remote/remote_event.test.ts index 19e07c16f88..dc71deaf62f 100644 --- a/packages/firestore/test/unit/remote/remote_event.test.ts +++ b/packages/firestore/test/unit/remote/remote_event.test.ts @@ -116,7 +116,8 @@ describe('RemoteEvent', () => { getRemoteKeysForTarget: () => options.existingKeys || documentKeySet(), getTargetDataForTarget: targetId => options.targets ? options.targets[targetId] : null - }); + } ,TEST_DATABASE_ID + ); if (options.outstandingResponses) { forEachNumber(options.outstandingResponses, (targetId, count) => { @@ -453,7 +454,7 @@ describe('RemoteEvent', () => { // The existence filter mismatch will remove the document from target 1, // but not synthesize a document delete. aggregator.handleExistenceFilter( - new ExistenceFilterChange(1, new ExistenceFilter(0), TEST_DATABASE_ID) + new ExistenceFilterChange(1, new ExistenceFilter(0)) ); event = aggregator.createRemoteEvent(version(3)); @@ -493,7 +494,7 @@ describe('RemoteEvent', () => { // The existence filter mismatch will clear the previous target mapping, // but not synthesize a document delete. aggregator.handleExistenceFilter( - new ExistenceFilterChange(1, new ExistenceFilter(0), TEST_DATABASE_ID) + new ExistenceFilterChange(1, new ExistenceFilter(0)) ); const event = aggregator.createRemoteEvent(version(3)); diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index c2aa8f82464..def4592a960 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -702,8 +702,7 @@ abstract class TestRunner { const filter = new ExistenceFilter(keys.length, bloomFilter); const change = new ExistenceFilterChange( targetIds[0], - filter, - this.serializer.databaseId + filter ); return this.doWatchEvent(change); } diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index 82c84d39316..e77df27df64 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -418,7 +418,9 @@ export function noChangeEvent( getRemoteKeysForTarget: () => documentKeySet(), getTargetDataForTarget: targetId => targetData(targetId, TargetPurpose.Listen, 'foo') - }); + }, + TEST_DATABASE_ID + ); aggregator.handleTargetChange( new WatchTargetChange( WatchTargetChangeState.NoChange, @@ -440,12 +442,13 @@ export function existenceFilterEvent( getRemoteKeysForTarget: () => syncedKeys, getTargetDataForTarget: targetId => targetData(targetId, TargetPurpose.Listen, 'foo') - }); + }, + TEST_DATABASE_ID + ); aggregator.handleExistenceFilter( new ExistenceFilterChange( targetId, new ExistenceFilter(remoteCount, bloomFilter), - TEST_DATABASE_ID ) ); return aggregator.createRemoteEvent(version(snapshotVersion)); @@ -478,7 +481,9 @@ export function docAddedRemoteEvent( return null; } } - }); + }, + TEST_DATABASE_ID + ); let version = SnapshotVersion.min(); @@ -525,7 +530,8 @@ export function docUpdateRemoteEvent( : TargetPurpose.Listen; return targetData(targetId, purpose, doc.key.toString()); } - }); + }, TEST_DATABASE_ID + ); aggregator.handleDocumentChange(docChange); return aggregator.createRemoteEvent(doc.version); } From fbac6efc617959c5ac0b649dbe932ae85a075721 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 1 Feb 2023 11:03:55 -0800 Subject: [PATCH 09/14] format --- .../firestore/src/platform/browser/base64.ts | 3 +- packages/firestore/src/platform/rn/base64.ts | 2 +- packages/firestore/src/remote/datastore.ts | 3 +- packages/firestore/src/remote/remote_store.ts | 13 +-- packages/firestore/src/remote/serializer.ts | 5 +- packages/firestore/src/remote/watch_change.ts | 9 ++- .../test/unit/local/local_store.test.ts | 33 +++++--- .../test/unit/remote/remote_event.test.ts | 12 +-- .../test/unit/specs/spec_test_runner.ts | 5 +- packages/firestore/test/util/helpers.ts | 81 ++++++++++--------- 10 files changed, 89 insertions(+), 77 deletions(-) diff --git a/packages/firestore/src/platform/browser/base64.ts b/packages/firestore/src/platform/browser/base64.ts index 8c79a7db7ef..c13108d5062 100644 --- a/packages/firestore/src/platform/browser/base64.ts +++ b/packages/firestore/src/platform/browser/base64.ts @@ -15,8 +15,7 @@ * limitations under the License. */ -import { Base64DecodeError } from "../../util/base64_decode_error"; - +import { Base64DecodeError } from '../../util/base64_decode_error'; /** Converts a Base64 encoded string to a binary string. */ export function decodeBase64(encoded: string): string { diff --git a/packages/firestore/src/platform/rn/base64.ts b/packages/firestore/src/platform/rn/base64.ts index f64a4131629..8d8733fbaab 100644 --- a/packages/firestore/src/platform/rn/base64.ts +++ b/packages/firestore/src/platform/rn/base64.ts @@ -16,8 +16,8 @@ */ import { base64, DecodeBase64StringError } from '@firebase/util'; -import { Base64DecodeError } from '../../util/base64_decode_error'; +import { Base64DecodeError } from '../../util/base64_decode_error'; // WebSafe uses a different URL-encoding safe alphabet that doesn't match // the encoding used on the backend. diff --git a/packages/firestore/src/remote/datastore.ts b/packages/firestore/src/remote/datastore.ts index df126183c49..70cfa6ee6a6 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/packages/firestore/src/remote/datastore.ts @@ -17,7 +17,6 @@ import { CredentialsProvider } from '../api/credentials'; import { User } from '../auth/user'; -import { DatabaseId } from '../core/database_info'; import { Query, queryToTarget } from '../core/query'; import { Document } from '../model/document'; import { DocumentKey } from '../model/document_key'; @@ -60,7 +59,7 @@ import { */ export abstract class Datastore { abstract terminate(): void; - abstract serializer: JsonProtoSerializer + abstract serializer: JsonProtoSerializer; } /** diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index b4149a13a25..07bfd279d97 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/packages/firestore/src/remote/remote_store.ts @@ -371,12 +371,13 @@ function startWatchStream(remoteStoreImpl: RemoteStoreImpl): void { 'getRemoteKeysForTarget() not set' ); - remoteStoreImpl.watchChangeAggregator = new WatchChangeAggregator({ - getRemoteKeysForTarget: targetId => - remoteStoreImpl.remoteSyncer.getRemoteKeysForTarget!(targetId), - getTargetDataForTarget: targetId => - remoteStoreImpl.listenTargets.get(targetId) || null - }, + remoteStoreImpl.watchChangeAggregator = new WatchChangeAggregator( + { + getRemoteKeysForTarget: targetId => + remoteStoreImpl.remoteSyncer.getRemoteKeysForTarget!(targetId), + getTargetDataForTarget: targetId => + remoteStoreImpl.listenTargets.get(targetId) || null + }, remoteStoreImpl.datastore.serializer.databaseId ); ensureWatchStream(remoteStoreImpl).start(); diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index e83ff0700ba..cbdcb3b2d88 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -563,10 +563,7 @@ export function fromWatchChange( const { count = 0, unchangedNames } = filter; const existenceFilter = new ExistenceFilter(count, unchangedNames); const targetId = filter.targetId; - watchChange = new ExistenceFilterChange( - targetId, - existenceFilter, - ); + watchChange = new ExistenceFilterChange(targetId, existenceFilter); } else { return fail('Unknown change type ' + JSON.stringify(change)); } diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 5a9318d83f1..03018b387f3 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -29,8 +29,8 @@ import { import { MutableDocument } from '../model/document'; import { DocumentKey } from '../model/document_key'; import { normalizeByteString } from '../model/normalize'; -import { Base64DecodeError } from '../util/base64_decode_error'; import { debugAssert, fail, hardAssert } from '../util/assert'; +import { Base64DecodeError } from '../util/base64_decode_error'; import { ByteString } from '../util/byte_string'; import { FirestoreError } from '../util/error'; import { logDebug, logWarn } from '../util/log'; @@ -75,7 +75,7 @@ export class DocumentWatchChange { export class ExistenceFilterChange { constructor( public targetId: TargetId, - public existenceFilter: ExistenceFilter, + public existenceFilter: ExistenceFilter ) {} } @@ -263,7 +263,10 @@ const LOG_TAG = 'WatchChangeAggregator'; * A helper class to accumulate watch changes into a RemoteEvent. */ export class WatchChangeAggregator { - constructor(private metadataProvider: TargetMetadataProvider, private readonly databaseId: DatabaseId) {} + constructor( + private metadataProvider: TargetMetadataProvider, + private readonly databaseId: DatabaseId + ) {} /** The internal state of all tracked targets. */ private targetStates = new Map(); diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index e48f423003c..16fad7d1b9e 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -1271,10 +1271,13 @@ function genericLocalStoreTests( [targetId], resumeToken ); - const aggregator = new WatchChangeAggregator({ - getRemoteKeysForTarget: () => documentKeySet(), - getTargetDataForTarget: () => targetData - },persistenceHelpers.TEST_DATABASE_ID); + const aggregator = new WatchChangeAggregator( + { + getRemoteKeysForTarget: () => documentKeySet(), + getTargetDataForTarget: () => targetData + }, + persistenceHelpers.TEST_DATABASE_ID + ); aggregator.handleTargetChange(watchChange); const remoteEvent = aggregator.createRemoteEvent(version(1000)); await localStoreApplyRemoteEventToLocalCache(localStore, remoteEvent); @@ -1311,10 +1314,13 @@ function genericLocalStoreTests( [targetId], byteStringFromString('abc') ); - const aggregator1 = new WatchChangeAggregator({ - getRemoteKeysForTarget: () => documentKeySet(), - getTargetDataForTarget: () => targetData - },persistenceHelpers.TEST_DATABASE_ID); + const aggregator1 = new WatchChangeAggregator( + { + getRemoteKeysForTarget: () => documentKeySet(), + getTargetDataForTarget: () => targetData + }, + persistenceHelpers.TEST_DATABASE_ID + ); aggregator1.handleTargetChange(watchChange1); const remoteEvent1 = aggregator1.createRemoteEvent(version(1000)); await localStoreApplyRemoteEventToLocalCache(localStore, remoteEvent1); @@ -1324,10 +1330,13 @@ function genericLocalStoreTests( [targetId], ByteString.EMPTY_BYTE_STRING ); - const aggregator2 = new WatchChangeAggregator({ - getRemoteKeysForTarget: () => documentKeySet(), - getTargetDataForTarget: () => targetData - },persistenceHelpers.TEST_DATABASE_ID); + const aggregator2 = new WatchChangeAggregator( + { + getRemoteKeysForTarget: () => documentKeySet(), + getTargetDataForTarget: () => targetData + }, + persistenceHelpers.TEST_DATABASE_ID + ); aggregator2.handleTargetChange(watchChange2); const remoteEvent2 = aggregator2.createRemoteEvent(version(2000)); await localStoreApplyRemoteEventToLocalCache(localStore, remoteEvent2); diff --git a/packages/firestore/test/unit/remote/remote_event.test.ts b/packages/firestore/test/unit/remote/remote_event.test.ts index dc71deaf62f..bedb6f82b7f 100644 --- a/packages/firestore/test/unit/remote/remote_event.test.ts +++ b/packages/firestore/test/unit/remote/remote_event.test.ts @@ -112,11 +112,13 @@ describe('RemoteEvent', () => { }); } - const aggregator = new WatchChangeAggregator({ - getRemoteKeysForTarget: () => options.existingKeys || documentKeySet(), - getTargetDataForTarget: targetId => - options.targets ? options.targets[targetId] : null - } ,TEST_DATABASE_ID + const aggregator = new WatchChangeAggregator( + { + getRemoteKeysForTarget: () => options.existingKeys || documentKeySet(), + getTargetDataForTarget: targetId => + options.targets ? options.targets[targetId] : null + }, + TEST_DATABASE_ID ); if (options.outstandingResponses) { diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index def4592a960..137f51d6d54 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -700,10 +700,7 @@ abstract class TestRunner { 'ExistenceFilters currently support exactly one target only.' ); const filter = new ExistenceFilter(keys.length, bloomFilter); - const change = new ExistenceFilterChange( - targetIds[0], - filter - ); + const change = new ExistenceFilterChange(targetIds[0], filter); return this.doWatchEvent(change); } diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index e77df27df64..f9219a42b82 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -414,12 +414,13 @@ export function noChangeEvent( snapshotVersion: number, resumeToken: ByteString = ByteString.EMPTY_BYTE_STRING ): RemoteEvent { - const aggregator = new WatchChangeAggregator({ - getRemoteKeysForTarget: () => documentKeySet(), - getTargetDataForTarget: targetId => - targetData(targetId, TargetPurpose.Listen, 'foo') - }, - TEST_DATABASE_ID + const aggregator = new WatchChangeAggregator( + { + getRemoteKeysForTarget: () => documentKeySet(), + getTargetDataForTarget: targetId => + targetData(targetId, TargetPurpose.Listen, 'foo') + }, + TEST_DATABASE_ID ); aggregator.handleTargetChange( new WatchTargetChange( @@ -438,17 +439,18 @@ export function existenceFilterEvent( snapshotVersion: number, bloomFilter?: api.BloomFilter ): RemoteEvent { - const aggregator = new WatchChangeAggregator({ - getRemoteKeysForTarget: () => syncedKeys, - getTargetDataForTarget: targetId => - targetData(targetId, TargetPurpose.Listen, 'foo') - }, - TEST_DATABASE_ID + const aggregator = new WatchChangeAggregator( + { + getRemoteKeysForTarget: () => syncedKeys, + getTargetDataForTarget: targetId => + targetData(targetId, TargetPurpose.Listen, 'foo') + }, + TEST_DATABASE_ID ); aggregator.handleExistenceFilter( new ExistenceFilterChange( targetId, - new ExistenceFilter(remoteCount, bloomFilter), + new ExistenceFilter(remoteCount, bloomFilter) ) ); return aggregator.createRemoteEvent(version(snapshotVersion)); @@ -467,22 +469,23 @@ export function docAddedRemoteEvent( ? activeTargets : (updatedInTargets || []).concat(removedFromTargets || []); - const aggregator = new WatchChangeAggregator({ - getRemoteKeysForTarget: () => documentKeySet(), - getTargetDataForTarget: targetId => { - if (allTargets.indexOf(targetId) !== -1) { - const collectionPath = docs[0].key.path.popLast(); - return targetData( - targetId, - TargetPurpose.Listen, - collectionPath.toString() - ); - } else { - return null; + const aggregator = new WatchChangeAggregator( + { + getRemoteKeysForTarget: () => documentKeySet(), + getTargetDataForTarget: targetId => { + if (allTargets.indexOf(targetId) !== -1) { + const collectionPath = docs[0].key.path.popLast(); + return targetData( + targetId, + TargetPurpose.Listen, + collectionPath.toString() + ); + } else { + return null; + } } - } - }, - TEST_DATABASE_ID + }, + TEST_DATABASE_ID ); let version = SnapshotVersion.min(); @@ -521,16 +524,18 @@ export function docUpdateRemoteEvent( doc.key, doc ); - const aggregator = new WatchChangeAggregator({ - getRemoteKeysForTarget: () => keys(doc), - getTargetDataForTarget: targetId => { - const purpose = - limboTargets && limboTargets.indexOf(targetId) !== -1 - ? TargetPurpose.LimboResolution - : TargetPurpose.Listen; - return targetData(targetId, purpose, doc.key.toString()); - } - }, TEST_DATABASE_ID + const aggregator = new WatchChangeAggregator( + { + getRemoteKeysForTarget: () => keys(doc), + getTargetDataForTarget: targetId => { + const purpose = + limboTargets && limboTargets.indexOf(targetId) !== -1 + ? TargetPurpose.LimboResolution + : TargetPurpose.Listen; + return targetData(targetId, purpose, doc.key.toString()); + } + }, + TEST_DATABASE_ID ); aggregator.handleDocumentChange(docChange); return aggregator.createRemoteEvent(doc.version); From 24ce794caca205d9ea143d6964fac96251270ccb Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 1 Feb 2023 15:23:24 -0800 Subject: [PATCH 10/14] use getter for databaseId --- packages/firestore/src/remote/datastore.ts | 7 ++++++- packages/firestore/src/remote/remote_store.ts | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/firestore/src/remote/datastore.ts b/packages/firestore/src/remote/datastore.ts index 70cfa6ee6a6..0b554bbb8d0 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/packages/firestore/src/remote/datastore.ts @@ -17,6 +17,7 @@ import { CredentialsProvider } from '../api/credentials'; import { User } from '../auth/user'; +import { DatabaseId } from '../core/database_info'; import { Query, queryToTarget } from '../core/query'; import { Document } from '../model/document'; import { DocumentKey } from '../model/document_key'; @@ -59,7 +60,7 @@ import { */ export abstract class Datastore { abstract terminate(): void; - abstract serializer: JsonProtoSerializer; + abstract databaseId: DatabaseId; } /** @@ -78,6 +79,10 @@ class DatastoreImpl extends Datastore { super(); } + get databaseId(): DatabaseId { + return this.serializer.databaseId + } + verifyInitialized(): void { debugAssert(!!this.connection, 'Datastore.start() not called'); if (this.terminated) { diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index 07bfd279d97..da824fa58fb 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/packages/firestore/src/remote/remote_store.ts @@ -378,7 +378,7 @@ function startWatchStream(remoteStoreImpl: RemoteStoreImpl): void { getTargetDataForTarget: targetId => remoteStoreImpl.listenTargets.get(targetId) || null }, - remoteStoreImpl.datastore.serializer.databaseId + remoteStoreImpl.datastore.databaseId ); ensureWatchStream(remoteStoreImpl).start(); remoteStoreImpl.onlineStateTracker.handleWatchStreamStart(); From 44b23052a87f1a6f9cf13f2bc5701611e1ea02c5 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 1 Feb 2023 15:48:01 -0800 Subject: [PATCH 11/14] FORMAT --- packages/firestore/src/remote/datastore.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/firestore/src/remote/datastore.ts b/packages/firestore/src/remote/datastore.ts index 0b554bbb8d0..8257c9a4332 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/packages/firestore/src/remote/datastore.ts @@ -79,8 +79,8 @@ class DatastoreImpl extends Datastore { super(); } - get databaseId(): DatabaseId { - return this.serializer.databaseId + get databaseId(): DatabaseId { + return this.serializer.databaseId; } verifyInitialized(): void { From aa21221a63ff5f711c1a4757a58752d56b1c4601 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 2 Feb 2023 11:19:09 -0800 Subject: [PATCH 12/14] reformat the way we get DatabaseId --- packages/firestore/src/remote/datastore.ts | 7 +- packages/firestore/src/remote/remote_store.ts | 16 ++-- packages/firestore/src/remote/watch_change.ts | 12 +-- .../firestore/test/lite/integration.test.ts | 2 +- .../test/unit/local/local_store.test.ts | 36 ++++----- .../test/unit/remote/remote_event.test.ts | 15 ++-- packages/firestore/test/util/helpers.ts | 80 +++++++++---------- 7 files changed, 73 insertions(+), 95 deletions(-) diff --git a/packages/firestore/src/remote/datastore.ts b/packages/firestore/src/remote/datastore.ts index 8257c9a4332..70cfa6ee6a6 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/packages/firestore/src/remote/datastore.ts @@ -17,7 +17,6 @@ import { CredentialsProvider } from '../api/credentials'; import { User } from '../auth/user'; -import { DatabaseId } from '../core/database_info'; import { Query, queryToTarget } from '../core/query'; import { Document } from '../model/document'; import { DocumentKey } from '../model/document_key'; @@ -60,7 +59,7 @@ import { */ export abstract class Datastore { abstract terminate(): void; - abstract databaseId: DatabaseId; + abstract serializer: JsonProtoSerializer; } /** @@ -79,10 +78,6 @@ class DatastoreImpl extends Datastore { super(); } - get databaseId(): DatabaseId { - return this.serializer.databaseId; - } - verifyInitialized(): void { debugAssert(!!this.connection, 'Datastore.start() not called'); if (this.terminated) { diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index da824fa58fb..68196a50ad2 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/packages/firestore/src/remote/remote_store.ts @@ -371,15 +371,13 @@ function startWatchStream(remoteStoreImpl: RemoteStoreImpl): void { 'getRemoteKeysForTarget() not set' ); - remoteStoreImpl.watchChangeAggregator = new WatchChangeAggregator( - { - getRemoteKeysForTarget: targetId => - remoteStoreImpl.remoteSyncer.getRemoteKeysForTarget!(targetId), - getTargetDataForTarget: targetId => - remoteStoreImpl.listenTargets.get(targetId) || null - }, - remoteStoreImpl.datastore.databaseId - ); + remoteStoreImpl.watchChangeAggregator = new WatchChangeAggregator({ + getRemoteKeysForTarget: targetId => + remoteStoreImpl.remoteSyncer.getRemoteKeysForTarget!(targetId), + getTargetDataForTarget: targetId => + remoteStoreImpl.listenTargets.get(targetId) || null, + getDatabaseId: () => remoteStoreImpl.datastore.serializer.databaseId + }); ensureWatchStream(remoteStoreImpl).start(); remoteStoreImpl.onlineStateTracker.handleWatchStreamStart(); } diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 03018b387f3..864924027cf 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -255,6 +255,11 @@ export interface TargetMetadataProvider { * has become inactive */ getTargetDataForTarget(targetId: TargetId): TargetData | null; + + /** + * Returns the database ID of the Firestore instance. + */ + getDatabaseId(): DatabaseId; } const LOG_TAG = 'WatchChangeAggregator'; @@ -263,10 +268,7 @@ const LOG_TAG = 'WatchChangeAggregator'; * A helper class to accumulate watch changes into a RemoteEvent. */ export class WatchChangeAggregator { - constructor( - private metadataProvider: TargetMetadataProvider, - private readonly databaseId: DatabaseId - ) {} + constructor(private metadataProvider: TargetMetadataProvider) {} /** The internal state of all tracked targets. */ private targetStates = new Map(); @@ -498,7 +500,7 @@ export class WatchChangeAggregator { existingKeys.forEach(key => { const documentPath = - this.databaseId.canonicalString() + + this.metadataProvider.getDatabaseId().canonicalString() + '/documents/' + key.path.canonicalString(); diff --git a/packages/firestore/test/lite/integration.test.ts b/packages/firestore/test/lite/integration.test.ts index 54c595f9684..0cb6170652c 100644 --- a/packages/firestore/test/lite/integration.test.ts +++ b/packages/firestore/test/lite/integration.test.ts @@ -852,7 +852,7 @@ describe('DocumentSnapshot', () => { it('returns Bytes', () => { return withTestDocAndInitialData( - { bytes: Bytes.fromBase64String('aa') }, + { bytes: Bytes.fromBase64String('aa==') }, async docRef => { const docSnap = await getDoc(docRef); const bytes = docSnap.get('bytes'); diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index 16fad7d1b9e..d9c93c78913 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -1271,13 +1271,11 @@ function genericLocalStoreTests( [targetId], resumeToken ); - const aggregator = new WatchChangeAggregator( - { - getRemoteKeysForTarget: () => documentKeySet(), - getTargetDataForTarget: () => targetData - }, - persistenceHelpers.TEST_DATABASE_ID - ); + const aggregator = new WatchChangeAggregator({ + getRemoteKeysForTarget: () => documentKeySet(), + getTargetDataForTarget: () => targetData, + getDatabaseId: () => persistenceHelpers.TEST_DATABASE_ID + }); aggregator.handleTargetChange(watchChange); const remoteEvent = aggregator.createRemoteEvent(version(1000)); await localStoreApplyRemoteEventToLocalCache(localStore, remoteEvent); @@ -1314,13 +1312,11 @@ function genericLocalStoreTests( [targetId], byteStringFromString('abc') ); - const aggregator1 = new WatchChangeAggregator( - { - getRemoteKeysForTarget: () => documentKeySet(), - getTargetDataForTarget: () => targetData - }, - persistenceHelpers.TEST_DATABASE_ID - ); + const aggregator1 = new WatchChangeAggregator({ + getRemoteKeysForTarget: () => documentKeySet(), + getTargetDataForTarget: () => targetData, + getDatabaseId: () => persistenceHelpers.TEST_DATABASE_ID + }); aggregator1.handleTargetChange(watchChange1); const remoteEvent1 = aggregator1.createRemoteEvent(version(1000)); await localStoreApplyRemoteEventToLocalCache(localStore, remoteEvent1); @@ -1330,13 +1326,11 @@ function genericLocalStoreTests( [targetId], ByteString.EMPTY_BYTE_STRING ); - const aggregator2 = new WatchChangeAggregator( - { - getRemoteKeysForTarget: () => documentKeySet(), - getTargetDataForTarget: () => targetData - }, - persistenceHelpers.TEST_DATABASE_ID - ); + const aggregator2 = new WatchChangeAggregator({ + getRemoteKeysForTarget: () => documentKeySet(), + getTargetDataForTarget: () => targetData, + getDatabaseId: () => persistenceHelpers.TEST_DATABASE_ID + }); aggregator2.handleTargetChange(watchChange2); const remoteEvent2 = aggregator2.createRemoteEvent(version(2000)); await localStoreApplyRemoteEventToLocalCache(localStore, remoteEvent2); diff --git a/packages/firestore/test/unit/remote/remote_event.test.ts b/packages/firestore/test/unit/remote/remote_event.test.ts index bedb6f82b7f..36646a5dba6 100644 --- a/packages/firestore/test/unit/remote/remote_event.test.ts +++ b/packages/firestore/test/unit/remote/remote_event.test.ts @@ -111,15 +111,12 @@ describe('RemoteEvent', () => { targetIds.push(targetId); }); } - - const aggregator = new WatchChangeAggregator( - { - getRemoteKeysForTarget: () => options.existingKeys || documentKeySet(), - getTargetDataForTarget: targetId => - options.targets ? options.targets[targetId] : null - }, - TEST_DATABASE_ID - ); + const aggregator = new WatchChangeAggregator({ + getRemoteKeysForTarget: () => options.existingKeys || documentKeySet(), + getTargetDataForTarget: targetId => + options.targets ? options.targets[targetId] : null, + getDatabaseId: () => TEST_DATABASE_ID + }); if (options.outstandingResponses) { forEachNumber(options.outstandingResponses, (targetId, count) => { diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index f9219a42b82..6cc59a5f779 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -414,14 +414,12 @@ export function noChangeEvent( snapshotVersion: number, resumeToken: ByteString = ByteString.EMPTY_BYTE_STRING ): RemoteEvent { - const aggregator = new WatchChangeAggregator( - { - getRemoteKeysForTarget: () => documentKeySet(), - getTargetDataForTarget: targetId => - targetData(targetId, TargetPurpose.Listen, 'foo') - }, - TEST_DATABASE_ID - ); + const aggregator = new WatchChangeAggregator({ + getRemoteKeysForTarget: () => documentKeySet(), + getTargetDataForTarget: targetId => + targetData(targetId, TargetPurpose.Listen, 'foo'), + getDatabaseId: () => TEST_DATABASE_ID + }); aggregator.handleTargetChange( new WatchTargetChange( WatchTargetChangeState.NoChange, @@ -439,14 +437,12 @@ export function existenceFilterEvent( snapshotVersion: number, bloomFilter?: api.BloomFilter ): RemoteEvent { - const aggregator = new WatchChangeAggregator( - { - getRemoteKeysForTarget: () => syncedKeys, - getTargetDataForTarget: targetId => - targetData(targetId, TargetPurpose.Listen, 'foo') - }, - TEST_DATABASE_ID - ); + const aggregator = new WatchChangeAggregator({ + getRemoteKeysForTarget: () => syncedKeys, + getTargetDataForTarget: targetId => + targetData(targetId, TargetPurpose.Listen, 'foo'), + getDatabaseId: () => TEST_DATABASE_ID + }); aggregator.handleExistenceFilter( new ExistenceFilterChange( targetId, @@ -469,24 +465,22 @@ export function docAddedRemoteEvent( ? activeTargets : (updatedInTargets || []).concat(removedFromTargets || []); - const aggregator = new WatchChangeAggregator( - { - getRemoteKeysForTarget: () => documentKeySet(), - getTargetDataForTarget: targetId => { - if (allTargets.indexOf(targetId) !== -1) { - const collectionPath = docs[0].key.path.popLast(); - return targetData( - targetId, - TargetPurpose.Listen, - collectionPath.toString() - ); - } else { - return null; - } + const aggregator = new WatchChangeAggregator({ + getRemoteKeysForTarget: () => documentKeySet(), + getTargetDataForTarget: targetId => { + if (allTargets.indexOf(targetId) !== -1) { + const collectionPath = docs[0].key.path.popLast(); + return targetData( + targetId, + TargetPurpose.Listen, + collectionPath.toString() + ); + } else { + return null; } }, - TEST_DATABASE_ID - ); + getDatabaseId: () => TEST_DATABASE_ID + }); let version = SnapshotVersion.min(); @@ -524,19 +518,17 @@ export function docUpdateRemoteEvent( doc.key, doc ); - const aggregator = new WatchChangeAggregator( - { - getRemoteKeysForTarget: () => keys(doc), - getTargetDataForTarget: targetId => { - const purpose = - limboTargets && limboTargets.indexOf(targetId) !== -1 - ? TargetPurpose.LimboResolution - : TargetPurpose.Listen; - return targetData(targetId, purpose, doc.key.toString()); - } + const aggregator = new WatchChangeAggregator({ + getRemoteKeysForTarget: () => keys(doc), + getTargetDataForTarget: targetId => { + const purpose = + limboTargets && limboTargets.indexOf(targetId) !== -1 + ? TargetPurpose.LimboResolution + : TargetPurpose.Listen; + return targetData(targetId, purpose, doc.key.toString()); }, - TEST_DATABASE_ID - ); + getDatabaseId: () => TEST_DATABASE_ID + }); aggregator.handleDocumentChange(docChange); return aggregator.createRemoteEvent(doc.version); } From 7293289c342eda3365ec6017433321ca873d2732 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 7 Feb 2023 21:06:53 -0800 Subject: [PATCH 13/14] resolve comments --- packages/firestore/src/core/database_info.ts | 4 ---- packages/firestore/src/platform/base64.ts | 2 +- .../firestore/src/platform/browser/base64.ts | 2 +- packages/firestore/src/remote/watch_change.ts | 18 +++++++++++------- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/firestore/src/core/database_info.ts b/packages/firestore/src/core/database_info.ts index ca8bdb888e7..306a9920ea9 100644 --- a/packages/firestore/src/core/database_info.ts +++ b/packages/firestore/src/core/database_info.ts @@ -77,10 +77,6 @@ export class DatabaseId { other.database === this.database ); } - - canonicalString(): string { - return `projects/${this.projectId}/databases/${this.database}`; - } } export function databaseIdFromApp( diff --git a/packages/firestore/src/platform/base64.ts b/packages/firestore/src/platform/base64.ts index 3fb735ef299..3efd0339e77 100644 --- a/packages/firestore/src/platform/base64.ts +++ b/packages/firestore/src/platform/base64.ts @@ -26,7 +26,7 @@ export function decodeBase64(encoded: string): string { const decoded = platform.decodeBase64(encoded); // A quick sanity check as node and rn will not throw error if input is an - // invalid base64 string, ie, "A===". + // invalid base64 string, e.g., "A===". const expectedEncodedLength = 4 * Math.ceil(decoded.length / 3); if (encoded.length !== expectedEncodedLength) { throw new Base64DecodeError('Invalid base64 string'); diff --git a/packages/firestore/src/platform/browser/base64.ts b/packages/firestore/src/platform/browser/base64.ts index c13108d5062..4cbbfe6ced9 100644 --- a/packages/firestore/src/platform/browser/base64.ts +++ b/packages/firestore/src/platform/browser/base64.ts @@ -23,7 +23,7 @@ export function decodeBase64(encoded: string): string { return atob(encoded); } catch (e) { if (e instanceof DOMException) { - throw new Base64DecodeError('Invalid base64 string'); + throw new Base64DecodeError('Invalid base64 string: ' + e); } else { throw e; } diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 864924027cf..82ef17213dd 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -459,11 +459,15 @@ export class WatchChangeAggregator { normalizedBitmap = normalizeByteString(bitmap).toUint8Array(); } catch (err) { if (err instanceof Base64DecodeError) { - logWarn('Base64 string error: ', err); + logWarn( + 'Decoding the base64 bloom filter in existence filter failed (' + + err.message + + '); ignoring the bloom filter and falling back to full re-query.' + ); + return false; } else { - logWarn('Normalizing bloom filter bitmap failed: ', err); + throw err; } - return false; } let bloomFilter: BloomFilter; @@ -499,10 +503,10 @@ export class WatchChangeAggregator { let removalCount = 0; existingKeys.forEach(key => { - const documentPath = - this.metadataProvider.getDatabaseId().canonicalString() + - '/documents/' + - key.path.canonicalString(); + const databaseId = this.metadataProvider.getDatabaseId(); + const documentPath = `projects/${databaseId.projectId}/databases/${ + databaseId.database + }/documents/${key.path.canonicalString()}`; if (!bloomFilter.mightContain(documentPath)) { this.removeDocumentFromTarget(targetId, key, /*updatedDocument=*/ null); From 7f5cfe78dd4653ed778cb131e26e24be2e577277 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 8 Feb 2023 09:35:32 -0800 Subject: [PATCH 14/14] add error message --- packages/firestore/src/platform/rn/base64.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/src/platform/rn/base64.ts b/packages/firestore/src/platform/rn/base64.ts index 8d8733fbaab..f50c604ebf7 100644 --- a/packages/firestore/src/platform/rn/base64.ts +++ b/packages/firestore/src/platform/rn/base64.ts @@ -35,7 +35,7 @@ export function decodeBase64(encoded: string): string { ); } catch (e) { if (e instanceof DecodeBase64StringError) { - throw new Base64DecodeError('Invalid base64 string'); + throw new Base64DecodeError('Invalid base64 string: ' + e); } else { throw e; }