Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix FAILED_PRECONDITION when writing to a deleted document in a transaction #4257

Merged
merged 6 commits into from
Nov 1, 2022
Merged

Fix FAILED_PRECONDITION when writing to a deleted document in a transaction #4257

merged 6 commits into from
Nov 1, 2022

Conversation

milaGGL
Copy link
Contributor

@milaGGL milaGGL commented Oct 28, 2022 •
edited
Loading

This fix is ported from Web SDK firebase/firebase-js-sdk#6550, and its follow-up PR firebase/firebase-js-sdk#6729.

@milaGGL milaGGL added the WIP label Oct 28, 2022
@milaGGL milaGGL self-assigned this Oct 28, 2022
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 28, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • firebase-firestore

    Overall coverage changed from ? (ffca832) to 44.43% (22e2d15) by ?.

    315 individual files with coverage change

    FilenameBase (ffca832)Merge (22e2d15)Diff
    AbstractStream.java?34.81%?
    ActivityScope.java?0.00%?
    AggregateQuery.java?15.00%?
    AggregateQuerySnapshot.java?0.00%?
    AggregateSource.java?0.00%?
    AggregationResult.java?0.00%?
    AggregationResultOrBuilder.java?0.00%?
    AggregationResultProto.java?0.00%?
    AndroidConnectivityMonitor.java?39.51%?
    ApiUtil.java?12.50%?
    ArrayContainsAnyFilter.java?100.00%?
    ArrayContainsFilter.java?100.00%?
    ArrayTransformOperation.java?86.49%?
    ArrayValue.java?48.60%?
    ArrayValueOrBuilder.java?0.00%?
    Assert.java?60.00%?
    AsyncEventListener.java?0.00%?
    AsyncQueue.java?70.65%?
    AutoValue_FieldIndex.java?58.33%?
    AutoValue_FieldIndex_IndexOffset.java?58.62%?
    AutoValue_FieldIndex_IndexState.java?59.09%?
    AutoValue_FieldIndex_Segment.java?54.17%?
    AutoValue_IndexEntry.java?34.21%?
    AutoValue_Overlay.java?54.55%?
    BackgroundQueue.java?81.25%?
    BasePath.java?86.54%?
    BatchGetDocumentsRequest.java?0.00%?
    BatchGetDocumentsRequestOrBuilder.java?0.00%?
    BatchGetDocumentsResponse.java?0.00%?
    BatchGetDocumentsResponseOrBuilder.java?0.00%?
    BeginTransactionRequest.java?0.00%?
    BeginTransactionRequestOrBuilder.java?0.00%?
    BeginTransactionResponse.java?0.00%?
    BeginTransactionResponseOrBuilder.java?0.00%?
    Blob.java?76.92%?
    Bound.java?39.29%?
    BundleCache.java?0.00%?
    BundleCallback.java?0.00%?
    BundledDocumentMetadata.java?0.00%?
    BundledDocumentMetadataOrBuilder.java?0.00%?
    BundleDocument.java?90.00%?
    BundledQuery.java?34.57%?
    BundledQueryOrBuilder.java?0.00%?
    BundleElement.java?0.00%?
    BundleElementOrBuilder.java?0.00%?
    BundleLoader.java?100.00%?
    BundleMetadata.java?0.00%?
    BundleMetadataOrBuilder.java?0.00%?
    BundleProto.java?0.00%?
    BundleReader.java?95.29%?
    BundleSerializer.java?89.47%?
    ByteBufferInputStream.java?83.33%?
    CollectionReference.java?13.64%?
    CommitRequest.java?0.00%?
    CommitRequestOrBuilder.java?0.00%?
    CommitResponse.java?0.00%?
    CommitResponseOrBuilder.java?0.00%?
    CommonProto.java?0.00%?
    ComponentProvider.java?100.00%?
    CompositeFilter.java?98.46%?
    ConnectivityMonitor.java?0.00%?
    Consumer.java?0.00%?
    CreateDocumentRequest.java?0.00%?
    CreateDocumentRequestOrBuilder.java?0.00%?
    CredentialsProvider.java?100.00%?
    Cursor.java?35.29%?
    CursorOrBuilder.java?0.00%?
    CustomClassMapper.java?84.25%?
    DatabaseId.java?70.37%?
    DatabaseInfo.java?90.91%?
    Datastore.java?23.28%?
    DatastoreTestTrace.java?0.00%?
    DeleteDocumentRequest.java?0.00%?
    DeleteDocumentRequestOrBuilder.java?0.00%?
    DeleteMutation.java?95.24%?
    DirectionalIndexByteEncoder.java?100.00%?
    Document.java?100.00%?
    DocumentChange.java?67.61%?
    DocumentChangeOrBuilder.java?0.00%?
    DocumentCollections.java?83.33%?
    DocumentDelete.java?34.62%?
    DocumentDeleteOrBuilder.java?0.00%?
    DocumentId.java?0.00%?
    DocumentKey.java?97.14%?
    DocumentMask.java?40.66%?
    DocumentMaskOrBuilder.java?0.00%?
    DocumentOrBuilder.java?0.00%?
    DocumentOverlayCache.java?0.00%?
    DocumentProto.java?0.00%?
    DocumentReference.java?13.24%?
    DocumentRemove.java?29.23%?
    DocumentRemoveOrBuilder.java?0.00%?
    DocumentSet.java?83.78%?
    DocumentSnapshot.java?37.50%?
    DocumentTransform.java?29.31%?
    DocumentTransformOrBuilder.java?0.00%?
    DocumentViewChange.java?90.91%?
    DocumentViewChangeSet.java?87.88%?
    EncodedPath.java?93.22%?
    EventListener.java?0.00%?
    EventManager.java?96.15%?
    Exclude.java?0.00%?
    Executors.java?100.00%?
    ExistenceFilter.java?80.00%?
    ExistenceFilterOrBuilder.java?0.00%?
    ExponentialBackoff.java?30.43%?
    FieldFilter.java?93.06%?
    FieldIndex.java?98.18%?
    FieldMask.java?57.89%?
    FieldPath.java?89.66%?
    FieldTransform.java?64.71%?
    FieldValue.java?82.14%?
    FileUtil.java?0.00%?
    Filter.java?0.00%?
    FirebaseAppCheckTokenProvider.java?93.75%?
    FirebaseAuthCredentialsProvider.java?100.00%?
    FirebaseClientGrpcMetadataProvider.java?44.00%?
    FirebaseFirestore.java?36.67%?
    FirebaseFirestoreException.java?83.72%?
    FirebaseFirestoreSettings.java?75.00%?
    FirestoreCallCredentials.java?18.60%?
    FirestoreChannel.java?18.10%?
    FirestoreClient.java?36.11%?
    FirestoreGrpc.java?2.46%?
    FirestoreIndexValueWriter.java?77.11%?
    FirestoreMultiDbComponent.java?100.00%?
    FirestoreProto.java?0.00%?
    FirestoreRegistrar.java?100.00%?
    Function.java?0.00%?
    GeoPoint.java?91.67%?
    GetDocumentRequest.java?0.00%?
    GetDocumentRequestOrBuilder.java?0.00%?
    GrpcCallProvider.java?69.41%?
    GrpcMetadataProvider.java?0.00%?
    IgnoreExtraProperties.java?0.00%?
    IncomingStreamObserver.java?0.00%?
    Index.java?30.02%?
    IndexBackfiller.java?100.00%?
    IndexByteEncoder.java?88.57%?
    IndexEntry.java?100.00%?
    IndexManager.java?100.00%?
    IndexOrBuilder.java?0.00%?
    IndexProto.java?0.00%?
    InFilter.java?100.00%?
    IntMath.java?41.67%?
    KeyFieldFilter.java?66.67%?
    KeyFieldInFilter.java?0.00%?
    KeyFieldNotInFilter.java?0.00%?
    LimboDocumentChange.java?70.59%?
    ListCollectionIdsRequest.java?0.00%?
    ListCollectionIdsRequestOrBuilder.java?0.00%?
    ListCollectionIdsResponse.java?0.00%?
    ListCollectionIdsResponseOrBuilder.java?0.00%?
    ListDocumentsRequest.java?0.00%?
    ListDocumentsRequestOrBuilder.java?0.00%?
    ListDocumentsResponse.java?0.00%?
    ListDocumentsResponseOrBuilder.java?0.00%?
    Listener.java?0.00%?
    ListenerRegistration.java?0.00%?
    ListenerRegistrationImpl.java?0.00%?
    ListenRequest.java?10.66%?
    ListenRequestOrBuilder.java?0.00%?
    ListenResponse.java?32.64%?
    ListenResponseOrBuilder.java?0.00%?
    ListenSequence.java?100.00%?
    LoadBundleTask.java?25.26%?
    LoadBundleTaskProgress.java?60.47%?
    LocalDocumentsResult.java?100.00%?
    LocalDocumentsView.java?100.00%?
    LocalSerializer.java?97.35%?
    LocalStore.java?99.37%?
    LocalViewChanges.java?100.00%?
    Logger.java?75.00%?
    LogicUtils.java?96.67%?
    LruDelegate.java?0.00%?
    LruGarbageCollector.java?93.46%?
    MapValue.java?53.00%?
    MapValueOrBuilder.java?0.00%?
    MemoryBundleCache.java?100.00%?
    MemoryComponentProvider.java?100.00%?
    MemoryDocumentOverlayCache.java?100.00%?
    MemoryEagerReferenceDelegate.java?100.00%?
    MemoryIndexManager.java?68.97%?
    MemoryLruReferenceDelegate.java?96.00%?
    MemoryMutationQueue.java?98.57%?
    MemoryOverlayMigrationManager.java?100.00%?
    MemoryPersistence.java?100.00%?
    MemoryRemoteDocumentCache.java?98.21%?
    MemoryTargetCache.java?100.00%?
    MetadataChanges.java?100.00%?
    MutableDocument.java?100.00%?
    Mutation.java?98.48%?
    MutationBatch.java?89.06%?
    MutationBatchResult.java?100.00%?
    MutationQueue.java?0.00%?
    MutationResult.java?100.00%?
    NamedQuery.java?0.00%?
    NamedQueryOrBuilder.java?0.00%?
    NotInFilter.java?85.71%?
    NumberComparisonHelper.java?100.00%?
    NumericIncrementTransformOperation.java?87.50%?
    ObjectValue.java?99.06%?
    OnlineState.java?100.00%?
    OnlineStateTracker.java?98.11%?
    OnProgressListener.java?0.00%?
    OrderBy.java?96.55%?
    OrderedCodeWriter.java?68.52%?
    Overlay.java?100.00%?
    OverlayedDocument.java?100.00%?
    OverlayMigrationManager.java?0.00%?
    PatchMutation.java?100.00%?
    Persistence.java?100.00%?
    Precondition.java?37.82%?
    PreconditionOrBuilder.java?0.00%?
    Preconditions.java?57.89%?
    PropertyName.java?0.00%?
    Query.java?3.97%?
    QueryDocumentSnapshot.java?64.71%?
    QueryEngine.java?98.63%?
    QueryListener.java?100.00%?
    QueryProto.java?0.00%?
    QueryPurpose.java?100.00%?
    QueryResult.java?100.00%?
    QuerySnapshot.java?76.36%?
    QueryView.java?100.00%?
    ReferenceDelegate.java?0.00%?
    ReferenceSet.java?92.45%?
    RemoteDocumentCache.java?0.00%?
    RemoteEvent.java?92.31%?
    RemoteSerializer.java?84.63%?
    RemoteStore.java?88.49%?
    ResourcePath.java?94.74%?
    RollbackRequest.java?0.00%?
    RollbackRequestOrBuilder.java?0.00%?
    RunAggregationQueryRequest.java?0.00%?
    RunAggregationQueryRequestOrBuilder.java?0.00%?
    RunAggregationQueryResponse.java?0.00%?
    RunAggregationQueryResponseOrBuilder.java?0.00%?
    RunQueryRequest.java?0.00%?
    RunQueryRequestOrBuilder.java?0.00%?
    RunQueryResponse.java?0.00%?
    RunQueryResponseOrBuilder.java?0.00%?
    Scheduler.java?0.00%?
    ServerTimestamp.java?0.00%?
    ServerTimestampOperation.java?100.00%?
    ServerTimestamps.java?79.17%?
    SetMutation.java?97.22%?
    SetOptions.java?0.00%?
    SnapshotMetadata.java?68.75%?
    SnapshotVersion.java?87.50%?
    Source.java?0.00%?
    SQLiteBundleCache.java?90.00%?
    SQLiteComponentProvider.java?100.00%?
    SQLiteDocumentOverlayCache.java?97.85%?
    SQLiteIndexManager.java?99.48%?
    SQLiteLruReferenceDelegate.java?98.67%?
    SQLiteMutationQueue.java?82.76%?
    SQLiteOverlayMigrationManager.java?90.48%?
    SQLitePersistence.java?86.34%?
    SQLiteRemoteDocumentCache.java?98.17%?
    SQLiteSchema.java?96.63%?
    SQLiteTargetCache.java?98.47%?
    Stream.java?100.00%?
    StructuredAggregationQuery.java?0.00%?
    StructuredAggregationQueryOrBuilder.java?0.00%?
    StructuredQuery.java?32.54%?
    StructuredQueryOrBuilder.java?0.00%?
    Supplier.java?0.00%?
    SyncEngine.java?93.31%?
    Target.java?95.79%?
    TargetCache.java?0.00%?
    TargetChange.java?80.00%?
    TargetChangeOrBuilder.java?0.00%?
    TargetData.java?77.50%?
    TargetIdGenerator.java?100.00%?
    TargetIndexMatcher.java?100.00%?
    TargetOrBuilder.java?0.00%?
    TargetState.java?97.78%?
    ThrottledForwardingExecutor.java?100.00%?
    ThrowOnExtraProperties.java?0.00%?
    Timestamp.java?80.39%?
    Token.java?0.00%?
    Transaction.java?0.00%?
    TransactionOptions.java?0.00%?
    TransactionOptionsOrBuilder.java?0.00%?
    TransactionRunner.java?0.00%?
    TransformOperation.java?0.00%?
    UpdateDocumentRequest.java?0.00%?
    UpdateDocumentRequestOrBuilder.java?0.00%?
    User.java?78.57%?
    UserData.java?66.00%?
    UserDataReader.java?70.48%?
    UserDataWriter.java?51.02%?
    Util.java?67.63%?
    Value.java?40.97%?
    ValueOrBuilder.java?0.00%?
    Values.java?95.65%?
    VerifyMutation.java?46.15%?
    View.java?96.82%?
    ViewChange.java?100.00%?
    ViewSnapshot.java?89.23%?
    WatchChange.java?70.51%?
    WatchChangeAggregator.java?98.21%?
    WatchStream.java?19.23%?
    Write.java?36.11%?
    WriteBatch.java?0.00%?
    WriteOrBuilder.java?0.00%?
    WriteProto.java?0.00%?
    WriteRequest.java?10.41%?
    WriteRequestOrBuilder.java?0.00%?
    WriteResponse.java?11.70%?
    WriteResponseOrBuilder.java?0.00%?
    WriteResult.java?0.00%?
    WriteResultOrBuilder.java?0.00%?
    WriteStream.java?31.82%?

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/WUs6DI9vV1.html

