commit 3e864c0088346546f8e3f3aabbe241ad108497bd
author Alex Vakulenko <avakulenko@chromium.org> Tue Mar 24 10:19:32 2015 -0700
committer ChromeOS Commit Bot <chromeos-commit-bot@chromium.org> Wed Mar 25 01:34:05 2015 +0000
tree a68a7a5d1e1c4af9a44a49dc97799a8e643d91b0
parent 29e644404d65eb5fd4e6e294b292da317544d0ee

buffet: PropType::CreateValue() should do constraint validation

Make PropType::CreateValue(v,...) validate the value against type
contraints which eliminates additional ValidateConstraints() call
and ensure the produced values conforms to the type schema specified.

Also parameter-less PropType::CreateValue() should use the default
value specified in PropType when constructing the value.

BUG=brillo:107
TEST=`FEATURES=test emerge-link buffet`

Change-Id: I3712b8e0a14515c153c987078863b2904c89cafd
Reviewed-on: https://chromium-review.googlesource.com/262205
Reviewed-by: Vitaly Buka <vitalybuka@chromium.org>
Commit-Queue: Alex Vakulenko <avakulenko@chromium.org>
Tested-by: Alex Vakulenko <avakulenko@chromium.org>
Trybot-Ready: Alex Vakulenko <avakulenko@chromium.org>

buffet/commands/prop_types.cc

diff --git a/buffet/commands/prop_types.cc b/buffet/commands/prop_types.cc
index a31f135..74e2be6 100644
--- a/buffet/commands/prop_types.cc
+++ b/buffet/commands/prop_types.cc
@@ -211,8 +211,7 @@
 
 bool PropType::ValidateValue(const chromeos::Any& value,
                              chromeos::ErrorPtr* error) const {
-  std::unique_ptr<PropValue> val = CreateValue(value, error);
-  return val && ValidateConstraints(*val, error);
+  return !!CreateValue(value, error);
 }
 
 bool PropType::ValidateConstraints(const PropValue& value,

buffet/commands/prop_types.h

diff --git a/buffet/commands/prop_types.h b/buffet/commands/prop_types.h
index 10ca853..473a6c8 100644
--- a/buffet/commands/prop_types.h
+++ b/buffet/commands/prop_types.h
@@ -186,6 +186,8 @@
  public:
   ValueType GetType() const override { return GetValueType<T>(); }
   std::unique_ptr<PropValue> CreateValue() const override {
+    if (GetDefaultValue())
+      return GetDefaultValue()->Clone();
     return std::unique_ptr<PropValue>{new Value{Clone()}};
   }
   std::unique_ptr<PropValue> CreateValue(
@@ -194,7 +196,8 @@
     if (v.IsTypeCompatible<T>()) {
       std::unique_ptr<Value> value{new Value{Clone()}};
       value->SetValue(v.Get<T>());
-      prop_value = std::move(value);
+      if (ValidateConstraints(*value, error))
+        prop_value = std::move(value);
     } else {
       GenerateErrorValueTypeMismatch(error);
     }

buffet/commands/schema_utils.cc

diff --git a/buffet/commands/schema_utils.cc b/buffet/commands/schema_utils.cc
index 10e9797..06e5ded 100644
--- a/buffet/commands/schema_utils.cc
+++ b/buffet/commands/schema_utils.cc
@@ -138,6 +138,7 @@
 
   const ObjectSchema* object_schema = type->GetObject()->GetObjectSchemaPtr();
   std::set<std::string> keys_processed;
+  value_out->clear();  // Clear possible default values already in |value_out|.
   for (const auto& pair : object_schema->GetProps()) {
     const PropValue* def_value = pair.second->GetDefaultValue();
     if (dict->HasKey(pair.first)) {
@@ -154,9 +155,9 @@
                                      pair.first.c_str());
         return false;
       }
-      value_out->insert(std::make_pair(pair.first, std::move(value)));
+      value_out->emplace_hint(value_out->end(), pair.first, std::move(value));
     } else if (def_value) {
-      value_out->insert(std::make_pair(pair.first, def_value->Clone()));
+      value_out->emplace_hint(value_out->end(), pair.first, def_value->Clone());
     } else {
       return ErrorMissingProperty(error, pair.first.c_str());
     }
@@ -207,6 +208,9 @@
   const PropType* item_type = type->GetArray()->GetItemTypePtr();
   CHECK(item_type) << "Incomplete array type definition";
 
+  // This value might already contain values from the type defaults.
+  // Clear them first before proceeding.
+  value_out->clear();
   value_out->reserve(list->GetSize());
   for (const base::Value* item : *list) {
     std::unique_ptr<PropValue> prop_value = item_type->CreateValue();
@@ -309,8 +313,6 @@
     result = type->CreateValue(value, error);
   }
 
-  if (result && !type->ValidateConstraints(*result, error))
-    result.reset();
   return result;
 }
 
@@ -319,6 +321,7 @@
                            native_types::Object* obj,
                            chromeos::ErrorPtr* error) {
   std::set<std::string> keys_processed;
+  obj->clear();
   // First go over all object parameters defined by type's object schema and
   // extract the corresponding parameters from the source dictionary.
   for (const auto& pair : object_schema->GetProps()) {

buffet/states/state_package.cc

diff --git a/buffet/states/state_package.cc b/buffet/states/state_package.cc
index a023c14..1292339 100644
--- a/buffet/states/state_package.cc
+++ b/buffet/states/state_package.cc
@@ -39,11 +39,7 @@
   for (const auto& pair : schema.GetProps()) {
     types_.AddProp(pair.first, pair.second->Clone());
     // Create default value for this state property.
-    if (pair.second->GetDefaultValue()) {
-      values_.emplace(pair.first, pair.second->GetDefaultValue()->Clone());
-    } else {
-      values_.emplace(pair.first, pair.second->CreateValue());
-    }
+    values_.emplace(pair.first, pair.second->CreateValue());
   }
 
   return true;