Eliminate TypedValue and serialize direct from FieldValue to bytes. (#860)

This will likely only apply for proto messages that use 'oneof's. (Because we need to serialize these manually.)

The problem was that as we were adding additional types, TypeValue was evolving into a parallel implementation of the FieldValue union.

When serializing/deserializing oneofs we need to supply a custom value to serialize and a custom function to do the work. There's no value in translating from FieldValue to TypeValue just to store as this custom object. We might as well just store the FieldValue model directly and write the custom serializer/deserializer to use the model directly.

2 files changed, 31 insertions, 124 deletions

diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc
index 474d890..02013fa 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.cc
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc
@@ -102,33 +102,7 @@ int64_t DecodeInteger(pb_istream_t* stream) {
 
 using firebase::firestore::model::FieldValue;
 
-Serializer::TypedValue Serializer::EncodeFieldValue(
-    const FieldValue& field_value) {
-  Serializer::TypedValue proto_value{
-      field_value.type(), google_firestore_v1beta1_Value_init_default};
-  switch (field_value.type()) {
-    case FieldValue::Type::Null:
-      proto_value.value.null_value = google_protobuf_NullValue_NULL_VALUE;
-      break;
-    case FieldValue::Type::Boolean:
-      if (field_value == FieldValue::TrueValue()) {
-        proto_value.value.boolean_value = true;
-      } else {
-        FIREBASE_DEV_ASSERT(field_value == FieldValue::FalseValue());
-        proto_value.value.boolean_value = false;
-      }
-      break;
-    case FieldValue::Type::Integer:
-      proto_value.value.integer_value = field_value.integer_value();
-      break;
-    default:
-      // TODO(rsgowman): implement the other types
-      abort();
-  }
-  return proto_value;
-}
-
-void Serializer::EncodeTypedValue(const TypedValue& value,
+void Serializer::EncodeFieldValue(const FieldValue& field_value,
                                   std::vector<uint8_t>* out_bytes) {
   // TODO(rsgowman): how large should the output buffer be? Do some
   // investigation to see if we can get nanopb to tell us how much space it's
@@ -139,7 +113,7 @@ void Serializer::EncodeTypedValue(const TypedValue& value,
   // TODO(rsgowman): some refactoring is in order... but will wait until after a
   // non-varint, non-fixed-size (i.e. string) type is present before doing so.
   bool status = false;
-  switch (value.type) {
+  switch (field_value.type()) {
     case FieldValue::Type::Null:
       status = pb_encode_tag(&stream, PB_WT_VARINT,
                              google_firestore_v1beta1_Value_null_value_tag);
@@ -157,7 +131,12 @@ void Serializer::EncodeTypedValue(const TypedValue& value,
         // TODO(rsgowman): figure out error handling
         abort();
       }
-      EncodeBool(&stream, value.value.boolean_value);
+      if (field_value == FieldValue::TrueValue()) {
+        EncodeBool(&stream, true);
+      } else {
+        FIREBASE_DEV_ASSERT(field_value == FieldValue::FalseValue());
+        EncodeBool(&stream, false);
+      }
       break;
 
     case FieldValue::Type::Integer:
@@ -167,7 +146,7 @@ void Serializer::EncodeTypedValue(const TypedValue& value,
         // TODO(rsgowman): figure out error handling
         abort();
       }
-      EncodeInteger(&stream, value.value.integer_value);
+      EncodeInteger(&stream, field_value.integer_value());
       break;
 
     default:
@@ -178,23 +157,7 @@ void Serializer::EncodeTypedValue(const TypedValue& value,
   out_bytes->insert(out_bytes->end(), buf, buf + stream.bytes_written);
 }
 
-FieldValue Serializer::DecodeFieldValue(
-    const Serializer::TypedValue& value_proto) {
-  switch (value_proto.type) {
-    case FieldValue::Type::Null:
-      return FieldValue::NullValue();
-    case FieldValue::Type::Boolean:
-      return FieldValue::BooleanValue(value_proto.value.boolean_value);
-    case FieldValue::Type::Integer:
-      return FieldValue::IntegerValue(value_proto.value.integer_value);
-    default:
-      // TODO(rsgowman): implement the other types
-      abort();
-  }
-}
-
-Serializer::TypedValue Serializer::DecodeTypedValue(const uint8_t* bytes,
-                                                    size_t length) {
+FieldValue Serializer::DecodeFieldValue(const uint8_t* bytes, size_t length) {
   pb_istream_t stream = pb_istream_from_buffer(bytes, length);
   pb_wire_type_t wire_type;
   uint32_t tag;
@@ -205,51 +168,19 @@ Serializer::TypedValue Serializer::DecodeTypedValue(const uint8_t* bytes,
     abort();
   }
 
-  Serializer::TypedValue result{FieldValue::Type::Null,
-                                google_firestore_v1beta1_Value_init_default};
   switch (tag) {
     case google_firestore_v1beta1_Value_null_value_tag:
-      result.type = FieldValue::Type::Null;
       DecodeNull(&stream);
-      break;
+      return FieldValue::NullValue();
     case google_firestore_v1beta1_Value_boolean_value_tag:
-      result.type = FieldValue::Type::Boolean;
-      result.value.boolean_value = DecodeBool(&stream);
-      break;
+      return FieldValue::BooleanValue(DecodeBool(&stream));
     case google_firestore_v1beta1_Value_integer_value_tag:
-      result.type = FieldValue::Type::Integer;
-      result.value.integer_value = DecodeInteger(&stream);
-      break;
+      return FieldValue::IntegerValue(DecodeInteger(&stream));
 
     default:
       // TODO(rsgowman): figure out error handling
       abort();
   }
-
-  return result;
-}
-
-bool operator==(const Serializer::TypedValue& lhs,
-                const Serializer::TypedValue& rhs) {
-  if (lhs.type != rhs.type) {
-    return false;
-  }
-
-  switch (lhs.type) {
-    case FieldValue::Type::Null:
-      FIREBASE_DEV_ASSERT(lhs.value.null_value ==
-                          google_protobuf_NullValue_NULL_VALUE);
-      FIREBASE_DEV_ASSERT(rhs.value.null_value ==
-                          google_protobuf_NullValue_NULL_VALUE);
-      return true;
-    case FieldValue::Type::Boolean:
-      return lhs.value.boolean_value == rhs.value.boolean_value;
-    case FieldValue::Type::Integer:
-      return lhs.value.integer_value == rhs.value.integer_value;
-    default:
-      // TODO(rsgowman): implement the other types
-      abort();
-  }
 }
 
 }  // namespace remote
diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.h b/Firestore/core/src/firebase/firestore/remote/serializer.h
index af65255..635ffc3 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.h
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.h
@@ -33,25 +33,17 @@ namespace remote {
  * @brief Converts internal model objects to their equivalent protocol buffer
  * form, and protocol buffer objects to their equivalent bytes.
  *
- * Methods starting with "Encode" either convert from a model object to a
- * protocol buffer or from a protocol buffer to bytes, and methods starting with
- * "Decode" either convert from a protocol buffer to a model object or from
- * bytes to a protocol buffer.
- *
+ * Methods starting with "Encode" convert from a model object to a protocol
+ * buffer (or directly to bytes in cases where the proto uses a 'oneof', due to
+ * limitations in nanopb), and methods starting with "Decode" convert from a
+ * protocol buffer to a model object (or from bytes directly to a model
+ * objects.)
  */
 // TODO(rsgowman): Original docs also has this: "Throws an exception if a
 // protocol buffer is missing a critical field or has a value we can't
 // interpret." Adjust for C++.
 class Serializer {
  public:
-  /**
-   * @brief Wraps (nanopb) google_firestore_v1beta1_Value with type information.
-   */
-  struct TypedValue {
-    firebase::firestore::model::FieldValue::Type type;
-    google_firestore_v1beta1_Value value;
-  };
-
   Serializer() {
   }
   // TODO(rsgowman): We eventually need the DatabaseId, but can't add it just
@@ -68,52 +60,39 @@ class Serializer {
   //}
 
   /**
-   * Converts the FieldValue model passed into the Value proto equivalent.
+   * Converts the FieldValue model passed into bytes.
    *
    * @param field_value the model to convert.
-   * @return the proto representation of the model.
-   */
-  static Serializer::TypedValue EncodeFieldValue(
-      const firebase::firestore::model::FieldValue& field_value);
-
-  /**
-   * @brief Converts the value proto passed into bytes.
-   *
    * @param[out] out_bytes A buffer to place the output. The bytes will be
    * appended to this vector.
    */
   // TODO(rsgowman): error handling, incl return code.
-  static void EncodeTypedValue(const TypedValue& value,
-                               std::vector<uint8_t>* out_bytes);
+  static void EncodeFieldValue(
+      const firebase::firestore::model::FieldValue& field_value,
+      std::vector<uint8_t>* out_bytes);
 
   /**
-   * Converts from the proto Value format to the model FieldValue format
-   *
-   * @return The model equivalent of the proto data.
-   */
-  static firebase::firestore::model::FieldValue DecodeFieldValue(
-      const Serializer::TypedValue& value_proto);
-
-  /**
-   * @brief Converts from bytes to the nanopb proto.
+   * @brief Converts from bytes to the model FieldValue format.
    *
    * @param bytes The bytes to convert. It's assumed that exactly all of the
    * bytes will be used by this conversion.
-   * @return The (nanopb) proto equivalent of the bytes.
+   * @return The model equivalent of the bytes.
    */
   // TODO(rsgowman): error handling.
-  static TypedValue DecodeTypedValue(const uint8_t* bytes, size_t length);
+  static firebase::firestore::model::FieldValue DecodeFieldValue(
+      const uint8_t* bytes, size_t length);
 
   /**
-   * @brief Converts from bytes to the nanopb proto.
+   * @brief Converts from bytes to the model FieldValue format.
    *
    * @param bytes The bytes to convert. It's assumed that exactly all of the
    * bytes will be used by this conversion.
-   * @return The (nanopb) proto equivalent of the bytes.
+   * @return The model equivalent of the bytes.
    */
   // TODO(rsgowman): error handling.
-  static TypedValue DecodeTypedValue(const std::vector<uint8_t>& bytes) {
-    return DecodeTypedValue(bytes.data(), bytes.size());
+  static firebase::firestore::model::FieldValue DecodeFieldValue(
+      const std::vector<uint8_t>& bytes) {
+    return DecodeFieldValue(bytes.data(), bytes.size());
   }
 
  private:
@@ -121,9 +100,6 @@ class Serializer {
   // const firebase::firestore::model::DatabaseId& database_id_;
 };
 
-bool operator==(const Serializer::TypedValue& lhs,
-                const Serializer::TypedValue& rhs);
-
 }  // namespace remote
 }  // namespace firestore
 }  // namespace firebase