Firestore `isEqual()` on Public Types #569

zxu123 · 2017-12-14T20:05:55Z

Discussion

See b/62143881
Also see inspiration on http://github.com/firebase/firebase-js-sdk/issues/202

Testing

Adds unit test for all relevant isEqual(), either already implemented in master or in this branch
All unit tests and integration tests pass.

API Changes

See proposal go/firestoreisequal

* add bone code for `FIRCollectionReference`, `FIRDocumentSnapshot`, `FIRFieldValue`, `FIRQuerySnapshot`, `FIRSnapshotMetadata`; * change inconsistenciness of `FIRFieldPath.isEqual` implementation; * add unit test (and file) for `FIRDocumentReferenceTest.m`, `FIRFieldPathTest.m`, `FIRQueryTests.testEquals`; `FIRGeoPoint` already has test and Blob is internal type.

adding the working code and unit test.

Since `FIRFieldValue` both types are singleton, we do not need override `isEqual`. Add test to test the default `NSObject` `isEqual` works just fine.

mikelehen · 2017-12-14T20:55:09Z

I think I may be able to get to this before Gil. :-)

mikelehen

This looks like a good start. I left some miscellaneous tips / suggestions, but the two high-level points:

I think we need to implement hash as well as isEqual.
I'd like to clean up the tests to reduce boilerplate. I think we can probably do that by using existing helpers and creating new ones as necessary.

mikelehen · 2017-12-14T22:50:08Z

Firestore/Source/API/FIRDocumentSnapshot.m

+// NSObject Methods
+- (BOOL)isEqual:(nullable id)other {
+  if (other == self) return YES;
+  if (!other || ![[other class] isEqual:[self class]]) return NO;


Note that this won't work for the FIRQueryDocumentSnapshot class that @schmidt-sebastian is adding. So we'll need to implement isEqual on it as well, or relax this to an isKindOfClass check.

mikelehen · 2017-12-14T22:53:06Z

Firestore/Source/API/FIRFieldPath.m

  }

-  if (![object isKindOfClass:[FIRFieldPath class]]) {
+  if (!object || ![object isKindOfClass:[FIRFieldPath class]]) {


In Obj-C, nil checks like this are not required nor recommended: http://google.github.io/styleguide/objcguide.html#nil-checks

mikelehen · 2017-12-14T22:55:16Z

Firestore/Source/API/FIRDocumentSnapshot.m

+- (BOOL)isEqualToSnapshot:(nullable FIRDocumentSnapshot *)snapshot {
+  if (self == snapshot) return YES;
+  if (snapshot == nil) return NO;
+  if (self.firestore != snapshot.firestore && ![self.firestore isEqual:snapshot.firestore])


I'm not sure if you're doing reference equality checks before isEqual equality checks as a performance improvement, or perhaps to deal with nil values, but I would drop them since:

These sorts of nil checks aren't necessary nor required: http://google.github.io/styleguide/objcguide.html#nil-checks

For performance we can instead make the isEqual implementation short-circuit on reference equality.

As an aside, some people on the team (Gil and I for sure) use AppCode for iOS development, which can automatically generate isEqual and hash implementations for you.

Let's ignore other properties for now. For the four combination of .firestore.

case 01) self.firestore == nil, snapshot.firestore != nil, should return NO;

case 00) self.firestore == nil, snapshot.firestore == nil, should return YES;

case 11) self.firestore != nil, snapshot.firestore != nil, should return whether self.firestore == snapshot.firestore;

case 10) self.firestore != nil, snapshot.firestore == nil, should return NO;

As far as self.firestore != nil and FIRFirestore:isEqual implemented correctly, [self.firestore isEqual:snapshot.firestore] is enough to get the result;
As soon as self.firestore == nil, [self.firestore isEqual:snapshot.firestore] will always returns nil and we cannot tell apart 01 and 00 cases. Additional self.firestore != snapshot.firestore helps in this case. (and as side effect, may or may not improve performance by checking reference.)

Thanks for walking me through this. Since firestore is non-nullable I think only case 11 can actually happen, but on the other hand I just checked and AppCode generates code very similar to yours. And it's probably better to follow a consistent pattern for nullable and non-nullable properties. So carry on! Thanks / sorry.

mikelehen · 2017-12-14T23:01:54Z

