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(Firestore): SnapshotTrait getSnapshot method #5807

Merged
merged 4 commits into from
Jan 25, 2023
Merged

fix(Firestore): SnapshotTrait getSnapshot method #5807

merged 4 commits into from
Jan 25, 2023

Conversation

yash30201
Copy link
Contributor

@yash30201 yash30201 commented Jan 23, 2023 •
edited
Loading

Fixed #5806

Changes

  • Decoded the API format timestamp received in batchGetDocument() method to Google\Protobuf\Timestamp using Serialize:decodeMessage().
  • Added system and unit test for this new change.

@yash30201
fix(Firestore): SnapshotTrait getSnapshot method
48ec5c1 
* Fixed Timestamp inconsistency with backend.
@yash30201
fix(Firestore): SnapshotTrait getSnapshot method
41fb48f 
* Added unit test for verifying the fix
@yash30201
fix(Firestore): SnapshotTrait getSnapshot method
48e27a1 
* Added system test for DocumentReference::snapshot() method with readTime optional parameter in the $args options array
@product-auto-label product-auto-label bot added the api: firestore Issues related to the Firestore API. label Jan 23, 2023
@yash30201 yash30201 marked this pull request as ready for review January 23, 2023 10:35
@yash30201 yash30201 requested review from a team as code owners January 23, 2023 10:35
vishwarajanand
vishwarajanand approved these changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@vishwarajanand vishwarajanand left a comment

Choose a reason for hiding this comment

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

LGTM, but pls make sure the feature is available in PROD before merging to main.

@yash30201
Copy link
Contributor Author

@vishwarajanand

LGTM, but pls make sure the feature is available in PROD before merging to main.

Talked internally and found that reading within the GC window using readTime optional parameter is already a GA feature. So fixing this right now won't be a problem.

@yash30201 yash30201 merged commit cfc8fef into main Jan 25, 2023
@yash30201 yash30201 deleted the firestore-snapshot-trait-bug-fix-1 branch January 25, 2023 08:29
@release-please release-please bot mentioned this pull request Jan 25, 2023
Merged
@vishwarajanand vishwarajanand mentioned this pull request Jan 27, 2023
Merged
@yash30201 yash30201 mentioned this pull request Feb 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bshaffer bshaffer bshaffer left review comments

@vishwarajanand vishwarajanand vishwarajanand approved these changes

Assignees

@vishwarajanand vishwarajanand

Labels
api: firestore Issues related to the Firestore API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Firestore SnapshotTrait::getSnapshot() Timestamp inconsistency
3 participants
@yash30201 @bshaffer @vishwarajanand