Migrate FSTReferenceValue to FSTDelegateValue (#3177)

wilhuff · web-flow · commit 64d8a40a702a · 2019-06-14T07:14:18.000-07:00
* Add FieldValue::Reference

* Migrate FSTReferenceValue to FSTDelegateValue

* Build Objective-C components with CMake

Build only the things required for FSTFieldValue
diff --git a/Firestore/Example/Tests/API/FSTUserDataConverterTests.mm b/Firestore/Example/Tests/API/FSTUserDataConverterTests.mm
@@ -144,9 +144,9 @@ - (void)testConvertsResourceNames {
   ];
   for (FSTDocumentKeyReference *value in values) {
     FSTFieldValue *wrapped = FSTTestFieldValue(value);
-    XCTAssertEqualObjects([wrapped class], [FSTReferenceValue class]);
+    XCTAssertEqualObjects([wrapped class], [FSTDelegateValue class]);
     XCTAssertEqualObjects([wrapped value], [FSTDocumentKey keyWithDocumentKey:value.key]);
-    XCTAssertTrue(((FSTReferenceValue *)wrapped).databaseID == value.databaseID);
+    XCTAssertTrue(((FSTDelegateValue *)wrapped).referenceValue.database_id() == value.databaseID);
     XCTAssertEqual(wrapped.type, FieldValue::Type::Reference);
   }
 }
diff --git a/Firestore/Example/Tests/Model/FSTFieldValueTests.mm b/Firestore/Example/Tests/Model/FSTFieldValueTests.mm
@@ -60,9 +60,7 @@
       } else if ([value isKindOfClass:[FSTDocumentKeyReference class]]) {
         // We directly convert these here so that the databaseIDs can be different.
         FSTDocumentKeyReference *reference = (FSTDocumentKeyReference *)value;
-        wrappedValue =
-            [FSTReferenceValue referenceValue:[FSTDocumentKey keyWithDocumentKey:reference.key]
-                                   databaseID:reference.databaseID];
+        wrappedValue = FieldValue::FromReference(reference.databaseID, reference.key).Wrap();
       } else {
         wrappedValue = FSTTestFieldValue(value);
       }
@@ -317,9 +315,7 @@ - (void)testValueEquality {
     @[ FSTTestFieldValue(FSTTestGeoPoint(0, 1)), FieldValue::FromGeoPoint(GeoPoint(0, 1)).Wrap() ],
     @[ FSTTestFieldValue(FSTTestGeoPoint(1, 0)) ],
     @[
-      [FSTReferenceValue
-          referenceValue:[FSTDocumentKey keyWithDocumentKey:FSTTestDocKey(@"coll/doc1")]
-              databaseID:database_id],
+      FieldValue::FromReference(database_id, FSTTestDocKey(@"coll/doc1")).Wrap(),
       FSTTestFieldValue(FSTTestRef("project", DatabaseId::kDefault, @"coll/doc1"))
     ],
     @[ FSTTestRef("project", "(default)", @"coll/doc2") ],
diff --git a/Firestore/Source/API/FIRDocumentSnapshot.mm b/Firestore/Source/API/FIRDocumentSnapshot.mm
@@ -193,8 +193,8 @@ - (id)convertedValue:(FSTFieldValue *)value options:(const FieldValueOptions &)o
   } else if (value.type == FieldValue::Type::Array) {
     return [self convertedArray:(FSTArrayValue *)value options:options];
   } else if (value.type == FieldValue::Type::Reference) {
-    FSTReferenceValue *ref = (FSTReferenceValue *)value;
-    const DatabaseId &refDatabase = ref.databaseID;
+    const auto &ref = ((FSTDelegateValue *)value).referenceValue;
+    const DatabaseId &refDatabase = ref.database_id();
     const DatabaseId &database = _snapshot.firestore()->database_id();
     if (refDatabase != database) {
       // TODO(b/32073923): Log this as a proper warning.
@@ -205,7 +205,7 @@ - (id)convertedValue:(FSTFieldValue *)value options:(const FieldValueOptions &)o
             refDatabase.database_id().c_str(), database.project_id().c_str(),
             database.database_id().c_str());
     }