Firestore/Source/API/FIRDocumentSnapshot.m

+    return NO;
+  if (self.fromCache != snapshot.fromCache) return NO;
+  return YES;
+}


I think we forgot to call this out explicitly in the bug / proposal, but by overriding isEqual, we also need to override hash (for justification, see this SO answer which is for java but the same argument applies).

FYI- Since JavaScript has no standardized equals / hashCode methods, we don't need to worry about it.

mikelehen · 2017-12-14T23:30:56Z

Firestore/Example/Tests/API/FIRCollectionReferenceTests.m

+                                                     persistenceKey:@"db123"
+                                                credentialsProvider:nil
+                                                workerDispatchQueue:nil
+                                                        firebaseApp:nil];


Can you define a helper rather than repeating this (e.g. in https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/Example/Tests/Util/FSTHelpers.h)?

mikelehen · 2017-12-15T00:02:40Z

Firestore/Example/Tests/API/FIRDocumentSnapshotTests.m

+                                                        firebaseApp:nil];
+  FSTDocumentKey *keyFoo = [FSTDocumentKey keyWithPathString:@"rooms/foo"];
+  FSTDocumentKey *keyBar = [FSTDocumentKey keyWithPathString:@"rooms/bar"];
+  FSTObjectValue *dateFoo = FSTTestObjectValue(@{ @"a" : @1 });


date => data I think

mikelehen · 2017-12-15T00:03:27Z

Firestore/Example/Tests/API/FIRDocumentSnapshotTests.m

+  XCTAssertEqualObjects([FIRDocumentSnapshot snapshotWithFirestore:firestore
+                                                       documentKey:keyFoo
+                                                          document:nil
+                                                         fromCache:YES],


Oh boy. This really turns into a giant wall of text. Can we create a helper to reduce this?

mikelehen · 2017-12-15T00:04:38Z

Firestore/Example/Tests/API/FIRFieldPathTests.m

+@implementation FIRFieldPathTests
+
+- (void)testEquals {
+  FSTFieldPath *pathFoo = [FSTFieldPath pathWithServerFormat:@"foo.ooo.oooo"];


We have a helper for this: https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/Example/Tests/Util/FSTHelpers.h#L158

mikelehen · 2017-12-15T00:05:23Z

Firestore/Example/Tests/API/FIRQuerySnapshotTests.m

+                           [FIRQuerySnapshot snapshotWithFirestore:firestore
+                                                     originalQuery:queryFoo
+                                                          snapshot:snapshotFoo
+                                                          metadata:metadataBar]);


Another giant wall of text. Can we improve with a helper of some kind?

mikelehen · 2017-12-15T00:05:51Z

Firestore/Example/Tests/API/FIRQuerySnapshotTests.m

+                                                        firebaseApp:nil];
+  FSTResourcePath *pathFoo = [FSTResourcePath pathWithString:@"foo"];
+  FSTResourcePath *pathBar = [FSTResourcePath pathWithString:@"bar"];
+  FSTQuery *queryFoo = [FSTQuery queryWithPath:pathFoo];


Helper for this. :-) https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/Example/Tests/Util/FSTHelpers.h#L196

…h, FSTTestDocKey

…`FSTTestQuery`, `FSTTestDoc`

mikelehen

Thanks! I appreciate you doing the cleanup I suggested (and finding/fixing existing places that needed the same cleanup!). I have a bit more cleanup to ask for (sorry!), but you can push back if you feel I'm going overboard. I do like to minimize boilerplate and maximize clarity in our tests though. And I have a few other miscellaneous comments / suggestions.

mikelehen · 2017-12-18T16:01:47Z

Firestore/Example/Tests/API/FIRCollectionReferenceTests.m

+
+- (void)testEquals {
+  // Everything is dummy for unit test here. Filtering does not require any app
+  // specific setting as far as we do not fetch data.


I think this comment should go inside FSTTestFirestore() rather than on every usage of the method. :-)

Done. That's some leftover comment I forgot to remove.

mikelehen · 2017-12-18T16:09:08Z

Firestore/Example/Tests/API/FIRDocumentSnapshotTests.m

+  XCTAssertNotEqualObjects(FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"a" : @1 }, NO, NO),
+                           FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"b" : @1 }, NO, NO));
+  XCTAssertNotEqualObjects(FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"a" : @1 }, YES, NO),
+                           FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"b" : @1 }, NO, NO));


