buffet: Cleanup GCD command definition type system. Moved a lot of duplicated code for different parameter types into base classes. Split ParamType::FromJson into three separate function (FromJson, ObjectSchemaFromJson, ConstraintsFromJson) which allows more fine-grained control of deserializing various pieces of schema amongst the members of type class hierarchy. This way, "enum" constraint (applicable to all types) can be handled in a base class while concrete type-specific properties will be read in respective top-level classes. Added error checking for invalid/unexpected object schema properties when parsing type definition. This allows correct reporting of unrecognized properties, as well as nice errors when incompatible properties are specified (e.g. both "enum" and "min/max" constraints at the same time). BUG=chromium:374860 TEST=All unit tests pass. Change-Id: I18b94338680b9b859ba964fd63b65273ca2521f6 Reviewed-on: https://chromium-review.googlesource.com/204971 Reviewed-by: Alex Vakulenko <avakulenko@chromium.org> Commit-Queue: Alex Vakulenko <avakulenko@chromium.org> Tested-by: Alex Vakulenko <avakulenko@chromium.org>
diff --git a/buffet/commands/object_schema.cc b/buffet/commands/object_schema.cc index 0312735..b978ec9 100644 --- a/buffet/commands/object_schema.cc +++ b/buffet/commands/object_schema.cc
@@ -6,7 +6,6 @@ #include <algorithm> #include <limits> -#include <set> #include <base/json/json_writer.h> #include <base/logging.h>
diff --git a/buffet/commands/object_schema_unittest.cc b/buffet/commands/object_schema_unittest.cc index be93073..64f4600 100644 --- a/buffet/commands/object_schema_unittest.cc +++ b/buffet/commands/object_schema_unittest.cc
@@ -857,5 +857,21 @@ &error)); EXPECT_EQ("type_mismatch", error->GetFirstError()->GetCode()); error.reset(); + + schema_str = "{" + "'param1':{'minimum':1, 'enum':[1,2,3]}" // can't have min/max & enum. + "}"; + EXPECT_FALSE(schema.FromJson(CreateDictionaryValue(schema_str).get(), nullptr, + &error)); + EXPECT_EQ("unexpected_parameter", error->GetFirstError()->GetCode()); + error.reset(); + + schema_str = "{" + "'param1':{'maximum':1, 'blah':2}" // 'blah' is unexpected. + "}"; + EXPECT_FALSE(schema.FromJson(CreateDictionaryValue(schema_str).get(), nullptr, + &error)); + EXPECT_EQ("unexpected_parameter", error->GetFirstError()->GetCode()); + error.reset(); }
diff --git a/buffet/commands/prop_types.cc b/buffet/commands/prop_types.cc index 3b71a56..9644eac 100644 --- a/buffet/commands/prop_types.cc +++ b/buffet/commands/prop_types.cc
@@ -93,12 +93,32 @@ } based_on_schema_ = (base_schema != nullptr); constraints_.clear(); + std::set<std::string> processed_keys{commands::attributes::kType}; + if (!ObjectSchemaFromJson(value, base_schema, &processed_keys, error)) + return false; if (base_schema) { for (const auto& pair : base_schema->GetConstraints()) { std::shared_ptr<Constraint> inherited(pair.second->CloneAsInherited()); constraints_.insert(std::make_pair(pair.first, inherited)); } } + if (!ConstraintsFromJson(value, &processed_keys, error)) + return false; + + // Now make sure there are no unexpected/unknown keys in the property schema + // definition object. + base::DictionaryValue::Iterator iter(*value); + while (!iter.IsAtEnd()) { + std::string key = iter.key(); + if (processed_keys.find(key) == processed_keys.end()) { + Error::AddToPrintf(error, commands::errors::kDomain, + commands::errors::kUnknownProperty, + "Unexpected property '%s'", key.c_str()); + return false; + } + iter.Advance(); + } + return true; } @@ -226,107 +246,79 @@ return std::make_shared<ConstraintClass>(limit); } -template<typename T> -bool NumericPropTypeBase::FromJsonHelper(const base::DictionaryValue* value, - const PropType* base_schema, - ErrorPtr* error) { - if (!PropType::FromJson(value, base_schema, error)) +// PropTypeBase ---------------------------------------------------------------- + +template<class Derived, class Value, typename T> +bool PropTypeBase<Derived, Value, T>::ConstraintsFromJson( + const base::DictionaryValue* value, std::set<std::string>* processed_keys, + ErrorPtr* error) { + if (!PropType::ConstraintsFromJson(value, processed_keys, error)) return false; if (value->HasKey(commands::attributes::kOneOf_Enum)) { - auto constraint = LoadOneOfConstraint<T>(value, GetObjectSchemaPtr(), + auto constraint = LoadOneOfConstraint<T>(value, this->GetObjectSchemaPtr(), error); if (!constraint) return false; - AddConstraint(constraint); - RemoveConstraint(ConstraintType::Min); - RemoveConstraint(ConstraintType::Max); - } else { + this->AddConstraint(constraint); + this->RemoveConstraint(ConstraintType::Min); + this->RemoveConstraint(ConstraintType::Max); + processed_keys->insert(commands::attributes::kOneOf_Enum); + } + + return true; +} + +// NumericPropTypeBase --------------------------------------------------------- + +template<class Derived, class Value, typename T> +bool NumericPropTypeBase<Derived, Value, T>::ConstraintsFromJson( + const base::DictionaryValue* value, std::set<std::string>* processed_keys, + ErrorPtr* error) { + if (!_Base::ConstraintsFromJson(value, processed_keys, error)) + return false; + + if (processed_keys->find(commands::attributes::kOneOf_Enum) == + processed_keys->end()) { + // Process min/max constraints only if "enum" constraint wasn't already + // specified. if (value->HasKey(commands::attributes::kNumeric_Min)) { auto constraint = LoadMinMaxConstraint<ConstraintMin<T>, T>( - commands::attributes::kNumeric_Min, value, GetObjectSchemaPtr(), + commands::attributes::kNumeric_Min, value, this->GetObjectSchemaPtr(), error); if (!constraint) return false; - AddConstraint(constraint); - RemoveConstraint(ConstraintType::OneOf); + this->AddConstraint(constraint); + this->RemoveConstraint(ConstraintType::OneOf); + processed_keys->insert(commands::attributes::kNumeric_Min); } if (value->HasKey(commands::attributes::kNumeric_Max)) { auto constraint = LoadMinMaxConstraint<ConstraintMax<T>, T>( - commands::attributes::kNumeric_Max, value, GetObjectSchemaPtr(), + commands::attributes::kNumeric_Max, value, this->GetObjectSchemaPtr(), error); if (!constraint) return false; - AddConstraint(constraint); - RemoveConstraint(ConstraintType::OneOf); + this->AddConstraint(constraint); + this->RemoveConstraint(ConstraintType::OneOf); + processed_keys->insert(commands::attributes::kNumeric_Max); } } return true; } -// IntPropType ---------------------------------------------------------------- - -std::shared_ptr<PropType> IntPropType::Clone() const { - return std::make_shared<IntPropType>(*this); -} - -std::shared_ptr<PropValue> IntPropType::CreateValue() const { - return std::make_shared<IntValue>(this); -} - -std::shared_ptr<PropValue> IntPropType::CreateValue(const Any& val) const { - auto v = std::make_shared<IntValue>(this); - v->SetValue(val.Get<int>()); - return std::move(v); -} - -// DoublePropType ------------------------------------------------------------- - -std::shared_ptr<PropType> DoublePropType::Clone() const { - return std::make_shared<DoublePropType>(*this); -} - -std::shared_ptr<PropValue> DoublePropType::CreateValue() const { - return std::make_shared<DoubleValue>(this); -} - -std::shared_ptr<PropValue> DoublePropType::CreateValue(const Any& val) const { - auto v = std::make_shared<DoubleValue>(this); - v->SetValue(val.Get<double>()); - return std::move(v); -} - // StringPropType ------------------------------------------------------------- -std::shared_ptr<PropType> StringPropType::Clone() const { - return std::make_shared<StringPropType>(*this); -} - -std::shared_ptr<PropValue> StringPropType::CreateValue() const { - return std::make_shared<StringValue>(this); -} - -std::shared_ptr<PropValue> StringPropType::CreateValue(const Any& val) const { - auto v = std::make_shared<StringValue>(this); - v->SetValue(val.Get<std::string>()); - return std::move(v); -} - -bool StringPropType::FromJson(const base::DictionaryValue* value, - const PropType* base_schema, ErrorPtr* error) { - if (!PropType::FromJson(value, base_schema, error)) +bool StringPropType::ConstraintsFromJson( + const base::DictionaryValue* value, std::set<std::string>* processed_keys, + ErrorPtr* error) { + if (!_Base::ConstraintsFromJson(value, processed_keys, error)) return false; - if (value->HasKey(commands::attributes::kOneOf_Enum)) { - auto constraint = LoadOneOfConstraint<std::string>(value, - GetObjectSchemaPtr(), - error); - if (!constraint) - return false; - AddConstraint(constraint); - RemoveConstraint(ConstraintType::StringLengthMin); - RemoveConstraint(ConstraintType::StringLengthMax); - } else { + + if (processed_keys->find(commands::attributes::kOneOf_Enum) == + processed_keys->end()) { + // Process min/max constraints only if "enum" constraint wasn't already + // specified. if (value->HasKey(commands::attributes::kString_MinLength)) { auto constraint = LoadMinMaxConstraint<ConstraintStringLengthMin, int>( commands::attributes::kString_MinLength, value, GetObjectSchemaPtr(), @@ -335,6 +327,7 @@ return false; AddConstraint(constraint); RemoveConstraint(ConstraintType::OneOf); + processed_keys->insert(commands::attributes::kString_MinLength); } if (value->HasKey(commands::attributes::kString_MaxLength)) { auto constraint = LoadMinMaxConstraint<ConstraintStringLengthMax, int>( @@ -344,6 +337,7 @@ return false; AddConstraint(constraint); RemoveConstraint(ConstraintType::OneOf); + processed_keys->insert(commands::attributes::kString_MaxLength); } } return true; @@ -368,63 +362,16 @@ return slc ? slc->limit_.value : std::numeric_limits<int>::max(); } -// BooleanPropType ----------------------------------------------------------- - -std::shared_ptr<PropType> BooleanPropType::Clone() const { - return std::make_shared<BooleanPropType>(*this); -} - -std::shared_ptr<PropValue> BooleanPropType::CreateValue() const { - return std::make_shared<BooleanValue>(this); -} - -std::shared_ptr<PropValue> BooleanPropType::CreateValue(const Any& val) const { - auto v = std::make_shared<BooleanValue>(this); - v->SetValue(val.Get<bool>()); - return std::move(v); -} - -bool BooleanPropType::FromJson(const base::DictionaryValue* value, - const PropType* base_schema, ErrorPtr* error) { - if (!PropType::FromJson(value, base_schema, error)) - return false; - - if (value->HasKey(commands::attributes::kOneOf_Enum)) { - auto constraint = LoadOneOfConstraint<bool>(value, GetObjectSchemaPtr(), - error); - if (!constraint) - return false; - AddConstraint(constraint); - } - - return true; -} - // ObjectPropType ------------------------------------------------------------- ObjectPropType::ObjectPropType() : object_schema_(std::make_shared<ObjectSchema>(), false) {} -std::shared_ptr<PropType> ObjectPropType::Clone() const { - return std::make_shared<ObjectPropType>(*this); -} - -std::shared_ptr<PropValue> ObjectPropType::CreateValue() const { - return std::make_shared<ObjectValue>(this); -} - -std::shared_ptr<PropValue> ObjectPropType::CreateValue(const Any& val) const { - auto v = std::make_shared<ObjectValue>(this); - v->SetValue(val.Get<native_types::Object>()); - return std::move(v); -} - bool ObjectPropType::HasOverriddenAttributes() const { return PropType::HasOverriddenAttributes() || !object_schema_.is_inherited; } - std::unique_ptr<base::Value> ObjectPropType::ToJson(bool full_schema, ErrorPtr* error) const { std::unique_ptr<base::Value> value = PropType::ToJson(full_schema, error); @@ -444,9 +391,10 @@ return value; } -bool ObjectPropType::FromJson(const base::DictionaryValue* value, - const PropType* base_schema, ErrorPtr* error) { - if (!PropType::FromJson(value, base_schema, error)) +bool ObjectPropType::ObjectSchemaFromJson( + const base::DictionaryValue* value, const PropType* base_schema, + std::set<std::string>* processed_keys, ErrorPtr* error) { + if (!_Base::ObjectSchemaFromJson(value, base_schema, processed_keys, error)) return false; using commands::attributes::kObject_Properties; @@ -457,6 +405,7 @@ const base::DictionaryValue* props = nullptr; if (value->GetDictionaryWithoutPathExpansion(kObject_Properties, &props)) { + processed_keys->insert(kObject_Properties); auto object_schema = std::make_shared<ObjectSchema>(); if (!object_schema->FromJson(props, base_object_schema.get(), error)) { Error::AddTo(error, commands::errors::kDomain, @@ -477,14 +426,6 @@ return false; } - if (value->HasKey(commands::attributes::kOneOf_Enum)) { - auto constraint = LoadOneOfConstraint<native_types::Object>( - value, GetObjectSchemaPtr(), error); - if (!constraint) - return false; - AddConstraint(constraint); - } - return true; }
diff --git a/buffet/commands/prop_types.h b/buffet/commands/prop_types.h index e39b356..016d2d5 100644 --- a/buffet/commands/prop_types.h +++ b/buffet/commands/prop_types.h
@@ -8,6 +8,7 @@ #include <limits> #include <map> #include <memory> +#include <set> #include <string> #include <utility> #include <vector> @@ -98,6 +99,19 @@ // error information. virtual bool FromJson(const base::DictionaryValue* value, const PropType* base_schema, ErrorPtr* error); + // Helper function to load object schema from JSON. + virtual bool ObjectSchemaFromJson(const base::DictionaryValue* value, + const PropType* base_schema, + std::set<std::string>* processed_keys, + ErrorPtr* error) { + return true; + } + // Helper function to load type-specific constraints from JSON. + virtual bool ConstraintsFromJson(const base::DictionaryValue* value, + std::set<std::string>* processed_keys, + ErrorPtr* error) { + return true; + } // Validates a JSON value for the parameter type to make sure it satisfies // the parameter type definition including any specified constraints. @@ -142,14 +156,6 @@ bool ValidateConstraints(const PropValue& value, ErrorPtr* error) const; protected: - // Helper method to obtaining a vector of OneOf constraint values. - template<typename T> - std::vector<T> GetOneOfValuesHelper() const { - auto ofc = static_cast<const ConstraintOneOf<T>*>( - GetConstraint(ConstraintType::OneOf)); - return ofc ? ofc->set_.value : std::vector<T>(); - } - // Specifies if this parameter definition is derived from a base // object schema. bool based_on_schema_ = false; @@ -163,169 +169,129 @@ InheritableAttribute<std::shared_ptr<PropValue>> default_; }; +// Base class for all the derived concrete implementations of property +// type classes. Provides implementations for common methods of PropType base. +template<class Derived, class Value, typename T> +class PropTypeBase : public PropType { + public: + virtual ValueType GetType() const override { return GetValueType<T>(); } + virtual std::shared_ptr<PropType> Clone() const override { + return std::make_shared<Derived>(*static_cast<const Derived*>(this)); + } + virtual std::shared_ptr<PropValue> CreateValue() const override { + return std::make_shared<Value>(this); + } + virtual std::shared_ptr<PropValue> CreateValue(const Any& v) const override { + auto value = std::make_shared<Value>(this); + value->SetValue(v.Get<T>()); + return std::move(value); + } + virtual bool ConstraintsFromJson(const base::DictionaryValue* value, + std::set<std::string>* processed_keys, + ErrorPtr* error) override; + + // Helper method to obtain a vector of OneOf constraint values. + std::vector<T> GetOneOfValues() const { + auto ofc = static_cast<const ConstraintOneOf<T>*>( + this->GetConstraint(ConstraintType::OneOf)); + return ofc ? ofc->set_.value : std::vector<T>(); + } +}; + // Helper base class for Int and Double parameter types. -class NumericPropTypeBase : public PropType { - protected: - // Helper method for implementing AddMinMaxConstraint in derived classes. - template<typename T> - void AddMinMaxConstraintHelper(T min_value, T max_value) { +template<class Derived, class Value, typename T> +class NumericPropTypeBase : public PropTypeBase<Derived, Value, T> { + public: + using _Base = PropTypeBase<Derived, Value, T>; + virtual bool ConstraintsFromJson(const base::DictionaryValue* value, + std::set<std::string>* processed_keys, + ErrorPtr* error) override; + + // Helper method to set and obtain a min/max constraint values. + // Used mostly for unit testing. + void AddMinMaxConstraint(T min_value, T max_value) { InheritableAttribute<T> min_attr(min_value, false); InheritableAttribute<T> max_attr(max_value, false); - AddConstraint(std::make_shared<ConstraintMin<T>>(min_attr)); - AddConstraint(std::make_shared<ConstraintMax<T>>(max_attr)); + this->AddConstraint(std::make_shared<ConstraintMin<T>>(min_attr)); + this->AddConstraint(std::make_shared<ConstraintMax<T>>(max_attr)); } - - // Helper method for implementing GetMinValue in derived classes. - template<typename T> - T GetMinValueHelper() const { + T GetMinValue() const { auto mmc = static_cast<const ConstraintMin<T>*>( - GetConstraint(ConstraintType::Min)); + this->GetConstraint(ConstraintType::Min)); return mmc ? mmc->limit_.value : std::numeric_limits<T>::lowest(); } - - // Helper method for implementing GetMaxValue in derived classes. - template<typename T> - T GetMaxValueHelper() const { + T GetMaxValue() const { auto mmc = static_cast<const ConstraintMax<T>*>( - GetConstraint(ConstraintType::Max)); + this->GetConstraint(ConstraintType::Max)); return mmc ? mmc->limit_.value : (std::numeric_limits<T>::max)(); } - - // Helper method for implementing FromJson in derived classes. - template<typename T> - bool FromJsonHelper(const base::DictionaryValue* value, - const PropType* base_schema, ErrorPtr* error); }; // Property definition of Integer type. -class IntPropType : public NumericPropTypeBase { +class IntPropType : public NumericPropTypeBase<IntPropType, IntValue, int> { public: // Overrides from the PropType base class. - virtual ValueType GetType() const override { return ValueType::Int; } - virtual IntPropType* GetInt() override { return this; } virtual IntPropType const* GetInt() const override { return this; } - - virtual std::shared_ptr<PropType> Clone() const override; - virtual std::shared_ptr<PropValue> CreateValue() const override; - virtual std::shared_ptr<PropValue> CreateValue(const Any& val) const override; - - virtual bool FromJson(const base::DictionaryValue* value, - const PropType* base_schema, ErrorPtr* error) override { - return FromJsonHelper<int>(value, base_schema, error); - } - - // Helper methods to add and inspect simple constraints. - // Used mostly for unit testing. - void AddMinMaxConstraint(int min_value, int max_value) { - AddMinMaxConstraintHelper(min_value, max_value); - } - int GetMinValue() const { return GetMinValueHelper<int>(); } - int GetMaxValue() const { return GetMaxValueHelper<int>(); } - std::vector<int> GetOneOfValues() const { - return GetOneOfValuesHelper<int>(); - } }; // Property definition of Number type. -class DoublePropType : public NumericPropTypeBase { +class DoublePropType + : public NumericPropTypeBase<DoublePropType, DoubleValue, double> { public: // Overrides from the PropType base class. - virtual ValueType GetType() const override { return ValueType::Double; } - virtual DoublePropType* GetDouble() override { return this; } virtual DoublePropType const* GetDouble() const override { return this; } - - virtual std::shared_ptr<PropType> Clone() const override; - virtual std::shared_ptr<PropValue> CreateValue() const override; - virtual std::shared_ptr<PropValue> CreateValue(const Any& val) const override; - - virtual bool FromJson(const base::DictionaryValue* value, - const PropType* base_schema, ErrorPtr* error) override { - return FromJsonHelper<double>(value, base_schema, error); - } - - // Helper methods to add and inspect simple constraints. - // Used mostly for unit testing. - void AddMinMaxConstraint(double min_value, double max_value) { - AddMinMaxConstraintHelper(min_value, max_value); - } - double GetMinValue() const { return GetMinValueHelper<double>(); } - double GetMaxValue() const { return GetMaxValueHelper<double>(); } - std::vector<double> GetOneOfValues() const { - return GetOneOfValuesHelper<double>(); - } }; // Property definition of String type. -class StringPropType : public PropType { +class StringPropType + : public PropTypeBase<StringPropType, StringValue, std::string> { public: + using _Base = PropTypeBase<StringPropType, StringValue, std::string>; // Overrides from the PropType base class. - virtual ValueType GetType() const override { return ValueType::String; } - virtual StringPropType* GetString() override { return this; } virtual StringPropType const* GetString() const override { return this; } - virtual std::shared_ptr<PropType> Clone() const override; - virtual std::shared_ptr<PropValue> CreateValue() const override; - virtual std::shared_ptr<PropValue> CreateValue(const Any& val) const override; - - virtual bool FromJson(const base::DictionaryValue* value, - const PropType* base_schema, ErrorPtr* error) override; + virtual bool ConstraintsFromJson(const base::DictionaryValue* value, + std::set<std::string>* processed_keys, + ErrorPtr* error) override; // Helper methods to add and inspect simple constraints. // Used mostly for unit testing. void AddLengthConstraint(int min_len, int max_len); int GetMinLength() const; int GetMaxLength() const; - std::vector<std::string> GetOneOfValues() const { - return GetOneOfValuesHelper<std::string>(); - } }; // Property definition of Boolean type. -class BooleanPropType : public PropType { +class BooleanPropType + : public PropTypeBase<BooleanPropType, BooleanValue, bool> { public: // Overrides from the PropType base class. - virtual ValueType GetType() const override { return ValueType::Boolean; } - virtual BooleanPropType* GetBoolean() override { return this; } virtual BooleanPropType const* GetBoolean() const override { return this; } - - virtual std::shared_ptr<PropType> Clone() const override; - virtual std::shared_ptr<PropValue> CreateValue() const override; - virtual std::shared_ptr<PropValue> CreateValue(const Any& val) const override; - - virtual bool FromJson(const base::DictionaryValue* value, - const PropType* base_schema, ErrorPtr* error) override; - - // Helper methods to add and inspect simple constraints. - // Used mostly for unit testing. - std::vector<bool> GetOneOfValues() const { - return GetOneOfValuesHelper<bool>(); - } }; // Parameter definition of Object type. -class ObjectPropType : public PropType { +class ObjectPropType + : public PropTypeBase<ObjectPropType, ObjectValue, native_types::Object> { public: + using _Base = PropTypeBase<ObjectPropType, ObjectValue, native_types::Object>; ObjectPropType(); // Overrides from the ParamType base class. - virtual ValueType GetType() const override { return ValueType::Object; } virtual bool HasOverriddenAttributes() const override; virtual ObjectPropType* GetObject() override { return this; } virtual ObjectPropType const* GetObject() const override { return this; } - virtual std::shared_ptr<PropType> Clone() const override; - virtual std::shared_ptr<PropValue> CreateValue() const override; - virtual std::shared_ptr<PropValue> CreateValue(const Any& val) const override; - virtual std::unique_ptr<base::Value> ToJson(bool full_schema, ErrorPtr* error) const override; - virtual bool FromJson(const base::DictionaryValue* value, - const PropType* base_schema, ErrorPtr* error) override; + virtual bool ObjectSchemaFromJson(const base::DictionaryValue* value, + const PropType* base_schema, + std::set<std::string>* processed_keys, + ErrorPtr* error) override; virtual std::shared_ptr<const ObjectSchema> GetObjectSchema() const override { return object_schema_.value;