diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index 2f226f41ed8..ab848534bb9 100644 --- a/Firestore/CHANGELOG.md +++ b/Firestore/CHANGELOG.md @@ -1,6 +1,9 @@ # Unreleased - [changed] Improved performance when querying over documents that contain subcollections. +- [feature] Added `FieldValue.increment()`, which can be used in + `updateData(_:)` and `setData(_:merge:)` to increment or decrement numeric + field values safely without transactions. # v1.0.2 - [changed] Internal improvements. diff --git a/Firestore/Example/Firestore.xcodeproj/project.pbxproj b/Firestore/Example/Firestore.xcodeproj/project.pbxproj index 182898371ef..e87f93bbc4d 100644 --- a/Firestore/Example/Firestore.xcodeproj/project.pbxproj +++ b/Firestore/Example/Firestore.xcodeproj/project.pbxproj @@ -223,6 +223,7 @@ C80B10E79CDD7EF7843C321E /* type_traits_apple_test.mm in Sources */ = {isa = PBXBuildFile; fileRef = 2A0CF41BA5AED6049B0BEB2C /* type_traits_apple_test.mm */; }; C8D3CE2343E53223E6487F2C /* Pods_Firestore_Example_iOS.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 5918805E993304321A05E82B /* Pods_Firestore_Example_iOS.framework */; }; CA989C0E6020C372A62B7062 /* testutil.cc in Sources */ = {isa = PBXBuildFile; fileRef = 54A0352820A3B3BD003E0143 /* testutil.cc */; }; + D5B252EE3F4037405DB1ECE3 /* FIRNumericTransformTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = D5B25E7E7D6873CBA4571841 /* FIRNumericTransformTests.mm */; }; D5B25CBF07F65E885C9D68AB /* perf_spec_test.json in Resources */ = {isa = PBXBuildFile; fileRef = D5B2593BCB52957D62F1C9D3 /* perf_spec_test.json */; }; D94A1862B8FB778225DB54A1 /* filesystem_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = F51859B394D01C0C507282F1 /* filesystem_test.cc */; }; DAFF0CF921E64AC30062958F /* AppDelegate.m in Sources */ = {isa = PBXBuildFile; fileRef = DAFF0CF821E64AC30062958F /* AppDelegate.m */; }; @@ -552,6 +553,7 @@ C8522DE226C467C54E6788D8 /* mutation_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = mutation_test.cc; sourceTree = ""; }; D3CC3DC5338DCAF43A211155 /* README.md */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = net.daringfireball.markdown; name = README.md; path = ../README.md; sourceTree = ""; }; D5B2593BCB52957D62F1C9D3 /* perf_spec_test.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = perf_spec_test.json; sourceTree = ""; }; + D5B25E7E7D6873CBA4571841 /* FIRNumericTransformTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = FIRNumericTransformTests.mm; sourceTree = ""; }; DAFF0CF521E64AC30062958F /* macOS_example.app */ = {isa = PBXFileReference; explicitFileType = wrapper.application; includeInIndex = 0; path = macOS_example.app; sourceTree = BUILT_PRODUCTS_DIR; }; DAFF0CF721E64AC30062958F /* AppDelegate.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = AppDelegate.h; sourceTree = ""; }; DAFF0CF821E64AC30062958F /* AppDelegate.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = AppDelegate.m; sourceTree = ""; }; @@ -1310,6 +1312,7 @@ 5492E06A202154D500B64F25 /* FIRFieldsTests.mm */, 6161B5012047140400A99DBB /* FIRFirestoreSourceTests.mm */, 5492E06B202154D500B64F25 /* FIRListenerRegistrationTests.mm */, + D5B25E7E7D6873CBA4571841 /* FIRNumericTransformTests.mm */, 5492E069202154D500B64F25 /* FIRQueryTests.mm */, 5492E06E202154D600B64F25 /* FIRServerTimestampTests.mm */, 5492E071202154D600B64F25 /* FIRTypeTests.mm */, @@ -2145,6 +2148,7 @@ 5492E073202154D600B64F25 /* FIRFieldsTests.mm in Sources */, 6161B5032047140C00A99DBB /* FIRFirestoreSourceTests.mm in Sources */, 5492E074202154D600B64F25 /* FIRListenerRegistrationTests.mm in Sources */, + D5B252EE3F4037405DB1ECE3 /* FIRNumericTransformTests.mm in Sources */, 5492E072202154D600B64F25 /* FIRQueryTests.mm in Sources */, 5492E077202154D600B64F25 /* FIRServerTimestampTests.mm in Sources */, 5492E07A202154D600B64F25 /* FIRTypeTests.mm in Sources */, diff --git a/Firestore/Example/Tests/Integration/API/FIRNumericTransformTests.mm b/Firestore/Example/Tests/Integration/API/FIRNumericTransformTests.mm new file mode 100644 index 00000000000..8cd1fa1e54b --- /dev/null +++ b/Firestore/Example/Tests/Integration/API/FIRNumericTransformTests.mm @@ -0,0 +1,161 @@ +/* + * Copyright 2019 Google + * + * 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. + */ + +#import + +#import + +#import "Firestore/Source/API/FIRFieldValue+Internal.h" + +#import "Firestore/Example/Tests/Util/FSTEventAccumulator.h" +#import "Firestore/Example/Tests/Util/FSTIntegrationTestCase.h" + +double DOUBLE_EPSILON = 0.000001; + +@interface FIRNumericTransformTests : FSTIntegrationTestCase +@end + +@implementation FIRNumericTransformTests { + // A document reference to read and write to. + FIRDocumentReference *_docRef; + + // Accumulator used to capture events during the test. + FSTEventAccumulator *_accumulator; + + // Listener registration for a listener maintained during the course of the test. + id _listenerRegistration; +} + +- (void)setUp { + [super setUp]; + + _docRef = [self documentRef]; + _accumulator = [FSTEventAccumulator accumulatorForTest:self]; + _listenerRegistration = + [_docRef addSnapshotListenerWithIncludeMetadataChanges:YES + listener:_accumulator.valueEventHandler]; + + // Wait for initial nil snapshot to avoid potential races. + FIRDocumentSnapshot *initialSnapshot = [_accumulator awaitEventWithName:@"initial event"]; + XCTAssertFalse(initialSnapshot.exists); +} + +- (void)tearDown { + [_listenerRegistration remove]; + + [super tearDown]; +} + +#pragma mark - Test Helpers + +/** Writes some initial data and consumes the events generated. */ +- (void)writeInitialData:(NSDictionary *)data { + [self writeDocumentRef:_docRef data:data]; + XCTAssertEqualObjects([_accumulator awaitLocalEvent].data, data); + XCTAssertEqualObjects([_accumulator awaitRemoteEvent].data, data); +} + +- (void)expectLocalAndRemoteValue:(int64_t)expectedSum { + FIRDocumentSnapshot *snap = [_accumulator awaitLocalEvent]; + XCTAssertEqual(@(expectedSum), snap[@"sum"]); + snap = [_accumulator awaitRemoteEvent]; + XCTAssertEqual(@(expectedSum), snap[@"sum"]); +} + +- (void)expectApproximateLocalAndRemoteValue:(double)expectedSum { + FIRDocumentSnapshot *snap = [_accumulator awaitLocalEvent]; + XCTAssertEqualWithAccuracy(expectedSum, [snap[@"sum"] doubleValue], DOUBLE_EPSILON); + snap = [_accumulator awaitRemoteEvent]; + XCTAssertEqualWithAccuracy(expectedSum, [snap[@"sum"] doubleValue], DOUBLE_EPSILON); +} + +#pragma mark - Test Cases + +- (void)testCreateDocumentWithIncrement { + [self writeDocumentRef:_docRef + data:@{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1337]}]; + [self expectLocalAndRemoteValue:1337]; +} + +- (void)testMergeOnNonExistingDocumentWithIncrement { + [self mergeDocumentRef:_docRef + data:@{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1337]}]; + [self expectLocalAndRemoteValue:1337]; +} + +- (void)testIntegerIncrementWithExistingInteger { + [self writeInitialData:@{@"sum" : @1337}]; + [self updateDocumentRef:_docRef data:@{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1]}]; + [self expectLocalAndRemoteValue:1338]; +} + +- (void)testDoubleIncrementWithExistingDouble { + [self writeInitialData:@{@"sum" : @13.37}]; + [self updateDocumentRef:_docRef + data:@{@"sum" : [FIRFieldValue fieldValueForDoubleIncrement:0.1]}]; + [self expectApproximateLocalAndRemoteValue:13.47]; +} + +- (void)testIntegerIncrementWithExistingDouble { + [self writeInitialData:@{@"sum" : @13.37}]; + [self updateDocumentRef:_docRef data:@{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1]}]; + [self expectApproximateLocalAndRemoteValue:14.37]; +} + +- (void)testDoubleIncrementWithExistingInteger { + [self writeInitialData:@{@"sum" : @1337}]; + [self updateDocumentRef:_docRef + data:@{@"sum" : [FIRFieldValue fieldValueForDoubleIncrement:0.1]}]; + [self expectApproximateLocalAndRemoteValue:1337.1]; +} + +- (void)testIntegerIncrementWithExistingString { + [self writeInitialData:@{@"sum" : @"overwrite"}]; + [self updateDocumentRef:_docRef + data:@{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1337]}]; + [self expectLocalAndRemoteValue:1337]; +} + +- (void)testDoubleIncrementWithExistingString { + [self writeInitialData:@{@"sum" : @"overwrite"}]; + [self updateDocumentRef:_docRef + data:@{@"sum" : [FIRFieldValue fieldValueForDoubleIncrement:13.37]}]; + [self expectApproximateLocalAndRemoteValue:13.37]; +} + +- (void)testMultipleDoubleIncrements { + [self writeInitialData:@{@"sum" : @"0.0"}]; + + [self disableNetwork]; + + [_docRef updateData:@{@"sum" : [FIRFieldValue fieldValueForDoubleIncrement:0.1]}]; + [_docRef updateData:@{@"sum" : [FIRFieldValue fieldValueForDoubleIncrement:0.01]}]; + [_docRef updateData:@{@"sum" : [FIRFieldValue fieldValueForDoubleIncrement:0.001]}]; + + FIRDocumentSnapshot *snap = [_accumulator awaitLocalEvent]; + + XCTAssertEqualWithAccuracy(0.1, [snap[@"sum"] doubleValue], DOUBLE_EPSILON); + snap = [_accumulator awaitLocalEvent]; + XCTAssertEqualWithAccuracy(0.11, [snap[@"sum"] doubleValue], DOUBLE_EPSILON); + snap = [_accumulator awaitLocalEvent]; + XCTAssertEqualWithAccuracy(0.111, [snap[@"sum"] doubleValue], DOUBLE_EPSILON); + + [self enableNetwork]; + snap = [_accumulator awaitRemoteEvent]; + XCTAssertEqualWithAccuracy(0.111, [snap[@"sum"] doubleValue], DOUBLE_EPSILON); +} + +@end diff --git a/Firestore/Example/Tests/Integration/FSTDatastoreTests.mm b/Firestore/Example/Tests/Integration/FSTDatastoreTests.mm index aeb917f4580..e3d3199ef42 100644 --- a/Firestore/Example/Tests/Integration/FSTDatastoreTests.mm +++ b/Firestore/Example/Tests/Integration/FSTDatastoreTests.mm @@ -221,6 +221,7 @@ - (void)testStreamingWrite { FSTSetMutation *mutation = [self setMutation]; FSTMutationBatch *batch = [[FSTMutationBatch alloc] initWithBatchID:23 localWriteTime:[FIRTimestamp timestamp] + baseMutations:{} mutations:{mutation}]; _testWorkerQueue->Enqueue([=] { _remoteStore->AddToWritePipeline(batch); diff --git a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm index c691c675f9b..41d185d4dea 100644 --- a/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm +++ b/Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm @@ -450,7 +450,7 @@ - (void)testRemoveOrphanedDocuments { // serve to keep the mutated documents from being GC'd while the mutations are outstanding. _persistence.run("actually register the mutations", [&]() { FIRTimestamp *writeTime = [FIRTimestamp timestamp]; - _mutationQueue->AddMutationBatch(writeTime, std::move(mutations)); + _mutationQueue->AddMutationBatch(writeTime, {}, std::move(mutations)); }); // Mark 5 documents eligible for GC. This simulates documents that were mutated then ack'd. diff --git a/Firestore/Example/Tests/Local/FSTLocalSerializerTests.mm b/Firestore/Example/Tests/Local/FSTLocalSerializerTests.mm index 32a0a55a6f9..a432f001619 100644 --- a/Firestore/Example/Tests/Local/FSTLocalSerializerTests.mm +++ b/Firestore/Example/Tests/Local/FSTLocalSerializerTests.mm @@ -82,6 +82,10 @@ - (void)setUp { } - (void)testEncodesMutationBatch { + FSTMutation *base = [[FSTPatchMutation alloc] initWithKey:FSTTestDocKey(@"bar/baz") + fieldMask:FieldMask{testutil::Field("a")} + value:FSTTestObjectValue(@{@"a" : @"b"}) + precondition:Precondition::Exists(true)]; FSTMutation *set = FSTTestSetMutation(@"foo/bar", @{@"a" : @"b", @"num" : @1}); FSTMutation *patch = [[FSTPatchMutation alloc] initWithKey:FSTTestDocKey(@"bar/baz") @@ -92,8 +96,17 @@ - (void)testEncodesMutationBatch { FIRTimestamp *writeTime = [FIRTimestamp timestamp]; FSTMutationBatch *model = [[FSTMutationBatch alloc] initWithBatchID:42 localWriteTime:writeTime + baseMutations:{base} mutations:{set, patch, del}]; + GCFSWrite *baseProto = [GCFSWrite message]; + baseProto.update.name = @"projects/p/databases/d/documents/bar/baz"; + [baseProto.update.fields addEntriesFromDictionary:@{ + @"a" : [self.remoteSerializer encodedString:@"b"], + }]; + [baseProto.updateMask.fieldPathsArray addObjectsFromArray:@[ @"a" ]]; + baseProto.currentDocument.exists = YES; + GCFSWrite *setProto = [GCFSWrite message]; setProto.update.name = @"projects/p/databases/d/documents/foo/bar"; [setProto.update.fields addEntriesFromDictionary:@{ @@ -119,6 +132,7 @@ - (void)testEncodesMutationBatch { FSTPBWriteBatch *batchProto = [FSTPBWriteBatch message]; batchProto.batchId = 42; + [batchProto.baseWritesArray addObject:baseProto]; [batchProto.writesArray addObjectsFromArray:@[ setProto, patchProto, delProto ]]; batchProto.localWriteTime = writeTimeProto; @@ -126,6 +140,7 @@ - (void)testEncodesMutationBatch { FSTMutationBatch *decoded = [self.serializer decodedMutationBatch:batchProto]; XCTAssertEqual(decoded.batchID, model.batchID); XCTAssertEqualObjects(decoded.localWriteTime, model.localWriteTime); + FSTAssertEqualVectors(decoded.baseMutations, model.baseMutations); FSTAssertEqualVectors(decoded.mutations, model.mutations); XCTAssertEqual([decoded keys], [model keys]); } diff --git a/Firestore/Example/Tests/Local/FSTLocalStoreTests.mm b/Firestore/Example/Tests/Local/FSTLocalStoreTests.mm index da8a8b0b4c9..15d30c7d6eb 100644 --- a/Firestore/Example/Tests/Local/FSTLocalStoreTests.mm +++ b/Firestore/Example/Tests/Local/FSTLocalStoreTests.mm @@ -22,6 +22,7 @@ #include #include +#import "Firestore/Source/API/FIRFieldValue+Internal.h" #import "Firestore/Source/Core/FSTQuery.h" #import "Firestore/Source/Local/FSTLocalWriteResult.h" #import "Firestore/Source/Local/FSTPersistence.h" @@ -134,6 +135,7 @@ - (void)writeMutations:(std::vector &&)mutations { XCTAssertNotNil(result); [self.batches addObject:[[FSTMutationBatch alloc] initWithBatchID:result.batchID localWriteTime:[FIRTimestamp timestamp] + baseMutations:{} mutations:std::move(mutations)]]; _lastChanges = result.changes; } @@ -146,13 +148,15 @@ - (void)notifyLocalViewChanges:(FSTLocalViewChanges *)changes { [self.localStore notifyLocalViewChanges:@[ changes ]]; } -- (void)acknowledgeMutationWithVersion:(FSTTestSnapshotVersion)documentVersion { +- (void)acknowledgeMutationWithVersion:(FSTTestSnapshotVersion)documentVersion + transformResult:(id _Nullable)transformResult { FSTMutationBatch *batch = [self.batches firstObject]; [self.batches removeObjectAtIndex:0]; XCTAssertEqual(batch.mutations.size(), 1, @"Acknowledging more than one mutation not supported."); SnapshotVersion version = testutil::Version(documentVersion); - FSTMutationResult *mutationResult = [[FSTMutationResult alloc] initWithVersion:version - transformResults:nil]; + FSTMutationResult *mutationResult = [[FSTMutationResult alloc] + initWithVersion:version + transformResults:transformResult != nil ? @[ FSTTestFieldValue(transformResult) ] : nil]; FSTMutationBatchResult *result = [FSTMutationBatchResult resultWithBatch:batch commitVersion:version mutationResults:{mutationResult} @@ -160,6 +164,10 @@ - (void)acknowledgeMutationWithVersion:(FSTTestSnapshotVersion)documentVersion { _lastChanges = [self.localStore acknowledgeBatchWithResult:result]; } +- (void)acknowledgeMutationWithVersion:(FSTTestSnapshotVersion)documentVersion { + [self acknowledgeMutationWithVersion:documentVersion transformResult:nil]; +} + - (void)rejectMutation { FSTMutationBatch *batch = [self.batches firstObject]; [self.batches removeObjectAtIndex:0]; @@ -225,10 +233,12 @@ - (TargetId)allocateQuery:(FSTQuery *)query { - (void)testMutationBatchKeys { if ([self isTestBaseClass]) return; + FSTMutation *base = FSTTestSetMutation(@"foo/ignore", @{@"foo" : @"bar"}); FSTMutation *set1 = FSTTestSetMutation(@"foo/bar", @{@"foo" : @"bar"}); FSTMutation *set2 = FSTTestSetMutation(@"bar/baz", @{@"bar" : @"baz"}); FSTMutationBatch *batch = [[FSTMutationBatch alloc] initWithBatchID:1 localWriteTime:[FIRTimestamp timestamp] + baseMutations:{base} mutations:{set1, set2}]; DocumentKeySet keys = [batch keys]; XCTAssertEqual(keys.size(), 2u); @@ -954,6 +964,202 @@ - (void)testRemoteDocumentKeysForTarget { XCTAssertEqual(keys, (DocumentKeySet{testutil::Key("foo/bar"), testutil::Key("foo/baz")})); } +// TODO(mrschmidt): The FieldValue.increment() field transform tests below would probably be +// better implemented as spec tests but currently they don't support transforms. + +- (void)testHandlesSetMutationThenTransformMutationThenTransformMutation { + if ([self isTestBaseClass]) return; + + [self writeMutation:FSTTestSetMutation(@"foo/bar", @{@"sum" : @0})]; + FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"sum" : @0}, FSTDocumentStateLocalMutations)); + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 0, @{@"sum" : @0}, FSTDocumentStateLocalMutations) ]); + + [self writeMutation:FSTTestTransformMutation( + @"foo/bar", @{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1]})]; + FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"sum" : @1}, FSTDocumentStateLocalMutations)); + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 0, @{@"sum" : @1}, FSTDocumentStateLocalMutations) ]); + + [self writeMutation:FSTTestTransformMutation( + @"foo/bar", @{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:2]})]; + FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"sum" : @3}, FSTDocumentStateLocalMutations)); + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 0, @{@"sum" : @3}, FSTDocumentStateLocalMutations) ]); +} + +- (void)testHandlesSetMutationThenAckThenTransformMutationThenAckThenTransformMutation { + if ([self isTestBaseClass]) return; + + // Since this test doesn't start a listen, Eager GC removes the documents from the cache as + // soon as the mutation is applied. This creates a lot of special casing in this unit test but + // does not expand its test coverage. + if ([self gcIsEager]) return; + + [self writeMutation:FSTTestSetMutation(@"foo/bar", @{@"sum" : @0})]; + FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"sum" : @0}, FSTDocumentStateLocalMutations)); + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 0, @{@"sum" : @0}, FSTDocumentStateLocalMutations) ]); + + [self acknowledgeMutationWithVersion:1]; + FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"sum" : @0}, FSTDocumentStateCommittedMutations)); + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 1, @{@"sum" : @0}, FSTDocumentStateCommittedMutations) ]); + + [self writeMutation:FSTTestTransformMutation( + @"foo/bar", @{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1]})]; + FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"sum" : @1}, FSTDocumentStateLocalMutations)); + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"sum" : @1}, FSTDocumentStateLocalMutations) ]); + + [self acknowledgeMutationWithVersion:2 transformResult:@1]; + FSTAssertContains(FSTTestDoc("foo/bar", 2, @{@"sum" : @1}, FSTDocumentStateCommittedMutations)); + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 2, @{@"sum" : @1}, FSTDocumentStateCommittedMutations) ]); + + [self writeMutation:FSTTestTransformMutation( + @"foo/bar", @{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:2]})]; + FSTAssertContains(FSTTestDoc("foo/bar", 2, @{@"sum" : @3}, FSTDocumentStateLocalMutations)); + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 2, @{@"sum" : @3}, FSTDocumentStateLocalMutations) ]); +} + +- (void)testHandlesSetMutationThenTransformMutationThenRemoteEventThenTransformMutation { + if ([self isTestBaseClass]) return; + + FSTQuery *query = FSTTestQuery("foo"); + [self allocateQuery:query]; + FSTAssertTargetID(2); + + [self writeMutation:FSTTestSetMutation(@"foo/bar", @{@"sum" : @0})]; + FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"sum" : @0}, FSTDocumentStateLocalMutations)); + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 0, @{@"sum" : @0}, FSTDocumentStateLocalMutations) ]); + + [self + applyRemoteEvent:FSTTestAddedRemoteEvent( + FSTTestDoc("foo/bar", 1, @{@"sum" : @0}, FSTDocumentStateSynced), {2})]; + + [self acknowledgeMutationWithVersion:1]; + FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"sum" : @0}, FSTDocumentStateSynced)); + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"sum" : @0}, FSTDocumentStateSynced) ]); + + [self writeMutation:FSTTestTransformMutation( + @"foo/bar", @{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1]})]; + FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"sum" : @1}, FSTDocumentStateLocalMutations)); + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"sum" : @1}, FSTDocumentStateLocalMutations) ]); + + // The value in this remote event gets ignored since we still have a pending transform mutation. + [self applyRemoteEvent:FSTTestUpdateRemoteEvent( + FSTTestDoc("foo/bar", 2, @{@"sum" : @0}, FSTDocumentStateSynced), {2}, + {})]; + FSTAssertContains(FSTTestDoc("foo/bar", 2, @{@"sum" : @1}, FSTDocumentStateLocalMutations)); + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 2, @{@"sum" : @1}, FSTDocumentStateLocalMutations) ]); + + // Add another increment. Note that we still compute the increment based on the local value. + [self writeMutation:FSTTestTransformMutation( + @"foo/bar", @{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:2]})]; + FSTAssertContains(FSTTestDoc("foo/bar", 2, @{@"sum" : @3}, FSTDocumentStateLocalMutations)); + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 2, @{@"sum" : @3}, FSTDocumentStateLocalMutations) ]); + + [self acknowledgeMutationWithVersion:3 transformResult:@1]; + FSTAssertContains(FSTTestDoc("foo/bar", 3, @{@"sum" : @3}, FSTDocumentStateLocalMutations)); + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 3, @{@"sum" : @3}, FSTDocumentStateLocalMutations) ]); + + [self acknowledgeMutationWithVersion:4 transformResult:@1339]; + FSTAssertContains( + FSTTestDoc("foo/bar", 4, @{@"sum" : @1339}, FSTDocumentStateCommittedMutations)); + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 4, @{@"sum" : @1339}, FSTDocumentStateCommittedMutations) ]); +} + +- (void)testHoldsBackOnlyNonIdempotentTransforms { + if ([self isTestBaseClass]) return; + + FSTQuery *query = FSTTestQuery("foo"); + [self allocateQuery:query]; + FSTAssertTargetID(2); + + [self writeMutation:FSTTestSetMutation(@"foo/bar", @{@"sum" : @0, @"array_union" : @[]})]; + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 0, @{@"sum" : @0, @"array_union" : @[]}, + FSTDocumentStateLocalMutations) ]); + + [self acknowledgeMutationWithVersion:1]; + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"sum" : @0, @"array_union" : @[]}, + FSTDocumentStateCommittedMutations) ]); + + [self applyRemoteEvent:FSTTestAddedRemoteEvent( + FSTTestDoc("foo/bar", 1, @{@"sum" : @0, @"array_union" : @[]}, + FSTDocumentStateSynced), + {2})]; + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 1, @{@"sum" : @0, @"array_union" : @[]}, FSTDocumentStateSynced) ]); + + [self writeMutations:{ + FSTTestTransformMutation(@"foo/bar", + @{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1]}), + FSTTestTransformMutation( + @"foo/bar", + @{@"array_union" : [FIRFieldValue fieldValueForArrayUnion:@[ @"foo" ]]}) + }]; + + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"sum" : @1, @"array_union" : @[ @"foo" ]}, + FSTDocumentStateLocalMutations) ]); + + // The sum transform is not idempotent and the backend's updated value is ignored. The + // ArrayUnion transform is recomputed and includes the backend value. + [self + applyRemoteEvent:FSTTestUpdateRemoteEvent( + FSTTestDoc("foo/bar", 1, @{@"sum" : @1337, @"array_union" : @[ @"bar" ]}, + FSTDocumentStateSynced), + {2}, {})]; + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"sum" : @1, @"array_union" : @[ @"bar", @"foo" ]}, + FSTDocumentStateLocalMutations) ]); +} + +- (void)testHandlesMergeMutationWithTransformThenRemoteEvent { + if ([self isTestBaseClass]) return; + + FSTQuery *query = FSTTestQuery("foo"); + [self allocateQuery:query]; + FSTAssertTargetID(2); + + [self writeMutations:{ + FSTTestPatchMutation("foo/bar", @{}, {firebase::firestore::testutil::Field("sum")}), + FSTTestTransformMutation(@"foo/bar", + @{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1]}) + }]; + + FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"sum" : @1}, FSTDocumentStateLocalMutations)); + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 0, @{@"sum" : @1}, FSTDocumentStateLocalMutations) ]); + + [self applyRemoteEvent:FSTTestAddedRemoteEvent( + FSTTestDoc("foo/bar", 1, @{@"sum" : @1337}, FSTDocumentStateSynced), + {2})]; + + FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"sum" : @1}, FSTDocumentStateLocalMutations)); + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"sum" : @1}, FSTDocumentStateLocalMutations) ]); +} + +- (void)testHandlesPatchMutationWithTransformThenRemoteEvent { + if ([self isTestBaseClass]) return; + + FSTQuery *query = FSTTestQuery("foo"); + [self allocateQuery:query]; + FSTAssertTargetID(2); + + [self writeMutations:{ + FSTTestPatchMutation("foo/bar", @{}, {}), + FSTTestTransformMutation(@"foo/bar", + @{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1]}) + }]; + + FSTAssertNotContains(@"foo/bar"); + FSTAssertChanged(@[ FSTTestDeletedDoc("foo/bar", 0, NO) ]); + + // Note: This test reflects the current behavior, but it may be preferable to replay the + // mutation once we receive the first value from the remote event. + [self applyRemoteEvent:FSTTestAddedRemoteEvent( + FSTTestDoc("foo/bar", 1, @{@"sum" : @1337}, FSTDocumentStateSynced), + {2})]; + + FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"sum" : @1}, FSTDocumentStateLocalMutations)); + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"sum" : @1}, FSTDocumentStateLocalMutations) ]); +} + @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Example/Tests/Local/FSTMutationQueueTests.mm b/Firestore/Example/Tests/Local/FSTMutationQueueTests.mm index c3fcc6f6131..d0346c2a641 100644 --- a/Firestore/Example/Tests/Local/FSTMutationQueueTests.mm +++ b/Firestore/Example/Tests/Local/FSTMutationQueueTests.mm @@ -201,7 +201,7 @@ - (void)testAllMutationBatchesAffectingDocumentKey { NSMutableArray *batches = [NSMutableArray array]; for (FSTMutation *mutation in mutations) { FSTMutationBatch *batch = - self.mutationQueue->AddMutationBatch([FIRTimestamp timestamp], { mutation }); + self.mutationQueue->AddMutationBatch([FIRTimestamp timestamp], {}, {mutation}); [batches addObject:batch]; } @@ -228,7 +228,7 @@ - (void)testAllMutationBatchesAffectingDocumentKeys { NSMutableArray *batches = [NSMutableArray array]; for (FSTMutation *mutation in mutations) { FSTMutationBatch *batch = - self.mutationQueue->AddMutationBatch([FIRTimestamp timestamp], { mutation }); + self.mutationQueue->AddMutationBatch([FIRTimestamp timestamp], {}, {mutation}); [batches addObject:batch]; } @@ -254,16 +254,16 @@ - (void)testAllMutationBatchesAffectingDocumentKeys_handlesOverlap { FSTTestSetMutation(@"foo/baz", @{@"a" : @1}), }; FSTMutationBatch *batch1 = - self.mutationQueue->AddMutationBatch([FIRTimestamp timestamp], std::move(group1)); + self.mutationQueue->AddMutationBatch([FIRTimestamp timestamp], {}, std::move(group1)); std::vector group2 = {FSTTestSetMutation(@"food/bar", @{@"a" : @1})}; - self.mutationQueue->AddMutationBatch([FIRTimestamp timestamp], std::move(group2)); + self.mutationQueue->AddMutationBatch([FIRTimestamp timestamp], {}, std::move(group2)); std::vector group3 = { FSTTestSetMutation(@"foo/bar", @{@"b" : @1}), }; FSTMutationBatch *batch3 = - self.mutationQueue->AddMutationBatch([FIRTimestamp timestamp], std::move(group3)); + self.mutationQueue->AddMutationBatch([FIRTimestamp timestamp], {}, std::move(group3)); DocumentKeySet keys{ Key("foo/bar"), @@ -293,7 +293,7 @@ - (void)testAllMutationBatchesAffectingQuery { NSMutableArray *batches = [NSMutableArray array]; for (FSTMutation *mutation in mutations) { FSTMutationBatch *batch = - self.mutationQueue->AddMutationBatch([FIRTimestamp timestamp], { mutation }); + self.mutationQueue->AddMutationBatch([FIRTimestamp timestamp], {}, {mutation}); [batches addObject:batch]; } @@ -397,7 +397,7 @@ - (FSTMutationBatch *)addMutationBatchWithKey:(NSString *)key { FSTSetMutation *mutation = FSTTestSetMutation(key, @{@"a" : @1}); FSTMutationBatch *batch = - self.mutationQueue->AddMutationBatch([FIRTimestamp timestamp], { mutation }); + self.mutationQueue->AddMutationBatch([FIRTimestamp timestamp], {}, {mutation}); return batch; } diff --git a/Firestore/Example/Tests/Model/FSTMutationTests.mm b/Firestore/Example/Tests/Model/FSTMutationTests.mm index 3ee31a5ed78..6abf37d7336 100644 --- a/Firestore/Example/Tests/Model/FSTMutationTests.mm +++ b/Firestore/Example/Tests/Model/FSTMutationTests.mm @@ -151,6 +151,93 @@ - (void)testAppliesLocalServerTimestampTransformToDocuments { XCTAssertEqualObjects(transformedDoc, expectedDoc); } +- (void)testAppliesIncrementTransformToDocument { + NSDictionary *baseDoc = @{ + @"longPlusLong" : @1, + @"longPlusDouble" : @2, + @"doublePlusLong" : @3.3, + @"doublePlusDouble" : @4.0, + @"longPlusNan" : @5, + @"doublePlusNan" : @6.6, + @"longPlusInfinity" : @7, + @"doublePlusInfinity" : @8.8 + }; + NSDictionary *transform = @{ + @"longPlusLong" : [FIRFieldValue fieldValueForIntegerIncrement:1], + @"longPlusDouble" : [FIRFieldValue fieldValueForDoubleIncrement:2.2], + @"doublePlusLong" : [FIRFieldValue fieldValueForIntegerIncrement:3], + @"doublePlusDouble" : [FIRFieldValue fieldValueForDoubleIncrement:4.4], + @"longPlusNan" : [FIRFieldValue fieldValueForDoubleIncrement:NAN], + @"doublePlusNan" : [FIRFieldValue fieldValueForDoubleIncrement:NAN], + @"longPlusInfinity" : [FIRFieldValue fieldValueForDoubleIncrement:INFINITY], + @"doublePlusInfinity" : [FIRFieldValue fieldValueForDoubleIncrement:INFINITY] + }; + NSDictionary *expected = @{ + @"longPlusLong" : @2L, + @"longPlusDouble" : @4.2, + @"doublePlusLong" : @6.3, + @"doublePlusDouble" : @8.4, + @"longPlusNan" : @(NAN), + @"doublePlusNan" : @(NAN), + @"longPlusInfinity" : @(INFINITY), + @"doublePlusInfinity" : @(INFINITY) + }; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; +} + +- (void)testAppliesIncrementTransformToUnexpectedType { + NSDictionary *baseDoc = @{@"string" : @"zero"}; + NSDictionary *transform = @{@"string" : [FIRFieldValue fieldValueForIntegerIncrement:1]}; + NSDictionary *expected = @{@"string" : @1}; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; +} + +- (void)testAppliesIncrementTransformToMissingField { + NSDictionary *baseDoc = @{}; + NSDictionary *transform = @{@"missing" : [FIRFieldValue fieldValueForIntegerIncrement:1]}; + NSDictionary *expected = @{@"missing" : @1}; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; +} + +- (void)testAppliesIncrementTransformsConsecutively { + NSDictionary *baseDoc = @{@"number" : @1}; + NSDictionary *transform1 = @{@"number" : [FIRFieldValue fieldValueForIntegerIncrement:2]}; + NSDictionary *transform2 = @{@"number" : [FIRFieldValue fieldValueForIntegerIncrement:3]}; + NSDictionary *transform3 = @{@"number" : [FIRFieldValue fieldValueForIntegerIncrement:4]}; + NSDictionary *expected = @{@"number" : @10}; + [self transformBaseDoc:baseDoc + applyTransforms:@[ transform1, transform2, transform3 ] + expecting:expected]; +} + +- (void)testAppliesIncrementWithoutOverflow { + NSDictionary *baseDoc = + @{@"a" : @(LONG_MAX - 1), @"b" : @(LONG_MAX - 1), @"c" : @(LONG_MAX), @"d" : @(LONG_MAX)}; + NSDictionary *transform = @{ + @"a" : [FIRFieldValue fieldValueForIntegerIncrement:1], + @"b" : [FIRFieldValue fieldValueForIntegerIncrement:LONG_MAX], + @"c" : [FIRFieldValue fieldValueForIntegerIncrement:1], + @"d" : [FIRFieldValue fieldValueForIntegerIncrement:LONG_MAX] + }; + NSDictionary *expected = + @{@"a" : @LONG_MAX, @"b" : @LONG_MAX, @"c" : @LONG_MAX, @"d" : @LONG_MAX}; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; +} + +- (void)testAppliesIncrementWithoutUnderflow { + NSDictionary *baseDoc = + @{@"a" : @(LONG_MIN + 1), @"b" : @(LONG_MIN + 1), @"c" : @(LONG_MIN), @"d" : @(LONG_MIN)}; + NSDictionary *transform = @{ + @"a" : [FIRFieldValue fieldValueForIntegerIncrement:-1], + @"b" : [FIRFieldValue fieldValueForIntegerIncrement:LONG_MIN], + @"c" : [FIRFieldValue fieldValueForIntegerIncrement:-1], + @"d" : [FIRFieldValue fieldValueForIntegerIncrement:LONG_MIN] + }; + NSDictionary *expected = + @{@"a" : @(LONG_MIN), @"b" : @(LONG_MIN), @"c" : @(LONG_MIN), @"d" : @(LONG_MIN)}; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; +} + // NOTE: This is more a test of FSTUserDataConverter code than FSTMutation code but we don't have // unit tests for it currently. We could consider removing this test once we have integration tests. - (void)testCreateArrayUnionTransform { @@ -201,28 +288,28 @@ - (void)testAppliesLocalArrayUnionTransformToMissingField { auto baseDoc = @{}; auto transform = @{@"missing" : [FIRFieldValue fieldValueForArrayUnion:@[ @1, @2 ]]}; auto expected = @{@"missing" : @[ @1, @2 ]}; - [self transformBaseDoc:baseDoc with:transform expecting:expected]; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; } - (void)testAppliesLocalArrayUnionTransformToNonArrayField { auto baseDoc = @{@"non-array" : @42}; auto transform = @{@"non-array" : [FIRFieldValue fieldValueForArrayUnion:@[ @1, @2 ]]}; auto expected = @{@"non-array" : @[ @1, @2 ]}; - [self transformBaseDoc:baseDoc with:transform expecting:expected]; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; } - (void)testAppliesLocalArrayUnionTransformWithNonExistingElements { auto baseDoc = @{@"array" : @[ @1, @3 ]}; auto transform = @{@"array" : [FIRFieldValue fieldValueForArrayUnion:@[ @2, @4 ]]}; auto expected = @{@"array" : @[ @1, @3, @2, @4 ]}; - [self transformBaseDoc:baseDoc with:transform expecting:expected]; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; } - (void)testAppliesLocalArrayUnionTransformWithExistingElements { auto baseDoc = @{@"array" : @[ @1, @3 ]}; auto transform = @{@"array" : [FIRFieldValue fieldValueForArrayUnion:@[ @1, @3 ]]}; auto expected = @{@"array" : @[ @1, @3 ]}; - [self transformBaseDoc:baseDoc with:transform expecting:expected]; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; } - (void)testAppliesLocalArrayUnionTransformWithDuplicateExistingElements { @@ -230,7 +317,7 @@ - (void)testAppliesLocalArrayUnionTransformWithDuplicateExistingElements { auto baseDoc = @{@"array" : @[ @1, @2, @2, @3 ]}; auto transform = @{@"array" : [FIRFieldValue fieldValueForArrayUnion:@[ @2 ]]}; auto expected = @{@"array" : @[ @1, @2, @2, @3 ]}; - [self transformBaseDoc:baseDoc with:transform expecting:expected]; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; } - (void)testAppliesLocalArrayUnionTransformWithDuplicateUnionElements { @@ -238,7 +325,7 @@ - (void)testAppliesLocalArrayUnionTransformWithDuplicateUnionElements { auto baseDoc = @{@"array" : @[ @1, @3 ]}; auto transform = @{@"array" : [FIRFieldValue fieldValueForArrayUnion:@[ @2, @2 ]]}; auto expected = @{@"array" : @[ @1, @3, @2 ]}; - [self transformBaseDoc:baseDoc with:transform expecting:expected]; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; } - (void)testAppliesLocalArrayUnionTransformWithNonPrimitiveElements { @@ -247,7 +334,7 @@ - (void)testAppliesLocalArrayUnionTransformWithNonPrimitiveElements { auto transform = @{@"array" : [FIRFieldValue fieldValueForArrayUnion:@[ @{@"a" : @"b"}, @{@"c" : @"d"} ]]}; auto expected = @{@"array" : @[ @1, @{@"a" : @"b"}, @{@"c" : @"d"} ]}; - [self transformBaseDoc:baseDoc with:transform expecting:expected]; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; } - (void)testAppliesLocalArrayUnionTransformWithPartiallyOverlappingElements { @@ -257,35 +344,35 @@ - (void)testAppliesLocalArrayUnionTransformWithPartiallyOverlappingElements { @{@"array" : [FIRFieldValue fieldValueForArrayUnion:@[ @{@"a" : @"b"}, @{@"c" : @"d"} ]]}; auto expected = @{@"array" : @[ @1, @{@"a" : @"b", @"c" : @"d"}, @{@"a" : @"b"}, @{@"c" : @"d"} ]}; - [self transformBaseDoc:baseDoc with:transform expecting:expected]; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; } - (void)testAppliesLocalArrayRemoveTransformToMissingField { auto baseDoc = @{}; auto transform = @{@"missing" : [FIRFieldValue fieldValueForArrayRemove:@[ @1, @2 ]]}; auto expected = @{@"missing" : @[]}; - [self transformBaseDoc:baseDoc with:transform expecting:expected]; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; } - (void)testAppliesLocalArrayRemoveTransformToNonArrayField { auto baseDoc = @{@"non-array" : @42}; auto transform = @{@"non-array" : [FIRFieldValue fieldValueForArrayRemove:@[ @1, @2 ]]}; auto expected = @{@"non-array" : @[]}; - [self transformBaseDoc:baseDoc with:transform expecting:expected]; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; } - (void)testAppliesLocalArrayRemoveTransformWithNonExistingElements { auto baseDoc = @{@"array" : @[ @1, @3 ]}; auto transform = @{@"array" : [FIRFieldValue fieldValueForArrayRemove:@[ @2, @4 ]]}; auto expected = @{@"array" : @[ @1, @3 ]}; - [self transformBaseDoc:baseDoc with:transform expecting:expected]; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; } - (void)testAppliesLocalArrayRemoveTransformWithExistingElements { auto baseDoc = @{@"array" : @[ @1, @2, @3, @4 ]}; auto transform = @{@"array" : [FIRFieldValue fieldValueForArrayRemove:@[ @1, @3 ]]}; auto expected = @{@"array" : @[ @2, @4 ]}; - [self transformBaseDoc:baseDoc with:transform expecting:expected]; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; } - (void)testAppliesLocalArrayRemoveTransformWithNonPrimitiveElements { @@ -294,27 +381,53 @@ - (void)testAppliesLocalArrayRemoveTransformWithNonPrimitiveElements { auto transform = @{@"array" : [FIRFieldValue fieldValueForArrayRemove:@[ @{@"a" : @"b"}, @{@"c" : @"d"} ]]}; auto expected = @{@"array" : @[ @1 ]}; - [self transformBaseDoc:baseDoc with:transform expecting:expected]; + [self transformBaseDoc:baseDoc applyTransform:transform expecting:expected]; } // Helper to test a particular transform scenario. - (void)transformBaseDoc:(NSDictionary *)baseData - with:(NSDictionary *)transformData + applyTransforms:(NSArray *> *)transforms expecting:(NSDictionary *)expectedData { - FSTDocument *baseDoc = FSTTestDoc("collection/key", 0, baseData, FSTDocumentStateSynced); + FSTMaybeDocument *currentDoc = FSTTestDoc("collection/key", 0, baseData, FSTDocumentStateSynced); - FSTMutation *transform = FSTTestTransformMutation(@"collection/key", transformData); - - FSTMaybeDocument *transformedDoc = [transform applyToLocalDocument:baseDoc - baseDocument:baseDoc - localWriteTime:_timestamp]; + for (NSDictionary *transformData in transforms) { + FSTMutation *transform = FSTTestTransformMutation(@"collection/key", transformData); + currentDoc = [transform applyToLocalDocument:currentDoc + baseDocument:currentDoc + localWriteTime:_timestamp]; + } FSTDocument *expectedDoc = [FSTDocument documentWithData:FSTTestObjectValue(expectedData) key:FSTTestDocKey(@"collection/key") version:testutil::Version(0) state:FSTDocumentStateLocalMutations]; - XCTAssertEqualObjects(transformedDoc, expectedDoc); + XCTAssertEqualObjects(currentDoc, expectedDoc); +} + +- (void)transformBaseDoc:(NSDictionary *)baseData + applyTransform:(NSDictionary *)transformData + expecting:(NSDictionary *)expectedData { + [self transformBaseDoc:baseData applyTransforms:@[ transformData ] expecting:expectedData]; +} + +- (void)testAppliesServerAckedIncrementTransformToDocuments { + NSDictionary *docData = @{@"sum" : @1}; + FSTDocument *baseDoc = FSTTestDoc("collection/key", 0, docData, FSTDocumentStateSynced); + + FSTMutation *transform = FSTTestTransformMutation( + @"collection/key", @{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:2]}); + + FSTMutationResult *mutationResult = + [[FSTMutationResult alloc] initWithVersion:testutil::Version(1) + transformResults:@[ [FSTIntegerValue integerValue:3] ]]; + + FSTMaybeDocument *transformedDoc = [transform applyToRemoteDocument:baseDoc + mutationResult:mutationResult]; + + NSDictionary *expectedData = @{@"sum" : @3}; + XCTAssertEqualObjects(transformedDoc, FSTTestDoc("collection/key", 1, expectedData, + FSTDocumentStateCommittedMutations)); } - (void)testAppliesServerAckedServerTimestampTransformToDocuments { diff --git a/Firestore/Example/Tests/Model/transform_operations_test.mm b/Firestore/Example/Tests/Model/transform_operations_test.mm index 247ea137121..18614136620 100644 --- a/Firestore/Example/Tests/Model/transform_operations_test.mm +++ b/Firestore/Example/Tests/Model/transform_operations_test.mm @@ -41,6 +41,10 @@ Type type() const override { return nil; } + bool idempotent() const override { + return true; + } + bool operator==(const TransformOperation& other) const override { return this == &other; } diff --git a/Firestore/Example/Tests/Remote/FSTSerializerBetaTests.mm b/Firestore/Example/Tests/Remote/FSTSerializerBetaTests.mm index aa9a3cc997a..c3215086081 100644 --- a/Firestore/Example/Tests/Remote/FSTSerializerBetaTests.mm +++ b/Firestore/Example/Tests/Remote/FSTSerializerBetaTests.mm @@ -386,7 +386,7 @@ - (void)testEncodesPatchMutation { "docs/1", @{@"a" : @"b", @"num" : @1, @"some.de\\\\ep.th\\ing'" : @2}, {}); GCFSWrite *proto = [GCFSWrite message]; proto.update = [self.serializer encodedDocumentWithFields:mutation.value key:mutation.key]; - proto.updateMask = [self.serializer encodedFieldMask:mutation.fieldMask]; + proto.updateMask = [self.serializer encodedFieldMask:*(mutation.fieldMask)]; proto.currentDocument.exists = YES; [self assertRoundTripForMutation:mutation proto:proto]; diff --git a/Firestore/Example/Tests/Util/FSTHelpers.mm b/Firestore/Example/Tests/Util/FSTHelpers.mm index 7f09be44bac..12facfbfcc2 100644 --- a/Firestore/Example/Tests/Util/FSTHelpers.mm +++ b/Firestore/Example/Tests/Util/FSTHelpers.mm @@ -269,10 +269,11 @@ NSComparator FSTTestDocComparator(const absl::string_view fieldPath) { DocumentKey key = testutil::Key(path); FieldMask mask(merge ? std::set(updateMask.begin(), updateMask.end()) : fieldMaskPaths); - return [[FSTPatchMutation alloc] initWithKey:key - fieldMask:mask - value:objectValue - precondition:Precondition::Exists(true)]; + return [[FSTPatchMutation alloc] + initWithKey:key + fieldMask:mask + value:objectValue + precondition:merge ? Precondition::None() : Precondition::Exists(true)]; } FSTTransformMutation *FSTTestTransformMutation(NSString *path, NSDictionary *data) { diff --git a/Firestore/Source/API/FIRFieldValue+Internal.h b/Firestore/Source/API/FIRFieldValue+Internal.h index 1618cd4cdc3..7e0193db39c 100644 --- a/Firestore/Source/API/FIRFieldValue+Internal.h +++ b/Firestore/Source/API/FIRFieldValue+Internal.h @@ -54,4 +54,10 @@ NS_ASSUME_NONNULL_BEGIN @property(strong, nonatomic, readonly) NSArray *elements; @end +/** FIRFieldValue class for number increments. */ +@interface FSTNumericIncrementFieldValue : FIRFieldValue +- (instancetype)init NS_UNAVAILABLE; +@property(strong, nonatomic, readonly) NSNumber *operand; +@end + NS_ASSUME_NONNULL_END diff --git a/Firestore/Source/API/FIRFieldValue.mm b/Firestore/Source/API/FIRFieldValue.mm index 5f7fbd753a1..884b1879529 100644 --- a/Firestore/Source/API/FIRFieldValue.mm +++ b/Firestore/Source/API/FIRFieldValue.mm @@ -122,6 +122,28 @@ - (NSString *)methodName { @end +#pragma mark - FSTNumericIncrementFieldValue + +/* FieldValue class for increment() transforms. */ +@interface FSTNumericIncrementFieldValue () +- (instancetype)initWithOperand:(NSNumber *)operand; +@end + +@implementation FSTNumericIncrementFieldValue +- (instancetype)initWithOperand:(NSNumber *)operand; +{ + if (self = [super initPrivate]) { + _operand = operand; + } + return self; +} + +- (NSString *)methodName { + return @"FieldValue.increment()"; +} + +@end + #pragma mark - FIRFieldValue @implementation FIRFieldValue @@ -147,6 +169,14 @@ + (instancetype)fieldValueForArrayRemove:(NSArray *)elements { return [[FSTArrayRemoveFieldValue alloc] initWithElements:elements]; } ++ (instancetype)fieldValueForDoubleIncrement:(double)d { + return [[FSTNumericIncrementFieldValue alloc] initWithOperand:@(d)]; +} + ++ (instancetype)fieldValueForIntegerIncrement:(int64_t)l { + return [[FSTNumericIncrementFieldValue alloc] initWithOperand:@(l)]; +} + @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Source/API/FSTUserDataConverter.mm b/Firestore/Source/API/FSTUserDataConverter.mm index 4b885c69769..0f0abd80114 100644 --- a/Firestore/Source/API/FSTUserDataConverter.mm +++ b/Firestore/Source/API/FSTUserDataConverter.mm @@ -58,6 +58,7 @@ using firebase::firestore::model::FieldMask; using firebase::firestore::model::FieldPath; using firebase::firestore::model::FieldTransform; +using firebase::firestore::model::NumericIncrementTransform; using firebase::firestore::model::Precondition; using firebase::firestore::model::ServerTimestampTransform; using firebase::firestore::model::TransformOperation; @@ -342,6 +343,15 @@ - (void)parseSentinelFieldValue:(FIRFieldValue *)fieldValue context:(ParseContex std::move(parsedElements)); context.AddToFieldTransforms(*context.path(), std::move(array_remove)); + } else if ([fieldValue isKindOfClass:[FSTNumericIncrementFieldValue class]]) { + FSTNumericIncrementFieldValue *numericIncrementFieldValue = + (FSTNumericIncrementFieldValue *)fieldValue; + FSTNumberValue *operand = + (FSTNumberValue *)[self parsedQueryValue:numericIncrementFieldValue.operand]; + auto numeric_increment = absl::make_unique(operand); + + context.AddToFieldTransforms(*context.path(), std::move(numeric_increment)); + } else { HARD_FAIL("Unknown FIRFieldValue type: %s", NSStringFromClass([fieldValue class])); } diff --git a/Firestore/Source/Local/FSTLocalSerializer.mm b/Firestore/Source/Local/FSTLocalSerializer.mm index 528af368266..97545aa4b63 100644 --- a/Firestore/Source/Local/FSTLocalSerializer.mm +++ b/Firestore/Source/Local/FSTLocalSerializer.mm @@ -183,6 +183,10 @@ - (FSTPBWriteBatch *)encodedMutationBatch:(FSTMutationBatch *)batch { proto.localWriteTime = [remoteSerializer encodedTimestamp:Timestamp{batch.localWriteTime.seconds, batch.localWriteTime.nanoseconds}]; + NSMutableArray *baseWrites = proto.baseWritesArray; + for (FSTMutation *baseMutation : [batch baseMutations]) { + [baseWrites addObject:[remoteSerializer encodedMutation:baseMutation]]; + } NSMutableArray *writes = proto.writesArray; for (FSTMutation *mutation : [batch mutations]) { [writes addObject:[remoteSerializer encodedMutation:mutation]]; @@ -194,6 +198,11 @@ - (FSTMutationBatch *)decodedMutationBatch:(FSTPBWriteBatch *)batch { FSTSerializerBeta *remoteSerializer = self.remoteSerializer; int batchID = batch.batchId; + + std::vector baseMutations; + for (GCFSWrite *write in batch.baseWritesArray) { + baseMutations.push_back([remoteSerializer decodedMutation:write]); + } std::vector mutations; for (GCFSWrite *write in batch.writesArray) { mutations.push_back([remoteSerializer decodedMutation:write]); @@ -205,6 +214,7 @@ - (FSTMutationBatch *)decodedMutationBatch:(FSTPBWriteBatch *)batch { initWithBatchID:batchID localWriteTime:[FIRTimestamp timestampWithSeconds:localWriteTime.seconds() nanoseconds:localWriteTime.nanoseconds()] + baseMutations:std::move(baseMutations) mutations:std::move(mutations)]; } diff --git a/Firestore/Source/Local/FSTLocalStore.mm b/Firestore/Source/Local/FSTLocalStore.mm index eabe3c58080..7a6785242f3 100644 --- a/Firestore/Source/Local/FSTLocalStore.mm +++ b/Firestore/Source/Local/FSTLocalStore.mm @@ -57,8 +57,11 @@ using firebase::firestore::model::DocumentKeySet; using firebase::firestore::model::DocumentMap; using firebase::firestore::model::DocumentVersionMap; +using firebase::firestore::model::FieldMask; +using firebase::firestore::model::FieldPath; using firebase::firestore::model::MaybeDocumentMap; using firebase::firestore::model::ListenSequenceNumber; +using firebase::firestore::model::Precondition; using firebase::firestore::model::SnapshotVersion; using firebase::firestore::model::TargetId; using firebase::firestore::remote::RemoteEvent; @@ -164,12 +167,54 @@ - (MaybeDocumentMap)userDidChange:(const User &)user { } - (FSTLocalWriteResult *)locallyWriteMutations:(std::vector &&)mutations { + FIRTimestamp *localWriteTime = [FIRTimestamp timestamp]; + DocumentKeySet keys; + for (FSTMutation *mutation : mutations) { + keys = keys.insert(mutation.key); + } + return self.persistence.run("Locally write mutations", [&]() -> FSTLocalWriteResult * { - FIRTimestamp *localWriteTime = [FIRTimestamp timestamp]; - FSTMutationBatch *batch = - _mutationQueue->AddMutationBatch(localWriteTime, std::move(mutations)); - DocumentKeySet keys = [batch keys]; - MaybeDocumentMap changedDocuments = [self.localDocuments documentsForKeys:keys]; + // Load and apply all existing mutations. This lets us compute the current base state for + // all non-idempotent transforms before applying any additional user-provided writes. + MaybeDocumentMap existingDocuments = [self.localDocuments documentsForKeys:keys]; + + // For non-idempotent mutations (such as `FieldValue.increment()`), we record the base + // state in a separate patch mutation. This is later used to guarantee consistent values + // and prevents flicker even if the backend sends us an update that already includes our + // transform. + std::vector baseMutations; + for (FSTMutation *mutation : mutations) { + if (mutation.idempotent) { + continue; + } + + // Theoretically, we should only include non-idempotent fields in this field mask as this mask + // is used to prevent flicker for non-idempotent transforms by providing consistent base + // values. By including the fields for all DocumentTransforms, we incorrectly prevent rebasing + // of idempotent transforms (such as `arrayUnion()`) when any non-idempotent transforms are + // present. + // TODO(mrschmidt): Expose a method that only returns the a field mask for non-idempotent + // transforms + const FieldMask *fieldMask = [mutation fieldMask]; + if (fieldMask) { + // `documentsForKeys` is guaranteed to return a (nullable) entry for every document key. + FSTMaybeDocument *maybeDocument = existingDocuments.find(mutation.key)->second; + FSTObjectValue *baseValues = + [maybeDocument isKindOfClass:[FSTDocument class]] + ? [((FSTDocument *)maybeDocument).data objectByApplyingFieldMask:*fieldMask] + : [FSTObjectValue objectValue]; + // NOTE: The base state should only be applied if there's some existing document to + // override, so use a Precondition of exists=true + baseMutations.push_back([[FSTPatchMutation alloc] initWithKey:mutation.key + fieldMask:*fieldMask + value:baseValues + precondition:Precondition::Exists(true)]); + } + } + + FSTMutationBatch *batch = _mutationQueue->AddMutationBatch( + localWriteTime, std::move(baseMutations), std::move(mutations)); + MaybeDocumentMap changedDocuments = [batch applyToLocalDocumentSet:existingDocuments]; return [FSTLocalWriteResult resultForBatchID:batch.batchID changes:std::move(changedDocuments)]; }); } diff --git a/Firestore/Source/Model/FSTFieldValue.h b/Firestore/Source/Model/FSTFieldValue.h index 80c5d8d3927..5a9b696ce08 100644 --- a/Firestore/Source/Model/FSTFieldValue.h +++ b/Firestore/Source/Model/FSTFieldValue.h @@ -19,6 +19,7 @@ #import "Firestore/third_party/Immutable/FSTImmutableSortedDictionary.h" #include "Firestore/core/src/firebase/firestore/model/database_id.h" +#include "Firestore/core/src/firebase/firestore/model/field_mask.h" #include "Firestore/core/src/firebase/firestore/model/field_path.h" @class FSTDocumentKey; @@ -247,6 +248,14 @@ enum class ServerTimestampBehavior { None, Estimate, Previous }; * path does not exist within this object's structure, no change is performed. */ - (FSTObjectValue *)objectByDeletingPath:(const firebase::firestore::model::FieldPath &)fieldPath; + +/** + * Applies this field mask to the provided object value and returns an object that only contains + * fields that are specified in both the input object and this field mask. + */ +// TODO(mrschmidt): Once FieldValues are C++, move this to FieldMask to match other platforms. +- (FSTObjectValue *)objectByApplyingFieldMask: + (const firebase::firestore::model::FieldMask &)fieldMask; @end /** diff --git a/Firestore/Source/Model/FSTFieldValue.mm b/Firestore/Source/Model/FSTFieldValue.mm index 88dfad107c2..7730f3987c1 100644 --- a/Firestore/Source/Model/FSTFieldValue.mm +++ b/Firestore/Source/Model/FSTFieldValue.mm @@ -32,6 +32,7 @@ namespace util = firebase::firestore::util; using firebase::firestore::model::DatabaseId; +using firebase::firestore::model::FieldMask; using firebase::firestore::model::FieldPath; using firebase::firestore::util::Comparator; using firebase::firestore::util::CompareMixedNumber; @@ -873,6 +874,21 @@ - (FSTObjectValue *)objectBySettingValue:(FSTFieldValue *)value forField:(NSStri initWithImmutableDictionary:[_internalValue dictionaryBySettingObject:value forKey:field]]; } +- (FSTObjectValue *)objectByApplyingFieldMask:(const FieldMask &)fieldMask { + FSTObjectValue *filteredObject = self; + for (const FieldPath &path : fieldMask) { + if (path.empty()) { + return self; + } else { + FSTFieldValue *newValue = [self valueForPath:path]; + if (newValue) { + filteredObject = [filteredObject objectBySettingValue:newValue forPath:path]; + } + } + } + return filteredObject; +} + @end @interface FSTArrayValue () diff --git a/Firestore/Source/Model/FSTMutation.h b/Firestore/Source/Model/FSTMutation.h index 4eb22bc754a..6eb38e23de7 100644 --- a/Firestore/Source/Model/FSTMutation.h +++ b/Firestore/Source/Model/FSTMutation.h @@ -156,6 +156,16 @@ NS_ASSUME_NONNULL_BEGIN - (const firebase::firestore::model::Precondition &)precondition; +/** + * If applicable, returns the field mask for this mutation. Fields that are not included in this + * field mask are not modified when this mutation is applied. Mutations that replace all document + * values return 'nullptr'. + */ +- (const firebase::firestore::model::FieldMask *)fieldMask; + +/** Returns whether all operations in the mutation are idempotent. */ +@property(nonatomic, readonly) BOOL idempotent; + @end #pragma mark - FSTSetMutation @@ -224,7 +234,7 @@ NS_ASSUME_NONNULL_BEGIN * A mask to apply to |value|, where only fields that are in both the fieldMask and the value * will be updated. */ -- (const firebase::firestore::model::FieldMask &)fieldMask; +- (const firebase::firestore::model::FieldMask *)fieldMask; /** The fields and associated values to use when patching the document. */ @property(nonatomic, strong, readonly) FSTObjectValue *value; diff --git a/Firestore/Source/Model/FSTMutation.mm b/Firestore/Source/Model/FSTMutation.mm index fb74a828450..a805671636e 100644 --- a/Firestore/Source/Model/FSTMutation.mm +++ b/Firestore/Source/Model/FSTMutation.mm @@ -17,6 +17,7 @@ #import "Firestore/Source/Model/FSTMutation.h" #include +#include #include #include #include @@ -103,6 +104,14 @@ - (nullable FSTMaybeDocument *)applyToLocalDocument:(nullable FSTMaybeDocument * return _precondition; } +- (BOOL)idempotent { + @throw FSTAbstractMethodException(); // NOLINT +} + +- (const FieldMask *)fieldMask { + @throw FSTAbstractMethodException(); // NOLINT +} + - (void)verifyKeyMatches:(nullable FSTMaybeDocument *)maybeDoc { if (maybeDoc) { HARD_ASSERT(maybeDoc.key == self.key, "Can only set a document with the same key"); @@ -185,6 +194,15 @@ - (FSTMaybeDocument *)applyToRemoteDocument:(nullable FSTMaybeDocument *)maybeDo version:mutationResult.version state:FSTDocumentStateCommittedMutations]; } + +- (const FieldMask *)fieldMask { + return nullptr; +} + +- (BOOL)idempotent { + return YES; +} + @end #pragma mark - FSTPatchMutation @@ -205,8 +223,8 @@ - (instancetype)initWithKey:(DocumentKey)key return self; } -- (const firebase::firestore::model::FieldMask &)fieldMask { - return _fieldMask; +- (const FieldMask *)fieldMask { + return &_fieldMask; } - (BOOL)isEqual:(id)other { @@ -218,18 +236,18 @@ - (BOOL)isEqual:(id)other { } FSTPatchMutation *otherMutation = (FSTPatchMutation *)other; - return self.key == otherMutation.key && self.fieldMask == otherMutation.fieldMask && + return self.key == otherMutation.key && _fieldMask == *(otherMutation.fieldMask) && [self.value isEqual:otherMutation.value] && self.precondition == otherMutation.precondition; } - (NSUInteger)hash { - return Hash(self.key, self.precondition, self.fieldMask, [self.value hash]); + return Hash(self.key, self.precondition, _fieldMask, [self.value hash]); } - (NSString *)description { return [NSString stringWithFormat:@"", - self.key.ToString().c_str(), self.fieldMask.ToString().c_str(), + self.key.ToString().c_str(), _fieldMask.ToString().c_str(), self.value, self.precondition.description()]; } @@ -288,7 +306,7 @@ - (FSTMaybeDocument *)applyToRemoteDocument:(nullable FSTMaybeDocument *)maybeDo - (FSTObjectValue *)patchObjectValue:(FSTObjectValue *)objectValue { FSTObjectValue *result = objectValue; - for (const FieldPath &fieldPath : self.fieldMask) { + for (const FieldPath &fieldPath : _fieldMask) { if (!fieldPath.empty()) { FSTFieldValue *newValue = [self.value valueForPath:fieldPath]; if (newValue) { @@ -301,11 +319,16 @@ - (FSTObjectValue *)patchObjectValue:(FSTObjectValue *)objectValue { return result; } +- (BOOL)idempotent { + return YES; +} + @end @implementation FSTTransformMutation { /** The field transforms to use when transforming the document. */ std::vector _fieldTransforms; + FieldMask _fieldMask; } - (instancetype)initWithKey:(DocumentKey)key @@ -315,6 +338,13 @@ - (instancetype)initWithKey:(DocumentKey)key // end up with an existing document. if (self = [super initWithKey:std::move(key) precondition:Precondition::Exists(true)]) { _fieldTransforms = std::move(fieldTransforms); + + std::set fields; + for (const auto &transform : _fieldTransforms) { + fields.insert(transform.path()); + } + + _fieldMask = FieldMask(std::move(fields)); } return self; } @@ -482,6 +512,19 @@ - (FSTObjectValue *)transformObject:(FSTObjectValue *)objectValue return objectValue; } +- (const FieldMask *)fieldMask { + return &_fieldMask; +} + +- (BOOL)idempotent { + for (const auto &transform : self.fieldTransforms) { + if (!transform.idempotent()) { + return NO; + } + } + return YES; +} + @end #pragma mark - FSTDeleteMutation @@ -542,6 +585,14 @@ - (FSTMaybeDocument *)applyToRemoteDocument:(nullable FSTMaybeDocument *)maybeDo hasCommittedMutations:YES]; } +- (const FieldMask *)fieldMask { + return nullptr; +} + +- (BOOL)idempotent { + return YES; +} + @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Source/Model/FSTMutationBatch.h b/Firestore/Source/Model/FSTMutationBatch.h index 64c55dafe96..af07fcbcba2 100644 --- a/Firestore/Source/Model/FSTMutationBatch.h +++ b/Firestore/Source/Model/FSTMutationBatch.h @@ -21,6 +21,7 @@ #include "Firestore/core/src/firebase/firestore/model/document_key.h" #include "Firestore/core/src/firebase/firestore/model/document_key_set.h" +#include "Firestore/core/src/firebase/firestore/model/document_map.h" #include "Firestore/core/src/firebase/firestore/model/snapshot_version.h" #include "Firestore/core/src/firebase/firestore/model/types.h" @@ -50,9 +51,13 @@ NS_ASSUME_NONNULL_BEGIN */ @interface FSTMutationBatch : NSObject -/** Initializes a mutation batch with the given batchID, localWriteTime, and mutations. */ +/** + * Initializes a mutation batch with the given batchID, localWriteTime, base mutations, and + * mutations. + */ - (instancetype)initWithBatchID:(firebase::firestore::model::BatchId)batchID localWriteTime:(FIRTimestamp *)localWriteTime + baseMutations:(std::vector &&)baseMutations mutations:(std::vector &&)mutations NS_DESIGNATED_INITIALIZER; - (id)init NS_UNAVAILABLE; @@ -80,14 +85,32 @@ NS_ASSUME_NONNULL_BEGIN applyToLocalDocument:(FSTMaybeDocument *_Nullable)maybeDoc documentKey:(const firebase::firestore::model::DocumentKey &)documentKey; +/** Computes the local view for all provided documents given the mutations in this batch. */ +- (firebase::firestore::model::MaybeDocumentMap)applyToLocalDocumentSet: + (const firebase::firestore::model::MaybeDocumentMap &)documentSet; + /** Returns the set of unique keys referenced by all mutations in the batch. */ - (firebase::firestore::model::DocumentKeySet)keys; -- (const std::vector &)mutations; - +/** The unique ID of this mutation batch. */ @property(nonatomic, assign, readonly) firebase::firestore::model::BatchId batchID; + +/** The original write time of this mutation. */ @property(nonatomic, strong, readonly) FIRTimestamp *localWriteTime; +/** + * Mutations that are used to populate the base values when this mutation is applied locally. This + * can be used to locally overwrite values that are persisted in the remote document cache. Base + * mutations are never sent to the backend. + */ +- (const std::vector &)baseMutations; + +/** + * The user-provided mutations in this mutation batch. User-provided mutations are applied both + * locally and remotely on the backend. + */ +- (const std::vector &)mutations; + @end #pragma mark - FSTMutationBatchResult diff --git a/Firestore/Source/Model/FSTMutationBatch.mm b/Firestore/Source/Model/FSTMutationBatch.mm index 5780614a499..2d1e2c3ffa4 100644 --- a/Firestore/Source/Model/FSTMutationBatch.mm +++ b/Firestore/Source/Model/FSTMutationBatch.mm @@ -24,6 +24,7 @@ #import "Firestore/Source/Model/FSTDocument.h" #import "Firestore/Source/Model/FSTMutation.h" +#include "Firestore/core/src/firebase/firestore/model/document_map.h" #include "Firestore/core/src/firebase/firestore/util/hard_assert.h" #include "Firestore/core/src/firebase/firestore/util/hashing.h" #include "Firestore/core/src/firebase/firestore/util/objc_compatibility.h" @@ -34,28 +35,36 @@ using firebase::firestore::model::DocumentKeyHash; using firebase::firestore::model::DocumentKeySet; using firebase::firestore::model::DocumentVersionMap; +using firebase::firestore::model::MaybeDocumentMap; using firebase::firestore::model::SnapshotVersion; using firebase::firestore::util::Hash; NS_ASSUME_NONNULL_BEGIN @implementation FSTMutationBatch { + std::vector _baseMutations; std::vector _mutations; } - (instancetype)initWithBatchID:(BatchId)batchID localWriteTime:(FIRTimestamp *)localWriteTime + baseMutations:(std::vector &&)baseMutations mutations:(std::vector &&)mutations { HARD_ASSERT(!mutations.empty(), "Cannot create an empty mutation batch"); self = [super init]; if (self) { _batchID = batchID; _localWriteTime = localWriteTime; + _baseMutations = std::move(baseMutations); _mutations = std::move(mutations); } return self; } +- (const std::vector &)baseMutations { + return _baseMutations; +} + - (const std::vector &)mutations { return _mutations; } @@ -70,13 +79,16 @@ - (BOOL)isEqual:(id)other { FSTMutationBatch *otherBatch = (FSTMutationBatch *)other; return self.batchID == otherBatch.batchID && [self.localWriteTime isEqual:otherBatch.localWriteTime] && - std::equal(_mutations.begin(), _mutations.end(), otherBatch.mutations.begin(), - [](FSTMutation *lhs, FSTMutation *rhs) { return [lhs isEqual:rhs]; }); + objc::Equals(_baseMutations, otherBatch.baseMutations) && + objc::Equals(_mutations, otherBatch.mutations); } - (NSUInteger)hash { NSUInteger result = (NSUInteger)self.batchID; result = result * 31 + self.localWriteTime.hash; + for (FSTMutation *mutation : _baseMutations) { + result = result * 31 + [mutation hash]; + } for (FSTMutation *mutation : _mutations) { result = result * 31 + [mutation hash]; } @@ -116,8 +128,20 @@ - (FSTMaybeDocument *_Nullable)applyToLocalDocument:(FSTMaybeDocument *_Nullable HARD_ASSERT(!maybeDoc || maybeDoc.key == documentKey, "applyTo: key %s doesn't match maybeDoc key %s", documentKey.ToString(), maybeDoc.key.ToString()); + + // First, apply the base state. This allows us to apply non-idempotent transform against a + // consistent set of values. + for (FSTMutation *mutation : _baseMutations) { + if (mutation.key == documentKey) { + maybeDoc = [mutation applyToLocalDocument:maybeDoc + baseDocument:maybeDoc + localWriteTime:self.localWriteTime]; + } + } + FSTMaybeDocument *baseDoc = maybeDoc; + // Second, apply all user-provided mutations. for (FSTMutation *mutation : _mutations) { if (mutation.key == documentKey) { maybeDoc = [mutation applyToLocalDocument:maybeDoc @@ -128,6 +152,24 @@ - (FSTMaybeDocument *_Nullable)applyToLocalDocument:(FSTMaybeDocument *_Nullable return maybeDoc; } +- (MaybeDocumentMap)applyToLocalDocumentSet:(const MaybeDocumentMap &)documentSet { + // TODO(mrschmidt): This implementation is O(n^2). If we iterate through the mutations first (as + // done in `applyToLocalDocument:documentKey:`), we can reduce the complexity to O(n). + + MaybeDocumentMap mutatedDocuments = documentSet; + for (FSTMutation *mutation : _mutations) { + const DocumentKey &key = mutation.key; + auto maybeDocument = mutatedDocuments.find(key); + FSTMaybeDocument *mutatedDocument = [self + applyToLocalDocument:(maybeDocument != mutatedDocuments.end() ? maybeDocument->second : nil) + documentKey:key]; + if (mutatedDocument) { + mutatedDocuments = mutatedDocuments.insert(key, mutatedDocument); + } + } + return mutatedDocuments; +} + - (DocumentKeySet)keys { DocumentKeySet set; for (FSTMutation *mutation : _mutations) { diff --git a/Firestore/Source/Public/FIRFieldValue.h b/Firestore/Source/Public/FIRFieldValue.h index d8965876cc2..7973157515b 100644 --- a/Firestore/Source/Public/FIRFieldValue.h +++ b/Firestore/Source/Public/FIRFieldValue.h @@ -61,6 +61,35 @@ NS_SWIFT_NAME(FieldValue) */ + (instancetype)fieldValueForArrayRemove:(NSArray *)elements NS_SWIFT_NAME(arrayRemove(_:)); +/** + * Returns a special value that can be used with setData() or updateData() that tells the server to + * increment the field's current value by the given value. + * + * If the current value is an integer or a double, both the current and the given value will be + * interpreted as doubles and all arithmetic will follow IEEE 754 semantics. Otherwise, the + * transformation will set the field to the given value. + * + * @param d The double value to increment by. + * @return The FieldValue sentinel for use in a call to setData() or update(). + */ ++ (instancetype)fieldValueForDoubleIncrement:(double)d NS_SWIFT_NAME(increment(_:)); + +/** + * Returns a special value that can be used with setData() or updateData() that tells the server to + * increment the field's current value by the given value. + * + * If the current field value is an integer, possible integer overflows are resolved to LONG_MAX or + * LONG_MIN. If the current field value is a double, both values will be interpreted as doubles and + * the arithmetic will follow IEEE 754 semantics. + * + * If field is not an integer or double, or if the field does not yet exist, the transformation + * will set the field to the given value. + * + * @param l The integer value to increment by. + * @return The FieldValue sentinel for use in a call to setData() or updateData(). + */ ++ (instancetype)fieldValueForIntegerIncrement:(int64_t)l NS_SWIFT_NAME(increment(_:)); + @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Source/Remote/FSTSerializerBeta.mm b/Firestore/Source/Remote/FSTSerializerBeta.mm index 3e9b34d4912..3e8fc6809da 100644 --- a/Firestore/Source/Remote/FSTSerializerBeta.mm +++ b/Firestore/Source/Remote/FSTSerializerBeta.mm @@ -66,6 +66,7 @@ using firebase::firestore::model::FieldMask; using firebase::firestore::model::FieldPath; using firebase::firestore::model::FieldTransform; +using firebase::firestore::model::NumericIncrementTransform; using firebase::firestore::model::Precondition; using firebase::firestore::model::ResourcePath; using firebase::firestore::model::ServerTimestampTransform; @@ -477,7 +478,7 @@ - (GCFSWrite *)encodedMutation:(FSTMutation *)mutation { } else if (mutationClass == [FSTPatchMutation class]) { FSTPatchMutation *patch = (FSTPatchMutation *)mutation; proto.update = [self encodedDocumentWithFields:patch.value key:patch.key]; - proto.updateMask = [self encodedFieldMask:patch.fieldMask]; + proto.updateMask = [self encodedFieldMask:*(patch.fieldMask)]; } else if (mutationClass == [FSTTransformMutation class]) { FSTTransformMutation *transform = (FSTTransformMutation *)mutation; @@ -611,7 +612,10 @@ - (GCFSDocumentTransform_FieldTransform *)encodedFieldTransform: } else if (fieldTransform.transformation().type() == TransformOperation::Type::ArrayRemove) { proto.removeAllFromArray_p = [self encodedArrayTransformElements:ArrayTransform::Elements(fieldTransform.transformation())]; - + } else if (fieldTransform.transformation().type() == TransformOperation::Type::Increment) { + const NumericIncrementTransform &incrementTransform = + static_cast(fieldTransform.transformation()); + proto.increment = [self encodedFieldValue:incrementTransform.operand()]; } else { HARD_FAIL("Unknown transform: %s type", fieldTransform.transformation().type()); } @@ -666,6 +670,14 @@ - (GCFSArrayValue *)encodedArrayTransformElements:(const std::vector([self decodedFieldValue:proto.increment]); + fieldTransforms.emplace_back(FieldPath::FromServerFormat(util::MakeString(proto.fieldPath)), + absl::make_unique(operand)); + break; + } + default: HARD_FAIL("Unknown transform: %s", proto); } diff --git a/Firestore/core/src/firebase/firestore/local/leveldb_mutation_queue.h b/Firestore/core/src/firebase/firestore/local/leveldb_mutation_queue.h index c8b00e572ed..676cc3ff1c3 100644 --- a/Firestore/core/src/firebase/firestore/local/leveldb_mutation_queue.h +++ b/Firestore/core/src/firebase/firestore/local/leveldb_mutation_queue.h @@ -72,6 +72,7 @@ class LevelDbMutationQueue : public MutationQueue { FSTMutationBatch* AddMutationBatch( FIRTimestamp* local_write_time, + std::vector&& base_mutations, std::vector&& mutations) override; void RemoveMutationBatch(FSTMutationBatch* batch) override; diff --git a/Firestore/core/src/firebase/firestore/local/leveldb_mutation_queue.mm b/Firestore/core/src/firebase/firestore/local/leveldb_mutation_queue.mm index 9bfce73c0c4..53928703206 100644 --- a/Firestore/core/src/firebase/firestore/local/leveldb_mutation_queue.mm +++ b/Firestore/core/src/firebase/firestore/local/leveldb_mutation_queue.mm @@ -156,13 +156,16 @@ BatchId LoadNextBatchIdFromDb(DB* db) { } FSTMutationBatch* LevelDbMutationQueue::AddMutationBatch( - FIRTimestamp* local_write_time, std::vector&& mutations) { + FIRTimestamp* local_write_time, + std::vector&& base_mutations, + std::vector&& mutations) { BatchId batch_id = next_batch_id_; next_batch_id_++; FSTMutationBatch* batch = [[FSTMutationBatch alloc] initWithBatchID:batch_id localWriteTime:local_write_time + baseMutations:std::move(base_mutations) mutations:std::move(mutations)]; std::string key = mutation_batch_key(batch_id); db_.currentTransaction->Put(key, [serializer_ encodedMutationBatch:batch]); diff --git a/Firestore/core/src/firebase/firestore/local/memory_mutation_queue.h b/Firestore/core/src/firebase/firestore/local/memory_mutation_queue.h index cf863d23092..463439e9331 100644 --- a/Firestore/core/src/firebase/firestore/local/memory_mutation_queue.h +++ b/Firestore/core/src/firebase/firestore/local/memory_mutation_queue.h @@ -60,6 +60,7 @@ class MemoryMutationQueue : public MutationQueue { FSTMutationBatch* AddMutationBatch( FIRTimestamp* local_write_time, + std::vector&& base_mutations, std::vector&& mutations) override; void RemoveMutationBatch(FSTMutationBatch* batch) override; diff --git a/Firestore/core/src/firebase/firestore/local/memory_mutation_queue.mm b/Firestore/core/src/firebase/firestore/local/memory_mutation_queue.mm index 9f5d9bd89f2..c17dfd2c90c 100644 --- a/Firestore/core/src/firebase/firestore/local/memory_mutation_queue.mm +++ b/Firestore/core/src/firebase/firestore/local/memory_mutation_queue.mm @@ -74,7 +74,9 @@ } FSTMutationBatch* MemoryMutationQueue::AddMutationBatch( - FIRTimestamp* local_write_time, std::vector&& mutations) { + FIRTimestamp* local_write_time, + std::vector&& base_mutations, + std::vector&& mutations) { HARD_ASSERT(!mutations.empty(), "Mutation batches should not be empty"); BatchId batch_id = next_batch_id_; @@ -89,6 +91,7 @@ FSTMutationBatch* batch = [[FSTMutationBatch alloc] initWithBatchID:batch_id localWriteTime:local_write_time + baseMutations:std::move(base_mutations) mutations:std::move(mutations)]; queue_.push_back(batch); diff --git a/Firestore/core/src/firebase/firestore/local/mutation_queue.h b/Firestore/core/src/firebase/firestore/local/mutation_queue.h index 3e66a7c082d..6977b73c1e1 100644 --- a/Firestore/core/src/firebase/firestore/local/mutation_queue.h +++ b/Firestore/core/src/firebase/firestore/local/mutation_queue.h @@ -59,9 +59,18 @@ class MutationQueue { virtual void AcknowledgeBatch(FSTMutationBatch* batch, NSData* _Nullable stream_token) = 0; - /** Creates a new mutation batch and adds it to this mutation queue. */ + /** + * Creates a new mutation batch and adds it to this mutation queue. + * + * @param local_write_time The original write time of this mutation. + * @param base_mutations Mutations that are used to populate the base values + * when this mutation is applied locally. These mutations are used to locally + * overwrite values that are persisted in the remote document cache. + * @param mutations The user-provided mutations in this mutation batch. + */ virtual FSTMutationBatch* AddMutationBatch( FIRTimestamp* local_write_time, + std::vector&& base_mutations, std::vector&& mutations) = 0; /** diff --git a/Firestore/core/src/firebase/firestore/model/field_mask.h b/Firestore/core/src/firebase/firestore/model/field_mask.h index eabad446c5d..d8cdfba6e61 100644 --- a/Firestore/core/src/firebase/firestore/model/field_mask.h +++ b/Firestore/core/src/firebase/firestore/model/field_mask.h @@ -51,6 +51,9 @@ class FieldMask { explicit FieldMask(std::set fields) : fields_{std::move(fields)} { } + FieldMask(const FieldMask& f) : fields_{f.begin(), f.end()} { + } + const_iterator begin() const { return fields_.begin(); } diff --git a/Firestore/core/src/firebase/firestore/model/field_transform.h b/Firestore/core/src/firebase/firestore/model/field_transform.h index 1a7127a8fa6..4591802dd7a 100644 --- a/Firestore/core/src/firebase/firestore/model/field_transform.h +++ b/Firestore/core/src/firebase/firestore/model/field_transform.h @@ -44,6 +44,10 @@ class FieldTransform { return *transformation_.get(); } + bool idempotent() const { + return transformation_->idempotent(); + } + bool operator==(const FieldTransform& other) const { return path_ == other.path_ && *transformation_ == *other.transformation_; } diff --git a/Firestore/core/src/firebase/firestore/model/transform_operations.h b/Firestore/core/src/firebase/firestore/model/transform_operations.h index a5c5f821336..c4ac9849a48 100644 --- a/Firestore/core/src/firebase/firestore/model/transform_operations.h +++ b/Firestore/core/src/firebase/firestore/model/transform_operations.h @@ -44,6 +44,7 @@ class TransformOperation { ServerTimestamp, ArrayUnion, ArrayRemove, + Increment, Test, // Purely for test purpose. }; @@ -67,6 +68,9 @@ class TransformOperation { virtual FSTFieldValue* ApplyToRemoteDocument( FSTFieldValue* previousValue, FSTFieldValue* transformResult) const = 0; + /** Returns whether this field transform is idempotent. */ + virtual bool idempotent() const = 0; + /** Returns whether the two are equal. */ virtual bool operator==(const TransformOperation& other) const = 0; @@ -83,9 +87,6 @@ class TransformOperation { /** Transforms a value into a server-generated timestamp. */ class ServerTimestampTransform : public TransformOperation { public: - ~ServerTimestampTransform() override { - } - Type type() const override { return Type::ServerTimestamp; } @@ -103,6 +104,10 @@ class ServerTimestampTransform : public TransformOperation { return transformResult; } + bool idempotent() const override { + return true; + } + bool operator==(const TransformOperation& other) const override { // All ServerTimestampTransform objects are equal. return other.type() == Type::ServerTimestamp; @@ -136,9 +141,6 @@ class ArrayTransform : public TransformOperation { : type_(type), elements_(std::move(elements)) { } - ~ArrayTransform() override { - } - Type type() const override { return type_; } @@ -162,6 +164,10 @@ class ArrayTransform : public TransformOperation { return elements_; } + bool idempotent() const override { + return true; + } + bool operator==(const TransformOperation& other) const override { if (other.type() != type()) { return false; @@ -233,6 +239,108 @@ class ArrayTransform : public TransformOperation { } }; +/** + * Implements the backend semantics for locally computed NUMERIC_ADD (increment) + * transforms. Converts all field values to longs or doubles and resolves + * overflows to LONG_MAX/LONG_MIN. + */ +class NumericIncrementTransform : public TransformOperation { + public: + explicit NumericIncrementTransform(FSTNumberValue* operand) + : operand_(operand) { + } + + Type type() const override { + return Type::Increment; + } + + FSTFieldValue* ApplyToLocalView( + FSTFieldValue* previousValue, + FIRTimestamp* /* localWriteTime */) const override { + // Return an integer value only if the previous value and the operand is an + // integer. + if ([previousValue isKindOfClass:[FSTIntegerValue class]] && + [operand_ isKindOfClass:[FSTIntegerValue class]]) { + int64_t sum = SafeIncrement( + (static_cast(previousValue)).internalValue, + (static_cast(operand_)).internalValue); + return [FSTIntegerValue integerValue:sum]; + } else if ([previousValue isKindOfClass:[FSTIntegerValue class]]) { + double sum = + (static_cast(previousValue)).internalValue + + OperandAsDouble(); + return [FSTDoubleValue doubleValue:sum]; + } else if ([previousValue isKindOfClass:[FSTDoubleValue class]]) { + double sum = (static_cast(previousValue)).internalValue + + OperandAsDouble(); + return [FSTDoubleValue doubleValue:sum]; + } else { + // If the existing value is not a number, use the value of the transform + // as the new base value. + return operand_; + } + } + + FSTFieldValue* ApplyToRemoteDocument( + FSTFieldValue* previousValue, + FSTFieldValue* transformResult) const override { + return transformResult; + } + + FSTNumberValue* operand() const { + return operand_; + } + + bool idempotent() const override { + return false; + } + + bool operator==(const TransformOperation& other) const override { + if (other.type() != type()) { + return false; + } + auto numeric_add = static_cast(other); + return [operand_ isEqual:numeric_add.operand_]; + } + + // For Objective-C++ hash; to be removed after migration. + // Do NOT use in C++ code. + NSUInteger Hash() const override { + NSUInteger result = 37; + result = 31 * result + [operand_ hash]; + return result; + } + + private: + FSTNumberValue* operand_; + + /** + * Implements integer addition. Overflows are resolved to LONG_MAX/LONG_MIN. + */ + int64_t SafeIncrement(int64_t x, int64_t y) const { + if (x > 0 && y > LONG_MAX - x) { + return LONG_MAX; + } + + if (x < 0 && y < LONG_MIN - x) { + return LONG_MIN; + } + + return x + y; + } + + double OperandAsDouble() const { + if ([operand_ isKindOfClass:[FSTDoubleValue class]]) { + return (static_cast(operand_)).internalValue; + } else if ([operand_ isKindOfClass:[FSTIntegerValue class]]) { + return (static_cast(operand_)).internalValue; + } else { + HARD_FAIL("Expected 'operand' to be of FSTNumerValue type, but was %s", + NSStringFromClass([operand_ class])); + } + } +}; + } // namespace model } // namespace firestore } // namespace firebase