These differ in both the data and metadata. Intentional?

mikelehen · 2017-12-18T16:09:15Z

Firestore/Example/Tests/API/FIRDocumentSnapshotTests.m

+  XCTAssertNotEqualObjects(FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"a" : @1 }, YES, NO),
+                           FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"b" : @1 }, NO, NO));
+  XCTAssertNotEqualObjects(FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"a" : @1 }, NO, YES),
+                           FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"b" : @1 }, NO, NO));


These differ in both the data and metadata. Intentional?

mikelehen · 2017-12-18T16:10:07Z

Firestore/Example/Tests/API/FIRDocumentSnapshotTests.m

+  XCTAssertEqualObjects(FSTTestDocSnapshot(@"rooms/bar", 1, @{ @"a" : @1 }, NO, NO),
+                        FSTTestDocSnapshot(@"rooms/bar", 1, @{ @"a" : @1 }, NO, NO));
+  XCTAssertNotEqualObjects(FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"a" : @1 }, NO, NO),
+                           FSTTestDocSnapshot(@"rooms/bar", 1, @{ @"a" : @1 }, NO, NO));


I don't think these are clang-formatted. Can you run scripts/style.sh on your change and commit the relevant changes (unfortunately there are a few unrelated formatting issues in the codebase right now which we need to get cleaned up separately).

mikelehen · 2017-12-18T16:18:59Z

Firestore/Example/Tests/API/FIRDocumentSnapshotTests.m

+  XCTAssertNotEqual([FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"a" : @1 }, YES, NO) hash],
+                    [FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"b" : @1 }, NO, NO) hash]);
+  XCTAssertNotEqual([FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"a" : @1 }, NO, YES) hash],
+                    [FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"b" : @1 }, NO, NO) hash]);


EDIT: After reviewing further, I see that you've used FSTAssertEqualityGroups() elsewhere, so I'd recommend using it here too!

I'm not a fan of all the repetition. It's hard to tell what's being tested and verify there aren't mistakes. Two possible suggestions:

We actually have an FSTAssertEqualityGroups helper macro that takes a lists of "equality groups" (one or more objects that should compare equal) and verifies that only objects in the same equality group compare equal with each other. For an example usage, see here. It also tests the hash implementation, though it only verifies hashes that should be equal are equal. It doesn't test for hashes not being equal, since hash collisions are okay (and unavoidable in the general case).

Perhaps create some useful snapshots up front (base, baseDup, nilData, nilDataDup, differentPath, differentData, hasMutations, fromCache, etc.?) and then re-use those in all of the equals / hash assertions? Then it's easy to verify that the snapshot you create matches the named variable, and it's easy to verify that the equals / not-equals assertions you do make sense for the named values.

I would probably do 1 or 2, but not both, and I leave it up to you what to choose. If you do 1, please do it in all of these tests for consistency. 2 is fairly consistent with what you're doing in the other tests already.

done with method 2

mikelehen · 2017-12-18T17:57:03Z

Firestore/Source/API/FIRDocumentSnapshot.m

