Skip to content

Commit e9ba9eb

Browse files
dconeybedconeybe
authored
Firestore: Fix FAILED_PRECONDITION when writing to a deleted document in a transaction (#10431)
1 parent ec7ab4b commit e9ba9eb

File tree

4 files changed

+163
-10
lines changed

4 files changed

+163
-10
lines changed

‎Firestore/CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
# Unreleased
2+
- [fixed] Fix FAILED_PRECONDITION when writing to a deleted document in a
3+
transaction (#10431).
4+
15
# 10.0.0
26
- [feature] Added `Query.count()`, which fetches the number of documents in the
37
result set without actually downloading the documents (#10246).

‎Firestore/Example/Tests/Integration/FSTTransactionTests.mm

Lines changed: 151 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,15 @@ - (void)assertSnapshot:(FIRDocumentSnapshot *)snapshot
104104
[transaction getDocument:doc error:&error];
105105
};
106106

107+
typedef NS_ENUM(NSUInteger, FIRFromDocumentType) {
108+
// The operation will be performed on a document that exists.
109+
FIRFromDocumentTypeExisting,
110+
// The operation will be performed on a document that has never existed.
111+
FIRFromDocumentTypeNonExistent,
112+
// The operation will be performed on a document that existed, but was deleted.
113+
FIRFromDocumentTypeDeleted,
114+
};
115+
107116
/**
108117
* Used for testing that all possible combinations of executing transactions result in the desired
109118
* document value or error.
@@ -117,14 +126,15 @@ - (void)assertSnapshot:(FIRDocumentSnapshot *)snapshot
117126
@interface FSTTransactionTester : NSObject
118127
- (FSTTransactionTester *)withExistingDoc;
119128
- (FSTTransactionTester *)withNonexistentDoc;
129+
- (FSTTransactionTester *)withDeletedDoc;
120130
- (FSTTransactionTester *)runWithStages:(NSArray<TransactionStage> *)stages;
121131
- (void)expectDoc:(NSObject *)expected;
122132
- (void)expectNoDoc;
123133
- (void)expectError:(FIRFirestoreErrorCode)expected;
124134

125135
@property(atomic, strong, readwrite) NSArray<TransactionStage> *stages;
126136
@property(atomic, strong, readwrite) FIRDocumentReference *docRef;
127-
@property(atomic, assign, readwrite) BOOL fromExistingDoc;
137+
@property(atomic, assign, readwrite) FIRFromDocumentType fromDocumentType;
128138
@end
129139

130140
@implementation FSTTransactionTester {
@@ -137,19 +147,25 @@ - (instancetype)initWithDb:(FIRFirestore *)db testCase:(FSTTransactionTests *)te
137147
if (self) {
138148
_db = db;
139149
_stages = [NSArray array];
150+
_fromDocumentType = FIRFromDocumentTypeNonExistent;
140151
_testCase = testCase;
141152
_testExpectations = [NSMutableArray array];
142153
}
143154
return self;
144155
}
145156

146157
- (FSTTransactionTester *)withExistingDoc {
147-
self.fromExistingDoc = YES;
158+
self.fromDocumentType = FIRFromDocumentTypeExisting;
148159
return self;
149160
}
150161

151162
- (FSTTransactionTester *)withNonexistentDoc {
152-
self.fromExistingDoc = NO;
163+
self.fromDocumentType = FIRFromDocumentTypeNonExistent;
164+
return self;
165+
}
166+
167+
- (FSTTransactionTester *)withDeletedDoc {
168+
self.fromDocumentType = FIRFromDocumentTypeDeleted;
153169
return self;
154170
}
155171

@@ -195,10 +211,30 @@ - (void)expectError:(FIRFirestoreErrorCode)expected {
195211

196212
- (void)prepareDoc {
197213
self.docRef = [[_db collectionWithPath:@"nonexistent"] documentWithAutoID];
198-
if (_fromExistingDoc) {
199-
NSError *setError = [self writeDocumentRef:self.docRef data:@{@"foo" : @"bar"}];
200-
NSString *message = [NSString stringWithFormat:@"Failed set at %@", [self stageNames]];
201-
[_testCase assertNilError:setError message:message];
214+
switch (_fromDocumentType) {
215+
case FIRFromDocumentTypeExisting: {
216+
NSError *setError = [self writeDocumentRef:self.docRef data:@{@"foo" : @"bar"}];
217+
NSString *message = [NSString stringWithFormat:@"Failed set at %@", [self stageNames]];
218+
[_testCase assertNilError:setError message:message];
219+
break;
220+
}
221+
case FIRFromDocumentTypeNonExistent: {
222+
// Nothing to do; document does not exist.
223+
break;
224+
}
225+
case FIRFromDocumentTypeDeleted: {
226+
{
227+
NSError *setError = [self writeDocumentRef:self.docRef data:@{@"foo" : @"bar"}];
228+
NSString *message = [NSString stringWithFormat:@"Failed set at %@", [self stageNames]];
229+
[_testCase assertNilError:setError message:message];
230+
}
231+
{
232+
NSError *deleteError = [self deleteDocumentRef:self.docRef];
233+
NSString *message = [NSString stringWithFormat:@"Failed delete at %@", [self stageNames]];
234+
[_testCase assertNilError:deleteError message:message];
235+
}
236+
break;
237+
}
202238
}
203239
}
204240

@@ -215,6 +251,17 @@ - (NSError *)writeDocumentRef:(FIRDocumentReference *)ref
215251
return errorResult;
216252
}
217253

254+
- (NSError *)deleteDocumentRef:(FIRDocumentReference *)ref {
255+
__block NSError *errorResult;
256+
XCTestExpectation *expectation = [_testCase expectationWithDescription:@"prepareDoc:delete"];
257+
[ref deleteDocumentWithCompletion:^(NSError *error) {
258+
errorResult = error;
259+
[expectation fulfill];
260+
}];
261+
[_testCase awaitExpectations];
262+
return errorResult;
263+
}
264+
218265
- (void)runSuccessfulTransaction {
219266
XCTestExpectation *expectation =
220267
[_testCase expectationWithDescription:@"runSuccessfulTransaction"];
@@ -322,6 +369,32 @@ - (void)testRunsTransactionsAfterGettingNonexistentDoc {
322369
[[[tt withNonexistentDoc] runWithStages:@[ get, set1, set2 ]] expectDoc:@{@"foo" : @"bar2"}];
323370
}
324371

372+
// This test is identical to the test above, except that withNonexistentDoc() is replaced by
373+
// withDeletedDoc(), to guard against regression of
374+
// https://github.com/firebase/firebase-js-sdk/issues/5871, where transactions would incorrectly
375+
// fail with FAILED_PRECONDITION when operations were performed on a deleted document (rather than
376+
// a non-existent document).
377+
- (void)testRunsTransactionsAfterGettingDeletedDoc {
378+
FIRFirestore *firestore = [self firestore];
379+
FSTTransactionTester *tt = [[FSTTransactionTester alloc] initWithDb:firestore testCase:self];
380+
381+
[[[tt withDeletedDoc] runWithStages:@[ get, delete1, delete1 ]] expectNoDoc];
382+
[[[tt withDeletedDoc] runWithStages:@[ get, delete1, update2 ]]
383+
expectError:FIRFirestoreErrorCodeInvalidArgument];
384+
[[[tt withDeletedDoc] runWithStages:@[ get, delete1, set2 ]] expectDoc:@{@"foo" : @"bar2"}];
385+
386+
[[[tt withDeletedDoc] runWithStages:@[ get, update1, delete1 ]]
387+
expectError:FIRFirestoreErrorCodeInvalidArgument];
388+
[[[tt withDeletedDoc] runWithStages:@[ get, update1, update2 ]]
389+
expectError:FIRFirestoreErrorCodeInvalidArgument];
390+
[[[tt withDeletedDoc] runWithStages:@[ get, update1, set2 ]]
391+
expectError:FIRFirestoreErrorCodeInvalidArgument];
392+
393+
[[[tt withDeletedDoc] runWithStages:@[ get, set1, delete1 ]] expectNoDoc];
394+
[[[tt withDeletedDoc] runWithStages:@[ get, set1, update2 ]] expectDoc:@{@"foo" : @"bar2"}];
395+
[[[tt withDeletedDoc] runWithStages:@[ get, set1, set2 ]] expectDoc:@{@"foo" : @"bar2"}];
396+
}
397+
325398
- (void)testRunsTransactionOnExistingDoc {
326399
FIRFirestore *firestore = [self firestore];
327400
FSTTransactionTester *tt = [[FSTTransactionTester alloc] initWithDb:firestore testCase:self];
@@ -361,6 +434,27 @@ - (void)testRunsTransactionsOnNonexistentDoc {
361434
[[[tt withNonexistentDoc] runWithStages:@[ set1, set2 ]] expectDoc:@{@"foo" : @"bar2"}];
362435
}
363436

437+
- (void)testRunsTransactionsOnDeletedDoc {
438+
FIRFirestore *firestore = [self firestore];
439+
FSTTransactionTester *tt = [[FSTTransactionTester alloc] initWithDb:firestore testCase:self];
440+
441+
[[[tt withDeletedDoc] runWithStages:@[ delete1, delete1 ]] expectNoDoc];
442+
[[[tt withDeletedDoc] runWithStages:@[ delete1, update2 ]]
443+
expectError:FIRFirestoreErrorCodeInvalidArgument];
444+
[[[tt withDeletedDoc] runWithStages:@[ delete1, set2 ]] expectDoc:@{@"foo" : @"bar2"}];
445+
446+
[[[tt withDeletedDoc] runWithStages:@[ update1, delete1 ]]
447+
expectError:FIRFirestoreErrorCodeNotFound];
448+
[[[tt withDeletedDoc] runWithStages:@[ update1, update2 ]]
449+
expectError:FIRFirestoreErrorCodeNotFound];
450+
[[[tt withDeletedDoc] runWithStages:@[ update1, set2 ]]
451+
expectError:FIRFirestoreErrorCodeNotFound];
452+
453+
[[[tt withDeletedDoc] runWithStages:@[ set1, delete1 ]] expectNoDoc];
454+
[[[tt withDeletedDoc] runWithStages:@[ set1, update2 ]] expectDoc:@{@"foo" : @"bar2"}];
455+
[[[tt withDeletedDoc] runWithStages:@[ set1, set2 ]] expectDoc:@{@"foo" : @"bar2"}];
456+
}
457+
364458
- (void)testSetDocumentWithMerge {
365459
FIRFirestore *firestore = [self firestore];
366460
FIRDocumentReference *doc = [[firestore collectionWithPath:@"towns"] documentWithAutoID];
@@ -653,6 +747,56 @@ - (void)testDoesNotRetryOnPermanentError {
653747
[self awaitExpectations];
654748
}
655749

750+
- (void)testRetryOnAlreadyExistsError {
751+
FIRFirestore *firestore = [self firestore];
752+
FIRDocumentReference *doc1 = [[firestore collectionWithPath:@"counters"] documentWithAutoID];
753+
auto transactionCallbackCallCount = std::make_shared<std::atomic_int>(0);
754+
755+
// Skip backoff delays.
756+
[firestore workerQueue]->SkipDelaysForTimerId(TimerId::RetryTransaction);
757+
758+
XCTestExpectation *expectation = [self expectationWithDescription:@"transaction"];
759+
[firestore
760+
runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **error) {
761+
int callbackNum = ++(*transactionCallbackCallCount);
762+
763+
FIRDocumentSnapshot *snapshot = [transaction getDocument:doc1 error:error];
764+
XCTAssertNil(*error);
765+
766+
if (callbackNum == 1) {
767+
XCTAssertFalse(snapshot.exists);
768+
// Create the document outside of the transaction to cause the commit to fail with
769+
// ALREADY_EXISTS.
770+
dispatch_semaphore_t writeSemaphore = dispatch_semaphore_create(0);
771+
[doc1 setData:@{@"foo1" : @"bar1"}
772+
completion:^(NSError *) {
773+
dispatch_semaphore_signal(writeSemaphore);
774+
}];
775+
// We can block on it, because transactions run on a background queue.
776+
dispatch_semaphore_wait(writeSemaphore, DISPATCH_TIME_FOREVER);
777+
} else if (callbackNum == 2) {
778+
XCTAssertTrue(snapshot.exists);
779+
} else {
780+
XCTFail(@"unexpected callbackNum: %@", @(callbackNum));
781+
}
782+
783+
[transaction setData:@{@"foo2" : @"bar2"} forDocument:doc1];
784+
785+
return nil;
786+
}
787+
completion:^(id, NSError *_Nullable error) {
788+
[expectation fulfill];
789+
XCTAssertNil(error);
790+
}];
791+
[self awaitExpectations];
792+
793+
XCTAssertEqual(transactionCallbackCallCount->load(), 2);
794+
FIRDocumentSnapshot *snapshot = [self readDocumentForRef:doc1];
795+
XCTAssertNotNil(snapshot);
796+
XCTAssertTrue(snapshot.exists);
797+
XCTAssertEqualObjects(snapshot.data, (@{@"foo2" : @"bar2"}));
798+
}
799+
656800
- (void)testMakesDefaultMaxAttempts {
657801
FIRFirestore *firestore = [self firestore];
658802
FIRDocumentReference *doc1 = [[firestore collectionWithPath:@"counters"] documentWithAutoID];

‎Firestore/core/src/core/transaction.cc

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,11 @@ void Transaction::WriteMutations(std::vector<Mutation>&& mutations) {
131131
Precondition Transaction::CreatePrecondition(const DocumentKey& key) {
132132
absl::optional<SnapshotVersion> version = GetVersion(key);
133133
if (written_docs_.count(key) == 0 && version.has_value()) {
134-
return Precondition::UpdateTime(version.value());
134+
if (version.value() == SnapshotVersion::None()) {
135+
return Precondition::Exists(false);
136+
} else {
137+
return Precondition::UpdateTime(version.value());
138+
}
135139
} else {
136140
return Precondition::None();
137141
}

‎Firestore/core/src/core/transaction_runner.cc

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,11 @@ using util::TimerId;
3333

3434
bool IsRetryableTransactionError(const util::Status& error) {
3535
// In transactions, the backend will fail outdated reads with
36-
// FAILED_PRECONDITION and non-matching document versions with ABORTED. These
36+
// FAILED_PRECONDITION, non-matching document versions with ABORTED, and
37+
// attempts to create already-existing document with ALREADY_EXISTS. These
3738
// errors should be retried.
3839
Error code = error.code();
39-
return code == Error::kErrorAborted ||
40+
return code == Error::kErrorAborted || code == Error::kErrorAlreadyExists ||
4041
code == Error::kErrorFailedPrecondition ||
4142
!remote::Datastore::IsPermanentError(error);
4243
}

0 commit comments

Comments
 (0)