@github-actions
Copy link
Contributor

github-actions bot commented Oct 28, 2022 •
edited
Loading

Unit Test Results

1 109 tests  +1 103   1 093 ✔️ +1 087   48s ⏱️ +31s
     79 suites +     76        16 💤 +     16 
     79 files   +     76          0 ❌ ±       0 

Results for commit 7e97f34. ± Comparison against base commit 854cba5.

This pull request removes 6 and adds 1109 tests. Note that renamed tests count towards both. 
com.google.firebase.functions.FunctionsRegistrarTest ‑ getComponents_publishesLibVersionComponent
com.google.firebase.functions.ktx.FunctionsTests ‑ Firebase#functions should delegate to FirebaseFunctions#getInstance(FirebaseApp, region)
com.google.firebase.functions.ktx.FunctionsTests ‑ Firebase#functions should delegate to FirebaseFunctions#getInstance(region)
com.google.firebase.functions.ktx.FunctionsTests ‑ FirebaseApp#functions should delegate to FirebaseFunctions#getInstance(FirebaseApp)
com.google.firebase.functions.ktx.FunctionsTests ‑ functions should delegate to FirebaseFunctions#getInstance()
com.google.firebase.functions.ktx.LibraryVersionTest ‑ library version should be registered with runtime

com.google.firebase.TimestampTest ‑ testCompare
com.google.firebase.TimestampTest ‑ testFromDate
com.google.firebase.TimestampTest ‑ testRejectBadDates
com.google.firebase.TimestampTest ‑ testTimestampParcelable
com.google.firebase.firestore.AggregateQueryTest ‑ testSourceMustNotBeNull
com.google.firebase.firestore.BlobTest ‑ testComparison
com.google.firebase.firestore.BlobTest ‑ testEquals
com.google.firebase.firestore.BlobTest ‑ testMutableBytes
com.google.firebase.firestore.CollectionReferenceTest ‑ testEquals
com.google.firebase.firestore.DocumentChangeTest ‑ randomTests
…