+- (BOOL)isEqualToSnapshot:(nullable FIRDocumentSnapshot *)snapshot {
+  if (self == snapshot) return YES;
+  if (snapshot == nil) return NO;
+  if (self.firestore != snapshot.firestore && ![self.firestore isEqual:snapshot.firestore])


Thanks for walking me through this. Since firestore is non-nullable I think only case 11 can actually happen, but on the other hand I just checked and AppCode generates code very similar to yours. And it's probably better to follow a consistent pattern for nullable and non-nullable properties. So carry on! Thanks / sorry.

mikelehen · 2017-12-18T18:04:50Z

Firestore/Source/API/FIRDocumentSnapshot.m

+  if (self.internalKey != snapshot.internalKey && ![self.internalKey isEqual:snapshot.internalKey])
+    return NO;
+  if (self.internalDocument != snapshot.internalDocument &&
+      (!self.internalDocument || ![self.internalDocument isEqual:snapshot.internalDocument]))


You don't need the !self.internalDocument check do you? nil inequality will be caught by the first check as per your explanation above. :-)

Yeah, the check is redundant. Removed.

mikelehen · 2017-12-18T18:15:00Z

Firestore/Source/API/FIRQuerySnapshot.m

+- (BOOL)isEqualToSnapshot:(nullable FIRQuerySnapshot *)snapshot {
+  if (self == snapshot) return YES;
+  if (snapshot == nil) return NO;
+  if (self.firestore != snapshot.firestore && ![self.firestore isEqual:snapshot.firestore])


shouldn't these be ||s ?

No, should be &&. Check the case-analysis above (nil v.s. non-nil, non-nil v.s. non-nil). Another way to look at this is to use double-negation of it. Say, self.firestore is equal iff the reference is equal OR the content is equal. So self.firestore is not equal (case here to return NO) iff the reference is not equal AND the content is not equal.

Er, sorry I keep trying to lead you astray. For some reason when I was looking at this I was thinking of the equals case instead of the not-equals case. Carry on!

mikelehen · 2017-12-18T18:24:00Z

Firestore/Source/API/FIRQuery.m

 - (BOOL)isEqual:(nullable id)other {
  if (other == self) return YES;
-  if (!other || ![[other class] isEqual:[self class]]) return NO;
+  if (![[other class] isEqual:[self class]]) return NO;


If you haven't already, can you just double-check that this still works? I'm a tiny bit nervous because AppCode auto-generates these methods with the "!other" check, and I don't know why.

Still works, see logic given below:

The only difference happens when other is nil. That case one return NO immediately while the other will check ![[other class] isEqual:[self class]], which is equivalent to ![nil isEqual:[self class]], which is equivalent to !nil, which makes the method return NO as well. So !other check is redundant.

I have no particular preference on one over the other. So I can change it back if you think with !other check is easier to understand.

mikelehen · 2017-12-18T18:24:58Z

Firestore/Source/Core/FSTView.m

+  NSUInteger hash = self.type;
+  hash = hash * 31u + [self.key hash];
+  return hash;
+}


Ooh! Good catch.

…nt}Ref`, better naming and style fix

zxu123

Updated.

zxu123 · 2017-12-18T19:05:49Z

Firestore/Example/Tests/API/FIRCollectionReferenceTests.m

+
+- (void)testEquals {
+  // Everything is dummy for unit test here. Filtering does not require any app
+  // specific setting as far as we do not fetch data.


Done. That's some leftover comment I forgot to remove.

zxu123 · 2017-12-18T19:22:58Z

Firestore/Example/Tests/API/FIRCollectionReferenceTests.m

+  FIRCollectionReference *referenceFooDup =
+      [FIRCollectionReference referenceWithPath:pathFoo firestore:firestore];
+  FIRCollectionReference *referenceBar =
+      [FIRCollectionReference referenceWithPath:pathBar firestore:firestore];


zxu123 · 2017-12-18T19:30:14Z

Firestore/Example/Tests/API/FIRDocumentSnapshotTests.m

+  XCTAssertEqualObjects(FSTTestDocSnapshot(@"rooms/bar", 1, @{ @"a" : @1 }, NO, NO),
+                        FSTTestDocSnapshot(@"rooms/bar", 1, @{ @"a" : @1 }, NO, NO));
+  XCTAssertNotEqualObjects(FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"a" : @1 }, NO, NO),
+                           FSTTestDocSnapshot(@"rooms/bar", 1, @{ @"a" : @1 }, NO, NO));


zxu123 · 2017-12-18T19:33:23Z

Firestore/Example/Tests/API/FIRDocumentSnapshotTests.m

+  XCTAssertNotEqualObjects(FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"a" : @1 }, NO, NO),
+                           FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"b" : @1 }, NO, NO));
+  XCTAssertNotEqualObjects(FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"a" : @1 }, YES, NO),
+                           FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"b" : @1 }, NO, NO));


zxu123 · 2017-12-18T19:33:31Z

Firestore/Example/Tests/API/FIRDocumentSnapshotTests.m

+  XCTAssertNotEqualObjects(FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"a" : @1 }, YES, NO),
+                           FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"b" : @1 }, NO, NO));
+  XCTAssertNotEqualObjects(FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"a" : @1 }, NO, YES),
+                           FSTTestDocSnapshot(@"rooms/foo", 1, @{ @"b" : @1 }, NO, NO));


zxu123 · 2017-12-18T21:08:43Z

Firestore/Example/Tests/Util/FSTHelpers.h

 /** A convenience method for creating a query for the given path (without any other filters). */
 FSTQuery *FSTTestQuery(NSString *path);

+/** A convenience method for creating a particular query snapshots for tests. */


zxu123 · 2017-12-18T21:20:22Z

Firestore/Example/Tests/Util/FSTHelpers.m

+                                                                               fromCache:fromCache];
+  FSTDocumentSet *oldDocuments = FSTTestDocSet(FSTDocumentComparatorByKey, @[]);
+  for (NSDictionary<NSString *, id> *data in oldData) {
+    oldDocuments = [oldDocuments documentSetByAddingDocument:FSTTestDoc(path, 1, data, hasPendingWrites)];


zxu123 · 2017-12-18T21:28:40Z

Firestore/Source/API/FIRQuery.m

 - (BOOL)isEqual:(nullable id)other {
  if (other == self) return YES;
-  if (!other || ![[other class] isEqual:[self class]]) return NO;
+  if (![[other class] isEqual:[self class]]) return NO;


Still works, see logic given below:

The only difference happens when other is nil. That case one return NO immediately while the other will check ![[other class] isEqual:[self class]], which is equivalent to ![nil isEqual:[self class]], which is equivalent to !nil, which makes the method return NO as well. So !other check is redundant.

I have no particular preference on one over the other. So I can change it back if you think with !other check is easier to understand.

zxu123 · 2017-12-18T21:39:40Z

Firestore/Source/API/FIRDocumentSnapshot.m

+  if (self.internalKey != snapshot.internalKey && ![self.internalKey isEqual:snapshot.internalKey])
+    return NO;
+  if (self.internalDocument != snapshot.internalDocument &&
+      (!self.internalDocument || ![self.internalDocument isEqual:snapshot.internalDocument]))


Yeah, the check is redundant. Removed.

zxu123 · 2017-12-18T21:45:20Z

Firestore/Source/API/FIRQuerySnapshot.m

+- (BOOL)isEqualToSnapshot:(nullable FIRQuerySnapshot *)snapshot {
+  if (self == snapshot) return YES;
+  if (snapshot == nil) return NO;
+  if (self.firestore != snapshot.firestore && ![self.firestore isEqual:snapshot.firestore])


No, should be &&. Check the case-analysis above (nil v.s. non-nil, non-nil v.s. non-nil). Another way to look at this is to use double-negation of it. Say, self.firestore is equal iff the reference is equal OR the content is equal. So self.firestore is not equal (case here to return NO) iff the reference is not equal AND the content is not equal.

mikelehen

Thanks for the changes! Found a few remaining inconsistencies, etc. but I think this is pretty close. :-)

mikelehen · 2017-12-18T23:59:43Z

Firestore/Example/Firestore.xcodeproj/project.pbxproj

 		AB99452E1FE30AC800DFC1E6 /* FIRFieldValueTests.m in Sources */ = {isa = PBXBuildFile; fileRef = AB99452D1FE30AC800DFC1E6 /* FIRFieldValueTests.m */; };
 		ABAEEF4F1FD5F8B100C966CB /* FIRQueryTests.m in Sources */ = {isa = PBXBuildFile; fileRef = ABAEEF4E1FD5F8B100C966CB /* FIRQueryTests.m */; };
+		ABF341051FE860CA00C48322 /* FSTAPIHelpers.m in Sources */ = {isa = PBXBuildFile; fileRef = ABF341021FE8593500C48322 /* FSTAPIHelpers.m */; };
+		ABF341061FE860CB00C48322 /* FSTAPIHelpers.m in Sources */ = {isa = PBXBuildFile; fileRef = ABF341021FE8593500C48322 /* FSTAPIHelpers.m */; };


I have trouble reading xcode proj files, but I think perhaps you added FSTAPIHelpers.m to both the Firestore_IntegrationTests target and the Firestore_Tests target, but it should only be in the latter.

mikelehen · 2017-12-19T00:04:55Z

Firestore/Example/Tests/API/FSTAPIHelpers.m

+#import "Firestore/Source/Model/FSTPath.h"
+#import "Firestore/Source/Remote/FSTRemoteEvent.h"
+#import "Firestore/Source/Remote/FSTWatchChange.h"
+#import "Firestore/Source/Util/FSTAssert.h"


Most of these imports are unused. Per AppCode, this is the set you need:

#import "Firestore/Example/Tests/API/FSTAPIHelpers.h" #import <Firestore/Source/API/FIRDocumentSnapshot+Internal.h> #import <Firestore/Source/API/FIRQuerySnapshot+Internal.h> #import <Firestore/Source/API/FIRSnapshotMetadata+Internal.h> #import <Firestore/Source/Core/FSTViewSnapshot.h> #import "FirebaseFirestore/FIRCollectionReference.h" #import "FirebaseFirestore/FIRDocumentReference.h" #import "FirebaseFirestore/FIRDocumentSnapshot.h" #import "FirebaseFirestore/FIRFirestore.h" #import "FirebaseFirestore/FIRGeoPoint.h" #import "FirebaseFirestore/FIRQuerySnapshot.h" #import "FirebaseFirestore/FIRSnapshotMetadata.h" #import "Firestore/Source/API/FIRCollectionReference+Internal.h" #import "Firestore/Source/API/FIRFirestore+Internal.h" #import "Firestore/Source/Core/FSTQuery.h" #import "Firestore/Source/Model/FSTDocument.h" #import "Firestore/Source/Model/FSTDocumentKey.h" #import "Firestore/Source/Model/FSTDocumentSet.h" #import "Firestore/Source/Model/FSTPath.h"

done, also clean-up more and make style consistent (<> vs "")

mikelehen · 2017-12-19T00:10:06Z

Firestore/Example/Tests/API/FIRGeoPointTests.m

+  FIRGeoPoint *foo = FSTTestGeoPoint(0, 0);
+  FIRGeoPoint *fooDup = FSTTestGeoPoint(0, 0);
+  FIRGeoPoint *bar = FSTTestGeoPoint(1.23, 4.56);
+  FIRGeoPoint *barDup = FSTTestGeoPoint(1.23, 4.56);


I'm not sure bar / barDup is adding anything to these tests. You do the same equals checks as with foo / fooDup. If you want non-zero values, I'd just change foo to be non-zero values.

mikelehen · 2017-12-19T00:23:03Z

Firestore/Example/Tests/API/FIRGeoPointTests.m

+  XCTAssertEqual([bar hash], [barDup hash]);
+  XCTAssertNotEqual([foo hash], [differentLatitude hash]);
+  XCTAssertNotEqual([foo hash], [differentLongitude hash]);
+  XCTAssertNotEqual([foo hash], [object hash]);


Why did you add the object hash test for this test but not others? (I would drop it in general. I'm not sure what it's protecting against)

mikelehen · 2017-12-19T00:31:11Z

Firestore/Example/Tests/API/FSTAPIHelpers.h

+NS_ASSUME_NONNULL_BEGIN
+
+#if __cplusplus
+extern "C" {


I don't believe this is required (usually you need this when you're going to be including the header file from C/C++ code).

For the most part this is going to be required in every Objective-C header as we go so we might as well put these in. It's not worth thinking about whether or not they're required.

ack, will leave as is.

mikelehen · 2017-12-19T00:48:03Z

Firestore/Example/Tests/Model/FSTFieldValueTests.m

 #import "Firestore/Source/Model/FSTFieldValue.h"
 #import "Firestore/Source/Model/FSTPath.h"

+#import "Firestore/Example/Tests/API/FSTAPIHelpers.h"


Hrm... I didn't realize that these tests would end up relying on FSTAPIHelpers.h. That makes it less obvious that my suggested separation makes sense. :-/

I think the difficulty comes from the fact that FIRGeoPoint is used as both a "model" class and an "API" class. I'd suggest moving FSTTestGeoPoint back into FSTHelpers.h/.m and adding a comment something like:

// Note that FIRGeoPoint is a model class in addition to an API class, so we put this helper here instead of FSTAPIHelpers.h

mikelehen · 2017-12-19T00:50:34Z

Firestore/Example/Tests/API/FSTAPIHelpers.h

+@class FSTView;
+@class FSTViewSnapshot;
+@class FSTObjectValue;
+@protocol FSTFilter;


The vast majority of these are unused. Unfortunately AppCode doesn't seem to tell me which ones. Can you take a manual pass through cleaning these up (e.g. delete them all and then re-add as necessary)

mikelehen · 2017-12-19T00:51:39Z

Firestore/Example/Tests/Util/FSTHelpers.h

+@class FIRDocumentSnapshot;
+@class FIRFirestore;
 @class FIRGeoPoint;
+@class FIRQuerySnapshot;


I think all of these added FIR* forward declarations are no longer necessary?

mikelehen · 2017-12-19T00:52:31Z

Firestore/Example/Tests/Util/FSTHelpers.m

 #import "FirebaseFirestore/FIRQuerySnapshot.h"
 #import "FirebaseFirestore/FIRSnapshotMetadata.h"
+#import "Firestore/Source/API/FIRCollectionReference+Internal.h"
+#import "Firestore/Source/API/FIRDocumentReference+Internal.h"


I think the added imports are no longer necessary?

mikelehen · 2017-12-19T00:59:19Z

Firestore/Source/API/FIRQuerySnapshot.m

+- (BOOL)isEqualToSnapshot:(nullable FIRQuerySnapshot *)snapshot {
+  if (self == snapshot) return YES;
+  if (snapshot == nil) return NO;
+  if (self.firestore != snapshot.firestore && ![self.firestore isEqual:snapshot.firestore])


Er, sorry I keep trying to lead you astray. For some reason when I was looking at this I was thinking of the equals case instead of the not-equals case. Carry on!

wilhuff · 2017-12-14T20:47:31Z

Firestore/Example/Tests/API/FIRCollectionReferenceTests.m

+- (void)testEquals {
+  // Everything is dummy for unit test here. Filtering does not require any app
+  // specific setting as far as we do not fetch data.
+  FIRFirestore *firestore = [[FIRFirestore alloc] initWithProjectID:@"abc"


Now that we've repeated this blurb multiple times, could you factor it out into a helper?

wilhuff · 2017-12-19T01:38:01Z

Firestore/Example/Tests/API/FSTAPIHelpers.h

+NS_ASSUME_NONNULL_BEGIN
+
+#if __cplusplus
+extern "C" {


For the most part this is going to be required in every Objective-C header as we go so we might as well put these in. It's not worth thinking about whether or not they're required.

mikelehen

Two tiny nits, but otherwise looks good. Thank you!

mikelehen · 2017-12-19T17:35:38Z

Firestore/Example/Tests/API/FSTAPIHelpers.h

-    NSDictionary<NSString *, NSDictionary<NSString *, id> *> *oldData,
-    NSDictionary<NSString *, NSDictionary<NSString *, id> *> *dataToAdd,
+    NSDictionary<NSString *, NSDictionary<NSString *, id> *> *oldDocs,
+    NSDictionary<NSString *, NSDictionary<NSString *, id> *> *DocsToAdd,


lowercase please. :-) (DocsToAdd=>docsToAdd)

mikelehen · 2017-12-19T17:38:52Z

Firestore/Example/Tests/API/FSTAPIHelpers.h

+ *
+ * @param path To be used in constructing the query.
+ * @param oldDocs Provides data to construct the query snapshot in the past. It maps each key to a
+ * document. The key is the document's path relative to the query path.


nit: "document's path" suggests this could be deeply nested under the query path, but it really must only be a single level deep. I would say:

Provides the prior set of documents in the QuerySnapshot. Each dictionary entry maps to a document, with the key being the document id, and the value being the document contents.

…s-sdk into zxu/isEqual

* add bone code for new isEqual and unit test for old isEqual * add bone code for `FIRCollectionReference`, `FIRDocumentSnapshot`, `FIRFieldValue`, `FIRQuerySnapshot`, `FIRSnapshotMetadata`; * change inconsistenciness of `FIRFieldPath.isEqual` implementation; * add unit test (and file) for `FIRDocumentReferenceTest.m`, `FIRFieldPathTest.m`, `FIRQueryTests.testEquals`; `FIRGeoPoint` already has test and Blob is internal type. * Implement isEqual for FIRCollectionReference adding the working code and unit test. * implement isEqual for FIRSnapshotMetadata * Implement isEqual for FIRDocumentSnapshot * Implement isEqual for FIRQuerySnapshot * (un)implement `isEqual` for `FIRFieldValue` Since `FIRFieldValue` both types are singleton, we do not need override `isEqual`. Add test to test the default `NSObject` `isEqual` works just fine. * fix style with `scripts/style.sh` * Implement hash for those with overridden isEqual without hash yet. * refactor to use test helper functions -- FSTTestFirestore, FSTTestPath, FSTTestDocKey * refactor using test helper `FSTTestDocSnapshot`, `FSTTestFieldPath`, `FSTTestQuery`, `FSTTestDoc` * refactoring to use test helper method `FSTTestQuerySnapshot`, * remove unneccessary nil-check, check isKindOfClass instead of isEqual * refactoring: adding `FSTAPIHelpers.{h,m}`, `FSTTest{Collection,Document}Ref`, better naming and style fix * a file forgot in last commit * mainly clean up import and some minor refactoring * fix style via style.sh * minor style fix * add pragma ignored -Wnonnull

zxu123 added 7 commits December 12, 2017 11:03

Implement isEqual for FIRCollectionReference

0686f88

adding the working code and unit test.

implement isEqual for FIRSnapshotMetadata

a3f3771

Implement isEqual for FIRDocumentSnapshot

9b02f22

Implement isEqual for FIRQuerySnapshot

d40c4c1

(un)implement isEqual for FIRFieldValue

ea5fdd7

Since `FIRFieldValue` both types are singleton, we do not need override `isEqual`. Add test to test the default `NSObject` `isEqual` works just fine.

fix style with scripts/style.sh

0c73729

googlebot added the cla: yes label Dec 14, 2017

zxu123 assigned wilhuff Dec 14, 2017

zxu123 added the api: firestore label Dec 14, 2017

mikelehen assigned mikelehen and unassigned wilhuff Dec 14, 2017

mikelehen suggested changes Dec 15, 2017

View reviewed changes

Implement hash for those with overridden isEqual without hash yet.

4e77490

mikelehen assigned zxu123 and unassigned mikelehen Dec 15, 2017

zxu123 added 4 commits December 15, 2017 12:12

refactor to use test helper functions -- FSTTestFirestore, FSTTestPat…

9a0117a

…h, FSTTestDocKey

refactor using test helper FSTTestDocSnapshot, FSTTestFieldPath, …

09b3804

…`FSTTestQuery`, `FSTTestDoc`

refactoring to use test helper method FSTTestQuerySnapshot,

ed88900

remove unneccessary nil-check, check isKindOfClass instead of isEqual

e4e3903

zxu123 assigned mikelehen Dec 18, 2017

mikelehen suggested changes Dec 18, 2017

View reviewed changes

mikelehen removed their assignment Dec 18, 2017

zxu123 added 2 commits December 18, 2017 16:54

refactoring: adding FSTAPIHelpers.{h,m}, `FSTTest{Collection,Docume…

f734f10

…nt}Ref`, better naming and style fix

a file forgot in last commit

73f4c55

zxu123 commented Dec 18, 2017

View reviewed changes

zxu123 assigned mikelehen Dec 18, 2017

mikelehen suggested changes Dec 19, 2017

View reviewed changes

mikelehen removed their assignment Dec 19, 2017

wilhuff reviewed Dec 19, 2017

View reviewed changes

zxu123 added 2 commits December 19, 2017 12:11

mainly clean up import and some minor refactoring

0df38d8

fix style via style.sh

a2fc4a0

zxu123 assigned mikelehen Dec 19, 2017

mikelehen approved these changes Dec 19, 2017

View reviewed changes

mikelehen removed their assignment Dec 19, 2017

zxu123 added 4 commits December 19, 2017 12:59

minor style fix

5714e7d

Merge branch 'firestore-api-changes' into zxu/isEqual

5f20358

add pragma ignored -Wnonnull

3fbd94d

Merge branch 'zxu/isEqual' of https://github.com/firebase/firebase-io…

a6d6bbc

…s-sdk into zxu/isEqual

zxu123 merged commit 9ddf363 into firestore-api-changes Dec 19, 2017

zxu123 deleted the zxu/isEqual branch December 19, 2017 18:49

firebase locked and limited conversation to collaborators Nov 10, 2019

Firestore isEqual() on Public Types #569

Firestore isEqual() on Public Types #569

Uh oh!

Conversation

zxu123 commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Discussion

Testing

API Changes

Uh oh!

mikelehen commented Dec 14, 2017

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Firestore `isEqual()` on Public Types #569

Firestore `isEqual()` on Public Types #569

zxu123 commented Dec 14, 2017 •

edited

Loading