libweave: Validate constraints in FromJson methods Also fixed public TypedValueBase::SetValue method that allows to create invalid values. BUG=brillo:1246 TEST='FEATURES=test emerge-gizmo buffet' Change-Id: Ia60354c3daf2f150fe3e8d1781d0f37420e03563 Reviewed-on: https://chromium-review.googlesource.com/289641 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/libweave/src/commands/prop_types.cc b/libweave/src/commands/prop_types.cc
index ffec8f4..a5db4cb 100644
--- a/libweave/src/commands/prop_types.cc
+++ b/libweave/src/commands/prop_types.cc

@@ -187,8 +187,7 @@
   const base::Value* defval = nullptr;  // Owned by value
   if (value->GetWithoutPathExpansion(commands::attributes::kDefault, &defval)) {
     std::unique_ptr<PropValue> prop_value = CreateValue();
-    if (!prop_value->FromJson(defval, error) ||
-        !ValidateValue(prop_value->GetValueAsAny(), error)) {
+    if (!prop_value->FromJson(defval, error)) {
       chromeos::Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain,
                                    errors::commands::kInvalidPropValue,
                                    "Invalid value for property '%s'",
@@ -237,7 +236,7 @@
                              chromeos::ErrorPtr* error) const {
   std::unique_ptr<PropValue> val = CreateValue();
   CHECK(val) << "Failed to create value object";
-  return val->FromJson(value, error) && ValidateConstraints(*val, error);
+  return val->FromJson(value, error);
 }
 
 bool PropType::ValidateValue(const chromeos::Any& value,

diff --git a/libweave/src/commands/prop_types.h b/libweave/src/commands/prop_types.h
index eb605a7..6b364f3 100644
--- a/libweave/src/commands/prop_types.h
+++ b/libweave/src/commands/prop_types.h

@@ -225,16 +225,14 @@
   std::unique_ptr<PropValue> CreateValue(
       const chromeos::Any& v,
       chromeos::ErrorPtr* error) const override {
-    std::unique_ptr<PropValue> prop_value;
-    if (v.IsTypeCompatible<T>()) {
-      std::unique_ptr<Value> value{new Value{Clone()}};
-      value->SetValue(v.Get<T>());
-      if (ValidateConstraints(*value, error))
-        prop_value = std::move(value);
-    } else {
+    if (!v.IsTypeCompatible<T>()) {
       GenerateErrorValueTypeMismatch(error);
+      return nullptr;
     }
-    return prop_value;
+    std::unique_ptr<Value> value{new Value{Clone()}};
+    if (!value->SetValue(v.Get<T>(), error))
+      return nullptr;
+    return std::move(value);
   }
 
   chromeos::Any ConvertArrayToDBusVariant(

diff --git a/libweave/src/commands/prop_values.h b/libweave/src/commands/prop_values.h
index d0b2dc7..fa9963a 100644
--- a/libweave/src/commands/prop_values.h
+++ b/libweave/src/commands/prop_values.h

@@ -156,7 +156,10 @@
   }
 
   bool FromJson(const base::Value* value, chromeos::ErrorPtr* error) override {
-    return TypedValueFromJson(value, GetPropType(), &value_, error);
+    T tmp_value;
+    if (!TypedValueFromJson(value, GetPropType(), &tmp_value, error))
+      return false;
+    return SetValue(tmp_value, error);
   }
 
   bool IsEqual(const PropValue* value) const override {
@@ -169,7 +172,13 @@
   // Helper methods to get and set the C++ representation of the value.
   chromeos::Any GetValueAsAny() const override { return value_; }
   const T& GetValue() const { return value_; }
-  void SetValue(T value) { value_ = std::move(value); }
+  bool SetValue(T value, chromeos::ErrorPtr* error) {
+    std::swap(value_, value);  // Backup.
+    if (GetPropType()->ValidateConstraints(*this, error))
+      return true;
+    std::swap(value_, value);  // Restore.
+    return false;
+  }
 
  protected:
   T value_{};  // The value of the parameter in C++ data representation.

diff --git a/libweave/src/commands/schema_utils.cc b/libweave/src/commands/schema_utils.cc
index 04b8bcb..034eca9 100644
--- a/libweave/src/commands/schema_utils.cc
+++ b/libweave/src/commands/schema_utils.cc

@@ -147,8 +147,7 @@
       const base::Value* param_value = nullptr;
       CHECK(dict->GetWithoutPathExpansion(pair.first, &param_value))
           << "Unable to get parameter";
-      if (!value->FromJson(param_value, error) ||
-          !pair.second->ValidateValue(value->GetValueAsAny(), error)) {
+      if (!value->FromJson(param_value, error)) {
         chromeos::Error::AddToPrintf(
             error, FROM_HERE, errors::commands::kDomain,
             errors::commands::kInvalidPropValue,
@@ -215,8 +214,7 @@
   value_out->reserve(list->GetSize());
   for (const base::Value* item : *list) {
     std::unique_ptr<PropValue> prop_value = item_type->CreateValue();
-    if (!prop_value->FromJson(item, error) ||
-        !item_type->ValidateValue(prop_value->GetValueAsAny(), error)) {
+    if (!prop_value->FromJson(item, error)) {
       return false;
     }
     value_out->push_back(std::move(prop_value));

diff --git a/libweave/src/commands/schema_utils.h b/libweave/src/commands/schema_utils.h
index ea1639f..f00a89f 100644
--- a/libweave/src/commands/schema_utils.h
+++ b/libweave/src/commands/schema_utils.h

@@ -75,6 +75,9 @@
 // Similarly to TypedValueToJson() function above, the following overloaded
 // helper methods allow to extract specific C++ data types from base::Value.
 // Also used in template classes below to simplify specialization logic.
+// TODO(vitalybuka): Fix this. Interface is misleading. Seeing PropType internal
+// type validation is expected. In reality only ValueMap and ValueVector do
+// validation.
 bool TypedValueFromJson(const base::Value* value_in,
                         const PropType* type,
                         bool* value_out,

diff --git a/libweave/src/commands/unittest_utils.h b/libweave/src/commands/unittest_utils.h
index 345e92d..d0d39ee 100644
--- a/libweave/src/commands/unittest_utils.h
+++ b/libweave/src/commands/unittest_utils.h

@@ -34,7 +34,8 @@
 std::unique_ptr<const PropVal> make_prop_value(const T& value) {
   std::unique_ptr<PropVal> result{
       new PropVal{PropType::Create(GetValueType<T>())}};
-  result->SetValue(value);
+  if (!result->SetValue(value, nullptr))
+    return nullptr;
   return std::move(result);
 }