Port FieldFilter refactor from the JS SDK

firebase/firebase-js-sdk#1894

Author: wilhuff

Firestore/Example/Tests/Remote/FSTSerializerBetaTests.mm

@@ -530,7 +530,7 @@ - (void)testEncodesListenRequestLabels {
 
 - (void)testEncodesFieldFilter {
   auto input = std::static_pointer_cast<FieldFilter>(Filter("item.part.top", "==", "food"));
-  GCFSStructuredQuery_Filter *actual = [self.serializer encodedFieldFilter:*input];
+  GCFSStructuredQuery_Filter *actual = [self.serializer encodedUnaryOrFieldFilter:*input];
 
   GCFSStructuredQuery_Filter *expected = [GCFSStructuredQuery_Filter message];
   GCFSStructuredQuery_FieldFilter *prop = expected.fieldFilter;
@@ -542,7 +542,7 @@ - (void)testEncodesFieldFilter {
 
 - (void)testEncodesArrayContainsFilter {
   auto input = std::static_pointer_cast<FieldFilter>(Filter("item.tags", "array_contains", "food"));
-  GCFSStructuredQuery_Filter *actual = [self.serializer encodedFieldFilter:*input];
+  GCFSStructuredQuery_Filter *actual = [self.serializer encodedUnaryOrFieldFilter:*input];
 
   GCFSStructuredQuery_Filter *expected = [GCFSStructuredQuery_Filter message];
   GCFSStructuredQuery_FieldFilter *prop = expected.fieldFilter;

Firestore/Source/Core/FSTQuery.mm

@@ -29,8 +29,6 @@
 #include "Firestore/core/src/firebase/firestore/api/input_validation.h"
 #include "Firestore/core/src/firebase/firestore/core/field_filter.h"
 #include "Firestore/core/src/firebase/firestore/core/filter.h"
-#include "Firestore/core/src/firebase/firestore/core/nan_filter.h"
-#include "Firestore/core/src/firebase/firestore/core/null_filter.h"
 #include "Firestore/core/src/firebase/firestore/core/query.h"
 #include "Firestore/core/src/firebase/firestore/model/document_key.h"
 #include "Firestore/core/src/firebase/firestore/model/field_path.h"

Firestore/Source/Remote/FSTSerializerBeta.h

@@ -120,7 +120,7 @@ NS_ASSUME_NONNULL_BEGIN
 - (GCFSTarget_QueryTarget *)encodedQueryTarget:(FSTQuery *)query;
 - (FSTQuery *)decodedQueryFromQueryTarget:(GCFSTarget_QueryTarget *)target;
 
-- (GCFSStructuredQuery_Filter *)encodedFieldFilter:(const core::FieldFilter &)filter;
+- (GCFSStructuredQuery_Filter *)encodedUnaryOrFieldFilter:(const core::FieldFilter &)filter;
 
 - (std::unique_ptr<remote::WatchChange>)decodedWatchChange:(GCFSListenResponse *)watchChange;
 - (model::SnapshotVersion)versionFromListenResponse:(GCFSListenResponse *)watchChange;

Firestore/Source/Remote/FSTSerializerBeta.mm

@@ -43,8 +43,6 @@
 #include "Firestore/core/include/firebase/firestore/geo_point.h"
 #include "Firestore/core/src/firebase/firestore/core/field_filter.h"
 #include "Firestore/core/src/firebase/firestore/core/filter.h"
-#include "Firestore/core/src/firebase/firestore/core/nan_filter.h"
-#include "Firestore/core/src/firebase/firestore/core/null_filter.h"
 #include "Firestore/core/src/firebase/firestore/core/query.h"
 #include "Firestore/core/src/firebase/firestore/model/database_id.h"
 #include "Firestore/core/src/firebase/firestore/model/document_key.h"
@@ -98,6 +96,8 @@
 using firebase::firestore::remote::WatchTargetChange;
 using firebase::firestore::remote::WatchTargetChangeState;
 
+using Operator = Filter::Operator;
+
 NS_ASSUME_NONNULL_BEGIN
 
 @implementation FSTSerializerBeta {
@@ -888,10 +888,8 @@ - (GCFSStructuredQuery_Filter *_Nullable)encodedFilters:(const Query::FilterList
   }
   NSMutableArray<GCFSStructuredQuery_Filter *> *protos = [NSMutableArray array];
   for (const auto &filter : filters) {
-    if (filter->type() == Filter::Type::kFieldFilter) {
-      [protos addObject:[self encodedFieldFilter:static_cast<const FieldFilter &>(*filter)]];
-    } else {
-      [protos addObject:[self encodedUnaryFilter:*filter]];
+    if (filter->IsFieldFilter()) {
+      [protos addObject:[self encodedUnaryOrFieldFilter:static_cast<const FieldFilter &>(*filter)]];
     }
   }
   if (protos.count == 1) {
@@ -938,8 +936,23 @@ - (GCFSStructuredQuery_Filter *_Nullable)encodedFilters:(const Query::FilterList
   return result;
 }
 
-- (GCFSStructuredQuery_Filter *)encodedFieldFilter:(const FieldFilter &)filter {
+- (GCFSStructuredQuery_Filter *)encodedUnaryOrFieldFilter:(const FieldFilter &)filter {
   GCFSStructuredQuery_Filter *proto = [GCFSStructuredQuery_Filter message];
+
+  if (filter.op() == Operator::Equal) {
+    if (filter.value().is_null()) {
+      proto.unaryFilter.field = [self encodedFieldPath:filter.field()];
+      proto.unaryFilter.op = GCFSStructuredQuery_UnaryFilter_Operator_IsNull;
+      return proto;
+    }
+
+    if (filter.value().is_nan()) {
+      proto.unaryFilter.field = [self encodedFieldPath:filter.field()];
+      proto.unaryFilter.op = GCFSStructuredQuery_UnaryFilter_Operator_IsNan;
+      return proto;
+    }
+  }
+
   GCFSStructuredQuery_FieldFilter *fieldFilter = proto.fieldFilter;
   fieldFilter.field = [self encodedFieldPath:filter.field()];
   fieldFilter.op = [self encodedFieldFilterOperator:filter.op()];
@@ -954,27 +967,14 @@ - (GCFSStructuredQuery_Filter *)encodedFieldFilter:(const FieldFilter &)filter {
   return FieldFilter::Create(std::move(fieldPath), op, std::move(value));
 }
 
-- (GCFSStructuredQuery_Filter *)encodedUnaryFilter:(const Filter &)filter {
-  GCFSStructuredQuery_Filter *proto = [GCFSStructuredQuery_Filter message];
-  proto.unaryFilter.field = [self encodedFieldPath:filter.field()];
-  if (filter.type() == Filter::Type::kNanFilter) {
-    proto.unaryFilter.op = GCFSStructuredQuery_UnaryFilter_Operator_IsNan;
-  } else if (filter.type() == Filter::Type::kNullFilter) {
-    proto.unaryFilter.op = GCFSStructuredQuery_UnaryFilter_Operator_IsNull;
-  } else {
-    HARD_FAIL("Unrecognized filter: %s", filter.ToString());
-  }
-  return proto;
-}
-
 - (std::shared_ptr<Filter>)decodedUnaryFilter:(GCFSStructuredQuery_UnaryFilter *)proto {
   FieldPath field = FieldPath::FromServerFormat(util::MakeString(proto.field.fieldPath));
   switch (proto.op) {
     case GCFSStructuredQuery_UnaryFilter_Operator_IsNan:
-      return std::make_shared<core::NanFilter>(std::move(field));
+      return FieldFilter::Create(std::move(field), Operator::Equal, FieldValue::Nan());
 
     case GCFSStructuredQuery_UnaryFilter_Operator_IsNull:
-      return std::make_shared<core::NullFilter>(std::move(field));
+      return FieldFilter::Create(std::move(field), Operator::Equal, FieldValue::Null());
 
     default:
       HARD_FAIL("Unrecognized UnaryFilter.operator %s", proto.op);

Firestore/core/src/firebase/firestore/api/query_core.mm

@@ -267,32 +267,49 @@ QuerySnapshot result(firestore_, query_, std::move(snapshot),
   return Wrap([query() queryByAddingEndAt:bound]);
 }
 
-void Query::ValidateNewFilter(const Filter& filter) const {
-  if (filter->type() == Filter::Type::kRelationFilter) {
-    const auto& field_filter = static_cast<const FieldFilter&>(*filter);
-    Operator filter_op = field_filter.op();
+namespace {
+
+constexpr Operator kArrayOps[] = {
+    Operator::ArrayContains
+};
+
+}
+
+void Query::ValidateNewFilter(const class Filter& filter) const {
+  if (filter.IsFieldFilter()) {
+    const auto& field_filter = static_cast<const FieldFilter&>(filter);
+
     if (field_filter.IsInequality()) {
       const FieldPath* existing_inequality = [query_ inequalityFilterField];
       const FieldPath* new_inequality = &filter.field();
 
       if (existing_inequality && *existing_inequality != *new_inequality) {
         ThrowInvalidArgument(
             "Invalid Query. All where filters with an inequality (lessThan, "
-            "lessThanOrEqual, greaterThan, or greaterThanOrEqual) must be on the "
+            "lessThanOrEqual, greaterThan, or greaterThanOrEqual) must be on "
+            "the "
             "same field. But you have inequality filters on '%s' and '%s'",
-            existing_inequality->CanonicalString(), new_inequality->CanonicalString());
+            existing_inequality->CanonicalString(),
+            new_inequality->CanonicalString());
       }
 
       const FieldPath* first_order_by_field = [query_ firstSortOrderField];
       if (first_order_by_field) {
         ValidateOrderByField(*first_order_by_field, filter.field());
       }
-    } else if (filter_op == Operator::ArrayContains) {
-      if ([query_ hasArrayContainsFilter]) {
-        ThrowInvalidArgument(
-            "Invalid Query. Queries only support a single arrayContains filter.");
+
+    } else {
+      // You can have at most 1 disjunctive filter and 1 array filter. Check if
+      // the new filter conflicts with an existing one.
+      Operator filter_op = field_filter.op();
+      bool is_array_op = absl::c_linear_search(kArrayOps, filter_op);
+
+      if (is_array_op && [query_ hasArrayContainsFilter]) {
+        ThrowInvalidArgument("Invalid Query. Queries only support a single "
+                             "arrayContains filter.");
       }
     }
+  }
 }
 
 void Query::ValidateNewOrderByPath(const FieldPath& fieldPath) const {

Firestore/core/src/firebase/firestore/core/CMakeLists.txt

@@ -28,10 +28,6 @@ cc_library(
     key_field_filter.cc
     key_field_filter.h
     listen_options.h
-    nan_filter.cc
-    nan_filter.h
-    null_filter.cc
-    null_filter.h
     query.cc
     query.h
     target_id_generator.cc

Firestore/core/src/firebase/firestore/core/array_contains_filter.h

@@ -35,6 +35,10 @@ class ArrayContainsFilter : public FieldFilter {
 
   ArrayContainsFilter(model::FieldPath field, model::FieldValue value);
 
+  Type type() const override {
+    return Type::kArrayContainsFilter;
+  }
+
   bool Matches(const model::Document& doc) const override;
 };
 

Firestore/core/src/firebase/firestore/core/field_filter.cc

@@ -20,8 +20,8 @@
 #include <vector>
 
 #include "Firestore/core/src/firebase/firestore/api/input_validation.h"
-#include "Firestore/core/src/firebase/firestore/core/nan_filter.h"
-#include "Firestore/core/src/firebase/firestore/core/null_filter.h"
+#include "Firestore/core/src/firebase/firestore/core/array_contains_filter.h"
+#include "Firestore/core/src/firebase/firestore/core/key_field_filter.h"
 #include "Firestore/core/src/firebase/firestore/util/hashing.h"
 #include "absl/algorithm/container.h"
 #include "absl/strings/str_cat.h"
@@ -62,21 +62,37 @@ const char* Describe(Filter::Operator op) {
 std::shared_ptr<FieldFilter> FieldFilter::Create(FieldPath path,
                                                  Operator op,
                                                  FieldValue value_rhs) {
-  if (value_rhs.type() == FieldValue::Type::Null) {
+  if (path.IsKeyFieldPath()) {
+    HARD_ASSERT(value_rhs.type() == FieldValue::Type::Reference,
+                "Comparing on key, but filter value not a Reference.");
+    HARD_ASSERT(op != Filter::Operator::ArrayContains,
+                "arrayContains queries don't make sense on document keys.");
+    return std::make_shared<KeyFieldFilter>(std::move(path), op,
+                                            std::move(value_rhs));
+
+  } else if (value_rhs.type() == FieldValue::Type::Null) {
     if (op != Filter::Operator::Equal) {
       ThrowInvalidArgument(
           "Invalid Query. Null supports only equality comparisons.");
     }
-    return std::make_shared<NullFilter>(std::move(path));
+    FieldFilter filter(std::move(path), op, std::move(value_rhs));
+    return std::make_shared<FieldFilter>(std::move(filter));
+
   } else if (value_rhs.is_nan()) {
     if (op != Filter::Operator::Equal) {
       ThrowInvalidArgument(
           "Invalid Query. NaN supports only equality comparisons.");
     }
-    return std::make_shared<NanFilter>(std::move(path));
+    FieldFilter filter(std::move(path), op, std::move(value_rhs));
+    return std::make_shared<FieldFilter>(std::move(filter));
+
+  } else if (op == Operator::ArrayContains) {
+    return std::make_shared<ArrayContainsFilter>(std::move(path),
+                                                 std::move(value_rhs));
+
   } else {
-    return std::make_shared<FieldFilter>(std::move(path), op,
-                                         std::move(value_rhs));
+    FieldFilter filter(std::move(path), op, std::move(value_rhs));
+    return std::make_shared<FieldFilter>(std::move(filter));
   }
 }
 
@@ -89,31 +105,14 @@ const FieldPath& FieldFilter::field() const {
 }
 
 bool FieldFilter::Matches(const model::Document& doc) const {
-  if (field_.IsKeyFieldPath()) {
-    HARD_ASSERT(value_rhs_.type() == FieldValue::Type::Reference,
-                "Comparing on key, but filter value not a Reference.");
-    HARD_ASSERT(op_ != Filter::Operator::ArrayContains,
-                "arrayContains queries don't make sense on document keys.");
-    const auto& ref = value_rhs_.reference_value();
-    ComparisonResult comparison = doc.key().CompareTo(ref.key());
-    return MatchesComparison(comparison);
-  } else {
-    absl::optional<FieldValue> doc_field_value = doc.field(field_);
-    return doc_field_value && MatchesValue(doc_field_value.value());
-  }
-}
+  absl::optional<FieldValue> maybe_lhs = doc.field(field_);
+  if (!maybe_lhs) return false;
 
-bool FieldFilter::MatchesValue(const FieldValue& lhs) const {
-  if (op_ == Filter::Operator::ArrayContains) {
-    if (lhs.type() != FieldValue::Type::Array) return false;
+  const FieldValue& lhs = *maybe_lhs;
 
-    const auto& contents = lhs.array_value();
-    return absl::c_linear_search(contents, value_rhs_);
-  } else {
-    // Only compare types with matching backend order (such as double and int).
-    return FieldValue::Comparable(lhs.type(), value_rhs_.type()) &&
-           MatchesComparison(lhs.CompareTo(value_rhs_));
-  }
+  // Only compare types with matching backend order (such as double and int).
+  return FieldValue::Comparable(lhs.type(), value_rhs_.type()) &&
+         MatchesComparison(lhs.CompareTo(value_rhs_));
 }
 
 bool FieldFilter::MatchesComparison(ComparisonResult comparison) const {
@@ -130,10 +129,9 @@ bool FieldFilter::MatchesComparison(ComparisonResult comparison) const {
              comparison == ComparisonResult::Same;
     case Operator::GreaterThan:
       return comparison == ComparisonResult::Descending;
-    case Operator::ArrayContains:
-      HARD_FAIL("Should have been handled in MatchesValue()");
+    default:
+      HARD_FAIL("Operator %s unsuitable for comparison", op_);
   }
-  UNREACHABLE();
 }
 
 std::string FieldFilter::CanonicalId() const {
@@ -155,7 +153,7 @@ bool FieldFilter::IsInequality() const {
 }
 
 bool FieldFilter::Equals(const Filter& other) const {
-  if (other.type() != Type::kFieldFilter) return false;
+  if (type() != other.type()) return false;
 
   const auto& other_filter = static_cast<const FieldFilter&>(other);
   return op_ == other_filter.op_ && field_ == other_filter.field_ &&

Firestore/core/src/firebase/firestore/core/field_filter.h

@@ -48,6 +48,10 @@ class FieldFilter : public Filter {
     return Type::kFieldFilter;
   }
 
+  bool IsFieldFilter() const override {
+    return true;
+  }
+
   const model::FieldPath& field() const override;
 
   Operator op() const {

Firestore/core/src/firebase/firestore/core/filter.h

@@ -48,15 +48,26 @@ class Filter {
   // For lack of RTTI, all subclasses must identify themselves so that
   // comparisons properly take type into account.
   enum class Type {
+    kArrayContainsFilter,
     kFieldFilter,
-    kNanFilter,
-    kNullFilter,
+    kKeyFieldFilter,
   };
 
   virtual ~Filter() = default;
 
   virtual Type type() const = 0;
 
+  /**
+   * Returns true if this instance is FieldFilter or any derived class.
+   * Equivalent to `instanceof FieldFilter` on other platforms.
+   *
+   * Note this is different than checking `type() == Type::kFieldFilter` which
+   * is only true if the type is exactly FieldFilter.
+   */
+  virtual bool IsFieldFilter() const {
+    return false;
+  }
+
   /** Returns the field the Filter operates over. */
   virtual const model::FieldPath& field() const = 0;
 

Firestore/core/src/firebase/firestore/core/key_field_filter.h

@@ -36,6 +36,10 @@ class KeyFieldFilter : public FieldFilter {
                  core::Filter::Operator op,
                  model::FieldValue value);
 
+  Type type() const override {
+    return Type::kKeyFieldFilter;
+  }
+
   bool Matches(const model::Document& doc) const override;
 };