libweave: Switch StatePackage and StateManager from Any type Updating properties was implemented using chromeos::Any. We are removing this with dbus references. Also seems 'Any' version threated all properties as required so had to update GetTestSchema in StatePackageTest. BUG=brillo:1246 TEST='FEATURES=test emerge-gizmo buffet' Change-Id: Ic7a2809d6d2e3f17f6993cf0fa6938c2f94cb0ec Reviewed-on: https://chromium-review.googlesource.com/289643 Trybot-Ready: Vitaly Buka <vitalybuka@chromium.org> Tested-by: Vitaly Buka <vitalybuka@chromium.org> Reviewed-by: Alex Vakulenko <avakulenko@chromium.org> Commit-Queue: Vitaly Buka <vitalybuka@chromium.org>
diff --git a/buffet/manager.cc b/buffet/manager.cc index 1ee1793..b1070b5 100644 --- a/buffet/manager.cc +++ b/buffet/manager.cc
@@ -23,6 +23,8 @@ #include <dbus/object_path.h> #include <dbus/values_util.h> +// TODO(vitalybuka): Will be moved into buffet soon. +#include "libweave/src/commands/dbus_conversion.h" #include "weave/enum_to_string.h" using chromeos::dbus_utils::AsyncEventSequencer; @@ -137,7 +139,12 @@ void Manager::UpdateState(DBusMethodResponsePtr<> response, const chromeos::VariantDictionary& property_set) { chromeos::ErrorPtr error; - if (!device_->GetState()->SetProperties(property_set, &error)) + auto properties = + weave::DictionaryFromDBusVariantDictionary(property_set, &error); + if (!properties) + response->ReplyWithError(error.get()); + + if (!device_->GetState()->SetProperties(*properties, &error)) response->ReplyWithError(error.get()); else response->Return();
diff --git a/libweave/include/weave/state.h b/libweave/include/weave/state.h index d34b408..c817a75 100644 --- a/libweave/include/weave/state.h +++ b/libweave/include/weave/state.h
@@ -7,7 +7,6 @@ #include <base/callback.h> #include <base/values.h> -#include <chromeos/variant_dictionary.h> namespace weave { @@ -17,7 +16,7 @@ virtual void AddOnChangedCallback(const base::Closure& callback) = 0; // Updates a multiple property values. - virtual bool SetProperties(const chromeos::VariantDictionary& property_set, + virtual bool SetProperties(const base::DictionaryValue& property_set, chromeos::ErrorPtr* error) = 0; // Returns aggregated state properties across all registered packages as
diff --git a/libweave/src/base_api_handler.cc b/libweave/src/base_api_handler.cc index 28be501..5ecc5fc 100644 --- a/libweave/src/base_api_handler.cc +++ b/libweave/src/base_api_handler.cc
@@ -44,11 +44,13 @@ parameters->GetBoolean("localDiscoveryEnabled", &discovery_enabled); parameters->GetBoolean("localPairingEnabled", &pairing_enabled); - chromeos::VariantDictionary state{ - {"base.localAnonymousAccessMaxRole", anonymous_access_role}, - {"base.localDiscoveryEnabled", discovery_enabled}, - {"base.localPairingEnabled", pairing_enabled}, - }; + base::DictionaryValue state; + state.SetStringWithoutPathExpansion("base.localAnonymousAccessMaxRole", + anonymous_access_role); + state.SetBooleanWithoutPathExpansion("base.localDiscoveryEnabled", + discovery_enabled); + state.SetBooleanWithoutPathExpansion("base.localPairingEnabled", + pairing_enabled); if (!state_manager_->SetProperties(state, nullptr)) { return command->Abort(); }
diff --git a/libweave/src/commands/object_schema.cc b/libweave/src/commands/object_schema.cc index f117b6b..1aa3bfa 100644 --- a/libweave/src/commands/object_schema.cc +++ b/libweave/src/commands/object_schema.cc
@@ -367,7 +367,7 @@ chromeos::Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, errors::commands::kUnknownType, "Unexpected JSON value type: %s", type_name); - return {}; + return nullptr; } std::unique_ptr<ObjectSchema> ObjectSchema::Create() {
diff --git a/libweave/src/states/state_manager.cc b/libweave/src/states/state_manager.cc index 850541d..0c5bad5 100644 --- a/libweave/src/states/state_manager.cc +++ b/libweave/src/states/state_manager.cc
@@ -90,8 +90,9 @@ } else { LOG(ERROR) << "Failed to read file for firmwareVersion."; } - CHECK(SetPropertyValue(kBaseStateFirmwareVersion, firmware_version, - base::Time::Now(), nullptr)); + CHECK(SetPropertyValue(kBaseStateFirmwareVersion, + base::StringValue{firmware_version}, base::Time::Now(), + nullptr)); for (const auto& cb : on_changed_) cb.Run(); @@ -108,13 +109,13 @@ return dict; } -bool StateManager::SetProperties( - const chromeos::VariantDictionary& property_set, - chromeos::ErrorPtr* error) { +bool StateManager::SetProperties(const base::DictionaryValue& property_set, + chromeos::ErrorPtr* error) { base::Time timestamp = base::Time::Now(); bool all_success = true; - for (const auto& pair : property_set) { - if (!SetPropertyValue(pair.first, pair.second, timestamp, error)) { + for (base::DictionaryValue::Iterator it(property_set); !it.IsAtEnd(); + it.Advance()) { + if (!SetPropertyValue(it.key(), it.value(), timestamp, error)) { // Remember that an error occurred but keep going and update the rest of // the properties if possible. all_success = false; @@ -126,7 +127,7 @@ } bool StateManager::SetPropertyValue(const std::string& full_property_name, - const chromeos::Any& value, + const base::Value& value, const base::Time& timestamp, chromeos::ErrorPtr* error) { std::string package_name;
diff --git a/libweave/src/states/state_manager.h b/libweave/src/states/state_manager.h index 8b22c2f..50a3836 100644 --- a/libweave/src/states/state_manager.h +++ b/libweave/src/states/state_manager.h
@@ -15,7 +15,6 @@ #include <base/callback.h> #include <base/macros.h> #include <chromeos/errors/error.h> -#include <chromeos/variant_dictionary.h> #include "libweave/src/states/state_change_queue_interface.h" #include "libweave/src/states/state_package.h" @@ -39,7 +38,7 @@ // State overrides. void AddOnChangedCallback(const base::Closure& callback) override; - bool SetProperties(const chromeos::VariantDictionary& property_set, + bool SetProperties(const base::DictionaryValue& property_set, chromeos::ErrorPtr* error) override; std::unique_ptr<base::DictionaryValue> GetStateValuesAsJson() const override; @@ -72,7 +71,7 @@ // Updates a single property value. |full_property_name| must be the full // name of the property to update in format "package.property". bool SetPropertyValue(const std::string& full_property_name, - const chromeos::Any& value, + const base::Value& value, const base::Time& timestamp, chromeos::ErrorPtr* error);
diff --git a/libweave/src/states/state_manager_unittest.cc b/libweave/src/states/state_manager_unittest.cc index 2f63c14..8325677 100644 --- a/libweave/src/states/state_manager_unittest.cc +++ b/libweave/src/states/state_manager_unittest.cc
@@ -74,7 +74,7 @@ } bool SetPropertyValue(const std::string& name, - const chromeos::Any& value, + const base::Value& value, chromeos::ErrorPtr* error) { return mgr_->SetPropertyValue(name, value, timestamp_, error); } @@ -140,7 +140,7 @@ NotifyPropertiesUpdated(timestamp_, expected_prop_set)) .WillOnce(Return(true)); ASSERT_TRUE(SetPropertyValue("device.state_property", - std::string{"Test Value"}, nullptr)); + base::StringValue{"Test Value"}, nullptr)); auto expected = R"({ 'base': { 'manufacturer': 'Test Factory', @@ -155,28 +155,31 @@ TEST_F(StateManagerTest, SetPropertyValue_Error_NoName) { chromeos::ErrorPtr error; - ASSERT_FALSE(SetPropertyValue("", int{0}, &error)); + ASSERT_FALSE(SetPropertyValue("", base::FundamentalValue{0}, &error)); EXPECT_EQ(errors::state::kDomain, error->GetDomain()); EXPECT_EQ(errors::state::kPropertyNameMissing, error->GetCode()); } TEST_F(StateManagerTest, SetPropertyValue_Error_NoPackage) { chromeos::ErrorPtr error; - ASSERT_FALSE(SetPropertyValue("state_property", int{0}, &error)); + ASSERT_FALSE( + SetPropertyValue("state_property", base::FundamentalValue{0}, &error)); EXPECT_EQ(errors::state::kDomain, error->GetDomain()); EXPECT_EQ(errors::state::kPackageNameMissing, error->GetCode()); } TEST_F(StateManagerTest, SetPropertyValue_Error_UnknownPackage) { chromeos::ErrorPtr error; - ASSERT_FALSE(SetPropertyValue("power.level", int{0}, &error)); + ASSERT_FALSE( + SetPropertyValue("power.level", base::FundamentalValue{0}, &error)); EXPECT_EQ(errors::state::kDomain, error->GetDomain()); EXPECT_EQ(errors::state::kPropertyNotDefined, error->GetCode()); } TEST_F(StateManagerTest, SetPropertyValue_Error_UnknownProperty) { chromeos::ErrorPtr error; - ASSERT_FALSE(SetPropertyValue("base.level", int{0}, &error)); + ASSERT_FALSE( + SetPropertyValue("base.level", base::FundamentalValue{0}, &error)); EXPECT_EQ(errors::state::kDomain, error->GetDomain()); EXPECT_EQ(errors::state::kPropertyNotDefined, error->GetCode()); } @@ -185,7 +188,7 @@ EXPECT_CALL(mock_state_change_queue_, NotifyPropertiesUpdated(timestamp_, _)) .WillOnce(Return(true)); ASSERT_TRUE(SetPropertyValue("device.state_property", - std::string{"Test Value"}, nullptr)); + base::StringValue{"Test Value"}, nullptr)); std::vector<StateChange> expected_val; expected_val.emplace_back( timestamp_, ValueMap{{"device.state_property", @@ -209,7 +212,7 @@ EXPECT_CALL(*this, OnStateChanged()).Times(1); ASSERT_TRUE(mgr_->SetProperties( - {{"base.manufacturer", std::string("No Name")}}, nullptr)); + *CreateDictionaryValue("{'base.manufacturer': 'No Name'}"), nullptr)); auto expected = R"({ 'base': {
diff --git a/libweave/src/states/state_package.cc b/libweave/src/states/state_package.cc index b34fc39..170b03c 100644 --- a/libweave/src/states/state_package.cc +++ b/libweave/src/states/state_package.cc
@@ -46,22 +46,9 @@ bool StatePackage::AddValuesFromJson(const base::DictionaryValue* json, chromeos::ErrorPtr* error) { - base::DictionaryValue::Iterator iter(*json); - while (!iter.IsAtEnd()) { - std::string property_name = iter.key(); - auto it = values_.find(property_name); - if (it == values_.end()) { - chromeos::Error::AddToPrintf(error, FROM_HERE, errors::state::kDomain, - errors::state::kPropertyNotDefined, - "State property '%s.%s' is not defined", - name_.c_str(), property_name.c_str()); + for (base::DictionaryValue::Iterator it(*json); !it.IsAtEnd(); it.Advance()) { + if (!SetPropertyValue(it.key(), it.value(), error)) return false; - } - auto new_value = it->second->GetPropType()->CreateValue(); - if (!new_value->FromJson(&iter.value(), error)) - return false; - it->second = std::move(new_value); - iter.Advance(); } return true; } @@ -76,21 +63,23 @@ return dict; } -chromeos::Any StatePackage::GetPropertyValue(const std::string& property_name, - chromeos::ErrorPtr* error) const { +std::unique_ptr<base::Value> StatePackage::GetPropertyValue( + const std::string& property_name, + chromeos::ErrorPtr* error) const { auto it = values_.find(property_name); if (it == values_.end()) { chromeos::Error::AddToPrintf(error, FROM_HERE, errors::state::kDomain, errors::state::kPropertyNotDefined, "State property '%s.%s' is not defined", name_.c_str(), property_name.c_str()); - return chromeos::Any(); + return nullptr; } - return PropValueToDBusVariant(it->second.get()); + + return it->second->ToJson(); } bool StatePackage::SetPropertyValue(const std::string& property_name, - const chromeos::Any& value, + const base::Value& value, chromeos::ErrorPtr* error) { auto it = values_.find(property_name); if (it == values_.end()) { @@ -100,9 +89,8 @@ name_.c_str(), property_name.c_str()); return false; } - auto new_value = - PropValueFromDBusVariant(it->second->GetPropType(), value, error); - if (!new_value) + auto new_value = it->second->GetPropType()->CreateValue(); + if (!new_value->FromJson(&value, error)) return false; it->second = std::move(new_value); return true;
diff --git a/libweave/src/states/state_package.h b/libweave/src/states/state_package.h index 1fceb2e..0ee7cc9 100644 --- a/libweave/src/states/state_package.h +++ b/libweave/src/states/state_package.h
@@ -10,7 +10,6 @@ #include <string> #include <base/macros.h> -#include <chromeos/any.h> #include <chromeos/errors/error.h> #include "libweave/src/commands/object_schema.h" @@ -56,12 +55,13 @@ // Gets the value for a specific state property. |property_name| must not // include the package name as part of the property name. - chromeos::Any GetPropertyValue(const std::string& property_name, - chromeos::ErrorPtr* error) const; + std::unique_ptr<base::Value> GetPropertyValue( + const std::string& property_name, + chromeos::ErrorPtr* error) const; // Sets the value for a specific state property. |property_name| must not // include the package name as part of the property name. bool SetPropertyValue(const std::string& property_name, - const chromeos::Any& value, + const base::Value& value, chromeos::ErrorPtr* error); std::shared_ptr<const PropValue> GetProperty(
diff --git a/libweave/src/states/state_package_unittest.cc b/libweave/src/states/state_package_unittest.cc index 6e76062..8900e7f 100644 --- a/libweave/src/states/state_package_unittest.cc +++ b/libweave/src/states/state_package_unittest.cc
@@ -8,7 +8,6 @@ #include <string> #include <base/values.h> -#include <chromeos/variant_dictionary.h> #include <gtest/gtest.h> #include "libweave/src/commands/schema_constants.h" @@ -36,7 +35,12 @@ return CreateDictionaryValue(R"({ 'light': 'boolean', 'color': 'string', - 'direction':{'properties':{'azimuth':'number','altitude':{'maximum':90.0}}}, + 'direction':{ + 'properties':{ + 'azimuth': {'type': 'number', 'isRequired': true}, + 'altitude': {'maximum': 90.0} + } + }, 'iso': [50, 100, 200, 400] })"); } @@ -99,7 +103,8 @@ 'type': 'number' } }, - 'type': 'object' + 'type': 'object', + 'required': [ 'azimuth' ] }, 'iso': { 'enum': [50, 100, 200, 400], @@ -161,7 +166,8 @@ 'type': 'number' } }, - 'type': 'object' + 'type': 'object', + 'required': [ 'azimuth' ] }, 'iso': { 'enum': [50, 100, 200, 400], @@ -222,50 +228,39 @@ } TEST_F(StatePackageTest, GetPropertyValue) { - chromeos::Any value = package_->GetPropertyValue("color", nullptr); - EXPECT_EQ("white", value.TryGet<std::string>()); - - value = package_->GetPropertyValue("light", nullptr); - EXPECT_TRUE(value.TryGet<bool>()); - - value = package_->GetPropertyValue("iso", nullptr); - EXPECT_EQ(200, value.TryGet<int>()); - - value = package_->GetPropertyValue("direction", nullptr); - auto direction = value.TryGet<chromeos::VariantDictionary>(); - ASSERT_FALSE(direction.empty()); - EXPECT_DOUBLE_EQ(89.9, direction["altitude"].TryGet<double>()); - EXPECT_DOUBLE_EQ(57.2957795, direction["azimuth"].TryGet<double>()); + EXPECT_JSON_EQ("'white'", *package_->GetPropertyValue("color", nullptr)); + EXPECT_JSON_EQ("true", *package_->GetPropertyValue("light", nullptr)); + EXPECT_JSON_EQ("200", *package_->GetPropertyValue("iso", nullptr)); + EXPECT_JSON_EQ("{'altitude': 89.9, 'azimuth': 57.2957795}", + *package_->GetPropertyValue("direction", nullptr)); } TEST_F(StatePackageTest, GetPropertyValue_Unknown) { chromeos::ErrorPtr error; - chromeos::Any value = package_->GetPropertyValue("volume", &error); - EXPECT_TRUE(value.IsEmpty()); + EXPECT_EQ(nullptr, package_->GetPropertyValue("volume", &error)); EXPECT_EQ(errors::state::kDomain, error->GetDomain()); EXPECT_EQ(errors::state::kPropertyNotDefined, error->GetCode()); } TEST_F(StatePackageTest, SetPropertyValue_Simple) { EXPECT_TRUE( - package_->SetPropertyValue("color", std::string{"blue"}, nullptr)); - chromeos::Any value = package_->GetPropertyValue("color", nullptr); - EXPECT_EQ("blue", value.TryGet<std::string>()); - - EXPECT_TRUE(package_->SetPropertyValue("light", bool{false}, nullptr)); - value = package_->GetPropertyValue("light", nullptr); - EXPECT_FALSE(value.TryGet<bool>()); - - EXPECT_TRUE(package_->SetPropertyValue("iso", int{400}, nullptr)); - value = package_->GetPropertyValue("iso", nullptr); - EXPECT_EQ(400, value.TryGet<int>()); + package_->SetPropertyValue("color", base::StringValue{"blue"}, nullptr)); + EXPECT_JSON_EQ("'blue'", *package_->GetPropertyValue("color", nullptr)); + EXPECT_TRUE(package_->SetPropertyValue("light", base::FundamentalValue{false}, + nullptr)); + bool light = false; + ASSERT_TRUE( + package_->GetPropertyValue("light", nullptr)->GetAsBoolean(&light)); + EXPECT_FALSE(light); + EXPECT_TRUE( + package_->SetPropertyValue("iso", base::FundamentalValue{400}, nullptr)); + EXPECT_JSON_EQ("400", *package_->GetPropertyValue("iso", nullptr)); } TEST_F(StatePackageTest, SetPropertyValue_Object) { - chromeos::VariantDictionary direction{ - {"altitude", double{45.0}}, {"azimuth", double{15.0}}, - }; - EXPECT_TRUE(package_->SetPropertyValue("direction", direction, nullptr)); + EXPECT_TRUE(package_->SetPropertyValue( + "direction", + *CreateDictionaryValue("{'altitude': 45.0, 'azimuth': 15.0}"), nullptr)); auto expected = R"({ 'color': 'white', @@ -281,29 +276,31 @@ TEST_F(StatePackageTest, SetPropertyValue_Error_TypeMismatch) { chromeos::ErrorPtr error; - ASSERT_FALSE(package_->SetPropertyValue("color", int{12}, &error)); + ASSERT_FALSE( + package_->SetPropertyValue("color", base::FundamentalValue{12}, &error)); EXPECT_EQ(errors::commands::kDomain, error->GetDomain()); EXPECT_EQ(errors::commands::kTypeMismatch, error->GetCode()); error.reset(); - ASSERT_FALSE(package_->SetPropertyValue("iso", bool{false}, &error)); + ASSERT_FALSE( + package_->SetPropertyValue("iso", base::FundamentalValue{false}, &error)); EXPECT_EQ(errors::commands::kDomain, error->GetDomain()); EXPECT_EQ(errors::commands::kTypeMismatch, error->GetCode()); } TEST_F(StatePackageTest, SetPropertyValue_Error_OutOfRange) { chromeos::ErrorPtr error; - ASSERT_FALSE(package_->SetPropertyValue("iso", int{150}, &error)); + ASSERT_FALSE( + package_->SetPropertyValue("iso", base::FundamentalValue{150}, &error)); EXPECT_EQ(errors::commands::kDomain, error->GetDomain()); EXPECT_EQ(errors::commands::kOutOfRange, error->GetCode()); } TEST_F(StatePackageTest, SetPropertyValue_Error_Object_TypeMismatch) { chromeos::ErrorPtr error; - chromeos::VariantDictionary direction{ - {"altitude", double{45.0}}, {"azimuth", int{15}}, - }; - ASSERT_FALSE(package_->SetPropertyValue("direction", direction, &error)); + ASSERT_FALSE(package_->SetPropertyValue( + "direction", *CreateDictionaryValue("{'altitude': 45.0, 'azimuth': '15'}"), + &error)); EXPECT_EQ(errors::commands::kDomain, error->GetDomain()); EXPECT_EQ(errors::commands::kInvalidPropValue, error->GetCode()); const chromeos::Error* inner = error->GetInnerError(); @@ -313,10 +310,9 @@ TEST_F(StatePackageTest, SetPropertyValue_Error_Object_OutOfRange) { chromeos::ErrorPtr error; - chromeos::VariantDictionary direction{ - {"altitude", double{100.0}}, {"azimuth", double{290.0}}, - }; - ASSERT_FALSE(package_->SetPropertyValue("direction", direction, &error)); + ASSERT_FALSE(package_->SetPropertyValue( + "direction", + *CreateDictionaryValue("{'altitude': 100.0, 'azimuth': 290.0}"), &error)); EXPECT_EQ(errors::commands::kDomain, error->GetDomain()); EXPECT_EQ(errors::commands::kInvalidPropValue, error->GetCode()); const chromeos::Error* inner = error->GetInnerError(); @@ -326,29 +322,28 @@ TEST_F(StatePackageTest, SetPropertyValue_Error_Object_UnknownProperty) { chromeos::ErrorPtr error; - chromeos::VariantDictionary direction{ - {"altitude", double{10.0}}, - {"azimuth", double{20.0}}, - {"spin", double{30.0}}, - }; - ASSERT_FALSE(package_->SetPropertyValue("direction", direction, &error)); + ASSERT_FALSE(package_->SetPropertyValue( + "direction", *CreateDictionaryValue( + "{'altitude': 10.0, 'azimuth': 20.0, 'spin': 30.0}"), + &error)); EXPECT_EQ(errors::commands::kDomain, error->GetDomain()); EXPECT_EQ(errors::commands::kUnknownProperty, error->GetCode()); } TEST_F(StatePackageTest, SetPropertyValue_Error_Object_MissingProperty) { chromeos::ErrorPtr error; - chromeos::VariantDictionary direction{ - {"altitude", double{10.0}}, - }; - ASSERT_FALSE(package_->SetPropertyValue("direction", direction, &error)); + ASSERT_TRUE(package_->SetPropertyValue( + "direction", *CreateDictionaryValue("{'azimuth': 10.0}"), &error)); + ASSERT_FALSE(package_->SetPropertyValue( + "direction", *CreateDictionaryValue("{'altitude': 10.0}"), &error)); EXPECT_EQ(errors::commands::kDomain, error->GetDomain()); EXPECT_EQ(errors::commands::kPropertyMissing, error->GetCode()); } TEST_F(StatePackageTest, SetPropertyValue_Error_Unknown) { chromeos::ErrorPtr error; - ASSERT_FALSE(package_->SetPropertyValue("volume", int{100}, &error)); + ASSERT_FALSE(package_->SetPropertyValue("volume", base::FundamentalValue{100}, + &error)); EXPECT_EQ(errors::state::kDomain, error->GetDomain()); EXPECT_EQ(errors::state::kPropertyNotDefined, error->GetCode()); }