♻️ This comment has been updated with latest results.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 28, 2022 •
edited
Loading

Size Report 1

Affected Products

  • firebase-firestore

    TypeBase (ffca832)Merge (22e2d15)Diff
    aar1.30 MB1.30 MB+61 B (+0.0%)
    apk (release)3.32 MB3.32 MB+108 B (+0.0%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/IAvlierdF2.html

@milaGGL milaGGL removed the WIP label Oct 28, 2022
@milaGGL milaGGL requested a review from dconeybe October 28, 2022 22:36
dconeybe
dconeybe requested changes Oct 30, 2022
View reviewed changes
firebase-firestore/CHANGELOG.md Outdated Show resolved Hide resolved
firebase-firestore/CHANGELOG.md Outdated Show resolved Hide resolved
firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java Outdated Show resolved Hide resolved
firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java Outdated Show resolved Hide resolved
firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java Outdated Show resolved Hide resolved
firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java Outdated Show resolved Hide resolved
firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java Outdated Show resolved Hide resolved
@milaGGL milaGGL requested a review from dconeybe October 31, 2022 21:30
dconeybe
dconeybe requested changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more minor suggestions. Looks great!

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java Outdated Show resolved Hide resolved
firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java Outdated Show resolved Hide resolved
firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java Outdated Show resolved Hide resolved
firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java Outdated Show resolved Hide resolved
@dconeybe
Copy link
Contributor

dconeybe commented Nov 1, 2022

Oh also, please remove Fixes #5871 from the PR description, since it does not, in fact, fix an issue in the other repository. And add a link to the PRs (there are two of them) that are being ported in this PR.

@dconeybe dconeybe mentioned this pull request Nov 1, 2022
Merged
@milaGGL milaGGL requested a review from dconeybe November 1, 2022 19:16
dconeybe
dconeybe approved these changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it!

@milaGGL milaGGL merged commit a0fcca1 into master Nov 1, 2022
@milaGGL milaGGL deleted the mila/fix-transaction-set-error-on-deleted-doc branch November 1, 2022 19:41
@firebase firebase locked and limited conversation to collaborators Dec 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dconeybe dconeybe dconeybe approved these changes

Assignees

@milaGGL milaGGL

Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@milaGGL @google-oss-bot @dconeybe