Address feedback

Author: a-maurice

remote_config/src/android/remote_config_android.cc

@@ -392,7 +392,7 @@ static jobject ConfigKeyValueVariantArrayToHashMap(
 
 // Convert a std::map<std::string, Variant> into a Java CustomSignals object
 // using CustomSignals.Builder to populate String, Long, and Double signal
-// entries.
+// entries, or null values to clear signals.
 static jobject CustomSignalsFromMap(JNIEnv* env,
                                     const std::map<std::string, Variant>& map) {
   jobject builder = env->NewObject(custom_signals_builder::GetClass(),
@@ -403,7 +403,13 @@ static jobject CustomSignalsFromMap(JNIEnv* env,
   for (const auto& kv : map) {
     jstring key = env->NewStringUTF(kv.first.c_str());
     jobject result_builder = nullptr;
-    if (kv.second.is_string()) {
+    if (kv.second.is_null()) {
+      result_builder =
+          env->CallObjectMethod(builder,
+                                custom_signals_builder::GetMethodId(
+                                    custom_signals_builder::kPutString),
+                                key, nullptr);
+    } else if (kv.second.is_string()) {
       jstring str_val = env->NewStringUTF(kv.second.string_value());
       result_builder =
           env->CallObjectMethod(builder,

remote_config/src/desktop/metadata.cc

@@ -20,6 +20,7 @@
 #include <limits>
 #include <map>
 #include <string>
+#include <vector>
 
 #include "flatbuffers/flexbuffers.h"
 #include "remote_config/src/include/firebase/remote_config.h"
@@ -56,7 +57,15 @@ std::string RemoteConfigMetadata::Serialize() const {
 
     fbb.Map("custom_signals", [&]() {
       for (const auto& signal : custom_signals_) {
-        fbb.String(signal.first.c_str(), signal.second);
+        const std::string& key = signal.first;
+        const Variant& val = signal.second;
+        if (val.is_string()) {
+          fbb.String(key.c_str(), val.string_value());
+        } else if (val.is_int64()) {
+          fbb.Int(key.c_str(), val.int64_value());
+        } else if (val.is_double()) {
+          fbb.Double(key.c_str(), val.double_value());
+        }
       }
     });
   });
@@ -107,7 +116,19 @@ void RemoteConfigMetadata::Deserialize(const std::string& buffer) {
 
   custom_signals_.clear();
   flexbuffers::Map custom_signals = struct_map["custom_signals"].AsMap();
-  DeserializeMap(&custom_signals_, custom_signals);
+  for (int i = 0, n = custom_signals.size(); i < n; ++i) {
+    const char* key_str = custom_signals.Keys()[i].AsKey();
+    if (!key_str) continue;
+    flexbuffers::Reference val_ref = custom_signals.Values()[i];
+    if (val_ref.IsString()) {
+      custom_signals_[key_str] =
+          Variant(std::string(val_ref.AsString().c_str()));
+    } else if (val_ref.IsInt()) {
+      custom_signals_[key_str] = Variant(val_ref.AsInt64());
+    } else if (val_ref.IsFloat()) {
+      custom_signals_[key_str] = Variant(val_ref.AsDouble());
+    }
+  }
 }
 
 void RemoteConfigMetadata::AddSetting(const ConfigSetting& setting,

remote_config/src/desktop/metadata.h

@@ -18,6 +18,7 @@
 #include <map>
 #include <string>
 
+#include "firebase/variant.h"
 #include "flatbuffers/flexbuffers.h"
 #include "remote_config/src/include/firebase/remote_config.h"
 
@@ -27,7 +28,7 @@ namespace internal {
 
 typedef std::map<std::string, std::string> MetaDigestMap;
 typedef std::map<ConfigSetting, std::string> MetaSettingsMap;
-typedef std::map<std::string, std::string> MetaCustomSignalsMap;
+typedef std::map<std::string, Variant> MetaCustomSignalsMap;
 
 // Contains different data about Remote Config Client.
 //

remote_config/src/desktop/remote_config_desktop.cc

@@ -274,22 +274,16 @@ Future<void> RemoteConfigInternal::SetCustomSignals(
 
     // Merge incoming signals with existing metadata:
     // - Null variants remove the existing signal entry.
-    // - Numeric and string types are converted and stored as string
-    // representations.
+    // - Non-null variants (String, Int64, Double) are stored with their
+    // original type preserved.
     for (const auto& kv : custom_signals) {
       const std::string& key = kv.first;
       const Variant& value = kv.second;
 
       if (value.is_null()) {
         updated_signals.erase(key);
-      } else if (value.is_string()) {
-        updated_signals[key] = value.string_value();
-      } else if (value.is_int64()) {
-        updated_signals[key] = std::to_string(value.int64_value());
-      } else if (value.is_double()) {
-        std::ostringstream ss;
-        ss << value.double_value();
-        updated_signals[key] = ss.str();
+      } else {
+        updated_signals[key] = value;
       }
     }
 

remote_config/src/desktop/remote_config_request.cc

@@ -16,6 +16,9 @@
 
 #include "remote_config/src/desktop/remote_config_request.h"
 
+#include <map>
+#include <string>
+
 #include "app/src/app_common.h"
 #include "app/src/assert.h"
 #include "app/src/variant_util.h"
@@ -43,7 +46,7 @@ void RemoteConfigRequest::UpdatePostFields() {
   if (!custom_signals_.empty()) {
     std::map<Variant, Variant> variant_map;
     for (const auto& kv : custom_signals_) {
-      variant_map[Variant(kv.first)] = Variant(kv.second);
+      variant_map[Variant(kv.first)] = kv.second;
     }
     std::string custom_signals_json = util::VariantToJson(variant_map);
 

remote_config/src/desktop/remote_config_request.h

@@ -17,6 +17,10 @@
 #ifndef FIREBASE_REMOTE_CONFIG_SRC_DESKTOP_REMOTE_CONFIG_REQUEST_H_
 #define FIREBASE_REMOTE_CONFIG_SRC_DESKTOP_REMOTE_CONFIG_REQUEST_H_
 
+#include <map>
+#include <string>
+#include <utility>
+
 #include "app/rest/request_json.h"
 #include "remote_config/request_generated.h"
 #include "remote_config/request_resource.h"
@@ -87,11 +91,11 @@ class RemoteConfigRequest
         std::move(analytics_user_properties);
   }
 
-  void SetCustomSignals(std::map<std::string, std::string> custom_signals) {
+  void SetCustomSignals(std::map<std::string, Variant> custom_signals) {
     custom_signals_ = std::move(custom_signals);
   }
 
-  const std::map<std::string, std::string>& custom_signals() const {
+  const std::map<std::string, Variant>& custom_signals() const {
     return custom_signals_;
   }
 
@@ -101,7 +105,7 @@ class RemoteConfigRequest
   void UpdatePostFields() override;
 
  private:
-  std::map<std::string, std::string> custom_signals_;
+  std::map<std::string, Variant> custom_signals_;
 };
 
 }  // namespace internal

remote_config/src/ios/remote_config_ios.mm

@@ -407,11 +407,17 @@ static int64_t FutureCompleteWithError(NSError *error, ReferenceCountedFutureImp
   NSMutableDictionary *dict = [[NSMutableDictionary alloc] initWithCapacity:custom_signals.size()];
   for (const auto& pair : custom_signals) {
     const char* key = pair.first.c_str();
-    id value = VariantToNSObject(pair.second);
-    if (value) {
-      dict[@(key)] = value;
+    if (pair.second.is_null()) {
+      dict[@(key)] = [NSNull null];
     } else {
-      LogError("Remote Config: Invalid Variant type for SetCustomSignals() key %s", key);
+      id value = VariantToNSObject(pair.second);
+      if (value) {
+        dict[@(key)] = value;
+      } else {
+        LogError(
+            "Remote Config: Invalid Variant type for SetCustomSignals() key %s",
+            key);
+      }
     }
   }
   [impl() setCustomSignals:dict

remote_config/tests/desktop/metadata_test.cc

@@ -40,7 +40,9 @@ TEST(RemoteConfigMetadataTest, Serialization) {
       MetaDigestMap({{"namespace1", "digest1"}, {"namespace2", "digest2"}}));
   remote_config_metadata.AddSetting(kConfigSettingDeveloperMode, "0");
   remote_config_metadata.set_custom_signals(
-      MetaCustomSignalsMap({{"sig1", "val1"}, {"sig2", "123"}}));
+      MetaCustomSignalsMap({{"sig_str", Variant("val1")},
+                            {"sig_int", Variant(123)},
+                            {"sig_double", Variant(45.6)}}));
 
   std::string buffer = remote_config_metadata.Serialize();
   RemoteConfigMetadata new_remote_config_metadata;
@@ -102,7 +104,9 @@ TEST(RemoteConfigMetadataTest, SetAndGetCustomSignals) {
   RemoteConfigMetadata m;
   EXPECT_TRUE(m.custom_signals().empty());
 
-  MetaCustomSignalsMap signals = {{"key1", "val1"}, {"key2", "42"}};
+  MetaCustomSignalsMap signals = {{"key_str", Variant("val1")},
+                                  {"key_int", Variant(42)},
+                                  {"key_double", Variant(3.14)}};
   m.set_custom_signals(signals);
   EXPECT_EQ(m.custom_signals(), signals);
 }

remote_config/tests/desktop/remote_config_desktop_test.cc

@@ -519,9 +519,9 @@ TEST_F(RemoteConfigDesktopTest, SetCustomSignals) {
   const MetaCustomSignalsMap& stored_signals =
       instance_->configs_.metadata.custom_signals();
   EXPECT_EQ(stored_signals.size(), 3);
-  EXPECT_EQ(stored_signals.at("key_string"), "val");
-  EXPECT_EQ(stored_signals.at("key_int"), "123");
-  EXPECT_EQ(stored_signals.at("key_double"), "45.6");
+  EXPECT_EQ(stored_signals.at("key_string"), Variant("val"));
+  EXPECT_EQ(stored_signals.at("key_int"), Variant(123));
+  EXPECT_EQ(stored_signals.at("key_double"), Variant(45.6));
 }
 
 TEST_F(RemoteConfigDesktopTest, SetCustomSignalsMergeAndRemove) {
@@ -542,9 +542,9 @@ TEST_F(RemoteConfigDesktopTest, SetCustomSignalsMergeAndRemove) {
   const MetaCustomSignalsMap& stored_signals =
       instance_->configs_.metadata.custom_signals();
   EXPECT_EQ(stored_signals.size(), 3);
-  EXPECT_EQ(stored_signals.at("key1"), "val1");
-  EXPECT_EQ(stored_signals.at("key2"), "200");
-  EXPECT_EQ(stored_signals.at("key4"), "new_val");
+  EXPECT_EQ(stored_signals.at("key1"), Variant("val1"));
+  EXPECT_EQ(stored_signals.at("key2"), Variant(200));
+  EXPECT_EQ(stored_signals.at("key4"), Variant("new_val"));
   EXPECT_EQ(stored_signals.find("key3"), stored_signals.end());
 }
 
@@ -591,8 +591,8 @@ TEST_F(RemoteConfigDesktopTest, SetCustomSignalsPersistence) {
   const MetaCustomSignalsMap& loaded_signals =
       new_instance.configs_.metadata.custom_signals();
   EXPECT_EQ(loaded_signals.size(), 2);
-  EXPECT_EQ(loaded_signals.at("persisted_key"), "persisted_val");
-  EXPECT_EQ(loaded_signals.at("persisted_num"), "999");
+  EXPECT_EQ(loaded_signals.at("persisted_key"), Variant("persisted_val"));
+  EXPECT_EQ(loaded_signals.at("persisted_num"), Variant(999));
 }
 
 }  // namespace internal

remote_config/tests/desktop/rest_test.cc

@@ -185,18 +185,16 @@ TEST_F(RemoteConfigRESTTest, SetupRESTRequest) {
   std::string locale = firebase::internal::GetLocale();
   EXPECT_EQ(rest.rc_request_.application_data_->languageCode, locale);
 
-  if (locale.length() > 0) {
-    EXPECT_EQ(rest.rc_request_.application_data_->countryCode,
-              locale.substr(0, 2));
-  }
-
+  // TODO(cynthiajiang) Set Country code and verify installations id and token.
   // TODO(cynthiajiang) verify installations id and token.
 }
 
 TEST_F(RemoteConfigRESTTest, SetupRESTRequestWithCustomSignals) {
   RemoteConfigMetadata metadata = configs_.metadata;
   metadata.set_custom_signals(
-      MetaCustomSignalsMap({{"sig_str", "hello"}, {"sig_int", "42"}}));
+      MetaCustomSignalsMap({{"sig_str", Variant("hello")},
+                            {"sig_int", Variant(42)},
+                            {"sig_double", Variant(3.14)}}));
   LayeredConfigs configs_with_signals(configs_.fetched, configs_.active,
                                       configs_.defaults, metadata);
 
@@ -207,7 +205,8 @@ TEST_F(RemoteConfigRESTTest, SetupRESTRequestWithCustomSignals) {
   EXPECT_TRUE(rest.rc_request_.ReadBodyIntoString(&body));
   EXPECT_NE(body.find("\"custom_signals\":"), std::string::npos);
   EXPECT_NE(body.find("\"sig_str\":\"hello\""), std::string::npos);
-  EXPECT_NE(body.find("\"sig_int\":\"42\""), std::string::npos);
+  EXPECT_NE(body.find("\"sig_int\":42"), std::string::npos);
+  EXPECT_NE(body.find("\"sig_double\":3.14"), std::string::npos);
 }
 
 // Verify the rest request with mock project will return code 404