-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
[cloud_firestore] Fix DocumentReference comparison bug. #2524
Conversation
/cc @bparrishMines @collinjackson not sure who I should reference for this simple ticket. Can you have a look and tell me where the tests should go? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slightfoot The code looks good to me. It only lacks lacks a test and updating the pubspec.yaml
and CHANGELOG
. It looks like tests were placed in flutterfire/packages/cloud_firestore/cloud_firestore/example/test_driver/
for this plugin.
/// Gets the instance of Firestore for the default Firebase app. | ||
static Firestore get instance => Firestore(); | ||
static Firestore get instance => _instance ??= Firestore(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems redundant for me because Firestore()
is practically singleton.
static Firestore get instance
calls these:
- https://github.com/FirebaseExtended/flutterfire/blob/5e380cf85fde7eefa91d520bdb3ae0bfeb2d1282/packages/cloud_firestore/cloud_firestore/lib/src/firestore.dart#L26
- https://github.com/FirebaseExtended/flutterfire/blob/5e380cf85fde7eefa91d520bdb3ae0bfeb2d1282/packages/cloud_firestore/cloud_firestore_platform_interface/lib/cloud_firestore_platform_interface.dart#L61-L66
- This is singleton
@@ -98,4 +101,14 @@ class Firestore { | |||
host: host, | |||
sslEnabled: sslEnabled, | |||
cacheSizeBytes: cacheSizeBytes); | |||
|
|||
@override | |||
int get hashCode => _delegate.app.name.hashCode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int get hashCode => _delegate.app.name.hashCode; | |
int get hashCode => _delegate.app.hashCode; |
The hashCode
of FirebaseApp
is implemented appropriately, so .name
seems redundant.
Glad this is finally getting attention! i've literally had to wrap |
Description
I was just caught out by this.
You expect two
DocumentReferences
to be equal. https://github.com/FirebaseExtended/flutterfire/blob/master/packages/cloud_firestore/cloud_firestore/lib/src/document_reference.dart#L25 shows this to be the case if the firestore instance is the same and the instance getter above does not return a singleton instead it creates a new instance each time.This fix corrects this by firstly treating the
Firestore.instance
field as a singleton, and second adding a equality method that now compares the underlying app. Which now means even if you have multiple instances of the same Firestore class they will be considered equal.Tests
cloud_firestore
package does not contain tests.Checklist
///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?