-    DocumentKey key = [[ref valueWithOptions:options] key];
+    const DocumentKey &key = ref.key();
     return [[FIRDocumentReference alloc] initWithKey:key firestore:_snapshot.firestore()];
   } else {
     return [value valueWithOptions:options];
diff --git a/Firestore/Source/API/FIRQuery.mm b/Firestore/Source/API/FIRQuery.mm
@@ -467,9 +467,8 @@ - (FSTBound *)boundFromSnapshot:(FIRDocumentSnapshot *)snapshot isBefore:(BOOL)i
   // orders), multiple documents could match the position, yielding duplicate results.
   for (FSTSortOrder *sortOrder in self.query.sortOrders) {
     if (sortOrder.field == FieldPath::KeyFieldPath()) {
-      [components addObject:[FSTReferenceValue
-                                referenceValue:[FSTDocumentKey keyWithDocumentKey:document.key]
-                                    databaseID:self.firestore.databaseID]];
+      [components
+          addObject:FieldValue::FromReference(self.firestore.databaseID, document.key).Wrap()];
     } else {
       FSTFieldValue *value = [document fieldForPath:sortOrder.field];
 
@@ -524,8 +523,7 @@ - (FSTBound *)boundFromFieldValues:(NSArray<id> *)fieldValues isBefore:(BOOL)isB
                              path.CanonicalString());
       }
       DocumentKey key{path};
-      fieldValue = [FSTReferenceValue referenceValue:[FSTDocumentKey keyWithDocumentKey:key]
-                                          databaseID:self.firestore.databaseID];
+      fieldValue = FieldValue::FromReference(self.firestore.databaseID, key).Wrap();
     }
 
     [components addObject:fieldValue];
diff --git a/Firestore/Source/API/FSTUserDataConverter.mm b/Firestore/Source/API/FSTUserDataConverter.mm
@@ -476,8 +476,7 @@ - (nullable FSTFieldValue *)parseScalarValue:(nullable id)input context:(ParseCo
           other.project_id(), other.database_id(), _databaseID.project_id(),
           _databaseID.database_id(), context.FieldDescription());
     }
-    return [FSTReferenceValue referenceValue:[FSTDocumentKey keyWithDocumentKey:reference.key]
-                                  databaseID:_databaseID];
+    return FieldValue::FromReference(_databaseID, reference.key).Wrap();
 
   } else {
     ThrowInvalidArgument("Unsupported type: %s%s", NSStringFromClass([input class]),
diff --git a/Firestore/Source/Core/FSTQuery.mm b/Firestore/Source/Core/FSTQuery.mm
@@ -187,11 +187,11 @@ - (BOOL)isEqual:(id)other {
 - (BOOL)matchesDocument:(FSTDocument *)document {
   if (_field.IsKeyFieldPath()) {
     HARD_ASSERT(self.value.type == FieldValue::Type::Reference,
-                "Comparing on key, but filter value not a FSTReferenceValue.");
+                "Comparing on key, but filter value not a Reference.");
     HARD_ASSERT(self.filterOperator != Filter::Operator::ArrayContains,
                 "arrayContains queries don't make sense on document keys.");
-    FSTReferenceValue *refValue = (FSTReferenceValue *)self.value;
-    NSComparisonResult comparison = util::WrapCompare(document.key, refValue.value.key);
+    const auto &ref = ((FSTDelegateValue *)self.value).referenceValue;
+    NSComparisonResult comparison = util::WrapCompare(document.key, ref.key());
     return [self matchesComparison:comparison];
   } else {
     return [self matchesValue:[document fieldForPath:self.field]];
@@ -477,8 +477,8 @@ - (BOOL)sortsBeforeDocument:(FSTDocument *)document
     if (sortOrderComponent.field == FieldPath::KeyFieldPath()) {
       HARD_ASSERT(fieldValue.type == FieldValue::Type::Reference,
                   "FSTBound has a non-key value where the key path is being used %s", fieldValue);
-      FSTReferenceValue *refValue = (FSTReferenceValue *)fieldValue;
-      comparison = util::Compare(refValue.value.key, document.key);
+      const auto &ref = ((FSTDelegateValue *)fieldValue).referenceValue;
+      comparison = util::Compare(ref.key(), document.key);
     } else {
       FSTFieldValue *docValue = [document fieldForPath:sortOrderComponent.field];
       HARD_ASSERT(docValue != nil,
diff --git a/Firestore/Source/Model/FSTFieldValue.h b/Firestore/Source/Model/FSTFieldValue.h
@@ -103,17 +103,6 @@ typedef NS_ENUM(NSInteger, FSTTypeOrder) {
 
 @end
 
-/**
- * A reference value stored in Firestore.
- */
-@interface FSTReferenceValue : FSTFieldValue <FSTDocumentKey *>
-
-+ (instancetype)referenceValue:(FSTDocumentKey *)value databaseID:(model::DatabaseId)databaseID;
-
-@property(nonatomic, assign, readonly) const model::DatabaseId &databaseID;
-
-@end
-
 /**
  * A structured object value stored in Firestore.
  */
@@ -190,6 +179,8 @@ typedef NS_ENUM(NSInteger, FSTTypeOrder) {
 @interface FSTDelegateValue : FSTFieldValue <id>
 + (instancetype)delegateWithValue:(model::FieldValue &&)value;
 - (const model::FieldValue &)internalValue;
+
+- (const model::FieldValue::Reference &)referenceValue;
 @end
 
 NS_ASSUME_NONNULL_END
diff --git a/Firestore/Source/Model/FSTFieldValue.mm b/Firestore/Source/Model/FSTFieldValue.mm
@@ -128,76 +128,6 @@ - (double)doubleValue {
 
 @end
 
-#pragma mark - FSTReferenceValue
-
-@interface FSTReferenceValue ()
-@property(nonatomic, strong, readonly) FSTDocumentKey *key;
-@end
-
-@implementation FSTReferenceValue {
-  DatabaseId _databaseID;
-}
-
-+ (instancetype)referenceValue:(FSTDocumentKey *)value databaseID:(DatabaseId)databaseID {
-  return [[FSTReferenceValue alloc] initWithValue:value databaseID:std::move(databaseID)];
-}
-
-- (id)initWithValue:(FSTDocumentKey *)value databaseID:(DatabaseId)databaseID {
-  self = [super init];
-  if (self) {
-    _key = value;
-    _databaseID = std::move(databaseID);
-  }
-  return self;
-}
-
-- (id)value {
-  return self.key;
-}
-
-- (FieldValue::Type)type {
-  return FieldValue::Type::Reference;
-}
-
-- (FSTTypeOrder)typeOrder {
-  return FSTTypeOrderReference;
-}
-
-- (BOOL)isEqual:(id)other {
-  if (other == self) {
-    return YES;
-  }
-  if (!([other isKindOfClass:[FSTFieldValue class]] &&
-        ((FSTFieldValue *)other).type == FieldValue::Type::Reference)) {
-    return NO;
-  }
-
-  FSTReferenceValue *otherRef = (FSTReferenceValue *)other;
-  return self.key.key == otherRef.key.key && self.databaseID == otherRef.databaseID;
-}
-
-- (NSUInteger)hash {
-  NSUInteger result = self.databaseID.Hash();
-  result = 31 * result + [self.key hash];
-  return result;
-}
-
-- (NSComparisonResult)compare:(FSTFieldValue *)other {
-  if (other.type == FieldValue::Type::Reference) {
-    FSTReferenceValue *ref = (FSTReferenceValue *)other;
-    NSComparisonResult cmp = WrapCompare(self.databaseID.project_id(), ref.databaseID.project_id());
-    if (cmp != NSOrderedSame) {
-      return cmp;
-    }
-    cmp = WrapCompare(self.databaseID.database_id(), ref.databaseID.database_id());
-    return cmp != NSOrderedSame ? cmp : WrapCompare(self.key.key, ref.key.key);
-  } else {
-    return [self defaultCompare:other];
-  }
-}
-
-@end
-
 #pragma mark - FSTObjectValue
 
 /**
@@ -480,6 +410,10 @@ + (instancetype)delegateWithValue:(FieldValue &&)value {
   return _internalValue;
 }
 
+- (const FieldValue::Reference &)referenceValue {
+  return _internalValue.reference_value();
+}
+
 - (id)initWithValue:(FieldValue &&)value {
   self = [super init];
   if (self) {
@@ -560,7 +494,7 @@ - (id)value {
     case FieldValue::Type::Blob:
       return MakeNSData(self.internalValue.blob_value());
     case FieldValue::Type::Reference:
-      HARD_FAIL("TODO(rsgowman): implement");
+      return [FSTDocumentKey keyWithDocumentKey:self.referenceValue.key()];
     case FieldValue::Type::GeoPoint:
       return MakeFIRGeoPoint(self.internalValue.geo_point_value());
     case FieldValue::Type::Array:
diff --git a/Firestore/Source/Remote/FSTSerializerBeta.mm b/Firestore/Source/Remote/FSTSerializerBeta.mm
@@ -222,9 +222,8 @@ - (GCFSValue *)encodedFieldValue:(FSTFieldValue *)fieldValue {
     case FieldValue::Type::Blob:
       return [self encodedBlobValue:[fieldValue value]];
     case FieldValue::Type::Reference: {
-      FSTReferenceValue *ref = (FSTReferenceValue *)fieldValue;
-      DocumentKey key = [[ref value] key];
-      return [self encodedReferenceValueForDatabaseID:[ref databaseID] key:key];
+      const auto &ref = ((FSTDelegateValue *)fieldValue).referenceValue;
+      return [self encodedReferenceValueForDatabaseID:ref.database_id() key:ref.key()];
     }
     case FieldValue::Type::GeoPoint:
       return [self encodedGeoPointValue:[fieldValue value]];
@@ -345,7 +344,7 @@ - (GCFSValue *)encodedReferenceValueForDatabaseID:(const DatabaseId &)databaseID
   return result;
 }
 
-- (FSTReferenceValue *)decodedReferenceValue:(NSString *)resourceName {
+- (FSTFieldValue *)decodedReferenceValue:(NSString *)resourceName {
   const ResourcePath path = [self decodedResourcePathWithDatabaseID:resourceName];
   const std::string &project = path[1];
   const std::string &database = path[3];
@@ -355,8 +354,7 @@ - (FSTReferenceValue *)decodedReferenceValue:(NSString *)resourceName {
   HARD_ASSERT(database_id == _databaseID, "Database %s:%s cannot encode reference from %s:%s",
               _databaseID.project_id(), _databaseID.database_id(), database_id.project_id(),
               database_id.database_id());
-  return [FSTReferenceValue referenceValue:[FSTDocumentKey keyWithDocumentKey:key]
-                                databaseID:_databaseID];
+  return FieldValue::FromReference(_databaseID, key).Wrap();
 }
 
 - (GCFSArrayValue *)encodedArrayValue:(FSTArrayValue *)arrayValue {
diff --git a/Firestore/core/src/firebase/firestore/api/query_core.mm b/Firestore/core/src/firebase/firestore/api/query_core.mm
@@ -215,9 +215,9 @@ QuerySnapshot result(firestore_, query_, std::move(snapshot),
             "is not because it has an odd number of segments.",
             path.CanonicalString());
       }
-      field_value = [FSTReferenceValue
-          referenceValue:[FSTDocumentKey keyWithDocumentKey:DocumentKey{path}]
-              databaseID:firestore_->database_id()];
+      field_value = FieldValue::FromReference(firestore_->database_id(),
+                                              DocumentKey{path})
+                        .Wrap();
     } else if (field_value.type != FieldValue::Type::Reference) {
       ThrowInvalidArgument(
           "Invalid query. When querying by document ID you must provide a "
diff --git a/Firestore/core/src/firebase/firestore/model/field_value.cc b/Firestore/core/src/firebase/firestore/model/field_value.cc
@@ -42,6 +42,7 @@ namespace model {
 namespace {
 
 using BaseValue = FieldValue::BaseValue;
+using Reference = FieldValue::Reference;
 using ServerTimestamp = FieldValue::ServerTimestamp;
 using Type = FieldValue::Type;
 
@@ -332,8 +333,8 @@ class BlobValue : public SimpleFieldValue<Type::Blob, ByteString> {
 
 class ReferenceValue : public FieldValue::BaseValue {
  public:
-  ReferenceValue(DatabaseId database_id, DocumentKey key)
-      : database_id_(std::move(database_id)), key_(std::move(key)) {
+  explicit ReferenceValue(Reference reference)
+      : reference_(std::move(reference)) {
   }
 
   Type type() const override {
@@ -344,31 +345,43 @@ class ReferenceValue : public FieldValue::BaseValue {
     if (type() != other.type()) return false;
 
     auto& other_value = Cast<ReferenceValue>(other);
-    return database_id_ == other_value.database_id_ && key_ == other_value.key_;
+    return database_id() == other_value.database_id() &&
+           key() == other_value.key();
   }
 
   ComparisonResult CompareTo(const BaseValue& other) const override {
     ComparisonResult cmp = CompareTypes(other);
     if (!util::Same(cmp)) return cmp;
 
     auto& other_value = Cast<ReferenceValue>(other);
-    cmp = Compare(database_id_, other_value.database_id_);
+    cmp = Compare(database_id(), other_value.database_id());
     if (!util::Same(cmp)) return cmp;
 
-    return Compare(key_, other_value.key_);
+    return Compare(key(), other_value.key());
   }
 
   std::string ToString() const override {
-    return absl::StrCat("Reference(key=", key_.ToString(), ")");
+    return absl::StrCat("Reference(key=", key().ToString(), ")");
   }
 
   size_t Hash() const override {
-    return util::Hash(database_id_, key_);
+    return util::Hash(database_id(), key());
+  }
+
+  const Reference& value() const {
+    return reference_;
+  }
+
+  const DatabaseId& database_id() const {
+    return reference_.database_id();
+  }
+
+  const DocumentKey& key() const {
+    return reference_.key();
   }
 
  private:
-  DatabaseId database_id_;
-  DocumentKey key_;
+  Reference reference_;
 };
 
 class GeoPointValue : public BaseValue {
@@ -548,6 +561,11 @@ const ByteString& FieldValue::blob_value() const {
   return Cast<BlobValue>(*rep_).value();
 }
 
+const Reference& FieldValue::reference_value() const {
+  HARD_ASSERT(type() == Type::Reference);
+  return Cast<ReferenceValue>(*rep_).value();
+}
+
 const GeoPoint& FieldValue::geo_point_value() const {
   HARD_ASSERT(type() == Type::GeoPoint);
   return Cast<GeoPointValue>(*rep_).value();
@@ -712,8 +730,8 @@ FieldValue FieldValue::FromBlob(ByteString blob) {
 }
 
 FieldValue FieldValue::FromReference(DatabaseId database_id, DocumentKey key) {
-  return FieldValue(
-      std::make_shared<ReferenceValue>(std::move(database_id), std::move(key)));
+  return FieldValue(std::make_shared<ReferenceValue>(
+      Reference(std::move(database_id), std::move(key))));
 }
 
 FieldValue FieldValue::FromGeoPoint(const GeoPoint& value) {
diff --git a/Firestore/core/src/firebase/firestore/model/field_value.h b/Firestore/core/src/firebase/firestore/model/field_value.h
diff --git a/Firestore/core/test/firebase/firestore/testutil/xcgmock.h b/Firestore/core/test/firebase/firestore/testutil/xcgmock.h

Original file line number	Diff line number	Diff line change
`@@ -144,9 +144,9 @@ - (void)testConvertsResourceNames {`
`144`	`144`	`];`
`145`	`145`	`for (FSTDocumentKeyReference *value in values) {`
`146`	`146`	`FSTFieldValue *wrapped = FSTTestFieldValue(value);`
`147`		`- XCTAssertEqualObjects([wrapped class], [FSTReferenceValue class]);`
	`147`	`+ XCTAssertEqualObjects([wrapped class], [FSTDelegateValue class]);`
`148`	`148`	`XCTAssertEqualObjects([wrapped value], [FSTDocumentKey keyWithDocumentKey:value.key]);`
`149`		`- XCTAssertTrue(((FSTReferenceValue *)wrapped).databaseID == value.databaseID);`
	`149`	`+ XCTAssertTrue(((FSTDelegateValue *)wrapped).referenceValue.database_id() == value.databaseID);`
`150`	`150`	`XCTAssertEqual(wrapped.type, FieldValue::Type::Reference);`
`151`	`151`	`}`
`152`	`152`	`}`
Original file line number	Diff line number	Diff line change
`@@ -476,8 +476,7 @@ - (nullable FSTFieldValue *)parseScalarValue:(nullable id)input context:(ParseCo`
`476`	`476`	`other.project_id(), other.database_id(), _databaseID.project_id(),`
`477`	`477`	`_databaseID.database_id(), context.FieldDescription());`
`478`	`478`	`}`
`479`		`- return [FSTReferenceValue referenceValue:[FSTDocumentKey keyWithDocumentKey:reference.key]`
`480`		`- databaseID:_databaseID];`
	`479`	`+ return FieldValue::FromReference(_databaseID, reference.key).Wrap();`
`481`	`480`
`482`	`481`	`} else {`
`483`	`482`	`ThrowInvalidArgument("Unsupported type: %s%s", NSStringFromClass([input class]),`