From 35b40f5c98e55c805b17525d29dc5963f45e91b4 Mon Sep 17 00:00:00 2001 From: iorveth Date: Mon, 13 Apr 2020 13:03:57 +0300 Subject: [PATCH 01/26] avoid potential panic, when completing entity property values update --- runtime-modules/content-directory/src/lib.rs | 24 ++++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index 71060e048c..37880cf6fc 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -809,21 +809,21 @@ impl Module { // Iterate over a vector of new values and update corresponding properties // of this entity if new values are valid. - for (id, new_value) in new_property_values.iter() { + for (id, new_value) in new_property_values.into_iter() { // Try to find a current property value in the entity // by matching its id to the id of a property with an updated value. - if let Some(current_prop_value) = updated_values.get_mut(id) { + if let Some(current_prop_value) = updated_values.get_mut(&id) { // Get class-level information about this property - let class_prop = &class.properties[*id as usize]; - - // Validate a new property value against the type of this property - // and check any additional constraints like the length of a vector - // if it's a vector property or the length of a text if it's a text property. - Self::ensure_property_value_is_valid(new_value, class_prop)?; - - // Update a current prop value in a mutable vector, if a new value is valid. - *current_prop_value = new_value.to_owned(); - updates_count += 1; + if let Some(class_prop) = class.properties.get(id as usize) { + // Validate a new property value against the type of this property + // and check any additional constraints like the length of a vector + // if it's a vector property or the length of a text if it's a text property. + Self::ensure_property_value_is_valid(&new_value, class_prop)?; + + // Update a current prop value in a mutable vector, if a new value is valid. + *current_prop_value = new_value; + updates_count += 1; + } } else { // Throw an error if a property was not found on entity // by an in-class index of a property update. From d7f3269e267a732ef0edd50a9483edd23f961ed5 Mon Sep 17 00:00:00 2001 From: iorveth Date: Mon, 13 Apr 2020 23:48:13 +0300 Subject: [PATCH 02/26] clear, remove_at property value level vector operations initial implementation. Begun work on insert_at implementation --- runtime-modules/content-directory/src/lib.rs | 277 ++++++++++++++++++- 1 file changed, 269 insertions(+), 8 deletions(-) diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index 37880cf6fc..a6d48aafca 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -334,6 +334,53 @@ pub enum PropertyValue { // ExternalVec(Vec), } +#[cfg_attr(feature = "std", derive(Serialize, Deserialize))] +#[derive(Encode, Decode, Clone, PartialEq, Eq, Debug)] +enum PropertyValueType { + SingleValue, + Vector +} + +impl PropertyValue { + fn vec_clear(&mut self) { + match self { + PropertyValue::BoolVec(vec) => *vec = vec![], + PropertyValue::Uint16Vec(vec) => *vec = vec![], + PropertyValue::Uint32Vec(vec) => *vec = vec![], + PropertyValue::Uint64Vec(vec) => *vec = vec![], + PropertyValue::Int16Vec(vec) => *vec = vec![], + PropertyValue::Int32Vec(vec) => *vec = vec![], + PropertyValue::Int64Vec(vec) => *vec = vec![], + PropertyValue::TextVec(vec) => *vec = vec![], + PropertyValue::ReferenceVec(vec) => *vec = vec![], + _ => () + } + } + + fn vec_remove_at(&mut self, index_in_property_vec: u32) { + + fn remove_at_checked(vec: &mut Vec, index_in_property_vec: u32) { + if (index_in_property_vec as usize) < vec.len() { + vec.remove(index_in_property_vec as usize); + } + } + + match self { + PropertyValue::BoolVec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Uint16Vec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Uint32Vec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Uint64Vec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Int16Vec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Int32Vec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Int64Vec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::TextVec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::ReferenceVec(vec) => remove_at_checked(vec, index_in_property_vec), + _ => () + } + } + +} + impl Default for PropertyValue { fn default() -> Self { PropertyValue::Bool(false) @@ -648,6 +695,50 @@ decl_module! { Self::do_update_entity_property_values(&raw_origin, with_credential, as_entity_maintainer, entity_id, property_values) } + pub fn clear_entity_property_vector( + origin, + with_credential: Option, + as_entity_maintainer: bool, + entity_id: EntityId, + in_class_schema_property_id: u16 + ) -> dispatch::Result { + let raw_origin = Self::ensure_root_or_signed(origin)?; + Self::do_clear_entity_property_vector(&raw_origin, with_credential, as_entity_maintainer, entity_id, in_class_schema_property_id) + } + + pub fn remove_at_entity_property_vector( + origin, + with_credential: Option, + as_entity_maintainer: bool, + entity_id: EntityId, + in_class_schema_property_id: u16, + index_in_property_vec: u32 + ) -> dispatch::Result { + let raw_origin = Self::ensure_root_or_signed(origin)?; + Self::do_remove_at_entity_property_vector(&raw_origin, with_credential, as_entity_maintainer, entity_id, in_class_schema_property_id, index_in_property_vec) + } + + pub fn insert_at_entity_property_vector( + origin, + with_credential: Option, + as_entity_maintainer: bool, + entity_id: EntityId, + in_class_schema_property_id: u16, + index_in_property_vec: u32, + property_value: PropertyValue + ) -> dispatch::Result { + let raw_origin = Self::ensure_root_or_signed(origin)?; + Self::do_insert_at_entity_property_vector( + &raw_origin, + with_credential, + as_entity_maintainer, + entity_id, + in_class_schema_property_id, + index_in_property_vec, + property_value + ) + } + pub fn transaction(origin, operations: Vec>) -> dispatch::Result { // This map holds the EntityId of the entity created as a result of executing a CreateEntity Operation // keyed by the indexed of the operation, in the operations vector. @@ -781,6 +872,90 @@ impl Module { ) } + fn do_clear_entity_property_vector( + raw_origin: &system::RawOrigin, + with_credential: Option, + as_entity_maintainer: bool, + entity_id: EntityId, + in_class_schema_property_id: u16, + ) -> dispatch::Result { + let class_id = Self::get_class_id_by_entity_id(entity_id)?; + + let as_entity_maintainer = if as_entity_maintainer { + Some(entity_id) + } else { + None + }; + + Self::if_class_permissions_satisfied( + raw_origin, + with_credential, + as_entity_maintainer, + ClassPermissions::can_update_entity, + class_id, + |_class_permissions, _access_level| { + Self::complete_entity_property_vector_cleaning(entity_id, in_class_schema_property_id) + }, + ) + } + + fn do_remove_at_entity_property_vector( + raw_origin: &system::RawOrigin, + with_credential: Option, + as_entity_maintainer: bool, + entity_id: EntityId, + in_class_schema_property_id: u16, + index_in_property_vec: u32 + ) -> dispatch::Result { + let class_id = Self::get_class_id_by_entity_id(entity_id)?; + + let as_entity_maintainer = if as_entity_maintainer { + Some(entity_id) + } else { + None + }; + + Self::if_class_permissions_satisfied( + raw_origin, + with_credential, + as_entity_maintainer, + ClassPermissions::can_update_entity, + class_id, + |_class_permissions, _access_level| { + Self::complete_remove_at_entity_property_vector(entity_id, in_class_schema_property_id, index_in_property_vec) + }, + ) + } + + fn do_insert_at_entity_property_vector( + raw_origin: &system::RawOrigin, + with_credential: Option, + as_entity_maintainer: bool, + entity_id: EntityId, + in_class_schema_property_id: u16, + index_in_property_vec: u32, + property_value: PropertyValue + ) -> dispatch::Result { + let class_id = Self::get_class_id_by_entity_id(entity_id)?; + + let as_entity_maintainer = if as_entity_maintainer { + Some(entity_id) + } else { + None + }; + + Self::if_class_permissions_satisfied( + raw_origin, + with_credential, + as_entity_maintainer, + ClassPermissions::can_update_entity, + class_id, + |_class_permissions, _access_level| { + Self::complete_insert_at_entity_property_vector(entity_id, in_class_schema_property_id, index_in_property_vec, property_value) + }, + ) + } + pub fn complete_class_schema_status_update( class_id: ClassId, schema_id: u16, // Do not type alias u16!! - u16, @@ -805,7 +980,7 @@ impl Module { // Get current property values of an entity as a mutable vector, // so we can update them if new values provided present in new_property_values. let mut updated_values = entity.values; - let mut updates_count = 0; + let mut updated = false; // Iterate over a vector of new values and update corresponding properties // of this entity if new values are valid. @@ -818,11 +993,11 @@ impl Module { // Validate a new property value against the type of this property // and check any additional constraints like the length of a vector // if it's a vector property or the length of a text if it's a text property. - Self::ensure_property_value_is_valid(&new_value, class_prop)?; + Self::ensure_property_value_to_update_is_valid(&new_value, class_prop)?; // Update a current prop value in a mutable vector, if a new value is valid. *current_prop_value = new_value; - updates_count += 1; + updated = !updated; } } else { // Throw an error if a property was not found on entity @@ -831,8 +1006,8 @@ impl Module { } } - // If at least one of the entity property values should be update: - if updates_count > 0 { + // If property values should be update: + if updated { EntityById::mutate(entity_id, |entity| { entity.values = updated_values; }); @@ -841,6 +1016,82 @@ impl Module { Ok(()) } + fn complete_entity_property_vector_cleaning( + entity_id: EntityId, + in_class_schema_property_id: u16, + ) -> dispatch::Result { + Self::ensure_known_entity_id(&entity_id)?; + let entity = Self::entity_by_id(entity_id); + + if !entity.values.contains_key(&in_class_schema_property_id) { + // Throw an error if a property was not found on entity + // by an in-class index of a property update. + return Err(ERROR_UNKNOWN_ENTITY_PROP_ID); + } + + // Clear property value vector: + EntityById::mutate(entity_id, |entity| entity.values.get_mut(&in_class_schema_property_id).as_deref_mut() + .map(|current_property_value_vec| current_property_value_vec.vec_clear())); + + Ok(()) + } + + fn complete_remove_at_entity_property_vector( + entity_id: EntityId, + in_class_schema_property_id: u16, + index_in_property_vec: u32 + ) -> dispatch::Result { + + Self::ensure_known_entity_id(&entity_id)?; + let entity = Self::entity_by_id(entity_id); + + if !entity.values.contains_key(&in_class_schema_property_id) { + // Throw an error if a property was not found on entity + // by an in-class index of a property update. + return Err(ERROR_UNKNOWN_ENTITY_PROP_ID); + } + + // Remove property value vector + EntityById::mutate(entity_id, |entity| entity.values.get_mut(&in_class_schema_property_id).as_deref_mut() + .map(|current_prop_value| current_prop_value.vec_remove_at(index_in_property_vec))); + + Ok(()) + } + + fn complete_insert_at_entity_property_vector( + entity_id: EntityId, + in_class_schema_property_id: u16, + index_in_property_vec: u32, + property_value: PropertyValue + ) -> dispatch::Result { + + Self::ensure_known_entity_id(&entity_id)?; + + let (mut entity, class) = Self::get_entity_and_class(entity_id); + + // Try to find a current property value in the entity + // by matching its id to the id of a property with an updated value. + if let Some(current_prop_value) = entity.values.get_mut(&in_class_schema_property_id) { + // Get class-level information about this property + if let Some(class_prop) = class.properties.get(in_class_schema_property_id as usize) { + // Validate a new property value against the type of this property + // and check any additional constraints like the length of a vector + // if it's a vector property or the length of a text if it's a text property. + Self::ensure_value_to_insert_at_vec_is_valid(&property_value, class_prop)?; + } + } else { + // Throw an error if a property was not found on entity + // by an in-class index of a property update. + return Err(ERROR_UNKNOWN_ENTITY_PROP_ID); + } + + // Insert property value into property value vector + // EntityById::mutate(entity_id, |entity| entity.values.get_mut(&in_class_schema_property_id).as_deref_mut() + // .map(|current_prop_value| current_prop_value.vec_insert_at(index_in_property_vec, property_value))); + + Ok(()) + } + fn do_add_schema_support_to_entity( raw_origin: &system::RawOrigin, with_credential: Option, @@ -996,7 +1247,7 @@ impl Module { Ok(entity.class_id) } - // Ensures property_values of type Internal that point to a class, + // Ensures property_values of type Reference that point to a class, // the target entity and class exists and constraint allows it. fn ensure_internal_property_values_permitted( source_class_id: ClassId, @@ -1142,7 +1393,7 @@ impl Module { // If a value was not povided for the property of this schema: if let Some(new_value) = property_values.get(prop_id) { - Self::ensure_property_value_is_valid(new_value, class_prop)?; + Self::ensure_property_value_to_update_is_valid(new_value, class_prop)?; appended_entity_values.insert(*prop_id, new_value.to_owned()); } else { @@ -1250,7 +1501,7 @@ impl Module { (entity, class) } - pub fn ensure_property_value_is_valid( + pub fn ensure_property_value_to_update_is_valid( value: &PropertyValue, prop: &Property, ) -> dispatch::Result { @@ -1261,6 +1512,16 @@ impl Module { Ok(()) } + pub fn ensure_value_to_insert_at_vec_is_valid( + value: &PropertyValue, + prop: &Property, + ) -> dispatch::Result { + Self::ensure_prop_value_matches_its_type(value, prop)?; + Self::ensure_valid_internal_prop(value, prop)?; + Self::validate_max_len_if_vec_prop(value, prop)?; + Ok(()) + } + pub fn validate_max_len_if_text_prop( value: &PropertyValue, prop: &Property, From d4a085dcac9fd861ca75e90a4dfb33ac64515f8e Mon Sep 17 00:00:00 2001 From: iorveth Date: Wed, 15 Apr 2020 01:48:42 +0300 Subject: [PATCH 03/26] Property value level insert_at vector operation implemented --- .../content-directory/src/errors.rs | 6 +- runtime-modules/content-directory/src/lib.rs | 260 ++++++++++++++---- 2 files changed, 204 insertions(+), 62 deletions(-) diff --git a/runtime-modules/content-directory/src/errors.rs b/runtime-modules/content-directory/src/errors.rs index 14180fe095..d6b35aa9ed 100644 --- a/runtime-modules/content-directory/src/errors.rs +++ b/runtime-modules/content-directory/src/errors.rs @@ -29,6 +29,8 @@ pub const ERROR_SCHEMA_ALREADY_ADDED_TO_ENTITY: &str = "Cannot add a schema that is already added to this entity"; pub const ERROR_PROP_VALUE_DONT_MATCH_TYPE: &str = "Some of the provided property values don't match the expected property type"; +pub const ERROR_PROP_VALUE_DONT_MATCH_VEC_TYPE: &str = + "Property value don't match the expected vector property type"; pub const ERROR_PROP_NAME_NOT_UNIQUE_IN_CLASS: &str = "Property name is not unique within its class"; pub const ERROR_MISSING_REQUIRED_PROP: &str = @@ -36,5 +38,7 @@ pub const ERROR_MISSING_REQUIRED_PROP: &str = pub const ERROR_UNKNOWN_ENTITY_PROP_ID: &str = "Some of the provided property ids cannot be found on the current list of propery values of this entity"; pub const ERROR_TEXT_PROP_IS_TOO_LONG: &str = "Text propery is too long"; pub const ERROR_VEC_PROP_IS_TOO_LONG: &str = "Vector propery is too long"; +pub const ERROR_ENTITY_PROP_VALUE_VECTOR_IS_TOO_LONG: &str = + "Propery value vector can`t contain more values"; pub const ERROR_INTERNAL_RPOP_DOES_NOT_MATCH_ITS_CLASS: &str = - "Internal property does not match its class"; + "Internal property does not match its class"; \ No newline at end of file diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index a6d48aafca..bf73936dfa 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -338,7 +338,7 @@ pub enum PropertyValue { #[derive(Encode, Decode, Clone, PartialEq, Eq, Debug)] enum PropertyValueType { SingleValue, - Vector + Vector, } impl PropertyValue { @@ -353,32 +353,69 @@ impl PropertyValue { PropertyValue::Int64Vec(vec) => *vec = vec![], PropertyValue::TextVec(vec) => *vec = vec![], PropertyValue::ReferenceVec(vec) => *vec = vec![], - _ => () + _ => (), } } fn vec_remove_at(&mut self, index_in_property_vec: u32) { - fn remove_at_checked(vec: &mut Vec, index_in_property_vec: u32) { - if (index_in_property_vec as usize) < vec.len() { + if (index_in_property_vec as usize) < vec.len() { vec.remove(index_in_property_vec as usize); } } match self { PropertyValue::BoolVec(vec) => remove_at_checked(vec, index_in_property_vec), - PropertyValue::Uint16Vec(vec) => remove_at_checked(vec, index_in_property_vec), - PropertyValue::Uint32Vec(vec) => remove_at_checked(vec, index_in_property_vec), - PropertyValue::Uint64Vec(vec) => remove_at_checked(vec, index_in_property_vec), - PropertyValue::Int16Vec(vec) => remove_at_checked(vec, index_in_property_vec), - PropertyValue::Int32Vec(vec) => remove_at_checked(vec, index_in_property_vec), - PropertyValue::Int64Vec(vec) => remove_at_checked(vec, index_in_property_vec), - PropertyValue::TextVec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Uint16Vec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Uint32Vec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Uint64Vec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Int16Vec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Int32Vec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Int64Vec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::TextVec(vec) => remove_at_checked(vec, index_in_property_vec), PropertyValue::ReferenceVec(vec) => remove_at_checked(vec, index_in_property_vec), - _ => () + _ => (), } } + fn vec_insert_at(&mut self, index_in_property_vec: u32, property_value: Self) { + fn insert_at(vec: &mut Vec, index_in_property_vec: u32, value: T) { + if (index_in_property_vec as usize) < vec.len() { + vec.insert(index_in_property_vec as usize, value); + } + } + + match (self, property_value) { + (PropertyValue::BoolVec(vec), PropertyValue::Bool(value)) => { + insert_at(vec, index_in_property_vec, value) + } + (PropertyValue::Uint16Vec(vec), PropertyValue::Uint16(value)) => { + insert_at(vec, index_in_property_vec, value) + } + (PropertyValue::Uint32Vec(vec), PropertyValue::Uint32(value)) => { + insert_at(vec, index_in_property_vec, value) + } + (PropertyValue::Uint64Vec(vec), PropertyValue::Uint64(value)) => { + insert_at(vec, index_in_property_vec, value) + } + (PropertyValue::Int16Vec(vec), PropertyValue::Int16(value)) => { + insert_at(vec, index_in_property_vec, value) + } + (PropertyValue::Int32Vec(vec), PropertyValue::Int32(value)) => { + insert_at(vec, index_in_property_vec, value) + } + (PropertyValue::Int64Vec(vec), PropertyValue::Int64(value)) => { + insert_at(vec, index_in_property_vec, value) + } + (PropertyValue::TextVec(vec), PropertyValue::Text(ref value)) => { + insert_at(vec, index_in_property_vec, value.to_owned()) + } + (PropertyValue::ReferenceVec(vec), PropertyValue::Reference(value)) => { + insert_at(vec, index_in_property_vec, value) + } + _ => (), + } + } } impl Default for PropertyValue { @@ -729,16 +766,16 @@ decl_module! { ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; Self::do_insert_at_entity_property_vector( - &raw_origin, - with_credential, - as_entity_maintainer, - entity_id, - in_class_schema_property_id, - index_in_property_vec, + &raw_origin, + with_credential, + as_entity_maintainer, + entity_id, + in_class_schema_property_id, + index_in_property_vec, property_value ) } - + pub fn transaction(origin, operations: Vec>) -> dispatch::Result { // This map holds the EntityId of the entity created as a result of executing a CreateEntity Operation // keyed by the indexed of the operation, in the operations vector. @@ -894,7 +931,10 @@ impl Module { ClassPermissions::can_update_entity, class_id, |_class_permissions, _access_level| { - Self::complete_entity_property_vector_cleaning(entity_id, in_class_schema_property_id) + Self::complete_entity_property_vector_cleaning( + entity_id, + in_class_schema_property_id, + ) }, ) } @@ -905,7 +945,7 @@ impl Module { as_entity_maintainer: bool, entity_id: EntityId, in_class_schema_property_id: u16, - index_in_property_vec: u32 + index_in_property_vec: u32, ) -> dispatch::Result { let class_id = Self::get_class_id_by_entity_id(entity_id)?; @@ -922,7 +962,11 @@ impl Module { ClassPermissions::can_update_entity, class_id, |_class_permissions, _access_level| { - Self::complete_remove_at_entity_property_vector(entity_id, in_class_schema_property_id, index_in_property_vec) + Self::complete_remove_at_entity_property_vector( + entity_id, + in_class_schema_property_id, + index_in_property_vec, + ) }, ) } @@ -934,7 +978,7 @@ impl Module { entity_id: EntityId, in_class_schema_property_id: u16, index_in_property_vec: u32, - property_value: PropertyValue + property_value: PropertyValue, ) -> dispatch::Result { let class_id = Self::get_class_id_by_entity_id(entity_id)?; @@ -951,7 +995,12 @@ impl Module { ClassPermissions::can_update_entity, class_id, |_class_permissions, _access_level| { - Self::complete_insert_at_entity_property_vector(entity_id, in_class_schema_property_id, index_in_property_vec, property_value) + Self::complete_insert_at_entity_property_vector( + entity_id, + in_class_schema_property_id, + index_in_property_vec, + property_value, + ) }, ) } @@ -1007,7 +1056,7 @@ impl Module { } // If property values should be update: - if updated { + if updated { EntityById::mutate(entity_id, |entity| { entity.values = updated_values; }); @@ -1027,11 +1076,16 @@ impl Module { // Throw an error if a property was not found on entity // by an in-class index of a property update. return Err(ERROR_UNKNOWN_ENTITY_PROP_ID); - } - + } + // Clear property value vector: - EntityById::mutate(entity_id, |entity| entity.values.get_mut(&in_class_schema_property_id).as_deref_mut() - .map(|current_property_value_vec| current_property_value_vec.vec_clear())); + EntityById::mutate(entity_id, |entity| { + entity + .values + .get_mut(&in_class_schema_property_id) + .as_deref_mut() + .map(|current_property_value_vec| current_property_value_vec.vec_clear()) + }); Ok(()) } @@ -1039,9 +1093,8 @@ impl Module { fn complete_remove_at_entity_property_vector( entity_id: EntityId, in_class_schema_property_id: u16, - index_in_property_vec: u32 + index_in_property_vec: u32, ) -> dispatch::Result { - Self::ensure_known_entity_id(&entity_id)?; let entity = Self::entity_by_id(entity_id); @@ -1049,12 +1102,17 @@ impl Module { // Throw an error if a property was not found on entity // by an in-class index of a property update. return Err(ERROR_UNKNOWN_ENTITY_PROP_ID); - } - + } + // Remove property value vector - EntityById::mutate(entity_id, |entity| entity.values.get_mut(&in_class_schema_property_id).as_deref_mut() - .map(|current_prop_value| current_prop_value.vec_remove_at(index_in_property_vec))); - + EntityById::mutate(entity_id, |entity| { + entity + .values + .get_mut(&in_class_schema_property_id) + .as_deref_mut() + .map(|current_prop_value| current_prop_value.vec_remove_at(index_in_property_vec)) + }); + Ok(()) } @@ -1062,22 +1120,25 @@ impl Module { entity_id: EntityId, in_class_schema_property_id: u16, index_in_property_vec: u32, - property_value: PropertyValue + property_value: PropertyValue, ) -> dispatch::Result { - Self::ensure_known_entity_id(&entity_id)?; - let (mut entity, class) = Self::get_entity_and_class(entity_id); + let (entity, class) = Self::get_entity_and_class(entity_id); // Try to find a current property value in the entity // by matching its id to the id of a property with an updated value. - if let Some(current_prop_value) = entity.values.get_mut(&in_class_schema_property_id) { + if let Some(entity_prop_value) = entity.values.get(&in_class_schema_property_id) { // Get class-level information about this property if let Some(class_prop) = class.properties.get(in_class_schema_property_id as usize) { // Validate a new property value against the type of this property // and check any additional constraints like the length of a vector // if it's a vector property or the length of a text if it's a text property. - Self::ensure_value_to_insert_at_vec_is_valid(&property_value, class_prop)?; + Self::ensure_prop_value_can_be_insert_at_prop_vec( + &property_value, + entity_prop_value, + class_prop, + )?; } } else { // Throw an error if a property was not found on entity @@ -1086,8 +1147,15 @@ impl Module { } // Insert property value into property value vector - // EntityById::mutate(entity_id, |entity| entity.values.get_mut(&in_class_schema_property_id).as_deref_mut() - // .map(|current_prop_value| current_prop_value.vec_insert_at(index_in_property_vec, property_value))); + EntityById::mutate(entity_id, |entity| { + entity + .values + .get_mut(&in_class_schema_property_id) + .as_deref_mut() + .map(|current_prop_value| { + current_prop_value.vec_insert_at(index_in_property_vec, property_value) + }) + }); Ok(()) } @@ -1512,13 +1580,13 @@ impl Module { Ok(()) } - pub fn ensure_value_to_insert_at_vec_is_valid( + pub fn ensure_prop_value_can_be_insert_at_prop_vec( value: &PropertyValue, + entity_prop_value: &PropertyValue, prop: &Property, ) -> dispatch::Result { - Self::ensure_prop_value_matches_its_type(value, prop)?; Self::ensure_valid_internal_prop(value, prop)?; - Self::validate_max_len_if_vec_prop(value, prop)?; + Self::validate_prop_value_can_be_insert_at_prop_vec(value, entity_prop_value, prop)?; Ok(()) } @@ -1544,21 +1612,21 @@ impl Module { value: &PropertyValue, prop: &Property, ) -> dispatch::Result { - fn validate_slice_len(vec: &[T], max_len: &u16) -> bool { + fn validate_vec_len(vec: &[T], max_len: &u16) -> bool { vec.len() <= *max_len as usize } let is_valid_len = match (value, &prop.prop_type) { - (PV::BoolVec(vec), PT::BoolVec(max_len)) => validate_slice_len(vec, max_len), - (PV::Uint16Vec(vec), PT::Uint16Vec(max_len)) => validate_slice_len(vec, max_len), - (PV::Uint32Vec(vec), PT::Uint32Vec(max_len)) => validate_slice_len(vec, max_len), - (PV::Uint64Vec(vec), PT::Uint64Vec(max_len)) => validate_slice_len(vec, max_len), - (PV::Int16Vec(vec), PT::Int16Vec(max_len)) => validate_slice_len(vec, max_len), - (PV::Int32Vec(vec), PT::Int32Vec(max_len)) => validate_slice_len(vec, max_len), - (PV::Int64Vec(vec), PT::Int64Vec(max_len)) => validate_slice_len(vec, max_len), + (PV::BoolVec(vec), PT::BoolVec(max_len)) => validate_vec_len(vec, max_len), + (PV::Uint16Vec(vec), PT::Uint16Vec(max_len)) => validate_vec_len(vec, max_len), + (PV::Uint32Vec(vec), PT::Uint32Vec(max_len)) => validate_vec_len(vec, max_len), + (PV::Uint64Vec(vec), PT::Uint64Vec(max_len)) => validate_vec_len(vec, max_len), + (PV::Int16Vec(vec), PT::Int16Vec(max_len)) => validate_vec_len(vec, max_len), + (PV::Int32Vec(vec), PT::Int32Vec(max_len)) => validate_vec_len(vec, max_len), + (PV::Int64Vec(vec), PT::Int64Vec(max_len)) => validate_vec_len(vec, max_len), (PV::TextVec(vec), PT::TextVec(vec_max_len, text_max_len)) => { - if validate_slice_len(vec, vec_max_len) { + if validate_vec_len(vec, vec_max_len) { for text_item in vec.iter() { Self::validate_max_len_of_text(text_item, *text_max_len)?; } @@ -1570,7 +1638,7 @@ impl Module { (PV::ReferenceVec(vec), PT::ReferenceVec(vec_max_len, class_id)) => { Self::ensure_known_class_id(class_id)?; - if validate_slice_len(vec, vec_max_len) { + if validate_vec_len(vec, vec_max_len) { for entity_id in vec.iter() { Self::ensure_known_entity_id(entity_id)?; let entity = Self::entity_by_id(entity_id); @@ -1599,11 +1667,11 @@ impl Module { value: &PropertyValue, prop: &Property, ) -> dispatch::Result { - if Self::does_prop_value_match_type(value, prop) { - Ok(()) - } else { - Err(ERROR_PROP_VALUE_DONT_MATCH_TYPE) - } + ensure!( + Self::does_prop_value_match_type(value, prop), + ERROR_PROP_VALUE_DONT_MATCH_TYPE + ); + Ok(()) } pub fn does_prop_value_match_type(value: &PropertyValue, prop: &Property) -> bool { @@ -1638,6 +1706,76 @@ impl Module { } } + pub fn validate_prop_value_can_be_insert_at_prop_vec( + value: &PropertyValue, + entity_prop_value: &PropertyValue, + prop: &Property, + ) -> dispatch::Result { + fn validate_prop_vec_len_after_value_insert(vec: &[T], max_len: &u16) -> bool { + vec.len() < *max_len as usize + } + + // A non required property can be updated to None: + if !prop.required && *value == PV::Bool(false) { + return Ok(()); + } + + let is_valid_len = match (value, entity_prop_value, &prop.prop_type) { + // Single values + (PV::Bool(_), PV::BoolVec(vec), PT::BoolVec(max_len)) => { + validate_prop_vec_len_after_value_insert(vec, max_len) + } + (PV::Uint16(_), PV::Uint16Vec(vec), PT::Uint16Vec(max_len)) => { + validate_prop_vec_len_after_value_insert(vec, max_len) + } + (PV::Uint32(_), PV::Uint32Vec(vec), PT::Uint32Vec(max_len)) => { + validate_prop_vec_len_after_value_insert(vec, max_len) + } + (PV::Uint64(_), PV::Uint64Vec(vec), PT::Uint64Vec(max_len)) => { + validate_prop_vec_len_after_value_insert(vec, max_len) + } + (PV::Int16(_), PV::Int16Vec(vec), PT::Int16Vec(max_len)) => { + validate_prop_vec_len_after_value_insert(vec, max_len) + } + (PV::Int32(_), PV::Int32Vec(vec), PT::Int32Vec(max_len)) => { + validate_prop_vec_len_after_value_insert(vec, max_len) + } + (PV::Int64(_), PV::Int64Vec(vec), PT::Int64Vec(max_len)) => { + validate_prop_vec_len_after_value_insert(vec, max_len) + } + (PV::Text(text_item), PV::TextVec(vec), PT::TextVec(vec_max_len, text_max_len)) => { + if validate_prop_vec_len_after_value_insert(vec, vec_max_len) { + Self::validate_max_len_of_text(text_item, *text_max_len)?; + true + } else { + false + } + } + ( + PV::Reference(entity_id), + PV::ReferenceVec(vec), + PT::ReferenceVec(vec_max_len, class_id), + ) => { + Self::ensure_known_class_id(class_id)?; + if validate_prop_vec_len_after_value_insert(vec, vec_max_len) { + Self::ensure_known_entity_id(entity_id)?; + let entity = Self::entity_by_id(entity_id); + ensure!( + entity.class_id == *class_id, + ERROR_INTERNAL_RPOP_DOES_NOT_MATCH_ITS_CLASS + ); + true + } else { + false + } + } + _ => false, + }; + + ensure!(is_valid_len, ERROR_ENTITY_PROP_VALUE_VECTOR_IS_TOO_LONG); + Ok(()) + } + pub fn ensure_property_name_is_valid(text: &Vec) -> dispatch::Result { T::PropertyNameConstraint::get().ensure_valid( text.len(), From a778bedd8e5911183f410621055432afe97175d2 Mon Sep 17 00:00:00 2001 From: iorveth Date: Wed, 15 Apr 2020 16:09:22 +0300 Subject: [PATCH 04/26] insert_at error improvements --- runtime-modules/content-directory/src/errors.rs | 2 ++ runtime-modules/content-directory/src/lib.rs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/runtime-modules/content-directory/src/errors.rs b/runtime-modules/content-directory/src/errors.rs index d6b35aa9ed..45ebb946b2 100644 --- a/runtime-modules/content-directory/src/errors.rs +++ b/runtime-modules/content-directory/src/errors.rs @@ -40,5 +40,7 @@ pub const ERROR_TEXT_PROP_IS_TOO_LONG: &str = "Text propery is too long"; pub const ERROR_VEC_PROP_IS_TOO_LONG: &str = "Vector propery is too long"; pub const ERROR_ENTITY_PROP_VALUE_VECTOR_IS_TOO_LONG: &str = "Propery value vector can`t contain more values"; +pub const ERROR_PROP_VALUE_TYPE_DOESNT_MATCH_INTERNAL_ENTITY_VECTOR_TYPE: &str = + "Propery value type does not match internal entity vector type"; pub const ERROR_INTERNAL_RPOP_DOES_NOT_MATCH_ITS_CLASS: &str = "Internal property does not match its class"; \ No newline at end of file diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index bf73936dfa..d59f40e357 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -1769,7 +1769,7 @@ impl Module { false } } - _ => false, + _ => return Err(ERROR_PROP_VALUE_TYPE_DOESNT_MATCH_INTERNAL_ENTITY_VECTOR_TYPE), }; ensure!(is_valid_len, ERROR_ENTITY_PROP_VALUE_VECTOR_IS_TOO_LONG); From eaf1aefa15f923cef32db10e44a685bb6d149915 Mon Sep 17 00:00:00 2001 From: iorveth Date: Wed, 15 Apr 2020 21:54:38 +0300 Subject: [PATCH 05/26] Security: implement mechanism for entity vector level specific operations to avoid possible data races --- .../content-directory/src/errors.rs | 8 +- runtime-modules/content-directory/src/lib.rs | 220 +++++++++++++----- .../content-directory/src/tests.rs | 22 +- 3 files changed, 181 insertions(+), 69 deletions(-) diff --git a/runtime-modules/content-directory/src/errors.rs b/runtime-modules/content-directory/src/errors.rs index 45ebb946b2..6ecf91ac25 100644 --- a/runtime-modules/content-directory/src/errors.rs +++ b/runtime-modules/content-directory/src/errors.rs @@ -31,6 +31,10 @@ pub const ERROR_PROP_VALUE_DONT_MATCH_TYPE: &str = "Some of the provided property values don't match the expected property type"; pub const ERROR_PROP_VALUE_DONT_MATCH_VEC_TYPE: &str = "Property value don't match the expected vector property type"; +pub const ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR: &str = + "Property value under given index is not a vector"; +pub const ERROR_PROP_VALUE_VEC_WAS_ALREADY_UPDATED: &str = + "Property value vector was already updated in this block, vector specific operations forbidden to avoid possible data races"; pub const ERROR_PROP_NAME_NOT_UNIQUE_IN_CLASS: &str = "Property name is not unique within its class"; pub const ERROR_MISSING_REQUIRED_PROP: &str = @@ -42,5 +46,5 @@ pub const ERROR_ENTITY_PROP_VALUE_VECTOR_IS_TOO_LONG: &str = "Propery value vector can`t contain more values"; pub const ERROR_PROP_VALUE_TYPE_DOESNT_MATCH_INTERNAL_ENTITY_VECTOR_TYPE: &str = "Propery value type does not match internal entity vector type"; -pub const ERROR_INTERNAL_RPOP_DOES_NOT_MATCH_ITS_CLASS: &str = - "Internal property does not match its class"; \ No newline at end of file +pub const ERROR_INTERNAL_PROP_DOES_NOT_MATCH_ITS_CLASS: &str = + "Internal property does not match its class"; diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index d59f40e357..ce0614c150 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -210,8 +210,8 @@ pub type ClassPermissionsType = ClassPermissions::Credential, u16, ::BlockNumber>; #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] -#[derive(Encode, Decode, Default, Clone, PartialEq, Eq, Debug)] -pub struct Entity { +#[derive(Encode, Decode, Clone, PartialEq, Eq, Debug)] +pub struct Entity { /// The class id of this entity. pub class_id: ClassId, @@ -223,9 +223,39 @@ pub struct Entity { /// Values for properties on class that are used by some schema used by this entity! /// Length is no more than Class.properties. pub values: BTreeMap, + + /// Map, representing relation between entity vec_values index and block, where vec_value was updated + /// Used to forbid mutating property vec value more than once per block for purpose of race update conditions avoiding + pub vec_values_last_update: BTreeMap::BlockNumber>, // pub deleted: bool, } +impl Default for Entity { + fn default() -> Self { + Self { + class_id: ClassId::default(), + supported_schemas: BTreeSet::new(), + values: BTreeMap::new(), + vec_values_last_update: BTreeMap::new(), + } + } +} + +impl Entity { + fn new( + class_id: ClassId, + supported_schemas: BTreeSet, + values: BTreeMap, + ) -> Self { + Self { + class_id, + supported_schemas, + values, + ..Entity::default() + } + } +} + /// A schema defines what properties describe an entity #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] #[derive(Encode, Decode, Clone, PartialEq, Eq, Debug)] @@ -334,14 +364,22 @@ pub enum PropertyValue { // ExternalVec(Vec), } -#[cfg_attr(feature = "std", derive(Serialize, Deserialize))] -#[derive(Encode, Decode, Clone, PartialEq, Eq, Debug)] -enum PropertyValueType { - SingleValue, - Vector, -} - impl PropertyValue { + fn is_vec(&self) -> bool { + match self { + PropertyValue::BoolVec(_) + | PropertyValue::Uint16Vec(_) + | PropertyValue::Uint32Vec(_) + | PropertyValue::Uint64Vec(_) + | PropertyValue::Int16Vec(_) + | PropertyValue::Int32Vec(_) + | PropertyValue::Int64Vec(_) + | PropertyValue::TextVec(_) + | PropertyValue::ReferenceVec(_) => true, + _ => false, + } + } + fn vec_clear(&mut self) { match self { PropertyValue::BoolVec(vec) => *vec = vec![], @@ -433,7 +471,7 @@ decl_storage! { /// ClassPermissions of corresponding Classes in the versioned store pub ClassById get(class_by_id) config(): linked_map ClassId => Class; - pub EntityById get(entity_by_id) config(): map EntityId => Entity; + pub EntityById get(entity_by_id) config(): map EntityId => Entity; /// Owner of an entity in the versioned store. If it is None then it is owned by the system. pub EntityMaintainerByEntityId get(entity_maintainer_by_entity_id): linked_map EntityId => Option; @@ -865,11 +903,7 @@ impl Module { fn perform_entity_creation(class_id: ClassId) -> EntityId { let entity_id = NextEntityId::get(); - let new_entity = Entity { - class_id, - supported_schemas: BTreeSet::new(), - values: BTreeMap::new(), - }; + let new_entity = Entity::::new(class_id, BTreeSet::new(), BTreeMap::new()); // Save newly created entity: EntityById::insert(entity_id, new_entity); @@ -1029,8 +1063,8 @@ impl Module { // Get current property values of an entity as a mutable vector, // so we can update them if new values provided present in new_property_values. let mut updated_values = entity.values; + let mut vec_values_last_update = entity.vec_values_last_update; let mut updated = false; - // Iterate over a vector of new values and update corresponding properties // of this entity if new values are valid. for (id, new_value) in new_property_values.into_iter() { @@ -1046,7 +1080,11 @@ impl Module { // Update a current prop value in a mutable vector, if a new value is valid. *current_prop_value = new_value; - updated = !updated; + if current_prop_value.is_vec() { + // Update last block of vec prop value if update performed + Self::refresh_vec_values_last_update(id, &mut vec_values_last_update); + } + updated = true; } } else { // Throw an error if a property was not found on entity @@ -1055,16 +1093,33 @@ impl Module { } } - // If property values should be update: + // If property values should be updated: if updated { - EntityById::mutate(entity_id, |entity| { + >::mutate(entity_id, |entity| { entity.values = updated_values; + entity.vec_values_last_update = vec_values_last_update; }); } Ok(()) } + fn refresh_vec_values_last_update( + id: u16, + vec_values_last_update: &mut BTreeMap, + ) { + match vec_values_last_update.get_mut(&id) { + Some(block_number) if *block_number < >::block_number() => { + *block_number = >::block_number(); + return; + } + Some(_) => return, + None => (), + } + // If there no last block number entry under a given key, we need to initialize it manually, as given entity value exist + vec_values_last_update.insert(id, >::block_number()); + } + fn complete_entity_property_vector_cleaning( entity_id: EntityId, in_class_schema_property_id: u16, @@ -1072,19 +1127,28 @@ impl Module { Self::ensure_known_entity_id(&entity_id)?; let entity = Self::entity_by_id(entity_id); - if !entity.values.contains_key(&in_class_schema_property_id) { + match entity.values.get(&in_class_schema_property_id) { + Some(current_prop_value) if current_prop_value.is_vec() => (), + Some(_) => return Err(ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR), + _ => // Throw an error if a property was not found on entity // by an in-class index of a property update. - return Err(ERROR_UNKNOWN_ENTITY_PROP_ID); + { + return Err(ERROR_UNKNOWN_ENTITY_PROP_ID) + } } // Clear property value vector: - EntityById::mutate(entity_id, |entity| { + >::mutate(entity_id, |entity| { entity .values .get_mut(&in_class_schema_property_id) .as_deref_mut() - .map(|current_property_value_vec| current_property_value_vec.vec_clear()) + .map(|current_property_value_vec| current_property_value_vec.vec_clear()); + Self::refresh_vec_values_last_update( + in_class_schema_property_id, + &mut entity.vec_values_last_update, + ); }); Ok(()) @@ -1098,19 +1162,35 @@ impl Module { Self::ensure_known_entity_id(&entity_id)?; let entity = Self::entity_by_id(entity_id); - if !entity.values.contains_key(&in_class_schema_property_id) { + // Ensure property value vector was not already updated in this block to avoid possible data races, + // when performing vector specific operations + Self::ensure_entity_prop_value_vec_was_not_updated( + in_class_schema_property_id, + &entity.vec_values_last_update, + )?; + + match entity.values.get(&in_class_schema_property_id) { + Some(current_prop_value) if current_prop_value.is_vec() => (), + Some(_) => return Err(ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR), + _ => // Throw an error if a property was not found on entity // by an in-class index of a property update. - return Err(ERROR_UNKNOWN_ENTITY_PROP_ID); + { + return Err(ERROR_UNKNOWN_ENTITY_PROP_ID) + } } // Remove property value vector - EntityById::mutate(entity_id, |entity| { + >::mutate(entity_id, |entity| { entity .values .get_mut(&in_class_schema_property_id) .as_deref_mut() - .map(|current_prop_value| current_prop_value.vec_remove_at(index_in_property_vec)) + .map(|current_prop_value| current_prop_value.vec_remove_at(index_in_property_vec)); + Self::refresh_vec_values_last_update( + in_class_schema_property_id, + &mut entity.vec_values_last_update, + ); }); Ok(()) @@ -1126,35 +1206,50 @@ impl Module { let (entity, class) = Self::get_entity_and_class(entity_id); - // Try to find a current property value in the entity - // by matching its id to the id of a property with an updated value. - if let Some(entity_prop_value) = entity.values.get(&in_class_schema_property_id) { - // Get class-level information about this property - if let Some(class_prop) = class.properties.get(in_class_schema_property_id as usize) { - // Validate a new property value against the type of this property - // and check any additional constraints like the length of a vector - // if it's a vector property or the length of a text if it's a text property. - Self::ensure_prop_value_can_be_insert_at_prop_vec( - &property_value, - entity_prop_value, - class_prop, - )?; + // Ensure property value vector was not already updated in this block to avoid possible data races, + // when performing vector specific operations + Self::ensure_entity_prop_value_vec_was_not_updated( + in_class_schema_property_id, + &entity.vec_values_last_update, + )?; + // Get class-level information about this property + if let Some(class_prop) = class.properties.get(in_class_schema_property_id as usize) { + // Try to find a current property value in the entity + // by matching its id to the id of a property with an updated value. + match entity.values.get(&in_class_schema_property_id) { + Some(entity_prop_value) if entity_prop_value.is_vec() => { + // Validate a new property value against the type of this property + // and check any additional constraints like the length of a vector + // if it's a vector property or the length of a text if it's a text property. + Self::ensure_prop_value_can_be_insert_at_prop_vec( + &property_value, + entity_prop_value, + class_prop, + )?; + } + Some(_) => return Err(ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR), + _ => + // Throw an error if a property was not found on entity + // by an in-class index of a property update. + { + return Err(ERROR_UNKNOWN_ENTITY_PROP_ID) + } } - } else { - // Throw an error if a property was not found on entity - // by an in-class index of a property update. - return Err(ERROR_UNKNOWN_ENTITY_PROP_ID); } // Insert property value into property value vector - EntityById::mutate(entity_id, |entity| { + >::mutate(entity_id, |entity| { entity .values .get_mut(&in_class_schema_property_id) .as_deref_mut() .map(|current_prop_value| { current_prop_value.vec_insert_at(index_in_property_vec, property_value) - }) + }); + Self::refresh_vec_values_last_update( + in_class_schema_property_id, + &mut entity.vec_values_last_update, + ); }); Ok(()) @@ -1310,7 +1405,7 @@ impl Module { fn get_class_id_by_entity_id(entity_id: EntityId) -> Result { // use a utility method on versioned_store module - ensure!(EntityById::exists(entity_id), "EntityNotFound"); + ensure!(>::exists(entity_id), "EntityNotFound"); let entity = Self::entity_by_id(entity_id); Ok(entity.class_id) } @@ -1352,6 +1447,19 @@ impl Module { Ok(()) } + fn ensure_entity_prop_value_vec_was_not_updated( + in_class_schema_property_id: u16, + vec_values_last_update: &BTreeMap, + ) -> dispatch::Result { + if let Some(last_update_block) = vec_values_last_update.get(&in_class_schema_property_id) { + ensure!( + *last_update_block < >::block_number(), + ERROR_PROP_VALUE_VEC_WAS_ALREADY_UPDATED + ); + } + Ok(()) + } + /// Returns an index of a newly added class schema on success. pub fn append_class_schema( class_id: ClassId, @@ -1476,7 +1584,7 @@ impl Module { } } - EntityById::mutate(entity_id, |entity| { + >::mutate(entity_id, |entity| { // Add a new schema to the list of schemas supported by this entity. entity.supported_schemas.insert(schema_id); @@ -1513,7 +1621,7 @@ impl Module { } pub fn ensure_known_entity_id(entity_id: &EntityId) -> dispatch::Result { - ensure!(EntityById::exists(entity_id), ERROR_ENTITY_NOT_FOUND); + ensure!(>::exists(entity_id), ERROR_ENTITY_NOT_FOUND); Ok(()) } @@ -1533,7 +1641,7 @@ impl Module { Ok(()) } - pub fn ensure_schema_id_is_not_added(entity: &Entity, schema_id: u16) -> dispatch::Result { + pub fn ensure_schema_id_is_not_added(entity: &Entity, schema_id: u16) -> dispatch::Result { let schema_not_added = !entity.supported_schemas.contains(&schema_id); ensure!(schema_not_added, ERROR_SCHEMA_ALREADY_ADDED_TO_ENTITY); Ok(()) @@ -1547,7 +1655,7 @@ impl Module { let entity = Self::entity_by_id(entity_id); ensure!( entity.class_id == *class_id, - ERROR_INTERNAL_RPOP_DOES_NOT_MATCH_ITS_CLASS + ERROR_INTERNAL_PROP_DOES_NOT_MATCH_ITS_CLASS ); Ok(()) } @@ -1557,14 +1665,14 @@ impl Module { pub fn is_unknown_internal_entity_id(id: PropertyValue) -> bool { if let PropertyValue::Reference(entity_id) = id { - !EntityById::exists(entity_id) + !>::exists(entity_id) } else { false } } - pub fn get_entity_and_class(entity_id: EntityId) -> (Entity, Class) { - let entity = EntityById::get(entity_id); + pub fn get_entity_and_class(entity_id: EntityId) -> (Entity, Class) { + let entity = >::get(entity_id); let class = ClassById::get(entity.class_id); (entity, class) } @@ -1644,7 +1752,7 @@ impl Module { let entity = Self::entity_by_id(entity_id); ensure!( entity.class_id == *class_id, - ERROR_INTERNAL_RPOP_DOES_NOT_MATCH_ITS_CLASS + ERROR_INTERNAL_PROP_DOES_NOT_MATCH_ITS_CLASS ); } true @@ -1762,7 +1870,7 @@ impl Module { let entity = Self::entity_by_id(entity_id); ensure!( entity.class_id == *class_id, - ERROR_INTERNAL_RPOP_DOES_NOT_MATCH_ITS_CLASS + ERROR_INTERNAL_PROP_DOES_NOT_MATCH_ITS_CLASS ); true } else { diff --git a/runtime-modules/content-directory/src/tests.rs b/runtime-modules/content-directory/src/tests.rs index 3726d05898..b4fa89eb8e 100644 --- a/runtime-modules/content-directory/src/tests.rs +++ b/runtime-modules/content-directory/src/tests.rs @@ -577,8 +577,8 @@ fn batch_transaction_simple() { )); // two entities created - assert!(EntityById::exists(entity_id)); - assert!(EntityById::exists(entity_id + 1)); + assert!(>::exists(entity_id)); + assert!(>::exists(entity_id + 1)); }) } @@ -658,20 +658,20 @@ fn batch_transaction_vector_of_entities() { )); // three entities created - assert!(EntityById::exists(entity_id)); - assert!(EntityById::exists(entity_id + 1)); - assert!(EntityById::exists(entity_id + 2)); + assert!(>::exists(entity_id)); + assert!(>::exists(entity_id + 1)); + assert!(>::exists(entity_id + 2)); assert_eq!( - EntityById::get(entity_id), - Entity { - class_id: new_class_id, - supported_schemas: BTreeSet::from_iter(vec![SCHEMA_ID_0].into_iter()), - values: prop_value( + TestModule::entity_by_id(entity_id), + Entity::new( + new_class_id, + BTreeSet::from_iter(vec![SCHEMA_ID_0].into_iter()), + prop_value( 0, PropertyValue::ReferenceVec(vec![entity_id + 1, entity_id + 2,]) ) - } + ) ); }) } From 6821daaf081c74f3150fd1ea218aeaa6fb3cc363 Mon Sep 17 00:00:00 2001 From: iorveth Date: Thu, 16 Apr 2020 00:47:43 +0300 Subject: [PATCH 06/26] Entity property vector cleaning basic test coverage --- runtime-modules/content-directory/src/mock.rs | 14 +++ .../content-directory/src/tests.rs | 106 ++++++++++++++++-- 2 files changed, 108 insertions(+), 12 deletions(-) diff --git a/runtime-modules/content-directory/src/mock.rs b/runtime-modules/content-directory/src/mock.rs index 8ad72604c9..b727e6c174 100644 --- a/runtime-modules/content-directory/src/mock.rs +++ b/runtime-modules/content-directory/src/mock.rs @@ -28,6 +28,9 @@ pub const SCHEMA_ID_1: u16 = 1; pub const PROP_ID_BOOL: u16 = 0; pub const PROP_ID_U32: u16 = 1; pub const PROP_ID_INTERNAL: u16 = 2; +pub const PROP_ID_U32_VEC: u16 = 3; +pub const PROP_ID_U32_VEC_MAX_LEN: u16 = 20; + pub const PRINCIPAL_GROUP_MEMBERS: [[u64; 2]; 2] = [ [ @@ -338,6 +341,7 @@ pub fn create_entity_with_schema_support() -> EntityId { let (_, schema_id, entity_id) = create_class_with_schema_and_entity(); let mut property_values = BTreeMap::new(); property_values.insert(PROP_ID_BOOL, PropertyValue::Bool(true)); + property_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44])); assert_ok!(TestModule::add_entity_schema_support( entity_id, schema_id, @@ -355,6 +359,7 @@ pub fn create_class_with_schema() -> (ClassId, u16) { good_prop_bool().required(), good_prop_u32(), new_internal_class_prop(class_id), + good_prop_u32_vec() ], ) .expect("This should not happen"); @@ -385,6 +390,15 @@ pub fn good_prop_u32() -> Property { } } +pub fn good_prop_u32_vec() -> Property { + Property { + prop_type: PropertyType::Uint32Vec(PROP_ID_U32_VEC_MAX_LEN), + required: false, + name: b"Name of a u32 vec property".to_vec(), + description: b"Description of a u32 vec property".to_vec(), + } +} + pub fn good_prop_text() -> Property { Property { prop_type: PropertyType::Text(20), diff --git a/runtime-modules/content-directory/src/tests.rs b/runtime-modules/content-directory/src/tests.rs index b4fa89eb8e..3460b3e037 100644 --- a/runtime-modules/content-directory/src/tests.rs +++ b/runtime-modules/content-directory/src/tests.rs @@ -1098,7 +1098,7 @@ fn should_add_schema_to_entity_when_some_optional_props_provided() { entity_id, schema_id, // Note that an optional internal prop is not provided here. - prop_values + prop_values.clone() )); let entity = TestModule::entity_by_id(entity_id); @@ -1106,12 +1106,14 @@ fn should_add_schema_to_entity_when_some_optional_props_provided() { entity.supported_schemas, BTreeSet::from_iter(vec![SCHEMA_ID_0].into_iter()) ); - prop_values = bool_prop_value(); - prop_values.append(&mut prop_value(PROP_ID_U32, PropertyValue::Uint32(123))); prop_values.append(&mut prop_value( PROP_ID_INTERNAL, PropertyValue::Bool(false), )); + prop_values.append(&mut prop_value( + PROP_ID_U32_VEC, + PropertyValue::Bool(false), + )); assert_eq!(entity.values, prop_values); }) } @@ -1119,6 +1121,40 @@ fn should_add_schema_to_entity_when_some_optional_props_provided() { // Update entity properties // -------------------------------------- + +#[test] +fn update_entity_props_successfully() { + with_test_externalities(|| { + let entity_id = create_entity_with_schema_support(); + let mut prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(true)); + prop_values.append(&mut prop_value(PROP_ID_U32, PropertyValue::Bool(false))); + prop_values.append(&mut prop_value( + PROP_ID_INTERNAL, + PropertyValue::Bool(false), + )); + prop_values.append(&mut prop_value( + PROP_ID_U32_VEC, + PropertyValue::Uint32Vec(vec![123, 234, 44]), + )); + assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); + prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(false)); + prop_values.append(&mut prop_value(PROP_ID_U32, PropertyValue::Uint32(123))); + prop_values.append(&mut prop_value( + PROP_ID_INTERNAL, + PropertyValue::Reference(entity_id), + )); + prop_values.append(&mut prop_value( + PROP_ID_U32_VEC, + PropertyValue::Uint32Vec(vec![123, 234, 44, 88, 43]), + )); + assert_ok!(TestModule::complete_entity_property_values_update( + entity_id, + prop_values.clone() + )); + assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); + }) +} + #[test] fn cannot_update_entity_props_when_entity_not_found() { with_test_externalities(|| { @@ -1174,31 +1210,77 @@ fn cannot_update_entity_props_when_unknown_entity_prop_id() { }) } +// Entity property vector cleaning +// -------------------------------------- + #[test] -fn update_entity_props_successfully() { +fn complete_entity_property_vector_cleaning_successfully() { with_test_externalities(|| { let entity_id = create_entity_with_schema_support(); let mut prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(true)); prop_values.append(&mut prop_value(PROP_ID_U32, PropertyValue::Bool(false))); + prop_values.append(&mut prop_value(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44]))); prop_values.append(&mut prop_value( PROP_ID_INTERNAL, PropertyValue::Bool(false), )); + + // Check property values runtime storage related to an entity before cleaning of entity property vector value under given schema id assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); - prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(false)); - prop_values.append(&mut prop_value(PROP_ID_U32, PropertyValue::Uint32(123))); - prop_values.append(&mut prop_value( - PROP_ID_INTERNAL, - PropertyValue::Reference(entity_id), - )); - assert_ok!(TestModule::complete_entity_property_values_update( + + // Perform cleaning of entity property vector value under given schema id + assert_ok!(TestModule::complete_entity_property_vector_cleaning( entity_id, - prop_values.clone() + PROP_ID_U32_VEC )); + + // Update prop_value to compare with empty vec under given index + prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![])); + + // Check property values runtime storage related to a entity right after + // cleaning entity property vector under given schema id assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); }) } +#[test] +fn cannot_complete_entity_property_vector_cleaning_when_entity_not_found() { + with_test_externalities(|| { + assert_entity_not_found(TestModule::complete_entity_property_vector_cleaning( + UNKNOWN_ENTITY_ID, + PROP_ID_U32_VEC, + )); + }) +} + +#[test] +fn cannot_complete_entity_property_vector_cleaning_when_unknown_entity_prop_id() { + with_test_externalities(|| { + let entity_id = create_entity_with_schema_support(); + assert_err!( + TestModule::complete_entity_property_vector_cleaning( + entity_id, + UNKNOWN_PROP_ID + ), + ERROR_UNKNOWN_ENTITY_PROP_ID + ); + }) +} + +#[test] +fn cannot_complete_entity_property_vector_cleaning_when_entity_prop_id_is_not_a_vector() { + with_test_externalities(|| { + let entity_id = create_entity_with_schema_support(); + assert_err!( + TestModule::complete_entity_property_vector_cleaning( + entity_id, + PROP_ID_U32 + ), + ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR + ); + }) +} + // TODO test text max len // TODO test vec max len From 10d8be7cfdbad320c10bd1f21540ff20ee57f3a3 Mon Sep 17 00:00:00 2001 From: iorveth Date: Thu, 16 Apr 2020 01:18:20 +0300 Subject: [PATCH 07/26] Entity property vector remove_at basic test coverage --- .../content-directory/src/tests.rs | 117 +++++++++++++++++- 1 file changed, 112 insertions(+), 5 deletions(-) diff --git a/runtime-modules/content-directory/src/tests.rs b/runtime-modules/content-directory/src/tests.rs index 3460b3e037..37ed128fdf 100644 --- a/runtime-modules/content-directory/src/tests.rs +++ b/runtime-modules/content-directory/src/tests.rs @@ -1218,12 +1218,12 @@ fn complete_entity_property_vector_cleaning_successfully() { with_test_externalities(|| { let entity_id = create_entity_with_schema_support(); let mut prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(true)); - prop_values.append(&mut prop_value(PROP_ID_U32, PropertyValue::Bool(false))); - prop_values.append(&mut prop_value(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44]))); - prop_values.append(&mut prop_value( + prop_values.insert(PROP_ID_U32, PropertyValue::Bool(false)); + prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44])); + prop_values.insert( PROP_ID_INTERNAL, PropertyValue::Bool(false), - )); + ); // Check property values runtime storage related to an entity before cleaning of entity property vector value under given schema id assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); @@ -1234,7 +1234,7 @@ fn complete_entity_property_vector_cleaning_successfully() { PROP_ID_U32_VEC )); - // Update prop_value to compare with empty vec under given index + // Update entity property values to compare with runtime storage entity value under given schema id prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![])); // Check property values runtime storage related to a entity right after @@ -1281,6 +1281,113 @@ fn cannot_complete_entity_property_vector_cleaning_when_entity_prop_id_is_not_a_ }) } +// Remove at entity property vector +// -------------------------------------- + +#[test] +fn complete_remove_at_entity_property_vector_successfully() { + with_test_externalities(|| { + let entity_id = create_entity_with_schema_support(); + let mut prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(true)); + prop_values.insert(PROP_ID_U32, PropertyValue::Bool(false)); + prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44])); + prop_values.insert( + PROP_ID_INTERNAL, + PropertyValue::Bool(false), + ); + + // Check property values runtime storage related to an entity before removing at given index of entity property vector value + assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); + + // Perform removing at given index of entity property vector value + assert_ok!(TestModule::complete_remove_at_entity_property_vector( + entity_id, + PROP_ID_U32_VEC, + 1 + )); + + // Update entity property values to compare with runtime storage entity value under given schema id + prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 44])); + + // Check property values runtime storage related to a entity right after + // removing at given index of entity property vector value + assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); + }) +} + +#[test] +fn cannot_complete_remove_at_entity_property_vector_when_entity_not_found() { + with_test_externalities(|| { + assert_entity_not_found(TestModule::complete_remove_at_entity_property_vector( + UNKNOWN_ENTITY_ID, + PROP_ID_U32_VEC, + 1 + )); + }) +} + +#[test] +fn cannot_complete_remove_at_entity_property_vector_when_unknown_entity_prop_id() { + with_test_externalities(|| { + let entity_id = create_entity_with_schema_support(); + assert_err!( + TestModule::complete_remove_at_entity_property_vector( + entity_id, + UNKNOWN_PROP_ID, + 1 + ), + ERROR_UNKNOWN_ENTITY_PROP_ID + ); + }) +} + +#[test] +fn cannot_complete_remove_at_entity_property_vector_when_entity_prop_id_is_not_a_vector() { + with_test_externalities(|| { + let entity_id = create_entity_with_schema_support(); + assert_err!( + TestModule::complete_remove_at_entity_property_vector( + entity_id, + PROP_ID_U32, + 1 + ), + ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR + ); + }) +} + +#[test] +fn cannot_complete_remove_at_entity_property_vector_when_already_updated() { + with_test_externalities(|| { + let entity_id = create_entity_with_schema_support(); + let mut prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(true)); + prop_values.insert(PROP_ID_U32, PropertyValue::Bool(false)); + prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44])); + prop_values.insert( + PROP_ID_INTERNAL, + PropertyValue::Bool(false), + ); + + // Check property values runtime storage related to an entity before removing at given index of entity property vector value + assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); + + // Perform removing at given index of entity property vector value + assert_ok!(TestModule::complete_remove_at_entity_property_vector( + entity_id, + PROP_ID_U32_VEC, + 1 + )); + assert_err!( + TestModule::complete_remove_at_entity_property_vector( + entity_id, + PROP_ID_U32_VEC, + 1 + ), + ERROR_PROP_VALUE_VEC_WAS_ALREADY_UPDATED + ); + }) +} + // TODO test text max len // TODO test vec max len From 3dbdc40d596de79dd1366285c11653f2f5a6a8a5 Mon Sep 17 00:00:00 2001 From: iorveth Date: Thu, 16 Apr 2020 01:29:00 +0300 Subject: [PATCH 08/26] Tests: refactoring --- .../content-directory/src/tests.rs | 68 ++++++++----------- 1 file changed, 28 insertions(+), 40 deletions(-) diff --git a/runtime-modules/content-directory/src/tests.rs b/runtime-modules/content-directory/src/tests.rs index 37ed128fdf..054b35c374 100644 --- a/runtime-modules/content-directory/src/tests.rs +++ b/runtime-modules/content-directory/src/tests.rs @@ -1049,7 +1049,7 @@ fn cannot_add_schema_to_entity_when_prop_value_dont_match_type() { with_test_externalities(|| { let (_, schema_id, entity_id) = create_class_with_schema_and_entity(); let mut prop_values = bool_prop_value(); - prop_values.append(&mut prop_value(PROP_ID_U32, PropertyValue::Bool(true))); + prop_values.insert(PROP_ID_U32, PropertyValue::Bool(true)); assert_err!( TestModule::add_entity_schema_support(entity_id, schema_id, prop_values), ERROR_PROP_VALUE_DONT_MATCH_TYPE @@ -1062,10 +1062,10 @@ fn cannot_add_schema_to_entity_when_unknown_internal_entity_id() { with_test_externalities(|| { let (_, schema_id, entity_id) = create_class_with_schema_and_entity(); let mut prop_values = bool_prop_value(); - prop_values.append(&mut prop_value( + prop_values.insert( PROP_ID_INTERNAL, PropertyValue::Reference(UNKNOWN_ENTITY_ID), - )); + ); assert_err!( TestModule::add_entity_schema_support(entity_id, schema_id, prop_values), ERROR_ENTITY_NOT_FOUND @@ -1093,7 +1093,7 @@ fn should_add_schema_to_entity_when_some_optional_props_provided() { with_test_externalities(|| { let (_, schema_id, entity_id) = create_class_with_schema_and_entity(); let mut prop_values = bool_prop_value(); - prop_values.append(&mut prop_value(PROP_ID_U32, PropertyValue::Uint32(123))); + prop_values.insert(PROP_ID_U32, PropertyValue::Uint32(123)); assert_ok!(TestModule::add_entity_schema_support( entity_id, schema_id, @@ -1106,14 +1106,14 @@ fn should_add_schema_to_entity_when_some_optional_props_provided() { entity.supported_schemas, BTreeSet::from_iter(vec![SCHEMA_ID_0].into_iter()) ); - prop_values.append(&mut prop_value( + prop_values.insert( PROP_ID_INTERNAL, PropertyValue::Bool(false), - )); - prop_values.append(&mut prop_value( + ); + prop_values.insert( PROP_ID_U32_VEC, PropertyValue::Bool(false), - )); + ); assert_eq!(entity.values, prop_values); }) } @@ -1127,26 +1127,26 @@ fn update_entity_props_successfully() { with_test_externalities(|| { let entity_id = create_entity_with_schema_support(); let mut prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(true)); - prop_values.append(&mut prop_value(PROP_ID_U32, PropertyValue::Bool(false))); - prop_values.append(&mut prop_value( + prop_values.insert(PROP_ID_U32, PropertyValue::Bool(false)); + prop_values.insert( PROP_ID_INTERNAL, PropertyValue::Bool(false), - )); - prop_values.append(&mut prop_value( + ); + prop_values.insert( PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44]), - )); + ); assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(false)); - prop_values.append(&mut prop_value(PROP_ID_U32, PropertyValue::Uint32(123))); - prop_values.append(&mut prop_value( + prop_values.insert(PROP_ID_U32, PropertyValue::Uint32(123)); + prop_values.insert( PROP_ID_INTERNAL, PropertyValue::Reference(entity_id), - )); - prop_values.append(&mut prop_value( + ); + prop_values.insert( PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44, 88, 43]), - )); + ); assert_ok!(TestModule::complete_entity_property_values_update( entity_id, prop_values.clone() @@ -1284,10 +1284,8 @@ fn cannot_complete_entity_property_vector_cleaning_when_entity_prop_id_is_not_a_ // Remove at entity property vector // -------------------------------------- -#[test] -fn complete_remove_at_entity_property_vector_successfully() { - with_test_externalities(|| { - let entity_id = create_entity_with_schema_support(); +fn complete_remove_at_entity_property_vector() -> EntityId { + let entity_id = create_entity_with_schema_support(); let mut prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(true)); prop_values.insert(PROP_ID_U32, PropertyValue::Bool(false)); prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44])); @@ -1312,6 +1310,13 @@ fn complete_remove_at_entity_property_vector_successfully() { // Check property values runtime storage related to a entity right after // removing at given index of entity property vector value assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); + entity_id +} + +#[test] +fn complete_remove_at_entity_property_vector_successfully() { + with_test_externalities(|| { + complete_remove_at_entity_property_vector(); }) } @@ -1359,24 +1364,7 @@ fn cannot_complete_remove_at_entity_property_vector_when_entity_prop_id_is_not_a #[test] fn cannot_complete_remove_at_entity_property_vector_when_already_updated() { with_test_externalities(|| { - let entity_id = create_entity_with_schema_support(); - let mut prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(true)); - prop_values.insert(PROP_ID_U32, PropertyValue::Bool(false)); - prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44])); - prop_values.insert( - PROP_ID_INTERNAL, - PropertyValue::Bool(false), - ); - - // Check property values runtime storage related to an entity before removing at given index of entity property vector value - assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); - - // Perform removing at given index of entity property vector value - assert_ok!(TestModule::complete_remove_at_entity_property_vector( - entity_id, - PROP_ID_U32_VEC, - 1 - )); + let entity_id = complete_remove_at_entity_property_vector(); assert_err!( TestModule::complete_remove_at_entity_property_vector( entity_id, From 39e0cf2c55cbbe8ee59687bcfab31f81d90762dd Mon Sep 17 00:00:00 2001 From: iorveth Date: Thu, 16 Apr 2020 02:07:51 +0300 Subject: [PATCH 09/26] Entity property vector insert_at basic test coverage --- runtime-modules/content-directory/src/lib.rs | 13 +- runtime-modules/content-directory/src/mock.rs | 8 +- .../content-directory/src/tests.rs | 239 +++++++++++++----- 3 files changed, 186 insertions(+), 74 deletions(-) diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index ce0614c150..6913056112 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -1228,12 +1228,13 @@ impl Module { )?; } Some(_) => return Err(ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR), - _ => - // Throw an error if a property was not found on entity - // by an in-class index of a property update. - { - return Err(ERROR_UNKNOWN_ENTITY_PROP_ID) - } + _ => (), + } + } else { + // Throw an error if a property was not found on entity + // by an in-class index of a property update. + { + return Err(ERROR_UNKNOWN_ENTITY_PROP_ID); } } diff --git a/runtime-modules/content-directory/src/mock.rs b/runtime-modules/content-directory/src/mock.rs index b727e6c174..1640e87446 100644 --- a/runtime-modules/content-directory/src/mock.rs +++ b/runtime-modules/content-directory/src/mock.rs @@ -31,7 +31,6 @@ pub const PROP_ID_INTERNAL: u16 = 2; pub const PROP_ID_U32_VEC: u16 = 3; pub const PROP_ID_U32_VEC_MAX_LEN: u16 = 20; - pub const PRINCIPAL_GROUP_MEMBERS: [[u64; 2]; 2] = [ [ MEMBER_ONE_WITH_CREDENTIAL_ZERO, @@ -341,7 +340,10 @@ pub fn create_entity_with_schema_support() -> EntityId { let (_, schema_id, entity_id) = create_class_with_schema_and_entity(); let mut property_values = BTreeMap::new(); property_values.insert(PROP_ID_BOOL, PropertyValue::Bool(true)); - property_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44])); + property_values.insert( + PROP_ID_U32_VEC, + PropertyValue::Uint32Vec(vec![123, 234, 44]), + ); assert_ok!(TestModule::add_entity_schema_support( entity_id, schema_id, @@ -359,7 +361,7 @@ pub fn create_class_with_schema() -> (ClassId, u16) { good_prop_bool().required(), good_prop_u32(), new_internal_class_prop(class_id), - good_prop_u32_vec() + good_prop_u32_vec(), ], ) .expect("This should not happen"); diff --git a/runtime-modules/content-directory/src/tests.rs b/runtime-modules/content-directory/src/tests.rs index 054b35c374..9891a87f19 100644 --- a/runtime-modules/content-directory/src/tests.rs +++ b/runtime-modules/content-directory/src/tests.rs @@ -1106,14 +1106,8 @@ fn should_add_schema_to_entity_when_some_optional_props_provided() { entity.supported_schemas, BTreeSet::from_iter(vec![SCHEMA_ID_0].into_iter()) ); - prop_values.insert( - PROP_ID_INTERNAL, - PropertyValue::Bool(false), - ); - prop_values.insert( - PROP_ID_U32_VEC, - PropertyValue::Bool(false), - ); + prop_values.insert(PROP_ID_INTERNAL, PropertyValue::Bool(false)); + prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Bool(false)); assert_eq!(entity.values, prop_values); }) } @@ -1121,17 +1115,13 @@ fn should_add_schema_to_entity_when_some_optional_props_provided() { // Update entity properties // -------------------------------------- - #[test] fn update_entity_props_successfully() { with_test_externalities(|| { let entity_id = create_entity_with_schema_support(); let mut prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(true)); prop_values.insert(PROP_ID_U32, PropertyValue::Bool(false)); - prop_values.insert( - PROP_ID_INTERNAL, - PropertyValue::Bool(false), - ); + prop_values.insert(PROP_ID_INTERNAL, PropertyValue::Bool(false)); prop_values.insert( PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44]), @@ -1139,10 +1129,7 @@ fn update_entity_props_successfully() { assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(false)); prop_values.insert(PROP_ID_U32, PropertyValue::Uint32(123)); - prop_values.insert( - PROP_ID_INTERNAL, - PropertyValue::Reference(entity_id), - ); + prop_values.insert(PROP_ID_INTERNAL, PropertyValue::Reference(entity_id)); prop_values.insert( PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44, 88, 43]), @@ -1219,26 +1206,26 @@ fn complete_entity_property_vector_cleaning_successfully() { let entity_id = create_entity_with_schema_support(); let mut prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(true)); prop_values.insert(PROP_ID_U32, PropertyValue::Bool(false)); - prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44])); prop_values.insert( - PROP_ID_INTERNAL, - PropertyValue::Bool(false), + PROP_ID_U32_VEC, + PropertyValue::Uint32Vec(vec![123, 234, 44]), ); - - // Check property values runtime storage related to an entity before cleaning of entity property vector value under given schema id + prop_values.insert(PROP_ID_INTERNAL, PropertyValue::Bool(false)); + + // Check property values runtime storage related to an entity before cleaning of entity property vector value under given schema id assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); - // Perform cleaning of entity property vector value under given schema id + // Perform cleaning of entity property vector value under given schema id assert_ok!(TestModule::complete_entity_property_vector_cleaning( entity_id, PROP_ID_U32_VEC )); - // Update entity property values to compare with runtime storage entity value under given schema id + // Update entity property values to compare with runtime storage entity value under given schema id prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![])); - // Check property values runtime storage related to a entity right after - // cleaning entity property vector under given schema id + // Check property values runtime storage related to a entity right after + // cleaning entity property vector under given schema id assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); }) } @@ -1258,10 +1245,7 @@ fn cannot_complete_entity_property_vector_cleaning_when_unknown_entity_prop_id() with_test_externalities(|| { let entity_id = create_entity_with_schema_support(); assert_err!( - TestModule::complete_entity_property_vector_cleaning( - entity_id, - UNKNOWN_PROP_ID - ), + TestModule::complete_entity_property_vector_cleaning(entity_id, UNKNOWN_PROP_ID), ERROR_UNKNOWN_ENTITY_PROP_ID ); }) @@ -1272,10 +1256,7 @@ fn cannot_complete_entity_property_vector_cleaning_when_entity_prop_id_is_not_a_ with_test_externalities(|| { let entity_id = create_entity_with_schema_support(); assert_err!( - TestModule::complete_entity_property_vector_cleaning( - entity_id, - PROP_ID_U32 - ), + TestModule::complete_entity_property_vector_cleaning(entity_id, PROP_ID_U32), ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR ); }) @@ -1286,60 +1267,141 @@ fn cannot_complete_entity_property_vector_cleaning_when_entity_prop_id_is_not_a_ fn complete_remove_at_entity_property_vector() -> EntityId { let entity_id = create_entity_with_schema_support(); + let mut prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(true)); + prop_values.insert(PROP_ID_U32, PropertyValue::Bool(false)); + prop_values.insert( + PROP_ID_U32_VEC, + PropertyValue::Uint32Vec(vec![123, 234, 44]), + ); + prop_values.insert(PROP_ID_INTERNAL, PropertyValue::Bool(false)); + + // Check property values runtime storage related to an entity before removing at given index of entity property vector value + assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); + + // Perform removing at given index of entity property vector value + assert_ok!(TestModule::complete_remove_at_entity_property_vector( + entity_id, + PROP_ID_U32_VEC, + 1 + )); + + // Update entity property values to compare with runtime storage entity value under given schema id + prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 44])); + + // Check property values runtime storage related to a entity right after + // removing at given index of entity property vector value + assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); + entity_id +} + +#[test] +fn complete_remove_at_entity_property_vector_successfully() { + with_test_externalities(|| { + complete_remove_at_entity_property_vector(); + }) +} + +#[test] +fn cannot_complete_remove_at_entity_property_vector_when_entity_not_found() { + with_test_externalities(|| { + assert_entity_not_found(TestModule::complete_remove_at_entity_property_vector( + UNKNOWN_ENTITY_ID, + PROP_ID_U32_VEC, + 1, + )); + }) +} + +#[test] +fn cannot_complete_remove_at_entity_property_vector_when_unknown_entity_prop_id() { + with_test_externalities(|| { + let entity_id = create_entity_with_schema_support(); + assert_err!( + TestModule::complete_remove_at_entity_property_vector(entity_id, UNKNOWN_PROP_ID, 1), + ERROR_UNKNOWN_ENTITY_PROP_ID + ); + }) +} + +#[test] +fn cannot_complete_remove_at_entity_property_vector_when_entity_prop_id_is_not_a_vector() { + with_test_externalities(|| { + let entity_id = create_entity_with_schema_support(); + assert_err!( + TestModule::complete_remove_at_entity_property_vector(entity_id, PROP_ID_U32, 1), + ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR + ); + }) +} + +#[test] +fn cannot_complete_remove_at_entity_property_vector_when_already_updated() { + with_test_externalities(|| { + let entity_id = complete_remove_at_entity_property_vector(); + assert_err!( + TestModule::complete_remove_at_entity_property_vector(entity_id, PROP_ID_U32_VEC, 1), + ERROR_PROP_VALUE_VEC_WAS_ALREADY_UPDATED + ); + }) +} + +#[test] +fn complete_insert_at_entity_property_vector_successfully() { + with_test_externalities(|| { + let entity_id = create_entity_with_schema_support(); let mut prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(true)); prop_values.insert(PROP_ID_U32, PropertyValue::Bool(false)); - prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44])); prop_values.insert( - PROP_ID_INTERNAL, - PropertyValue::Bool(false), + PROP_ID_U32_VEC, + PropertyValue::Uint32Vec(vec![123, 234, 44]), ); - - // Check property values runtime storage related to an entity before removing at given index of entity property vector value + prop_values.insert(PROP_ID_INTERNAL, PropertyValue::Bool(false)); + + // Check property values runtime storage related to an entity before inserting at given index of entity property vector value assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); - // Perform removing at given index of entity property vector value - assert_ok!(TestModule::complete_remove_at_entity_property_vector( + // Perform inserting at given index of entity property vector value + assert_ok!(TestModule::complete_insert_at_entity_property_vector( entity_id, PROP_ID_U32_VEC, - 1 + 1, + PropertyValue::Uint32(33) )); - // Update entity property values to compare with runtime storage entity value under given schema id - prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 44])); + // Update entity property values to compare with runtime storage entity value under given schema id + prop_values.insert( + PROP_ID_U32_VEC, + PropertyValue::Uint32Vec(vec![123, 33, 234, 44]), + ); - // Check property values runtime storage related to a entity right after - // removing at given index of entity property vector value + // Check property values runtime storage related to a entity right after + // inserting at given index of entity property vector value assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); - entity_id -} - -#[test] -fn complete_remove_at_entity_property_vector_successfully() { - with_test_externalities(|| { - complete_remove_at_entity_property_vector(); }) } #[test] -fn cannot_complete_remove_at_entity_property_vector_when_entity_not_found() { +fn cannot_complete_insert_at_entity_property_vector_when_entity_not_found() { with_test_externalities(|| { - assert_entity_not_found(TestModule::complete_remove_at_entity_property_vector( + assert_entity_not_found(TestModule::complete_insert_at_entity_property_vector( UNKNOWN_ENTITY_ID, PROP_ID_U32_VEC, - 1 + 1, + PropertyValue::Uint32(33), )); }) } #[test] -fn cannot_complete_remove_at_entity_property_vector_when_unknown_entity_prop_id() { +fn cannot_complete_insert_at_entity_property_vector_when_unknown_entity_prop_id() { with_test_externalities(|| { let entity_id = create_entity_with_schema_support(); assert_err!( - TestModule::complete_remove_at_entity_property_vector( + TestModule::complete_insert_at_entity_property_vector( entity_id, UNKNOWN_PROP_ID, - 1 + 1, + PropertyValue::Uint32(33) ), ERROR_UNKNOWN_ENTITY_PROP_ID ); @@ -1347,14 +1409,15 @@ fn cannot_complete_remove_at_entity_property_vector_when_unknown_entity_prop_id( } #[test] -fn cannot_complete_remove_at_entity_property_vector_when_entity_prop_id_is_not_a_vector() { +fn cannot_complete_insert_at_entity_property_vector_when_entity_prop_id_is_not_a_vector() { with_test_externalities(|| { let entity_id = create_entity_with_schema_support(); assert_err!( - TestModule::complete_remove_at_entity_property_vector( + TestModule::complete_insert_at_entity_property_vector( entity_id, PROP_ID_U32, - 1 + 1, + PropertyValue::Uint32(17) ), ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR ); @@ -1362,20 +1425,66 @@ fn cannot_complete_remove_at_entity_property_vector_when_entity_prop_id_is_not_a } #[test] -fn cannot_complete_remove_at_entity_property_vector_when_already_updated() { +fn cannot_complete_insert_at_entity_property_type_does_not_match_internal_entity_vector_type() { + with_test_externalities(|| { + let entity_id = create_entity_with_schema_support(); + assert_err!( + TestModule::complete_insert_at_entity_property_vector( + entity_id, + PROP_ID_U32_VEC, + 1, + PropertyValue::Uint16(33) + ), + ERROR_PROP_VALUE_TYPE_DOESNT_MATCH_INTERNAL_ENTITY_VECTOR_TYPE + ); + }) +} + +#[test] +fn cannot_complete_insert_at_entity_property_vector_when_entity_prop_value_vector_is_too_long() { + with_test_externalities(|| { + let (_, schema_id, entity_id) = create_class_with_schema_and_entity(); + let mut property_values = BTreeMap::new(); + property_values.insert(PROP_ID_BOOL, PropertyValue::Bool(true)); + property_values.insert( + PROP_ID_U32_VEC, + PropertyValue::Uint32Vec(vec![5; PROP_ID_U32_VEC_MAX_LEN as usize]), + ); + assert_ok!(TestModule::add_entity_schema_support( + entity_id, + schema_id, + property_values + )); + assert_err!( + TestModule::complete_insert_at_entity_property_vector( + entity_id, + PROP_ID_U32_VEC, + 1, + PropertyValue::Uint32(33) + ), + ERROR_ENTITY_PROP_VALUE_VECTOR_IS_TOO_LONG + ); + }) +} + +#[test] +fn cannot_complete_insert_at_entity_property_vector_when_already_updated() { with_test_externalities(|| { let entity_id = complete_remove_at_entity_property_vector(); assert_err!( - TestModule::complete_remove_at_entity_property_vector( + TestModule::complete_insert_at_entity_property_vector( entity_id, PROP_ID_U32_VEC, - 1 + 1, + PropertyValue::Uint32(33) ), ERROR_PROP_VALUE_VEC_WAS_ALREADY_UPDATED ); }) } +// TODO insert at ReferenceVec test + // TODO test text max len // TODO test vec max len From 639e63712dd172e99e7e5b703dcd2362c619fb16 Mon Sep 17 00:00:00 2001 From: iorveth Date: Thu, 16 Apr 2020 18:59:15 +0300 Subject: [PATCH 10/26] Core logic refactoring, insert_at ReferenceVec, when uknown internal entity_id failure test added --- runtime-modules/content-directory/src/lib.rs | 129 ++++++++---------- runtime-modules/content-directory/src/mock.rs | 18 ++- .../content-directory/src/tests.rs | 58 ++++++-- 3 files changed, 118 insertions(+), 87 deletions(-) diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index 6913056112..a0ecfd301e 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -1694,8 +1694,63 @@ impl Module { entity_prop_value: &PropertyValue, prop: &Property, ) -> dispatch::Result { - Self::ensure_valid_internal_prop(value, prop)?; - Self::validate_prop_value_can_be_insert_at_prop_vec(value, entity_prop_value, prop)?; + fn validate_prop_vec_len_after_value_insert(vec: &[T], max_len: &u16) -> bool { + vec.len() < *max_len as usize + } + + let is_valid_len = match (value, entity_prop_value, &prop.prop_type) { + // Single values + (PV::Bool(_), PV::BoolVec(vec), PT::BoolVec(max_len)) => { + validate_prop_vec_len_after_value_insert(vec, max_len) + } + (PV::Uint16(_), PV::Uint16Vec(vec), PT::Uint16Vec(max_len)) => { + validate_prop_vec_len_after_value_insert(vec, max_len) + } + (PV::Uint32(_), PV::Uint32Vec(vec), PT::Uint32Vec(max_len)) => { + validate_prop_vec_len_after_value_insert(vec, max_len) + } + (PV::Uint64(_), PV::Uint64Vec(vec), PT::Uint64Vec(max_len)) => { + validate_prop_vec_len_after_value_insert(vec, max_len) + } + (PV::Int16(_), PV::Int16Vec(vec), PT::Int16Vec(max_len)) => { + validate_prop_vec_len_after_value_insert(vec, max_len) + } + (PV::Int32(_), PV::Int32Vec(vec), PT::Int32Vec(max_len)) => { + validate_prop_vec_len_after_value_insert(vec, max_len) + } + (PV::Int64(_), PV::Int64Vec(vec), PT::Int64Vec(max_len)) => { + validate_prop_vec_len_after_value_insert(vec, max_len) + } + (PV::Text(text_item), PV::TextVec(vec), PT::TextVec(vec_max_len, text_max_len)) => { + if validate_prop_vec_len_after_value_insert(vec, vec_max_len) { + Self::validate_max_len_of_text(text_item, *text_max_len)?; + true + } else { + false + } + } + ( + PV::Reference(entity_id), + PV::ReferenceVec(vec), + PT::ReferenceVec(vec_max_len, class_id), + ) => { + Self::ensure_known_class_id(class_id)?; + if validate_prop_vec_len_after_value_insert(vec, vec_max_len) { + Self::ensure_known_entity_id(entity_id)?; + let entity = Self::entity_by_id(entity_id); + ensure!( + entity.class_id == *class_id, + ERROR_INTERNAL_PROP_DOES_NOT_MATCH_ITS_CLASS + ); + true + } else { + false + } + } + _ => return Err(ERROR_PROP_VALUE_TYPE_DOESNT_MATCH_INTERNAL_ENTITY_VECTOR_TYPE), + }; + + ensure!(is_valid_len, ERROR_ENTITY_PROP_VALUE_VECTOR_IS_TOO_LONG); Ok(()) } @@ -1815,76 +1870,6 @@ impl Module { } } - pub fn validate_prop_value_can_be_insert_at_prop_vec( - value: &PropertyValue, - entity_prop_value: &PropertyValue, - prop: &Property, - ) -> dispatch::Result { - fn validate_prop_vec_len_after_value_insert(vec: &[T], max_len: &u16) -> bool { - vec.len() < *max_len as usize - } - - // A non required property can be updated to None: - if !prop.required && *value == PV::Bool(false) { - return Ok(()); - } - - let is_valid_len = match (value, entity_prop_value, &prop.prop_type) { - // Single values - (PV::Bool(_), PV::BoolVec(vec), PT::BoolVec(max_len)) => { - validate_prop_vec_len_after_value_insert(vec, max_len) - } - (PV::Uint16(_), PV::Uint16Vec(vec), PT::Uint16Vec(max_len)) => { - validate_prop_vec_len_after_value_insert(vec, max_len) - } - (PV::Uint32(_), PV::Uint32Vec(vec), PT::Uint32Vec(max_len)) => { - validate_prop_vec_len_after_value_insert(vec, max_len) - } - (PV::Uint64(_), PV::Uint64Vec(vec), PT::Uint64Vec(max_len)) => { - validate_prop_vec_len_after_value_insert(vec, max_len) - } - (PV::Int16(_), PV::Int16Vec(vec), PT::Int16Vec(max_len)) => { - validate_prop_vec_len_after_value_insert(vec, max_len) - } - (PV::Int32(_), PV::Int32Vec(vec), PT::Int32Vec(max_len)) => { - validate_prop_vec_len_after_value_insert(vec, max_len) - } - (PV::Int64(_), PV::Int64Vec(vec), PT::Int64Vec(max_len)) => { - validate_prop_vec_len_after_value_insert(vec, max_len) - } - (PV::Text(text_item), PV::TextVec(vec), PT::TextVec(vec_max_len, text_max_len)) => { - if validate_prop_vec_len_after_value_insert(vec, vec_max_len) { - Self::validate_max_len_of_text(text_item, *text_max_len)?; - true - } else { - false - } - } - ( - PV::Reference(entity_id), - PV::ReferenceVec(vec), - PT::ReferenceVec(vec_max_len, class_id), - ) => { - Self::ensure_known_class_id(class_id)?; - if validate_prop_vec_len_after_value_insert(vec, vec_max_len) { - Self::ensure_known_entity_id(entity_id)?; - let entity = Self::entity_by_id(entity_id); - ensure!( - entity.class_id == *class_id, - ERROR_INTERNAL_PROP_DOES_NOT_MATCH_ITS_CLASS - ); - true - } else { - false - } - } - _ => return Err(ERROR_PROP_VALUE_TYPE_DOESNT_MATCH_INTERNAL_ENTITY_VECTOR_TYPE), - }; - - ensure!(is_valid_len, ERROR_ENTITY_PROP_VALUE_VECTOR_IS_TOO_LONG); - Ok(()) - } - pub fn ensure_property_name_is_valid(text: &Vec) -> dispatch::Result { T::PropertyNameConstraint::get().ensure_valid( text.len(), diff --git a/runtime-modules/content-directory/src/mock.rs b/runtime-modules/content-directory/src/mock.rs index 1640e87446..2c7cb38f2b 100644 --- a/runtime-modules/content-directory/src/mock.rs +++ b/runtime-modules/content-directory/src/mock.rs @@ -26,8 +26,9 @@ pub const SCHEMA_ID_0: u16 = 0; pub const SCHEMA_ID_1: u16 = 1; pub const PROP_ID_BOOL: u16 = 0; +pub const PROP_ID_REFERENCE_VEC: u16 = 1; pub const PROP_ID_U32: u16 = 1; -pub const PROP_ID_INTERNAL: u16 = 2; +pub const PROP_ID_REFERENCE: u16 = 2; pub const PROP_ID_U32_VEC: u16 = 3; pub const PROP_ID_U32_VEC_MAX_LEN: u16 = 20; @@ -251,7 +252,7 @@ pub fn with_test_externalities R>(f: F) -> R { } impl Property { - fn required(&self) -> Property { + pub fn required(&self) -> Property { let mut new_self = self.clone(); new_self.required = true; new_self @@ -360,7 +361,7 @@ pub fn create_class_with_schema() -> (ClassId, u16) { vec![ good_prop_bool().required(), good_prop_u32(), - new_internal_class_prop(class_id), + new_reference_class_prop(class_id), good_prop_u32_vec(), ], ) @@ -410,7 +411,7 @@ pub fn good_prop_text() -> Property { } } -pub fn new_internal_class_prop(class_id: ClassId) -> Property { +pub fn new_reference_class_prop(class_id: ClassId) -> Property { Property { prop_type: PropertyType::Reference(class_id), required: false, @@ -419,6 +420,15 @@ pub fn new_internal_class_prop(class_id: ClassId) -> Property { } } +pub fn new_reference_class_prop_vec(class_id: ClassId) -> Property { + Property { + prop_type: PropertyType::ReferenceVec(PROP_ID_U32_VEC_MAX_LEN, class_id), + required: false, + name: b"Name of a internal property".to_vec(), + description: b"Description of a internal property".to_vec(), + } +} + pub fn good_class_name() -> Vec { b"Name of a class".to_vec() } diff --git a/runtime-modules/content-directory/src/tests.rs b/runtime-modules/content-directory/src/tests.rs index 9891a87f19..36e5f5980a 100644 --- a/runtime-modules/content-directory/src/tests.rs +++ b/runtime-modules/content-directory/src/tests.rs @@ -738,7 +738,7 @@ fn cannot_add_class_schema_when_it_refers_unknown_prop_index() { fn cannot_add_class_schema_when_it_refers_unknown_internal_id() { with_test_externalities(|| { let class_id = create_simple_class_with_default_permissions(); - let bad_internal_prop = new_internal_class_prop(UNKNOWN_CLASS_ID); + let bad_internal_prop = new_reference_class_prop(UNKNOWN_CLASS_ID); assert_err!( TestModule::append_class_schema( @@ -755,7 +755,7 @@ fn cannot_add_class_schema_when_it_refers_unknown_internal_id() { fn should_add_class_schema_with_internal_class_prop() { with_test_externalities(|| { let class_id = create_simple_class_with_default_permissions(); - let internal_class_prop = new_internal_class_prop(class_id); + let internal_class_prop = new_reference_class_prop(class_id); // Add first schema with new props. // No other props on the class at this time. @@ -1063,7 +1063,7 @@ fn cannot_add_schema_to_entity_when_unknown_internal_entity_id() { let (_, schema_id, entity_id) = create_class_with_schema_and_entity(); let mut prop_values = bool_prop_value(); prop_values.insert( - PROP_ID_INTERNAL, + PROP_ID_REFERENCE, PropertyValue::Reference(UNKNOWN_ENTITY_ID), ); assert_err!( @@ -1106,7 +1106,7 @@ fn should_add_schema_to_entity_when_some_optional_props_provided() { entity.supported_schemas, BTreeSet::from_iter(vec![SCHEMA_ID_0].into_iter()) ); - prop_values.insert(PROP_ID_INTERNAL, PropertyValue::Bool(false)); + prop_values.insert(PROP_ID_REFERENCE, PropertyValue::Bool(false)); prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Bool(false)); assert_eq!(entity.values, prop_values); }) @@ -1121,7 +1121,7 @@ fn update_entity_props_successfully() { let entity_id = create_entity_with_schema_support(); let mut prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(true)); prop_values.insert(PROP_ID_U32, PropertyValue::Bool(false)); - prop_values.insert(PROP_ID_INTERNAL, PropertyValue::Bool(false)); + prop_values.insert(PROP_ID_REFERENCE, PropertyValue::Bool(false)); prop_values.insert( PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44]), @@ -1129,7 +1129,7 @@ fn update_entity_props_successfully() { assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(false)); prop_values.insert(PROP_ID_U32, PropertyValue::Uint32(123)); - prop_values.insert(PROP_ID_INTERNAL, PropertyValue::Reference(entity_id)); + prop_values.insert(PROP_ID_REFERENCE, PropertyValue::Reference(entity_id)); prop_values.insert( PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44, 88, 43]), @@ -1174,7 +1174,7 @@ fn cannot_update_entity_props_when_unknown_internal_entity_id() { TestModule::complete_entity_property_values_update( entity_id, prop_value( - PROP_ID_INTERNAL, + PROP_ID_REFERENCE, PropertyValue::Reference(UNKNOWN_ENTITY_ID) ) ), @@ -1210,7 +1210,7 @@ fn complete_entity_property_vector_cleaning_successfully() { PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44]), ); - prop_values.insert(PROP_ID_INTERNAL, PropertyValue::Bool(false)); + prop_values.insert(PROP_ID_REFERENCE, PropertyValue::Bool(false)); // Check property values runtime storage related to an entity before cleaning of entity property vector value under given schema id assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); @@ -1273,7 +1273,7 @@ fn complete_remove_at_entity_property_vector() -> EntityId { PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44]), ); - prop_values.insert(PROP_ID_INTERNAL, PropertyValue::Bool(false)); + prop_values.insert(PROP_ID_REFERENCE, PropertyValue::Bool(false)); // Check property values runtime storage related to an entity before removing at given index of entity property vector value assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); @@ -1355,7 +1355,7 @@ fn complete_insert_at_entity_property_vector_successfully() { PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 234, 44]), ); - prop_values.insert(PROP_ID_INTERNAL, PropertyValue::Bool(false)); + prop_values.insert(PROP_ID_REFERENCE, PropertyValue::Bool(false)); // Check property values runtime storage related to an entity before inserting at given index of entity property vector value assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); @@ -1483,7 +1483,43 @@ fn cannot_complete_insert_at_entity_property_vector_when_already_updated() { }) } -// TODO insert at ReferenceVec test +#[test] +fn cannot_complete_insert_at_entity_property_vector_when_unknown_internal_entity_id() { + with_test_externalities(|| { + let class_id = create_simple_class_with_default_permissions(); + let schema_id = TestModule::append_class_schema( + class_id, + vec![], + vec![ + good_prop_bool().required(), + new_reference_class_prop_vec(class_id), + ], + ) + .expect("This should not happen"); + let entity_id = create_entity_of_class(class_id); + let entity_id_2 = create_entity_of_class(class_id); + let mut property_values = BTreeMap::new(); + property_values.insert(PROP_ID_BOOL, PropertyValue::Bool(true)); + property_values.insert( + PROP_ID_REFERENCE_VEC, + PropertyValue::ReferenceVec(vec![entity_id_2]), + ); + assert_ok!(TestModule::add_entity_schema_support( + entity_id, + schema_id, + property_values + )); + assert_err!( + TestModule::complete_insert_at_entity_property_vector( + entity_id, + PROP_ID_REFERENCE_VEC, + 1, + PropertyValue::Reference(entity_id) + ), + ERROR_ENTITY_NOT_FOUND + ); + }) +} // TODO test text max len From 2eccac757960e0971fd734b654b007aab6b191b2 Mon Sep 17 00:00:00 2001 From: iorveth Date: Fri, 17 Apr 2020 12:30:49 +0300 Subject: [PATCH 11/26] Fix clippy warnings --- runtime-modules/content-directory/src/lib.rs | 91 +++++++++---------- .../content-directory/src/tests.rs | 2 +- 2 files changed, 43 insertions(+), 50 deletions(-) diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index a0ecfd301e..1e3e544385 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -6,7 +6,6 @@ use rstd::collections::{btree_map::BTreeMap, btree_set::BTreeSet}; use rstd::prelude::*; use runtime_primitives::traits::{MaybeSerialize, Member, SimpleArithmetic}; use srml_support::{decl_module, decl_storage, dispatch, ensure, traits::Get, Parameter}; -use system; #[cfg(feature = "std")] pub use serde::{Deserialize, Serialize}; @@ -113,12 +112,12 @@ impl InputValidationLengthConstraint { } /// Helper for computing max - pub fn max(&self) -> u16 { + pub fn max(self) -> u16 { self.min + self.max_min_diff } pub fn ensure_valid( - &self, + self, len: usize, too_short_msg: &'static str, too_long_msg: &'static str, @@ -295,7 +294,7 @@ pub struct Property { } #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] -#[derive(Encode, Decode, Clone, PartialEq, Eq, Debug)] +#[derive(Encode, Decode, Clone, Copy, PartialEq, Eq, Debug)] pub enum PropertyType { // Single value: Bool, @@ -729,7 +728,7 @@ decl_module! { // at this point we don't enforce anything about reference constraints // because of the chicken and egg problem. Instead enforcement is done // at the time of creating an entity. - let _schema_index = Self::complete_class_schema_status_update(class_id, schema_id, is_active)?; + Self::complete_class_schema_status_update(class_id, schema_id, is_active)?; Ok(()) } ) @@ -1056,7 +1055,7 @@ impl Module { entity_id: EntityId, new_property_values: BTreeMap, ) -> dispatch::Result { - Self::ensure_known_entity_id(&entity_id)?; + Self::ensure_known_entity_id(entity_id)?; let (entity, class) = Self::get_entity_and_class(entity_id); @@ -1124,7 +1123,7 @@ impl Module { entity_id: EntityId, in_class_schema_property_id: u16, ) -> dispatch::Result { - Self::ensure_known_entity_id(&entity_id)?; + Self::ensure_known_entity_id(entity_id)?; let entity = Self::entity_by_id(entity_id); match entity.values.get(&in_class_schema_property_id) { @@ -1140,11 +1139,11 @@ impl Module { // Clear property value vector: >::mutate(entity_id, |entity| { - entity - .values - .get_mut(&in_class_schema_property_id) - .as_deref_mut() - .map(|current_property_value_vec| current_property_value_vec.vec_clear()); + if let Some(current_property_value_vec) = + entity.values.get_mut(&in_class_schema_property_id) + { + current_property_value_vec.vec_clear(); + } Self::refresh_vec_values_last_update( in_class_schema_property_id, &mut entity.vec_values_last_update, @@ -1159,7 +1158,7 @@ impl Module { in_class_schema_property_id: u16, index_in_property_vec: u32, ) -> dispatch::Result { - Self::ensure_known_entity_id(&entity_id)?; + Self::ensure_known_entity_id(entity_id)?; let entity = Self::entity_by_id(entity_id); // Ensure property value vector was not already updated in this block to avoid possible data races, @@ -1182,11 +1181,9 @@ impl Module { // Remove property value vector >::mutate(entity_id, |entity| { - entity - .values - .get_mut(&in_class_schema_property_id) - .as_deref_mut() - .map(|current_prop_value| current_prop_value.vec_remove_at(index_in_property_vec)); + if let Some(current_prop_value) = entity.values.get_mut(&in_class_schema_property_id) { + current_prop_value.vec_remove_at(index_in_property_vec) + } Self::refresh_vec_values_last_update( in_class_schema_property_id, &mut entity.vec_values_last_update, @@ -1202,7 +1199,7 @@ impl Module { index_in_property_vec: u32, property_value: PropertyValue, ) -> dispatch::Result { - Self::ensure_known_entity_id(&entity_id)?; + Self::ensure_known_entity_id(entity_id)?; let (entity, class) = Self::get_entity_and_class(entity_id); @@ -1240,13 +1237,9 @@ impl Module { // Insert property value into property value vector >::mutate(entity_id, |entity| { - entity - .values - .get_mut(&in_class_schema_property_id) - .as_deref_mut() - .map(|current_prop_value| { - current_prop_value.vec_insert_at(index_in_property_vec, property_value) - }); + if let Some(current_prop_value) = entity.values.get_mut(&in_class_schema_property_id) { + current_prop_value.vec_insert_at(index_in_property_vec, property_value) + } Self::refresh_vec_values_last_update( in_class_schema_property_id, &mut entity.vec_values_last_update, @@ -1467,7 +1460,7 @@ impl Module { existing_properties: Vec, new_properties: Vec, ) -> Result { - Self::ensure_known_class_id(&class_id)?; + Self::ensure_known_class_id(class_id)?; let non_empty_schema = !existing_properties.is_empty() || !new_properties.is_empty(); @@ -1540,7 +1533,7 @@ impl Module { schema_id: u16, property_values: BTreeMap, ) -> dispatch::Result { - Self::ensure_known_entity_id(&entity_id)?; + Self::ensure_known_entity_id(entity_id)?; let (entity, class) = Self::get_entity_and_class(entity_id); @@ -1616,12 +1609,12 @@ impl Module { // Helper functions: // ---------------------------------------------------------------- - pub fn ensure_known_class_id(class_id: &ClassId) -> dispatch::Result { + pub fn ensure_known_class_id(class_id: ClassId) -> dispatch::Result { ensure!(>::exists(class_id), ERROR_CLASS_NOT_FOUND); Ok(()) } - pub fn ensure_known_entity_id(entity_id: &EntityId) -> dispatch::Result { + pub fn ensure_known_entity_id(entity_id: EntityId) -> dispatch::Result { ensure!(>::exists(entity_id), ERROR_ENTITY_NOT_FOUND); Ok(()) } @@ -1649,13 +1642,13 @@ impl Module { } pub fn ensure_valid_internal_prop(value: &PropertyValue, prop: &Property) -> dispatch::Result { - match (value, &prop.prop_type) { + match (value, prop.prop_type) { (PV::Reference(entity_id), PT::Reference(class_id)) => { Self::ensure_known_class_id(class_id)?; - Self::ensure_known_entity_id(entity_id)?; + Self::ensure_known_entity_id(*entity_id)?; let entity = Self::entity_by_id(entity_id); ensure!( - entity.class_id == *class_id, + entity.class_id == class_id, ERROR_INTERNAL_PROP_DOES_NOT_MATCH_ITS_CLASS ); Ok(()) @@ -1694,11 +1687,11 @@ impl Module { entity_prop_value: &PropertyValue, prop: &Property, ) -> dispatch::Result { - fn validate_prop_vec_len_after_value_insert(vec: &[T], max_len: &u16) -> bool { - vec.len() < *max_len as usize + fn validate_prop_vec_len_after_value_insert(vec: &[T], max_len: u16) -> bool { + vec.len() < max_len as usize } - let is_valid_len = match (value, entity_prop_value, &prop.prop_type) { + let is_valid_len = match (value, entity_prop_value, prop.prop_type) { // Single values (PV::Bool(_), PV::BoolVec(vec), PT::BoolVec(max_len)) => { validate_prop_vec_len_after_value_insert(vec, max_len) @@ -1723,7 +1716,7 @@ impl Module { } (PV::Text(text_item), PV::TextVec(vec), PT::TextVec(vec_max_len, text_max_len)) => { if validate_prop_vec_len_after_value_insert(vec, vec_max_len) { - Self::validate_max_len_of_text(text_item, *text_max_len)?; + Self::validate_max_len_of_text(text_item, text_max_len)?; true } else { false @@ -1736,10 +1729,10 @@ impl Module { ) => { Self::ensure_known_class_id(class_id)?; if validate_prop_vec_len_after_value_insert(vec, vec_max_len) { - Self::ensure_known_entity_id(entity_id)?; + Self::ensure_known_entity_id(*entity_id)?; let entity = Self::entity_by_id(entity_id); ensure!( - entity.class_id == *class_id, + entity.class_id == class_id, ERROR_INTERNAL_PROP_DOES_NOT_MATCH_ITS_CLASS ); true @@ -1776,11 +1769,11 @@ impl Module { value: &PropertyValue, prop: &Property, ) -> dispatch::Result { - fn validate_vec_len(vec: &[T], max_len: &u16) -> bool { - vec.len() <= *max_len as usize + fn validate_vec_len(vec: &[T], max_len: u16) -> bool { + vec.len() <= max_len as usize } - let is_valid_len = match (value, &prop.prop_type) { + let is_valid_len = match (value, prop.prop_type) { (PV::BoolVec(vec), PT::BoolVec(max_len)) => validate_vec_len(vec, max_len), (PV::Uint16Vec(vec), PT::Uint16Vec(max_len)) => validate_vec_len(vec, max_len), (PV::Uint32Vec(vec), PT::Uint32Vec(max_len)) => validate_vec_len(vec, max_len), @@ -1792,7 +1785,7 @@ impl Module { (PV::TextVec(vec), PT::TextVec(vec_max_len, text_max_len)) => { if validate_vec_len(vec, vec_max_len) { for text_item in vec.iter() { - Self::validate_max_len_of_text(text_item, *text_max_len)?; + Self::validate_max_len_of_text(text_item, text_max_len)?; } true } else { @@ -1804,10 +1797,10 @@ impl Module { Self::ensure_known_class_id(class_id)?; if validate_vec_len(vec, vec_max_len) { for entity_id in vec.iter() { - Self::ensure_known_entity_id(entity_id)?; + Self::ensure_known_entity_id(*entity_id)?; let entity = Self::entity_by_id(entity_id); ensure!( - entity.class_id == *class_id, + entity.class_id == class_id, ERROR_INTERNAL_PROP_DOES_NOT_MATCH_ITS_CLASS ); } @@ -1870,7 +1863,7 @@ impl Module { } } - pub fn ensure_property_name_is_valid(text: &Vec) -> dispatch::Result { + pub fn ensure_property_name_is_valid(text: &[u8]) -> dispatch::Result { T::PropertyNameConstraint::get().ensure_valid( text.len(), ERROR_PROPERTY_NAME_TOO_SHORT, @@ -1878,7 +1871,7 @@ impl Module { ) } - pub fn ensure_property_description_is_valid(text: &Vec) -> dispatch::Result { + pub fn ensure_property_description_is_valid(text: &[u8]) -> dispatch::Result { T::PropertyDescriptionConstraint::get().ensure_valid( text.len(), ERROR_PROPERTY_DESCRIPTION_TOO_SHORT, @@ -1886,7 +1879,7 @@ impl Module { ) } - pub fn ensure_class_name_is_valid(text: &Vec) -> dispatch::Result { + pub fn ensure_class_name_is_valid(text: &[u8]) -> dispatch::Result { T::ClassNameConstraint::get().ensure_valid( text.len(), ERROR_CLASS_NAME_TOO_SHORT, @@ -1894,7 +1887,7 @@ impl Module { ) } - pub fn ensure_class_description_is_valid(text: &Vec) -> dispatch::Result { + pub fn ensure_class_description_is_valid(text: &[u8]) -> dispatch::Result { T::ClassDescriptionConstraint::get().ensure_valid( text.len(), ERROR_CLASS_DESCRIPTION_TOO_SHORT, diff --git a/runtime-modules/content-directory/src/tests.rs b/runtime-modules/content-directory/src/tests.rs index 36e5f5980a..889ac16cdd 100644 --- a/runtime-modules/content-directory/src/tests.rs +++ b/runtime-modules/content-directory/src/tests.rs @@ -1514,7 +1514,7 @@ fn cannot_complete_insert_at_entity_property_vector_when_unknown_internal_entity entity_id, PROP_ID_REFERENCE_VEC, 1, - PropertyValue::Reference(entity_id) + PropertyValue::Reference(UNKNOWN_ENTITY_ID) ), ERROR_ENTITY_NOT_FOUND ); From 52bd20fca9226c70ef6a86fd60b827dbabdf8996 Mon Sep 17 00:00:00 2001 From: iorveth Date: Fri, 17 Apr 2020 13:47:58 +0300 Subject: [PATCH 12/26] Introduce new nonce-like mechanism, tests updated --- .../content-directory/src/errors.rs | 4 +- runtime-modules/content-directory/src/lib.rs | 108 +++++++++++------- runtime-modules/content-directory/src/mock.rs | 5 + .../content-directory/src/tests.rs | 58 +++++++--- 4 files changed, 117 insertions(+), 58 deletions(-) diff --git a/runtime-modules/content-directory/src/errors.rs b/runtime-modules/content-directory/src/errors.rs index 6ecf91ac25..2e0c5687b9 100644 --- a/runtime-modules/content-directory/src/errors.rs +++ b/runtime-modules/content-directory/src/errors.rs @@ -33,8 +33,8 @@ pub const ERROR_PROP_VALUE_DONT_MATCH_VEC_TYPE: &str = "Property value don't match the expected vector property type"; pub const ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR: &str = "Property value under given index is not a vector"; -pub const ERROR_PROP_VALUE_VEC_WAS_ALREADY_UPDATED: &str = - "Property value vector was already updated in this block, vector specific operations forbidden to avoid possible data races"; +pub const ERROR_PROP_VALUE_VEC_NONCES_DOES_NOT_MATCH: &str = + "Current property value vector nonce does not equal to provided one"; pub const ERROR_PROP_NAME_NOT_UNIQUE_IN_CLASS: &str = "Property name is not unique within its class"; pub const ERROR_MISSING_REQUIRED_PROP: &str = diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index 1e3e544385..a35c063b7f 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -4,7 +4,7 @@ use codec::{Codec, Decode, Encode}; use rstd::collections::{btree_map::BTreeMap, btree_set::BTreeSet}; use rstd::prelude::*; -use runtime_primitives::traits::{MaybeSerialize, Member, SimpleArithmetic}; +use runtime_primitives::traits::{MaybeSerialize, MaybeSerializeDeserialize, One, Zero, Member, SimpleArithmetic}; use srml_support::{decl_module, decl_storage, dispatch, ensure, traits::Get, Parameter}; #[cfg(feature = "std")] @@ -41,6 +41,20 @@ pub trait Trait: system::Trait { + PartialEq + Ord; + type Nonce: Parameter + + Member + + SimpleArithmetic + + Codec + + Default + + Copy + + Clone + + One + + Zero + + MaybeSerializeDeserialize + + Eq + + PartialEq + + Ord; + /// Security/configuration constraints type PropertyNameConstraint: Get; @@ -223,9 +237,9 @@ pub struct Entity { /// Length is no more than Class.properties. pub values: BTreeMap, - /// Map, representing relation between entity vec_values index and block, where vec_value was updated - /// Used to forbid mutating property vec value more than once per block for purpose of race update conditions avoiding - pub vec_values_last_update: BTreeMap::BlockNumber>, + /// Map, representing relation between entity vec_values index and nonce, where vec_value was updated + /// Used to avoid race update conditions + pub vec_value_nonces: BTreeMap, // pub deleted: bool, } @@ -235,7 +249,7 @@ impl Default for Entity { class_id: ClassId::default(), supported_schemas: BTreeSet::new(), values: BTreeMap::new(), - vec_values_last_update: BTreeMap::new(), + vec_value_nonces: BTreeMap::new(), } } } @@ -786,10 +800,11 @@ decl_module! { as_entity_maintainer: bool, entity_id: EntityId, in_class_schema_property_id: u16, - index_in_property_vec: u32 + index_in_property_vec: u32, + nonce: T::Nonce ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; - Self::do_remove_at_entity_property_vector(&raw_origin, with_credential, as_entity_maintainer, entity_id, in_class_schema_property_id, index_in_property_vec) + Self::do_remove_at_entity_property_vector(&raw_origin, with_credential, as_entity_maintainer, entity_id, in_class_schema_property_id, index_in_property_vec, nonce) } pub fn insert_at_entity_property_vector( @@ -799,7 +814,8 @@ decl_module! { entity_id: EntityId, in_class_schema_property_id: u16, index_in_property_vec: u32, - property_value: PropertyValue + property_value: PropertyValue, + nonce: T::Nonce ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; Self::do_insert_at_entity_property_vector( @@ -809,7 +825,8 @@ decl_module! { entity_id, in_class_schema_property_id, index_in_property_vec, - property_value + property_value, + nonce ) } @@ -979,6 +996,7 @@ impl Module { entity_id: EntityId, in_class_schema_property_id: u16, index_in_property_vec: u32, + nonce: T::Nonce ) -> dispatch::Result { let class_id = Self::get_class_id_by_entity_id(entity_id)?; @@ -999,6 +1017,7 @@ impl Module { entity_id, in_class_schema_property_id, index_in_property_vec, + nonce ) }, ) @@ -1012,6 +1031,7 @@ impl Module { in_class_schema_property_id: u16, index_in_property_vec: u32, property_value: PropertyValue, + nonce: T::Nonce ) -> dispatch::Result { let class_id = Self::get_class_id_by_entity_id(entity_id)?; @@ -1033,6 +1053,7 @@ impl Module { in_class_schema_property_id, index_in_property_vec, property_value, + nonce ) }, ) @@ -1062,7 +1083,7 @@ impl Module { // Get current property values of an entity as a mutable vector, // so we can update them if new values provided present in new_property_values. let mut updated_values = entity.values; - let mut vec_values_last_update = entity.vec_values_last_update; + let mut vec_value_nonces = entity.vec_value_nonces; let mut updated = false; // Iterate over a vector of new values and update corresponding properties // of this entity if new values are valid. @@ -1081,7 +1102,7 @@ impl Module { *current_prop_value = new_value; if current_prop_value.is_vec() { // Update last block of vec prop value if update performed - Self::refresh_vec_values_last_update(id, &mut vec_values_last_update); + Self::refresh_vec_value_nonces(id, &mut vec_value_nonces); } updated = true; } @@ -1096,27 +1117,24 @@ impl Module { if updated { >::mutate(entity_id, |entity| { entity.values = updated_values; - entity.vec_values_last_update = vec_values_last_update; + entity.vec_value_nonces = vec_value_nonces; }); } Ok(()) } - fn refresh_vec_values_last_update( + fn refresh_vec_value_nonces( id: u16, - vec_values_last_update: &mut BTreeMap, + vec_value_nonces: &mut BTreeMap, ) { - match vec_values_last_update.get_mut(&id) { - Some(block_number) if *block_number < >::block_number() => { - *block_number = >::block_number(); - return; - } - Some(_) => return, - None => (), + if let Some(nonce) = vec_value_nonces.get_mut(&id) { + *nonce += T::Nonce::one(); + } else { + // If there no nonce entry under a given key, we need to initialize it manually with one nonce, as given entity value exist + // and first vec value specific operation was already performed + vec_value_nonces.insert(id, T::Nonce::one()); } - // If there no last block number entry under a given key, we need to initialize it manually, as given entity value exist - vec_values_last_update.insert(id, >::block_number()); } fn complete_entity_property_vector_cleaning( @@ -1144,9 +1162,9 @@ impl Module { { current_property_value_vec.vec_clear(); } - Self::refresh_vec_values_last_update( + Self::refresh_vec_value_nonces( in_class_schema_property_id, - &mut entity.vec_values_last_update, + &mut entity.vec_value_nonces, ); }); @@ -1157,15 +1175,17 @@ impl Module { entity_id: EntityId, in_class_schema_property_id: u16, index_in_property_vec: u32, + nonce: T::Nonce ) -> dispatch::Result { Self::ensure_known_entity_id(entity_id)?; let entity = Self::entity_by_id(entity_id); - // Ensure property value vector was not already updated in this block to avoid possible data races, + // Ensure property value vector nonces equality to avoid possible data races, // when performing vector specific operations - Self::ensure_entity_prop_value_vec_was_not_updated( + Self::ensure_nonce_equality( in_class_schema_property_id, - &entity.vec_values_last_update, + &entity.vec_value_nonces, + nonce )?; match entity.values.get(&in_class_schema_property_id) { @@ -1184,9 +1204,9 @@ impl Module { if let Some(current_prop_value) = entity.values.get_mut(&in_class_schema_property_id) { current_prop_value.vec_remove_at(index_in_property_vec) } - Self::refresh_vec_values_last_update( + Self::refresh_vec_value_nonces( in_class_schema_property_id, - &mut entity.vec_values_last_update, + &mut entity.vec_value_nonces, ); }); @@ -1198,16 +1218,18 @@ impl Module { in_class_schema_property_id: u16, index_in_property_vec: u32, property_value: PropertyValue, + nonce: T::Nonce ) -> dispatch::Result { Self::ensure_known_entity_id(entity_id)?; let (entity, class) = Self::get_entity_and_class(entity_id); - // Ensure property value vector was not already updated in this block to avoid possible data races, + // Ensure property value vector nonces equality to avoid possible data races, // when performing vector specific operations - Self::ensure_entity_prop_value_vec_was_not_updated( + Self::ensure_nonce_equality( in_class_schema_property_id, - &entity.vec_values_last_update, + &entity.vec_value_nonces, + nonce )?; // Get class-level information about this property if let Some(class_prop) = class.properties.get(in_class_schema_property_id as usize) { @@ -1240,9 +1262,9 @@ impl Module { if let Some(current_prop_value) = entity.values.get_mut(&in_class_schema_property_id) { current_prop_value.vec_insert_at(index_in_property_vec, property_value) } - Self::refresh_vec_values_last_update( + Self::refresh_vec_value_nonces( in_class_schema_property_id, - &mut entity.vec_values_last_update, + &mut entity.vec_value_nonces, ); }); @@ -1441,14 +1463,20 @@ impl Module { Ok(()) } - fn ensure_entity_prop_value_vec_was_not_updated( + fn ensure_nonce_equality( in_class_schema_property_id: u16, - vec_values_last_update: &BTreeMap, + vec_value_nonces: &BTreeMap, + nonce: T::Nonce ) -> dispatch::Result { - if let Some(last_update_block) = vec_values_last_update.get(&in_class_schema_property_id) { + if let Some(vec_value_nonce) = vec_value_nonces.get(&in_class_schema_property_id) { + ensure!( + *vec_value_nonce == nonce, + ERROR_PROP_VALUE_VEC_NONCES_DOES_NOT_MATCH + ); + } else { ensure!( - *last_update_block < >::block_number(), - ERROR_PROP_VALUE_VEC_WAS_ALREADY_UPDATED + nonce == T::Nonce::zero(), + ERROR_PROP_VALUE_VEC_NONCES_DOES_NOT_MATCH ); } Ok(()) diff --git a/runtime-modules/content-directory/src/mock.rs b/runtime-modules/content-directory/src/mock.rs index 2c7cb38f2b..e9b3e33b90 100644 --- a/runtime-modules/content-directory/src/mock.rs +++ b/runtime-modules/content-directory/src/mock.rs @@ -25,6 +25,10 @@ pub const UNKNOWN_SCHEMA_ID: u16 = 444; pub const SCHEMA_ID_0: u16 = 0; pub const SCHEMA_ID_1: u16 = 1; +pub const ZERO_NONCE: u64 = 0; +pub const FIRST_NONCE: u64 = 1; +pub const SECOND_NONCE: u64 = 2; + pub const PROP_ID_BOOL: u16 = 0; pub const PROP_ID_REFERENCE_VEC: u16 = 1; pub const PROP_ID_U32: u16 = 1; @@ -126,6 +130,7 @@ impl Get for ClassDescriptionConstraint { impl Trait for Runtime { type Credential = u64; + type Nonce = u64; type CredentialChecker = MockCredentialChecker; type CreateClassPermissionsChecker = MockCreateClassPermissionsChecker; type PropertyNameConstraint = PropertyNameConstraint; diff --git a/runtime-modules/content-directory/src/tests.rs b/runtime-modules/content-directory/src/tests.rs index 889ac16cdd..7eb990c061 100644 --- a/runtime-modules/content-directory/src/tests.rs +++ b/runtime-modules/content-directory/src/tests.rs @@ -1282,7 +1282,8 @@ fn complete_remove_at_entity_property_vector() -> EntityId { assert_ok!(TestModule::complete_remove_at_entity_property_vector( entity_id, PROP_ID_U32_VEC, - 1 + 1, + ZERO_NONCE )); // Update entity property values to compare with runtime storage entity value under given schema id @@ -1297,7 +1298,14 @@ fn complete_remove_at_entity_property_vector() -> EntityId { #[test] fn complete_remove_at_entity_property_vector_successfully() { with_test_externalities(|| { - complete_remove_at_entity_property_vector(); + let entity_id = complete_remove_at_entity_property_vector(); + // Perform second removal at given index of entity property vector value with new nonce + assert_ok!(TestModule::complete_remove_at_entity_property_vector( + entity_id, + PROP_ID_U32_VEC, + 1, + FIRST_NONCE + )); }) } @@ -1308,6 +1316,7 @@ fn cannot_complete_remove_at_entity_property_vector_when_entity_not_found() { UNKNOWN_ENTITY_ID, PROP_ID_U32_VEC, 1, + ZERO_NONCE )); }) } @@ -1317,7 +1326,7 @@ fn cannot_complete_remove_at_entity_property_vector_when_unknown_entity_prop_id( with_test_externalities(|| { let entity_id = create_entity_with_schema_support(); assert_err!( - TestModule::complete_remove_at_entity_property_vector(entity_id, UNKNOWN_PROP_ID, 1), + TestModule::complete_remove_at_entity_property_vector(entity_id, UNKNOWN_PROP_ID, 1, ZERO_NONCE), ERROR_UNKNOWN_ENTITY_PROP_ID ); }) @@ -1328,7 +1337,7 @@ fn cannot_complete_remove_at_entity_property_vector_when_entity_prop_id_is_not_a with_test_externalities(|| { let entity_id = create_entity_with_schema_support(); assert_err!( - TestModule::complete_remove_at_entity_property_vector(entity_id, PROP_ID_U32, 1), + TestModule::complete_remove_at_entity_property_vector(entity_id, PROP_ID_U32, 1, ZERO_NONCE), ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR ); }) @@ -1339,8 +1348,8 @@ fn cannot_complete_remove_at_entity_property_vector_when_already_updated() { with_test_externalities(|| { let entity_id = complete_remove_at_entity_property_vector(); assert_err!( - TestModule::complete_remove_at_entity_property_vector(entity_id, PROP_ID_U32_VEC, 1), - ERROR_PROP_VALUE_VEC_WAS_ALREADY_UPDATED + TestModule::complete_remove_at_entity_property_vector(entity_id, PROP_ID_U32_VEC, 1, SECOND_NONCE), + ERROR_PROP_VALUE_VEC_NONCES_DOES_NOT_MATCH ); }) } @@ -1365,13 +1374,23 @@ fn complete_insert_at_entity_property_vector_successfully() { entity_id, PROP_ID_U32_VEC, 1, - PropertyValue::Uint32(33) + PropertyValue::Uint32(33), + ZERO_NONCE + )); + + // Perform second inserting at given index of entity property vector value with new nonce + assert_ok!(TestModule::complete_insert_at_entity_property_vector( + entity_id, + PROP_ID_U32_VEC, + 1, + PropertyValue::Uint32(55), + FIRST_NONCE )); // Update entity property values to compare with runtime storage entity value under given schema id prop_values.insert( PROP_ID_U32_VEC, - PropertyValue::Uint32Vec(vec![123, 33, 234, 44]), + PropertyValue::Uint32Vec(vec![123, 55, 33, 234, 44]), ); // Check property values runtime storage related to a entity right after @@ -1388,6 +1407,7 @@ fn cannot_complete_insert_at_entity_property_vector_when_entity_not_found() { PROP_ID_U32_VEC, 1, PropertyValue::Uint32(33), + ZERO_NONCE )); }) } @@ -1401,7 +1421,8 @@ fn cannot_complete_insert_at_entity_property_vector_when_unknown_entity_prop_id( entity_id, UNKNOWN_PROP_ID, 1, - PropertyValue::Uint32(33) + PropertyValue::Uint32(33), + ZERO_NONCE ), ERROR_UNKNOWN_ENTITY_PROP_ID ); @@ -1417,7 +1438,8 @@ fn cannot_complete_insert_at_entity_property_vector_when_entity_prop_id_is_not_a entity_id, PROP_ID_U32, 1, - PropertyValue::Uint32(17) + PropertyValue::Uint32(17), + ZERO_NONCE ), ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR ); @@ -1433,7 +1455,8 @@ fn cannot_complete_insert_at_entity_property_type_does_not_match_internal_entity entity_id, PROP_ID_U32_VEC, 1, - PropertyValue::Uint16(33) + PropertyValue::Uint16(33), + ZERO_NONCE ), ERROR_PROP_VALUE_TYPE_DOESNT_MATCH_INTERNAL_ENTITY_VECTOR_TYPE ); @@ -1460,7 +1483,8 @@ fn cannot_complete_insert_at_entity_property_vector_when_entity_prop_value_vecto entity_id, PROP_ID_U32_VEC, 1, - PropertyValue::Uint32(33) + PropertyValue::Uint32(33), + ZERO_NONCE ), ERROR_ENTITY_PROP_VALUE_VECTOR_IS_TOO_LONG ); @@ -1468,7 +1492,7 @@ fn cannot_complete_insert_at_entity_property_vector_when_entity_prop_value_vecto } #[test] -fn cannot_complete_insert_at_entity_property_vector_when_already_updated() { +fn cannot_complete_insert_at_entity_property_vector_when_nonce_does_not_match() { with_test_externalities(|| { let entity_id = complete_remove_at_entity_property_vector(); assert_err!( @@ -1476,9 +1500,10 @@ fn cannot_complete_insert_at_entity_property_vector_when_already_updated() { entity_id, PROP_ID_U32_VEC, 1, - PropertyValue::Uint32(33) + PropertyValue::Uint32(33), + SECOND_NONCE ), - ERROR_PROP_VALUE_VEC_WAS_ALREADY_UPDATED + ERROR_PROP_VALUE_VEC_NONCES_DOES_NOT_MATCH ); }) } @@ -1514,7 +1539,8 @@ fn cannot_complete_insert_at_entity_property_vector_when_unknown_internal_entity entity_id, PROP_ID_REFERENCE_VEC, 1, - PropertyValue::Reference(UNKNOWN_ENTITY_ID) + PropertyValue::Reference(UNKNOWN_ENTITY_ID), + ZERO_NONCE ), ERROR_ENTITY_NOT_FOUND ); From 85c53546af330bedd097a20d48f04b7f6c448d6d Mon Sep 17 00:00:00 2001 From: iorveth Date: Fri, 17 Apr 2020 14:24:29 +0300 Subject: [PATCH 13/26] Improve error handling, when entity vector value index is out of range --- .../content-directory/src/errors.rs | 2 + runtime-modules/content-directory/src/lib.rs | 68 ++++++++++++------- .../content-directory/src/tests.rs | 2 +- 3 files changed, 47 insertions(+), 25 deletions(-) diff --git a/runtime-modules/content-directory/src/errors.rs b/runtime-modules/content-directory/src/errors.rs index 2e0c5687b9..4b90625187 100644 --- a/runtime-modules/content-directory/src/errors.rs +++ b/runtime-modules/content-directory/src/errors.rs @@ -44,6 +44,8 @@ pub const ERROR_TEXT_PROP_IS_TOO_LONG: &str = "Text propery is too long"; pub const ERROR_VEC_PROP_IS_TOO_LONG: &str = "Vector propery is too long"; pub const ERROR_ENTITY_PROP_VALUE_VECTOR_IS_TOO_LONG: &str = "Propery value vector can`t contain more values"; +pub const ERROR_ENTITY_PROP_VALUE_VECTOR_INDEX_IS_OUT_OF_RANGE: &str = + "Given property value vector index is out of range"; pub const ERROR_PROP_VALUE_TYPE_DOESNT_MATCH_INTERNAL_ENTITY_VECTOR_TYPE: &str = "Propery value type does not match internal entity vector type"; pub const ERROR_INTERNAL_PROP_DOES_NOT_MATCH_ITS_CLASS: &str = diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index a35c063b7f..7a906f7cbe 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -1149,7 +1149,7 @@ impl Module { Some(_) => return Err(ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR), _ => // Throw an error if a property was not found on entity - // by an in-class index of a property update. + // by an in-class index of a property. { return Err(ERROR_UNKNOWN_ENTITY_PROP_ID) } @@ -1188,17 +1188,14 @@ impl Module { nonce )?; - match entity.values.get(&in_class_schema_property_id) { - Some(current_prop_value) if current_prop_value.is_vec() => (), - Some(_) => return Err(ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR), - _ => + if let Some(current_prop_value) = entity.values.get(&in_class_schema_property_id) { + Self::ensure_index_in_property_vector_is_valid(current_prop_value, index_in_property_vec)?; + } else { // Throw an error if a property was not found on entity - // by an in-class index of a property update. - { - return Err(ERROR_UNKNOWN_ENTITY_PROP_ID) - } + // by an in-class index of a property. + return Err(ERROR_UNKNOWN_ENTITY_PROP_ID) } - + // Remove property value vector >::mutate(entity_id, |entity| { if let Some(current_prop_value) = entity.values.get_mut(&in_class_schema_property_id) { @@ -1235,19 +1232,16 @@ impl Module { if let Some(class_prop) = class.properties.get(in_class_schema_property_id as usize) { // Try to find a current property value in the entity // by matching its id to the id of a property with an updated value. - match entity.values.get(&in_class_schema_property_id) { - Some(entity_prop_value) if entity_prop_value.is_vec() => { - // Validate a new property value against the type of this property - // and check any additional constraints like the length of a vector - // if it's a vector property or the length of a text if it's a text property. - Self::ensure_prop_value_can_be_insert_at_prop_vec( - &property_value, - entity_prop_value, - class_prop, - )?; - } - Some(_) => return Err(ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR), - _ => (), + if let Some(entity_prop_value) = entity.values.get(&in_class_schema_property_id) { + // Validate a new property value against the type of this property + // and check any additional constraints like the length of a vector + // if it's a vector property or the length of a text if it's a text property. + Self::ensure_prop_value_can_be_inserted_at_prop_vec( + &property_value, + entity_prop_value, + index_in_property_vec, + class_prop, + )?; } } else { // Throw an error if a property was not found on entity @@ -1685,6 +1679,29 @@ impl Module { } } + pub fn ensure_index_in_property_vector_is_valid(value: &PropertyValue, index_in_property_vec: u32) -> dispatch::Result { + + fn is_valid_index(vec: &[T], index_in_property_vec: u32) -> bool { + (index_in_property_vec as usize) < vec.len() + } + + let is_valid_index = match value { + PropertyValue::BoolVec(vec) => is_valid_index(vec, index_in_property_vec), + PropertyValue::Uint16Vec(vec) => is_valid_index(vec, index_in_property_vec), + PropertyValue::Uint32Vec(vec) => is_valid_index(vec, index_in_property_vec), + PropertyValue::Uint64Vec(vec) => is_valid_index(vec, index_in_property_vec), + PropertyValue::Int16Vec(vec) => is_valid_index(vec, index_in_property_vec), + PropertyValue::Int32Vec(vec) => is_valid_index(vec, index_in_property_vec), + PropertyValue::Int64Vec(vec) => is_valid_index(vec, index_in_property_vec), + PropertyValue::TextVec(vec) => is_valid_index(vec, index_in_property_vec), + PropertyValue::ReferenceVec(vec) => is_valid_index(vec, index_in_property_vec), + _ => return Err(ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR), + }; + + ensure!(is_valid_index, ERROR_ENTITY_PROP_VALUE_VECTOR_INDEX_IS_OUT_OF_RANGE); + Ok(()) + } + pub fn is_unknown_internal_entity_id(id: PropertyValue) -> bool { if let PropertyValue::Reference(entity_id) = id { !>::exists(entity_id) @@ -1710,11 +1727,14 @@ impl Module { Ok(()) } - pub fn ensure_prop_value_can_be_insert_at_prop_vec( + pub fn ensure_prop_value_can_be_inserted_at_prop_vec( value: &PropertyValue, entity_prop_value: &PropertyValue, + index_in_property_vec: u32, prop: &Property, ) -> dispatch::Result { + Self::ensure_index_in_property_vector_is_valid(entity_prop_value, index_in_property_vec)?; + fn validate_prop_vec_len_after_value_insert(vec: &[T], max_len: u16) -> bool { vec.len() < max_len as usize } diff --git a/runtime-modules/content-directory/src/tests.rs b/runtime-modules/content-directory/src/tests.rs index 7eb990c061..378f95276b 100644 --- a/runtime-modules/content-directory/src/tests.rs +++ b/runtime-modules/content-directory/src/tests.rs @@ -1538,7 +1538,7 @@ fn cannot_complete_insert_at_entity_property_vector_when_unknown_internal_entity TestModule::complete_insert_at_entity_property_vector( entity_id, PROP_ID_REFERENCE_VEC, - 1, + 0, PropertyValue::Reference(UNKNOWN_ENTITY_ID), ZERO_NONCE ), From e0ee6300b00d6af0fc49fb9b5111a8d2baf58005 Mon Sep 17 00:00:00 2001 From: iorveth Date: Fri, 17 Apr 2020 14:46:38 +0300 Subject: [PATCH 14/26] Tests: add respective failure cases for insert_at, remove_at entity values vec operations, when provided vector index is out of range --- runtime-modules/content-directory/src/lib.rs | 57 ++++++------ runtime-modules/content-directory/src/mock.rs | 3 + .../content-directory/src/tests.rs | 89 ++++++++++++++----- 3 files changed, 100 insertions(+), 49 deletions(-) diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index 7a906f7cbe..55eb795dbd 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -4,7 +4,9 @@ use codec::{Codec, Decode, Encode}; use rstd::collections::{btree_map::BTreeMap, btree_set::BTreeSet}; use rstd::prelude::*; -use runtime_primitives::traits::{MaybeSerialize, MaybeSerializeDeserialize, One, Zero, Member, SimpleArithmetic}; +use runtime_primitives::traits::{ + MaybeSerialize, MaybeSerializeDeserialize, Member, One, SimpleArithmetic, Zero, +}; use srml_support::{decl_module, decl_storage, dispatch, ensure, traits::Get, Parameter}; #[cfg(feature = "std")] @@ -996,7 +998,7 @@ impl Module { entity_id: EntityId, in_class_schema_property_id: u16, index_in_property_vec: u32, - nonce: T::Nonce + nonce: T::Nonce, ) -> dispatch::Result { let class_id = Self::get_class_id_by_entity_id(entity_id)?; @@ -1017,7 +1019,7 @@ impl Module { entity_id, in_class_schema_property_id, index_in_property_vec, - nonce + nonce, ) }, ) @@ -1031,7 +1033,7 @@ impl Module { in_class_schema_property_id: u16, index_in_property_vec: u32, property_value: PropertyValue, - nonce: T::Nonce + nonce: T::Nonce, ) -> dispatch::Result { let class_id = Self::get_class_id_by_entity_id(entity_id)?; @@ -1053,7 +1055,7 @@ impl Module { in_class_schema_property_id, index_in_property_vec, property_value, - nonce + nonce, ) }, ) @@ -1124,14 +1126,11 @@ impl Module { Ok(()) } - fn refresh_vec_value_nonces( - id: u16, - vec_value_nonces: &mut BTreeMap, - ) { + fn refresh_vec_value_nonces(id: u16, vec_value_nonces: &mut BTreeMap) { if let Some(nonce) = vec_value_nonces.get_mut(&id) { *nonce += T::Nonce::one(); } else { - // If there no nonce entry under a given key, we need to initialize it manually with one nonce, as given entity value exist + // If there no nonce entry under a given key, we need to initialize it manually with one nonce, as given entity value exist // and first vec value specific operation was already performed vec_value_nonces.insert(id, T::Nonce::one()); } @@ -1175,27 +1174,26 @@ impl Module { entity_id: EntityId, in_class_schema_property_id: u16, index_in_property_vec: u32, - nonce: T::Nonce + nonce: T::Nonce, ) -> dispatch::Result { Self::ensure_known_entity_id(entity_id)?; let entity = Self::entity_by_id(entity_id); // Ensure property value vector nonces equality to avoid possible data races, // when performing vector specific operations - Self::ensure_nonce_equality( - in_class_schema_property_id, - &entity.vec_value_nonces, - nonce - )?; + Self::ensure_nonce_equality(in_class_schema_property_id, &entity.vec_value_nonces, nonce)?; if let Some(current_prop_value) = entity.values.get(&in_class_schema_property_id) { - Self::ensure_index_in_property_vector_is_valid(current_prop_value, index_in_property_vec)?; + Self::ensure_index_in_property_vector_is_valid( + current_prop_value, + index_in_property_vec, + )?; } else { // Throw an error if a property was not found on entity // by an in-class index of a property. - return Err(ERROR_UNKNOWN_ENTITY_PROP_ID) + return Err(ERROR_UNKNOWN_ENTITY_PROP_ID); } - + // Remove property value vector >::mutate(entity_id, |entity| { if let Some(current_prop_value) = entity.values.get_mut(&in_class_schema_property_id) { @@ -1215,7 +1213,7 @@ impl Module { in_class_schema_property_id: u16, index_in_property_vec: u32, property_value: PropertyValue, - nonce: T::Nonce + nonce: T::Nonce, ) -> dispatch::Result { Self::ensure_known_entity_id(entity_id)?; @@ -1223,11 +1221,7 @@ impl Module { // Ensure property value vector nonces equality to avoid possible data races, // when performing vector specific operations - Self::ensure_nonce_equality( - in_class_schema_property_id, - &entity.vec_value_nonces, - nonce - )?; + Self::ensure_nonce_equality(in_class_schema_property_id, &entity.vec_value_nonces, nonce)?; // Get class-level information about this property if let Some(class_prop) = class.properties.get(in_class_schema_property_id as usize) { // Try to find a current property value in the entity @@ -1460,7 +1454,7 @@ impl Module { fn ensure_nonce_equality( in_class_schema_property_id: u16, vec_value_nonces: &BTreeMap, - nonce: T::Nonce + nonce: T::Nonce, ) -> dispatch::Result { if let Some(vec_value_nonce) = vec_value_nonces.get(&in_class_schema_property_id) { ensure!( @@ -1679,8 +1673,10 @@ impl Module { } } - pub fn ensure_index_in_property_vector_is_valid(value: &PropertyValue, index_in_property_vec: u32) -> dispatch::Result { - + pub fn ensure_index_in_property_vector_is_valid( + value: &PropertyValue, + index_in_property_vec: u32, + ) -> dispatch::Result { fn is_valid_index(vec: &[T], index_in_property_vec: u32) -> bool { (index_in_property_vec as usize) < vec.len() } @@ -1698,7 +1694,10 @@ impl Module { _ => return Err(ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR), }; - ensure!(is_valid_index, ERROR_ENTITY_PROP_VALUE_VECTOR_INDEX_IS_OUT_OF_RANGE); + ensure!( + is_valid_index, + ERROR_ENTITY_PROP_VALUE_VECTOR_INDEX_IS_OUT_OF_RANGE + ); Ok(()) } diff --git a/runtime-modules/content-directory/src/mock.rs b/runtime-modules/content-directory/src/mock.rs index e9b3e33b90..58b150521e 100644 --- a/runtime-modules/content-directory/src/mock.rs +++ b/runtime-modules/content-directory/src/mock.rs @@ -29,6 +29,9 @@ pub const ZERO_NONCE: u64 = 0; pub const FIRST_NONCE: u64 = 1; pub const SECOND_NONCE: u64 = 2; +pub const VALID_PROPERTY_VEC_INDEX: u32 = 0; +pub const INVALID_PROPERTY_VEC_INDEX: u32 = 5; + pub const PROP_ID_BOOL: u16 = 0; pub const PROP_ID_REFERENCE_VEC: u16 = 1; pub const PROP_ID_U32: u16 = 1; diff --git a/runtime-modules/content-directory/src/tests.rs b/runtime-modules/content-directory/src/tests.rs index 378f95276b..d5499f8c25 100644 --- a/runtime-modules/content-directory/src/tests.rs +++ b/runtime-modules/content-directory/src/tests.rs @@ -1282,12 +1282,12 @@ fn complete_remove_at_entity_property_vector() -> EntityId { assert_ok!(TestModule::complete_remove_at_entity_property_vector( entity_id, PROP_ID_U32_VEC, - 1, + VALID_PROPERTY_VEC_INDEX, ZERO_NONCE )); // Update entity property values to compare with runtime storage entity value under given schema id - prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![123, 44])); + prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![234, 44])); // Check property values runtime storage related to a entity right after // removing at given index of entity property vector value @@ -1303,7 +1303,7 @@ fn complete_remove_at_entity_property_vector_successfully() { assert_ok!(TestModule::complete_remove_at_entity_property_vector( entity_id, PROP_ID_U32_VEC, - 1, + VALID_PROPERTY_VEC_INDEX, FIRST_NONCE )); }) @@ -1315,8 +1315,8 @@ fn cannot_complete_remove_at_entity_property_vector_when_entity_not_found() { assert_entity_not_found(TestModule::complete_remove_at_entity_property_vector( UNKNOWN_ENTITY_ID, PROP_ID_U32_VEC, - 1, - ZERO_NONCE + VALID_PROPERTY_VEC_INDEX, + ZERO_NONCE, )); }) } @@ -1326,18 +1326,44 @@ fn cannot_complete_remove_at_entity_property_vector_when_unknown_entity_prop_id( with_test_externalities(|| { let entity_id = create_entity_with_schema_support(); assert_err!( - TestModule::complete_remove_at_entity_property_vector(entity_id, UNKNOWN_PROP_ID, 1, ZERO_NONCE), + TestModule::complete_remove_at_entity_property_vector( + entity_id, + UNKNOWN_PROP_ID, + VALID_PROPERTY_VEC_INDEX, + ZERO_NONCE + ), ERROR_UNKNOWN_ENTITY_PROP_ID ); }) } +#[test] +fn cannot_complete_remove_at_entity_property_vector_when_entity_prop_vector_index_out_of_range() { + with_test_externalities(|| { + let entity_id = create_entity_with_schema_support(); + assert_err!( + TestModule::complete_remove_at_entity_property_vector( + entity_id, + PROP_ID_U32, + INVALID_PROPERTY_VEC_INDEX, + ZERO_NONCE + ), + ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR + ); + }) +} + #[test] fn cannot_complete_remove_at_entity_property_vector_when_entity_prop_id_is_not_a_vector() { with_test_externalities(|| { let entity_id = create_entity_with_schema_support(); assert_err!( - TestModule::complete_remove_at_entity_property_vector(entity_id, PROP_ID_U32, 1, ZERO_NONCE), + TestModule::complete_remove_at_entity_property_vector( + entity_id, + PROP_ID_U32, + VALID_PROPERTY_VEC_INDEX, + ZERO_NONCE + ), ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR ); }) @@ -1348,7 +1374,12 @@ fn cannot_complete_remove_at_entity_property_vector_when_already_updated() { with_test_externalities(|| { let entity_id = complete_remove_at_entity_property_vector(); assert_err!( - TestModule::complete_remove_at_entity_property_vector(entity_id, PROP_ID_U32_VEC, 1, SECOND_NONCE), + TestModule::complete_remove_at_entity_property_vector( + entity_id, + PROP_ID_U32_VEC, + VALID_PROPERTY_VEC_INDEX, + SECOND_NONCE + ), ERROR_PROP_VALUE_VEC_NONCES_DOES_NOT_MATCH ); }) @@ -1373,7 +1404,7 @@ fn complete_insert_at_entity_property_vector_successfully() { assert_ok!(TestModule::complete_insert_at_entity_property_vector( entity_id, PROP_ID_U32_VEC, - 1, + VALID_PROPERTY_VEC_INDEX, PropertyValue::Uint32(33), ZERO_NONCE )); @@ -1382,7 +1413,7 @@ fn complete_insert_at_entity_property_vector_successfully() { assert_ok!(TestModule::complete_insert_at_entity_property_vector( entity_id, PROP_ID_U32_VEC, - 1, + VALID_PROPERTY_VEC_INDEX, PropertyValue::Uint32(55), FIRST_NONCE )); @@ -1390,7 +1421,7 @@ fn complete_insert_at_entity_property_vector_successfully() { // Update entity property values to compare with runtime storage entity value under given schema id prop_values.insert( PROP_ID_U32_VEC, - PropertyValue::Uint32Vec(vec![123, 55, 33, 234, 44]), + PropertyValue::Uint32Vec(vec![55, 33, 123, 234, 44]), ); // Check property values runtime storage related to a entity right after @@ -1405,9 +1436,9 @@ fn cannot_complete_insert_at_entity_property_vector_when_entity_not_found() { assert_entity_not_found(TestModule::complete_insert_at_entity_property_vector( UNKNOWN_ENTITY_ID, PROP_ID_U32_VEC, - 1, + VALID_PROPERTY_VEC_INDEX, PropertyValue::Uint32(33), - ZERO_NONCE + ZERO_NONCE, )); }) } @@ -1420,7 +1451,7 @@ fn cannot_complete_insert_at_entity_property_vector_when_unknown_entity_prop_id( TestModule::complete_insert_at_entity_property_vector( entity_id, UNKNOWN_PROP_ID, - 1, + VALID_PROPERTY_VEC_INDEX, PropertyValue::Uint32(33), ZERO_NONCE ), @@ -1437,7 +1468,7 @@ fn cannot_complete_insert_at_entity_property_vector_when_entity_prop_id_is_not_a TestModule::complete_insert_at_entity_property_vector( entity_id, PROP_ID_U32, - 1, + VALID_PROPERTY_VEC_INDEX, PropertyValue::Uint32(17), ZERO_NONCE ), @@ -1447,14 +1478,32 @@ fn cannot_complete_insert_at_entity_property_vector_when_entity_prop_id_is_not_a } #[test] -fn cannot_complete_insert_at_entity_property_type_does_not_match_internal_entity_vector_type() { +fn cannot_complete_insert_at_entity_when_entity_prop_value_vector_index_out_of_range() { + with_test_externalities(|| { + let entity_id = create_entity_with_schema_support(); + assert_err!( + TestModule::complete_insert_at_entity_property_vector( + entity_id, + PROP_ID_U32_VEC, + INVALID_PROPERTY_VEC_INDEX, + PropertyValue::Uint32(33), + ZERO_NONCE + ), + ERROR_ENTITY_PROP_VALUE_VECTOR_INDEX_IS_OUT_OF_RANGE + ); + }) +} + +#[test] +fn cannot_complete_insert_at_entity_when_property_type_does_not_match_internal_entity_vector_type() +{ with_test_externalities(|| { let entity_id = create_entity_with_schema_support(); assert_err!( TestModule::complete_insert_at_entity_property_vector( entity_id, PROP_ID_U32_VEC, - 1, + VALID_PROPERTY_VEC_INDEX, PropertyValue::Uint16(33), ZERO_NONCE ), @@ -1482,7 +1531,7 @@ fn cannot_complete_insert_at_entity_property_vector_when_entity_prop_value_vecto TestModule::complete_insert_at_entity_property_vector( entity_id, PROP_ID_U32_VEC, - 1, + VALID_PROPERTY_VEC_INDEX, PropertyValue::Uint32(33), ZERO_NONCE ), @@ -1499,7 +1548,7 @@ fn cannot_complete_insert_at_entity_property_vector_when_nonce_does_not_match() TestModule::complete_insert_at_entity_property_vector( entity_id, PROP_ID_U32_VEC, - 1, + VALID_PROPERTY_VEC_INDEX, PropertyValue::Uint32(33), SECOND_NONCE ), @@ -1538,7 +1587,7 @@ fn cannot_complete_insert_at_entity_property_vector_when_unknown_internal_entity TestModule::complete_insert_at_entity_property_vector( entity_id, PROP_ID_REFERENCE_VEC, - 0, + VALID_PROPERTY_VEC_INDEX, PropertyValue::Reference(UNKNOWN_ENTITY_ID), ZERO_NONCE ), From 3de80c01996b6570b3e1f845d6cd5debdd08552f Mon Sep 17 00:00:00 2001 From: iorveth Date: Tue, 21 Apr 2020 17:23:03 +0300 Subject: [PATCH 15/26] Fix nonce mechanism design issue --- .../content-directory/src/example.rs | 20 +- runtime-modules/content-directory/src/lib.rs | 355 ++++++++++-------- runtime-modules/content-directory/src/mock.rs | 11 +- .../content-directory/src/operations.rs | 36 +- .../content-directory/src/tests.rs | 34 +- 5 files changed, 249 insertions(+), 207 deletions(-) diff --git a/runtime-modules/content-directory/src/example.rs b/runtime-modules/content-directory/src/example.rs index 99d9c65d1e..f7c680835c 100644 --- a/runtime-modules/content-directory/src/example.rs +++ b/runtime-modules/content-directory/src/example.rs @@ -421,10 +421,10 @@ fn create_podcast_class_schema() { // 15 p.next_text_value(b"crypto,blockchain,governance,staking,bitcoin,ethereum".to_vec()); // 16 - p.next_value(PropertyValue::TextVec(vec![ - b"Technology".to_vec(), - b"Software How-To".to_vec(), - ])); + p.next_value(PropertyValue::TextVec( + vec![b"Technology".to_vec(), b"Software How-To".to_vec()], + ::Nonce::default(), + )); // 17 p.next_text_value( b"https://ssl-static.libsyn.com/p/assets/2/d/2/5/2d25eb5fa72739f7/iTunes_Cover.png" @@ -518,20 +518,20 @@ fn create_podcast_class_schema() { }) } -struct PropHelper { +struct PropHelper { prop_idx: u16, - property_values: BTreeMap, + property_values: BTreeMap>, } -impl PropHelper { - fn new() -> PropHelper { +impl PropHelper { + fn new() -> PropHelper { PropHelper { prop_idx: 0, property_values: BTreeMap::new(), } } - fn next_value(&mut self, value: PropertyValue) { + fn next_value(&mut self, value: PropertyValue) { self.property_values.insert(self.prop_idx, value); self.prop_idx += 1; } @@ -540,7 +540,7 @@ impl PropHelper { self.next_value(PropertyValue::Text(text)) } - fn get_property_values(self) -> BTreeMap { + fn get_property_values(self) -> BTreeMap> { self.property_values } } diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index 55eb795dbd..39ebe8969b 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -24,12 +24,13 @@ mod permissions; mod tests; pub use constraint::*; +use core::fmt::Debug; pub use credentials::*; pub use errors::*; pub use operations::*; pub use permissions::*; -pub trait Trait: system::Trait { +pub trait Trait: system::Trait + Debug { /// Type that represents an actor or group of actors in the system. type Credential: Parameter + Member @@ -55,7 +56,8 @@ pub trait Trait: system::Trait { + MaybeSerializeDeserialize + Eq + PartialEq - + Ord; + + Ord + + From; /// Security/configuration constraints @@ -237,12 +239,8 @@ pub struct Entity { /// Values for properties on class that are used by some schema used by this entity! /// Length is no more than Class.properties. - pub values: BTreeMap, - - /// Map, representing relation between entity vec_values index and nonce, where vec_value was updated - /// Used to avoid race update conditions - pub vec_value_nonces: BTreeMap, - // pub deleted: bool, + pub values: BTreeMap>, // Map, representing relation between entity vec_values index and nonce, where vec_value was updated + // pub deleted: bool } impl Default for Entity { @@ -251,7 +249,6 @@ impl Default for Entity { class_id: ClassId::default(), supported_schemas: BTreeSet::new(), values: BTreeMap::new(), - vec_value_nonces: BTreeMap::new(), } } } @@ -260,13 +257,12 @@ impl Entity { fn new( class_id: ClassId, supported_schemas: BTreeSet, - values: BTreeMap, + values: BTreeMap>, ) -> Self { Self { class_id, supported_schemas, values, - ..Entity::default() } } } @@ -353,7 +349,7 @@ impl Default for PropertyType { #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] #[derive(Encode, Decode, Clone, PartialEq, Eq, Debug)] -pub enum PropertyValue { +pub enum PropertyValue { // Single value: Bool(bool), Uint16(u16), @@ -365,49 +361,109 @@ pub enum PropertyValue { Text(Vec), Reference(EntityId), - // Vector of values: - BoolVec(Vec), - Uint16Vec(Vec), - Uint32Vec(Vec), - Uint64Vec(Vec), - Int16Vec(Vec), - Int32Vec(Vec), - Int64Vec(Vec), - TextVec(Vec>), - ReferenceVec(Vec), + // Vector of values, nonce used to avoid race update conditions: + BoolVec(Vec, T::Nonce), + Uint16Vec(Vec, T::Nonce), + Uint32Vec(Vec, T::Nonce), + Uint64Vec(Vec, T::Nonce), + Int16Vec(Vec, T::Nonce), + Int32Vec(Vec, T::Nonce), + Int64Vec(Vec, T::Nonce), + TextVec(Vec>, T::Nonce), + ReferenceVec(Vec, T::Nonce), // External(ExternalPropertyType), // ExternalVec(Vec), } -impl PropertyValue { +impl PropertyValue { + fn update(&mut self, new_value: PropertyValue) { + if let Some(new_nonce) = self.try_increment_nonce() { + *self = new_value; + self.try_set_nonce(new_nonce) + } else { + *self = new_value; + } + } + + fn try_increment_nonce(&mut self) -> Option { + // Increment nonce if property value is vec + match self { + PropertyValue::BoolVec(_, nonce) + | PropertyValue::Uint16Vec(_, nonce) + | PropertyValue::Uint32Vec(_, nonce) + | PropertyValue::Uint64Vec(_, nonce) + | PropertyValue::Int16Vec(_, nonce) + | PropertyValue::Int32Vec(_, nonce) + | PropertyValue::Int64Vec(_, nonce) + | PropertyValue::TextVec(_, nonce) + | PropertyValue::ReferenceVec(_, nonce) => { + *nonce += T::Nonce::one(); + Some(*nonce) + } + _ => None, + } + } + + fn try_set_nonce(&mut self, new_nonce: T::Nonce) { + // Set new nonce if property value is vec + match self { + PropertyValue::BoolVec(_, nonce) + | PropertyValue::Uint16Vec(_, nonce) + | PropertyValue::Uint32Vec(_, nonce) + | PropertyValue::Uint64Vec(_, nonce) + | PropertyValue::Int16Vec(_, nonce) + | PropertyValue::Int32Vec(_, nonce) + | PropertyValue::Int64Vec(_, nonce) + | PropertyValue::TextVec(_, nonce) + | PropertyValue::ReferenceVec(_, nonce) => *nonce = new_nonce, + _ => (), + } + } + + fn get_nonce(&self) -> Option { + match self { + PropertyValue::BoolVec(_, nonce) + | PropertyValue::Uint16Vec(_, nonce) + | PropertyValue::Uint32Vec(_, nonce) + | PropertyValue::Uint64Vec(_, nonce) + | PropertyValue::Int16Vec(_, nonce) + | PropertyValue::Int32Vec(_, nonce) + | PropertyValue::Int64Vec(_, nonce) + | PropertyValue::TextVec(_, nonce) + | PropertyValue::ReferenceVec(_, nonce) => Some(*nonce), + _ => None, + } + } + fn is_vec(&self) -> bool { match self { - PropertyValue::BoolVec(_) - | PropertyValue::Uint16Vec(_) - | PropertyValue::Uint32Vec(_) - | PropertyValue::Uint64Vec(_) - | PropertyValue::Int16Vec(_) - | PropertyValue::Int32Vec(_) - | PropertyValue::Int64Vec(_) - | PropertyValue::TextVec(_) - | PropertyValue::ReferenceVec(_) => true, + PropertyValue::BoolVec(_, _) + | PropertyValue::Uint16Vec(_, _) + | PropertyValue::Uint32Vec(_, _) + | PropertyValue::Uint64Vec(_, _) + | PropertyValue::Int16Vec(_, _) + | PropertyValue::Int32Vec(_, _) + | PropertyValue::Int64Vec(_, _) + | PropertyValue::TextVec(_, _) + | PropertyValue::ReferenceVec(_, _) => true, _ => false, } } fn vec_clear(&mut self) { match self { - PropertyValue::BoolVec(vec) => *vec = vec![], - PropertyValue::Uint16Vec(vec) => *vec = vec![], - PropertyValue::Uint32Vec(vec) => *vec = vec![], - PropertyValue::Uint64Vec(vec) => *vec = vec![], - PropertyValue::Int16Vec(vec) => *vec = vec![], - PropertyValue::Int32Vec(vec) => *vec = vec![], - PropertyValue::Int64Vec(vec) => *vec = vec![], - PropertyValue::TextVec(vec) => *vec = vec![], - PropertyValue::ReferenceVec(vec) => *vec = vec![], + PropertyValue::BoolVec(vec, _) => *vec = vec![], + PropertyValue::Uint16Vec(vec, _) => *vec = vec![], + PropertyValue::Uint32Vec(vec, _) => *vec = vec![], + PropertyValue::Uint64Vec(vec, _) => *vec = vec![], + PropertyValue::Int16Vec(vec, _) => *vec = vec![], + PropertyValue::Int32Vec(vec, _) => *vec = vec![], + PropertyValue::Int64Vec(vec, _) => *vec = vec![], + PropertyValue::TextVec(vec, _) => *vec = vec![], + PropertyValue::ReferenceVec(vec, _) => *vec = vec![], _ => (), } + self.try_increment_nonce(); } fn vec_remove_at(&mut self, index_in_property_vec: u32) { @@ -418,17 +474,18 @@ impl PropertyValue { } match self { - PropertyValue::BoolVec(vec) => remove_at_checked(vec, index_in_property_vec), - PropertyValue::Uint16Vec(vec) => remove_at_checked(vec, index_in_property_vec), - PropertyValue::Uint32Vec(vec) => remove_at_checked(vec, index_in_property_vec), - PropertyValue::Uint64Vec(vec) => remove_at_checked(vec, index_in_property_vec), - PropertyValue::Int16Vec(vec) => remove_at_checked(vec, index_in_property_vec), - PropertyValue::Int32Vec(vec) => remove_at_checked(vec, index_in_property_vec), - PropertyValue::Int64Vec(vec) => remove_at_checked(vec, index_in_property_vec), - PropertyValue::TextVec(vec) => remove_at_checked(vec, index_in_property_vec), - PropertyValue::ReferenceVec(vec) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::BoolVec(vec, _) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Uint16Vec(vec, _) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Uint32Vec(vec, _) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Uint64Vec(vec, _) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Int16Vec(vec, _) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Int32Vec(vec, _) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::Int64Vec(vec, _) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::TextVec(vec, _) => remove_at_checked(vec, index_in_property_vec), + PropertyValue::ReferenceVec(vec, _) => remove_at_checked(vec, index_in_property_vec), _ => (), } + self.try_increment_nonce(); } fn vec_insert_at(&mut self, index_in_property_vec: u32, property_value: Self) { @@ -438,32 +495,34 @@ impl PropertyValue { } } + self.try_increment_nonce(); + match (self, property_value) { - (PropertyValue::BoolVec(vec), PropertyValue::Bool(value)) => { + (PropertyValue::BoolVec(vec, _), PropertyValue::Bool(value)) => { insert_at(vec, index_in_property_vec, value) } - (PropertyValue::Uint16Vec(vec), PropertyValue::Uint16(value)) => { + (PropertyValue::Uint16Vec(vec, _), PropertyValue::Uint16(value)) => { insert_at(vec, index_in_property_vec, value) } - (PropertyValue::Uint32Vec(vec), PropertyValue::Uint32(value)) => { + (PropertyValue::Uint32Vec(vec, _), PropertyValue::Uint32(value)) => { insert_at(vec, index_in_property_vec, value) } - (PropertyValue::Uint64Vec(vec), PropertyValue::Uint64(value)) => { + (PropertyValue::Uint64Vec(vec, _), PropertyValue::Uint64(value)) => { insert_at(vec, index_in_property_vec, value) } - (PropertyValue::Int16Vec(vec), PropertyValue::Int16(value)) => { + (PropertyValue::Int16Vec(vec, _), PropertyValue::Int16(value)) => { insert_at(vec, index_in_property_vec, value) } - (PropertyValue::Int32Vec(vec), PropertyValue::Int32(value)) => { + (PropertyValue::Int32Vec(vec, _), PropertyValue::Int32(value)) => { insert_at(vec, index_in_property_vec, value) } - (PropertyValue::Int64Vec(vec), PropertyValue::Int64(value)) => { + (PropertyValue::Int64Vec(vec, _), PropertyValue::Int64(value)) => { insert_at(vec, index_in_property_vec, value) } - (PropertyValue::TextVec(vec), PropertyValue::Text(ref value)) => { + (PropertyValue::TextVec(vec, _), PropertyValue::Text(ref value)) => { insert_at(vec, index_in_property_vec, value.to_owned()) } - (PropertyValue::ReferenceVec(vec), PropertyValue::Reference(value)) => { + (PropertyValue::ReferenceVec(vec, _), PropertyValue::Reference(value)) => { insert_at(vec, index_in_property_vec, value) } _ => (), @@ -471,7 +530,7 @@ impl PropertyValue { } } -impl Default for PropertyValue { +impl Default for PropertyValue { fn default() -> Self { PropertyValue::Bool(false) } @@ -768,7 +827,7 @@ decl_module! { as_entity_maintainer: bool, entity_id: EntityId, schema_id: u16, // Do not type alias u16!! - u16, - property_values: BTreeMap + property_values: BTreeMap> ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; Self::do_add_schema_support_to_entity(&raw_origin, with_credential, as_entity_maintainer, entity_id, schema_id, property_values) @@ -779,7 +838,7 @@ decl_module! { with_credential: Option, as_entity_maintainer: bool, entity_id: EntityId, - property_values: BTreeMap + property_values: BTreeMap> ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; Self::do_update_entity_property_values(&raw_origin, with_credential, as_entity_maintainer, entity_id, property_values) @@ -816,7 +875,7 @@ decl_module! { entity_id: EntityId, in_class_schema_property_id: u16, index_in_property_vec: u32, - property_value: PropertyValue, + property_value: PropertyValue, nonce: T::Nonce ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; @@ -832,7 +891,7 @@ decl_module! { ) } - pub fn transaction(origin, operations: Vec>) -> dispatch::Result { + pub fn transaction(origin, operations: Vec>) -> dispatch::Result { // This map holds the EntityId of the entity created as a result of executing a CreateEntity Operation // keyed by the indexed of the operation, in the operations vector. let mut entity_created_in_operation: BTreeMap = BTreeMap::new(); @@ -937,7 +996,7 @@ impl Module { with_credential: Option, as_entity_maintainer: bool, entity_id: EntityId, - property_values: BTreeMap, + property_values: BTreeMap>, ) -> dispatch::Result { let class_id = Self::get_class_id_by_entity_id(entity_id)?; @@ -1032,7 +1091,7 @@ impl Module { entity_id: EntityId, in_class_schema_property_id: u16, index_in_property_vec: u32, - property_value: PropertyValue, + property_value: PropertyValue, nonce: T::Nonce, ) -> dispatch::Result { let class_id = Self::get_class_id_by_entity_id(entity_id)?; @@ -1076,7 +1135,7 @@ impl Module { pub fn complete_entity_property_values_update( entity_id: EntityId, - new_property_values: BTreeMap, + new_property_values: BTreeMap>, ) -> dispatch::Result { Self::ensure_known_entity_id(entity_id)?; @@ -1085,7 +1144,6 @@ impl Module { // Get current property values of an entity as a mutable vector, // so we can update them if new values provided present in new_property_values. let mut updated_values = entity.values; - let mut vec_value_nonces = entity.vec_value_nonces; let mut updated = false; // Iterate over a vector of new values and update corresponding properties // of this entity if new values are valid. @@ -1101,11 +1159,7 @@ impl Module { Self::ensure_property_value_to_update_is_valid(&new_value, class_prop)?; // Update a current prop value in a mutable vector, if a new value is valid. - *current_prop_value = new_value; - if current_prop_value.is_vec() { - // Update last block of vec prop value if update performed - Self::refresh_vec_value_nonces(id, &mut vec_value_nonces); - } + current_prop_value.update(new_value); updated = true; } } else { @@ -1119,23 +1173,12 @@ impl Module { if updated { >::mutate(entity_id, |entity| { entity.values = updated_values; - entity.vec_value_nonces = vec_value_nonces; }); } Ok(()) } - fn refresh_vec_value_nonces(id: u16, vec_value_nonces: &mut BTreeMap) { - if let Some(nonce) = vec_value_nonces.get_mut(&id) { - *nonce += T::Nonce::one(); - } else { - // If there no nonce entry under a given key, we need to initialize it manually with one nonce, as given entity value exist - // and first vec value specific operation was already performed - vec_value_nonces.insert(id, T::Nonce::one()); - } - } - fn complete_entity_property_vector_cleaning( entity_id: EntityId, in_class_schema_property_id: u16, @@ -1161,10 +1204,6 @@ impl Module { { current_property_value_vec.vec_clear(); } - Self::refresh_vec_value_nonces( - in_class_schema_property_id, - &mut entity.vec_value_nonces, - ); }); Ok(()) @@ -1179,11 +1218,10 @@ impl Module { Self::ensure_known_entity_id(entity_id)?; let entity = Self::entity_by_id(entity_id); - // Ensure property value vector nonces equality to avoid possible data races, - // when performing vector specific operations - Self::ensure_nonce_equality(in_class_schema_property_id, &entity.vec_value_nonces, nonce)?; - if let Some(current_prop_value) = entity.values.get(&in_class_schema_property_id) { + // Ensure property value vector nonces equality to avoid possible data races, + // when performing vector specific operations + Self::ensure_nonce_equality(current_prop_value, nonce)?; Self::ensure_index_in_property_vector_is_valid( current_prop_value, index_in_property_vec, @@ -1199,10 +1237,6 @@ impl Module { if let Some(current_prop_value) = entity.values.get_mut(&in_class_schema_property_id) { current_prop_value.vec_remove_at(index_in_property_vec) } - Self::refresh_vec_value_nonces( - in_class_schema_property_id, - &mut entity.vec_value_nonces, - ); }); Ok(()) @@ -1212,21 +1246,21 @@ impl Module { entity_id: EntityId, in_class_schema_property_id: u16, index_in_property_vec: u32, - property_value: PropertyValue, + property_value: PropertyValue, nonce: T::Nonce, ) -> dispatch::Result { Self::ensure_known_entity_id(entity_id)?; let (entity, class) = Self::get_entity_and_class(entity_id); - // Ensure property value vector nonces equality to avoid possible data races, - // when performing vector specific operations - Self::ensure_nonce_equality(in_class_schema_property_id, &entity.vec_value_nonces, nonce)?; // Get class-level information about this property if let Some(class_prop) = class.properties.get(in_class_schema_property_id as usize) { // Try to find a current property value in the entity // by matching its id to the id of a property with an updated value. if let Some(entity_prop_value) = entity.values.get(&in_class_schema_property_id) { + // Ensure property value vector nonces equality to avoid possible data races, + // when performing vector specific operations + Self::ensure_nonce_equality(entity_prop_value, nonce)?; // Validate a new property value against the type of this property // and check any additional constraints like the length of a vector // if it's a vector property or the length of a text if it's a text property. @@ -1250,10 +1284,6 @@ impl Module { if let Some(current_prop_value) = entity.values.get_mut(&in_class_schema_property_id) { current_prop_value.vec_insert_at(index_in_property_vec, property_value) } - Self::refresh_vec_value_nonces( - in_class_schema_property_id, - &mut entity.vec_value_nonces, - ); }); Ok(()) @@ -1265,7 +1295,7 @@ impl Module { as_entity_maintainer: bool, entity_id: EntityId, schema_id: u16, - property_values: BTreeMap, + property_values: BTreeMap>, ) -> dispatch::Result { // class id of the entity being updated let class_id = Self::get_class_id_by_entity_id(entity_id)?; @@ -1418,7 +1448,7 @@ impl Module { // the target entity and class exists and constraint allows it. fn ensure_internal_property_values_permitted( source_class_id: ClassId, - property_values: &BTreeMap, + property_values: &BTreeMap>, ) -> dispatch::Result { for (in_class_index, property_value) in property_values.iter() { if let PropertyValue::Reference(ref target_entity_id) = property_value { @@ -1452,18 +1482,12 @@ impl Module { } fn ensure_nonce_equality( - in_class_schema_property_id: u16, - vec_value_nonces: &BTreeMap, - nonce: T::Nonce, + vec_value: &PropertyValue, + new_nonce: T::Nonce, ) -> dispatch::Result { - if let Some(vec_value_nonce) = vec_value_nonces.get(&in_class_schema_property_id) { - ensure!( - *vec_value_nonce == nonce, - ERROR_PROP_VALUE_VEC_NONCES_DOES_NOT_MATCH - ); - } else { + if let Some(nonce) = vec_value.get_nonce() { ensure!( - nonce == T::Nonce::zero(), + nonce == new_nonce, ERROR_PROP_VALUE_VEC_NONCES_DOES_NOT_MATCH ); } @@ -1547,7 +1571,7 @@ impl Module { pub fn add_entity_schema_support( entity_id: EntityId, schema_id: u16, - property_values: BTreeMap, + property_values: BTreeMap>, ) -> dispatch::Result { Self::ensure_known_entity_id(entity_id)?; @@ -1657,7 +1681,10 @@ impl Module { Ok(()) } - pub fn ensure_valid_internal_prop(value: &PropertyValue, prop: &Property) -> dispatch::Result { + pub fn ensure_valid_internal_prop( + value: &PropertyValue, + prop: &Property, + ) -> dispatch::Result { match (value, prop.prop_type) { (PV::Reference(entity_id), PT::Reference(class_id)) => { Self::ensure_known_class_id(class_id)?; @@ -1674,7 +1701,7 @@ impl Module { } pub fn ensure_index_in_property_vector_is_valid( - value: &PropertyValue, + value: &PropertyValue, index_in_property_vec: u32, ) -> dispatch::Result { fn is_valid_index(vec: &[T], index_in_property_vec: u32) -> bool { @@ -1682,15 +1709,15 @@ impl Module { } let is_valid_index = match value { - PropertyValue::BoolVec(vec) => is_valid_index(vec, index_in_property_vec), - PropertyValue::Uint16Vec(vec) => is_valid_index(vec, index_in_property_vec), - PropertyValue::Uint32Vec(vec) => is_valid_index(vec, index_in_property_vec), - PropertyValue::Uint64Vec(vec) => is_valid_index(vec, index_in_property_vec), - PropertyValue::Int16Vec(vec) => is_valid_index(vec, index_in_property_vec), - PropertyValue::Int32Vec(vec) => is_valid_index(vec, index_in_property_vec), - PropertyValue::Int64Vec(vec) => is_valid_index(vec, index_in_property_vec), - PropertyValue::TextVec(vec) => is_valid_index(vec, index_in_property_vec), - PropertyValue::ReferenceVec(vec) => is_valid_index(vec, index_in_property_vec), + PropertyValue::BoolVec(vec, _) => is_valid_index(vec, index_in_property_vec), + PropertyValue::Uint16Vec(vec, _) => is_valid_index(vec, index_in_property_vec), + PropertyValue::Uint32Vec(vec, _) => is_valid_index(vec, index_in_property_vec), + PropertyValue::Uint64Vec(vec, _) => is_valid_index(vec, index_in_property_vec), + PropertyValue::Int16Vec(vec, _) => is_valid_index(vec, index_in_property_vec), + PropertyValue::Int32Vec(vec, _) => is_valid_index(vec, index_in_property_vec), + PropertyValue::Int64Vec(vec, _) => is_valid_index(vec, index_in_property_vec), + PropertyValue::TextVec(vec, _) => is_valid_index(vec, index_in_property_vec), + PropertyValue::ReferenceVec(vec, _) => is_valid_index(vec, index_in_property_vec), _ => return Err(ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR), }; @@ -1701,7 +1728,7 @@ impl Module { Ok(()) } - pub fn is_unknown_internal_entity_id(id: PropertyValue) -> bool { + pub fn is_unknown_internal_entity_id(id: PropertyValue) -> bool { if let PropertyValue::Reference(entity_id) = id { !>::exists(entity_id) } else { @@ -1716,7 +1743,7 @@ impl Module { } pub fn ensure_property_value_to_update_is_valid( - value: &PropertyValue, + value: &PropertyValue, prop: &Property, ) -> dispatch::Result { Self::ensure_prop_value_matches_its_type(value, prop)?; @@ -1727,8 +1754,8 @@ impl Module { } pub fn ensure_prop_value_can_be_inserted_at_prop_vec( - value: &PropertyValue, - entity_prop_value: &PropertyValue, + value: &PropertyValue, + entity_prop_value: &PropertyValue, index_in_property_vec: u32, prop: &Property, ) -> dispatch::Result { @@ -1740,28 +1767,28 @@ impl Module { let is_valid_len = match (value, entity_prop_value, prop.prop_type) { // Single values - (PV::Bool(_), PV::BoolVec(vec), PT::BoolVec(max_len)) => { + (PV::Bool(_), PV::BoolVec(vec, _), PT::BoolVec(max_len)) => { validate_prop_vec_len_after_value_insert(vec, max_len) } - (PV::Uint16(_), PV::Uint16Vec(vec), PT::Uint16Vec(max_len)) => { + (PV::Uint16(_), PV::Uint16Vec(vec, _), PT::Uint16Vec(max_len)) => { validate_prop_vec_len_after_value_insert(vec, max_len) } - (PV::Uint32(_), PV::Uint32Vec(vec), PT::Uint32Vec(max_len)) => { + (PV::Uint32(_), PV::Uint32Vec(vec, _), PT::Uint32Vec(max_len)) => { validate_prop_vec_len_after_value_insert(vec, max_len) } - (PV::Uint64(_), PV::Uint64Vec(vec), PT::Uint64Vec(max_len)) => { + (PV::Uint64(_), PV::Uint64Vec(vec, _), PT::Uint64Vec(max_len)) => { validate_prop_vec_len_after_value_insert(vec, max_len) } - (PV::Int16(_), PV::Int16Vec(vec), PT::Int16Vec(max_len)) => { + (PV::Int16(_), PV::Int16Vec(vec, _), PT::Int16Vec(max_len)) => { validate_prop_vec_len_after_value_insert(vec, max_len) } - (PV::Int32(_), PV::Int32Vec(vec), PT::Int32Vec(max_len)) => { + (PV::Int32(_), PV::Int32Vec(vec, _), PT::Int32Vec(max_len)) => { validate_prop_vec_len_after_value_insert(vec, max_len) } - (PV::Int64(_), PV::Int64Vec(vec), PT::Int64Vec(max_len)) => { + (PV::Int64(_), PV::Int64Vec(vec, _), PT::Int64Vec(max_len)) => { validate_prop_vec_len_after_value_insert(vec, max_len) } - (PV::Text(text_item), PV::TextVec(vec), PT::TextVec(vec_max_len, text_max_len)) => { + (PV::Text(text_item), PV::TextVec(vec, _), PT::TextVec(vec_max_len, text_max_len)) => { if validate_prop_vec_len_after_value_insert(vec, vec_max_len) { Self::validate_max_len_of_text(text_item, text_max_len)?; true @@ -1771,7 +1798,7 @@ impl Module { } ( PV::Reference(entity_id), - PV::ReferenceVec(vec), + PV::ReferenceVec(vec, _), PT::ReferenceVec(vec_max_len, class_id), ) => { Self::ensure_known_class_id(class_id)?; @@ -1795,7 +1822,7 @@ impl Module { } pub fn validate_max_len_if_text_prop( - value: &PropertyValue, + value: &PropertyValue, prop: &Property, ) -> dispatch::Result { match (value, &prop.prop_type) { @@ -1813,7 +1840,7 @@ impl Module { } pub fn validate_max_len_if_vec_prop( - value: &PropertyValue, + value: &PropertyValue, prop: &Property, ) -> dispatch::Result { fn validate_vec_len(vec: &[T], max_len: u16) -> bool { @@ -1821,15 +1848,15 @@ impl Module { } let is_valid_len = match (value, prop.prop_type) { - (PV::BoolVec(vec), PT::BoolVec(max_len)) => validate_vec_len(vec, max_len), - (PV::Uint16Vec(vec), PT::Uint16Vec(max_len)) => validate_vec_len(vec, max_len), - (PV::Uint32Vec(vec), PT::Uint32Vec(max_len)) => validate_vec_len(vec, max_len), - (PV::Uint64Vec(vec), PT::Uint64Vec(max_len)) => validate_vec_len(vec, max_len), - (PV::Int16Vec(vec), PT::Int16Vec(max_len)) => validate_vec_len(vec, max_len), - (PV::Int32Vec(vec), PT::Int32Vec(max_len)) => validate_vec_len(vec, max_len), - (PV::Int64Vec(vec), PT::Int64Vec(max_len)) => validate_vec_len(vec, max_len), - - (PV::TextVec(vec), PT::TextVec(vec_max_len, text_max_len)) => { + (PV::BoolVec(vec, _), PT::BoolVec(max_len)) => validate_vec_len(vec, max_len), + (PV::Uint16Vec(vec, _), PT::Uint16Vec(max_len)) => validate_vec_len(vec, max_len), + (PV::Uint32Vec(vec, _), PT::Uint32Vec(max_len)) => validate_vec_len(vec, max_len), + (PV::Uint64Vec(vec, _), PT::Uint64Vec(max_len)) => validate_vec_len(vec, max_len), + (PV::Int16Vec(vec, _), PT::Int16Vec(max_len)) => validate_vec_len(vec, max_len), + (PV::Int32Vec(vec, _), PT::Int32Vec(max_len)) => validate_vec_len(vec, max_len), + (PV::Int64Vec(vec, _), PT::Int64Vec(max_len)) => validate_vec_len(vec, max_len), + + (PV::TextVec(vec, _), PT::TextVec(vec_max_len, text_max_len)) => { if validate_vec_len(vec, vec_max_len) { for text_item in vec.iter() { Self::validate_max_len_of_text(text_item, text_max_len)?; @@ -1840,7 +1867,7 @@ impl Module { } } - (PV::ReferenceVec(vec), PT::ReferenceVec(vec_max_len, class_id)) => { + (PV::ReferenceVec(vec, _), PT::ReferenceVec(vec_max_len, class_id)) => { Self::ensure_known_class_id(class_id)?; if validate_vec_len(vec, vec_max_len) { for entity_id in vec.iter() { @@ -1868,7 +1895,7 @@ impl Module { } pub fn ensure_prop_value_matches_its_type( - value: &PropertyValue, + value: &PropertyValue, prop: &Property, ) -> dispatch::Result { ensure!( @@ -1878,7 +1905,7 @@ impl Module { Ok(()) } - pub fn does_prop_value_match_type(value: &PropertyValue, prop: &Property) -> bool { + pub fn does_prop_value_match_type(value: &PropertyValue, prop: &Property) -> bool { // A non required property can be updated to None: if !prop.required && *value == PV::Bool(false) { return true; @@ -1895,15 +1922,15 @@ impl Module { (PV::Text(_), PT::Text(_)) | (PV::Reference(_), PT::Reference(_)) | // Vectors: - (PV::BoolVec(_), PT::BoolVec(_)) | - (PV::Uint16Vec(_), PT::Uint16Vec(_)) | - (PV::Uint32Vec(_), PT::Uint32Vec(_)) | - (PV::Uint64Vec(_), PT::Uint64Vec(_)) | - (PV::Int16Vec(_), PT::Int16Vec(_)) | - (PV::Int32Vec(_), PT::Int32Vec(_)) | - (PV::Int64Vec(_), PT::Int64Vec(_)) | - (PV::TextVec(_), PT::TextVec(_, _)) | - (PV::ReferenceVec(_), PT::ReferenceVec(_, _)) => true, + (PV::BoolVec(_, _), PT::BoolVec(_)) | + (PV::Uint16Vec(_, _), PT::Uint16Vec(_)) | + (PV::Uint32Vec(_, _), PT::Uint32Vec(_)) | + (PV::Uint64Vec(_, _), PT::Uint64Vec(_)) | + (PV::Int16Vec(_, _), PT::Int16Vec(_)) | + (PV::Int32Vec(_, _), PT::Int32Vec(_)) | + (PV::Int64Vec(_, _), PT::Int64Vec(_)) | + (PV::TextVec(_, _), PT::TextVec(_, _)) | + (PV::ReferenceVec(_, _), PT::ReferenceVec(_, _)) => true, // (PV::External(_), PT::External(_)) => true, // (PV::ExternalVec(_), PT::ExternalVec(_, _)) => true, _ => false, diff --git a/runtime-modules/content-directory/src/mock.rs b/runtime-modules/content-directory/src/mock.rs index 58b150521e..9e5cf4e38a 100644 --- a/runtime-modules/content-directory/src/mock.rs +++ b/runtime-modules/content-directory/src/mock.rs @@ -294,7 +294,7 @@ pub fn simple_test_schema() -> Vec { }] } -pub fn simple_test_entity_property_values() -> BTreeMap { +pub fn simple_test_entity_property_values() -> BTreeMap> { let mut property_values = BTreeMap::new(); property_values.insert(0, PropertyValue::Int64(1337)); property_values @@ -351,7 +351,7 @@ pub fn create_entity_with_schema_support() -> EntityId { property_values.insert(PROP_ID_BOOL, PropertyValue::Bool(true)); property_values.insert( PROP_ID_U32_VEC, - PropertyValue::Uint32Vec(vec![123, 234, 44]), + PropertyValue::Uint32Vec(vec![123, 234, 44], ::Nonce::default()), ); assert_ok!(TestModule::add_entity_schema_support( entity_id, @@ -453,13 +453,16 @@ pub fn good_prop_ids() -> Vec { vec![0, 1] } -pub fn bool_prop_value() -> BTreeMap { +pub fn bool_prop_value() -> BTreeMap> { let mut property_values = BTreeMap::new(); property_values.insert(0, PropertyValue::Bool(true)); property_values } -pub fn prop_value(index: u16, value: PropertyValue) -> BTreeMap { +pub fn prop_value( + index: u16, + value: PropertyValue, +) -> BTreeMap> { let mut property_values = BTreeMap::new(); property_values.insert(index, value); property_values diff --git a/runtime-modules/content-directory/src/operations.rs b/runtime-modules/content-directory/src/operations.rs index 06d4c8ca85..9c8facefb1 100644 --- a/runtime-modules/content-directory/src/operations.rs +++ b/runtime-modules/content-directory/src/operations.rs @@ -1,12 +1,12 @@ -use crate::{ClassId, EntityId, PropertyValue}; +use crate::{ClassId, EntityId, PropertyValue, Trait}; use codec::{Decode, Encode}; use rstd::collections::btree_map::BTreeMap; use rstd::prelude::*; #[derive(Encode, Decode, Eq, PartialEq, Clone, Debug)] -pub enum ParametrizedPropertyValue { +pub enum ParametrizedPropertyValue { /// Same fields as normal PropertyValue - PropertyValue(PropertyValue), + PropertyValue(PropertyValue), /// This is the index of an operation creating an entity in the transaction/batch operations InternalEntityJustAdded(u32), // should really be usize but it doesn't have Encode/Decode support @@ -22,12 +22,12 @@ pub enum ParameterizedEntity { } #[derive(Encode, Decode, Eq, PartialEq, Clone, Debug)] -pub struct ParametrizedClassPropertyValue { +pub struct ParametrizedClassPropertyValue { /// Index is into properties vector of class. pub in_class_index: u16, /// Value of property with index `in_class_index` in a given class. - pub value: ParametrizedPropertyValue, + pub value: ParametrizedPropertyValue, } #[derive(Encode, Decode, Eq, PartialEq, Clone, Debug)] @@ -36,30 +36,30 @@ pub struct CreateEntityOperation { } #[derive(Encode, Decode, Eq, PartialEq, Clone, Debug)] -pub struct UpdatePropertyValuesOperation { +pub struct UpdatePropertyValuesOperation { pub entity_id: ParameterizedEntity, - pub new_parametrized_property_values: Vec, + pub new_parametrized_property_values: Vec>, } #[derive(Encode, Decode, Eq, PartialEq, Clone, Debug)] -pub struct AddSchemaSupportToEntityOperation { +pub struct AddSchemaSupportToEntityOperation { pub entity_id: ParameterizedEntity, pub schema_id: u16, - pub parametrized_property_values: Vec, + pub parametrized_property_values: Vec>, } #[derive(Encode, Decode, Eq, PartialEq, Clone, Debug)] -pub enum OperationType { +pub enum OperationType { CreateEntity(CreateEntityOperation), - UpdatePropertyValues(UpdatePropertyValuesOperation), - AddSchemaSupportToEntity(AddSchemaSupportToEntityOperation), + UpdatePropertyValues(UpdatePropertyValuesOperation), + AddSchemaSupportToEntity(AddSchemaSupportToEntityOperation), } #[derive(Encode, Decode, Eq, PartialEq, Clone, Debug)] -pub struct Operation { +pub struct Operation { pub with_credential: Option, pub as_entity_maintainer: bool, - pub operation_type: OperationType, + pub operation_type: OperationType, } pub fn parametrized_entity_to_entity_id( @@ -80,10 +80,10 @@ pub fn parametrized_entity_to_entity_id( } } -pub fn parametrized_property_values_to_property_values( +pub fn parametrized_property_values_to_property_values( created_entities: &BTreeMap, - parametrized_property_values: Vec, -) -> Result, &'static str> { + parametrized_property_values: Vec>, +) -> Result>, &'static str> { let mut class_property_values = BTreeMap::new(); for parametrized_class_property_value in parametrized_property_values.into_iter() { @@ -121,7 +121,7 @@ pub fn parametrized_property_values_to_property_values( } } - PropertyValue::ReferenceVec(entities) + PropertyValue::ReferenceVec(entities, T::Nonce::default()) } }; diff --git a/runtime-modules/content-directory/src/tests.rs b/runtime-modules/content-directory/src/tests.rs index d5499f8c25..53d8adf27c 100644 --- a/runtime-modules/content-directory/src/tests.rs +++ b/runtime-modules/content-directory/src/tests.rs @@ -669,7 +669,10 @@ fn batch_transaction_vector_of_entities() { BTreeSet::from_iter(vec![SCHEMA_ID_0].into_iter()), prop_value( 0, - PropertyValue::ReferenceVec(vec![entity_id + 1, entity_id + 2,]) + PropertyValue::ReferenceVec( + vec![entity_id + 1, entity_id + 2,], + ::Nonce::default() + ) ) ) ); @@ -1124,7 +1127,7 @@ fn update_entity_props_successfully() { prop_values.insert(PROP_ID_REFERENCE, PropertyValue::Bool(false)); prop_values.insert( PROP_ID_U32_VEC, - PropertyValue::Uint32Vec(vec![123, 234, 44]), + PropertyValue::Uint32Vec(vec![123, 234, 44], ::Nonce::default()), ); assert_eq!(TestModule::entity_by_id(entity_id).values, prop_values); prop_values = prop_value(PROP_ID_BOOL, PropertyValue::Bool(false)); @@ -1132,7 +1135,7 @@ fn update_entity_props_successfully() { prop_values.insert(PROP_ID_REFERENCE, PropertyValue::Reference(entity_id)); prop_values.insert( PROP_ID_U32_VEC, - PropertyValue::Uint32Vec(vec![123, 234, 44, 88, 43]), + PropertyValue::Uint32Vec(vec![123, 234, 44, 88, 43], ::Nonce::one()), ); assert_ok!(TestModule::complete_entity_property_values_update( entity_id, @@ -1208,7 +1211,7 @@ fn complete_entity_property_vector_cleaning_successfully() { prop_values.insert(PROP_ID_U32, PropertyValue::Bool(false)); prop_values.insert( PROP_ID_U32_VEC, - PropertyValue::Uint32Vec(vec![123, 234, 44]), + PropertyValue::Uint32Vec(vec![123, 234, 44], ::Nonce::default()), ); prop_values.insert(PROP_ID_REFERENCE, PropertyValue::Bool(false)); @@ -1222,7 +1225,10 @@ fn complete_entity_property_vector_cleaning_successfully() { )); // Update entity property values to compare with runtime storage entity value under given schema id - prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![])); + prop_values.insert( + PROP_ID_U32_VEC, + PropertyValue::Uint32Vec(vec![], ::Nonce::one()), + ); // Check property values runtime storage related to a entity right after // cleaning entity property vector under given schema id @@ -1271,7 +1277,7 @@ fn complete_remove_at_entity_property_vector() -> EntityId { prop_values.insert(PROP_ID_U32, PropertyValue::Bool(false)); prop_values.insert( PROP_ID_U32_VEC, - PropertyValue::Uint32Vec(vec![123, 234, 44]), + PropertyValue::Uint32Vec(vec![123, 234, 44], ::Nonce::default()), ); prop_values.insert(PROP_ID_REFERENCE, PropertyValue::Bool(false)); @@ -1287,7 +1293,10 @@ fn complete_remove_at_entity_property_vector() -> EntityId { )); // Update entity property values to compare with runtime storage entity value under given schema id - prop_values.insert(PROP_ID_U32_VEC, PropertyValue::Uint32Vec(vec![234, 44])); + prop_values.insert( + PROP_ID_U32_VEC, + PropertyValue::Uint32Vec(vec![234, 44], ::Nonce::one()), + ); // Check property values runtime storage related to a entity right after // removing at given index of entity property vector value @@ -1393,7 +1402,7 @@ fn complete_insert_at_entity_property_vector_successfully() { prop_values.insert(PROP_ID_U32, PropertyValue::Bool(false)); prop_values.insert( PROP_ID_U32_VEC, - PropertyValue::Uint32Vec(vec![123, 234, 44]), + PropertyValue::Uint32Vec(vec![123, 234, 44], ::Nonce::default()), ); prop_values.insert(PROP_ID_REFERENCE, PropertyValue::Bool(false)); @@ -1421,7 +1430,7 @@ fn complete_insert_at_entity_property_vector_successfully() { // Update entity property values to compare with runtime storage entity value under given schema id prop_values.insert( PROP_ID_U32_VEC, - PropertyValue::Uint32Vec(vec![55, 33, 123, 234, 44]), + PropertyValue::Uint32Vec(vec![55, 33, 123, 234, 44], 2_u32.into()), ); // Check property values runtime storage related to a entity right after @@ -1520,7 +1529,10 @@ fn cannot_complete_insert_at_entity_property_vector_when_entity_prop_value_vecto property_values.insert(PROP_ID_BOOL, PropertyValue::Bool(true)); property_values.insert( PROP_ID_U32_VEC, - PropertyValue::Uint32Vec(vec![5; PROP_ID_U32_VEC_MAX_LEN as usize]), + PropertyValue::Uint32Vec( + vec![5; PROP_ID_U32_VEC_MAX_LEN as usize], + ::Nonce::default(), + ), ); assert_ok!(TestModule::add_entity_schema_support( entity_id, @@ -1576,7 +1588,7 @@ fn cannot_complete_insert_at_entity_property_vector_when_unknown_internal_entity property_values.insert(PROP_ID_BOOL, PropertyValue::Bool(true)); property_values.insert( PROP_ID_REFERENCE_VEC, - PropertyValue::ReferenceVec(vec![entity_id_2]), + PropertyValue::ReferenceVec(vec![entity_id_2], ::Nonce::default()), ); assert_ok!(TestModule::add_entity_schema_support( entity_id, From 766c8ceff869a464a8b70d2690eca37ab936df1a Mon Sep 17 00:00:00 2001 From: iorveth Date: Tue, 21 Apr 2020 21:58:42 +0300 Subject: [PATCH 16/26] Raw index u16 types, except of schema id type, replaced with respective type aliases --- .../content-directory/src/example.rs | 4 +- runtime-modules/content-directory/src/lib.rs | 127 +++++++++--------- runtime-modules/content-directory/src/mock.rs | 36 ++--- .../content-directory/src/permissions.rs | 3 - .../content-working-group/src/lib.rs | 2 +- 5 files changed, 88 insertions(+), 84 deletions(-) diff --git a/runtime-modules/content-directory/src/example.rs b/runtime-modules/content-directory/src/example.rs index f7c680835c..407b37350b 100644 --- a/runtime-modules/content-directory/src/example.rs +++ b/runtime-modules/content-directory/src/example.rs @@ -519,8 +519,8 @@ fn create_podcast_class_schema() { } struct PropHelper { - prop_idx: u16, - property_values: BTreeMap>, + prop_idx: PropertyId, + property_values: BTreeMap>, } impl PropHelper { diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index 39ebe8969b..c70614fc0d 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -153,6 +153,9 @@ impl InputValidationLengthConstraint { pub type ClassId = u64; pub type EntityId = u64; +pub type PropertyId = u16; +pub type VecMaxLength = u16; +pub type TextMaxLength = u16; #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] #[derive(Encode, Decode, Eq, PartialEq, Clone, Debug)] @@ -223,8 +226,12 @@ impl Class { } } -pub type ClassPermissionsType = - ClassPermissions::Credential, u16, ::BlockNumber>; +pub type ClassPermissionsType = ClassPermissions< + ClassId, + ::Credential, + PropertyId, + ::BlockNumber, +>; #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] #[derive(Encode, Decode, Clone, PartialEq, Eq, Debug)] @@ -239,8 +246,8 @@ pub struct Entity { /// Values for properties on class that are used by some schema used by this entity! /// Length is no more than Class.properties. - pub values: BTreeMap>, // Map, representing relation between entity vec_values index and nonce, where vec_value was updated - // pub deleted: bool + pub values: BTreeMap>, + // pub deleted: bool } impl Default for Entity { @@ -257,7 +264,7 @@ impl Entity { fn new( class_id: ClassId, supported_schemas: BTreeSet, - values: BTreeMap>, + values: BTreeMap>, ) -> Self { Self { class_id, @@ -272,7 +279,7 @@ impl Entity { #[derive(Encode, Decode, Clone, PartialEq, Eq, Debug)] pub struct Schema { /// Indices into properties vector for the corresponding class. - pub properties: Vec, + pub properties: Vec, pub is_active: bool, } @@ -287,7 +294,7 @@ impl Default for Schema { } impl Schema { - fn new(properties: Vec) -> Self { + fn new(properties: Vec) -> Self { Self { properties, // Default schema status @@ -316,27 +323,27 @@ pub enum PropertyType { Int16, Int32, Int64, - Text(u16), + Text(TextMaxLength), Reference(ClassId), // Vector of values. - // The first u16 value is the max length of this vector. - BoolVec(u16), - Uint16Vec(u16), - Uint32Vec(u16), - Uint64Vec(u16), - Int16Vec(u16), - Int32Vec(u16), - Int64Vec(u16), - - /// The first u16 value is the max length of this vector. - /// The second u16 value is the max length of every text item in this vector. - TextVec(u16, u16), - - /// The first u16 value is the max length of this vector. + // The first value is the max length of this vector. + BoolVec(VecMaxLength), + Uint16Vec(VecMaxLength), + Uint32Vec(VecMaxLength), + Uint64Vec(VecMaxLength), + Int16Vec(VecMaxLength), + Int32Vec(VecMaxLength), + Int64Vec(VecMaxLength), + + /// The first value is the max length of this vector. + /// The second value is the max length of every text item in this vector. + TextVec(VecMaxLength, TextMaxLength), + + /// The first value is the max length of this vector. /// The second ClassId value tells that an every element of this vector /// should be of a specific ClassId. - ReferenceVec(u16, ClassId), + ReferenceVec(VecMaxLength, ClassId), // External(ExternalProperty), // ExternalVec(u16, ExternalProperty), } @@ -361,7 +368,7 @@ pub enum PropertyValue { Text(Vec), Reference(EntityId), - // Vector of values, nonce used to avoid race update conditions: + // Vector of values, second value - nonce used to avoid race update conditions: BoolVec(Vec, T::Nonce), Uint16Vec(Vec, T::Nonce), Uint32Vec(Vec, T::Nonce), @@ -466,8 +473,8 @@ impl PropertyValue { self.try_increment_nonce(); } - fn vec_remove_at(&mut self, index_in_property_vec: u32) { - fn remove_at_checked(vec: &mut Vec, index_in_property_vec: u32) { + fn vec_remove_at(&mut self, index_in_property_vec: VecMaxLength) { + fn remove_at_checked(vec: &mut Vec, index_in_property_vec: VecMaxLength) { if (index_in_property_vec as usize) < vec.len() { vec.remove(index_in_property_vec as usize); } @@ -488,8 +495,8 @@ impl PropertyValue { self.try_increment_nonce(); } - fn vec_insert_at(&mut self, index_in_property_vec: u32, property_value: Self) { - fn insert_at(vec: &mut Vec, index_in_property_vec: u32, value: T) { + fn vec_insert_at(&mut self, index_in_property_vec: VecMaxLength, property_value: Self) { + fn insert_at(vec: &mut Vec, index_in_property_vec: VecMaxLength, value: T) { if (index_in_property_vec as usize) < vec.len() { vec.insert(index_in_property_vec as usize, value); } @@ -686,7 +693,7 @@ decl_module! { origin, with_credential: Option, class_id: ClassId, - constraint: ReferenceConstraint + constraint: ReferenceConstraint ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; @@ -761,7 +768,7 @@ decl_module! { origin, with_credential: Option, class_id: ClassId, - existing_properties: Vec, + existing_properties: Vec, new_properties: Vec ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; @@ -827,7 +834,7 @@ decl_module! { as_entity_maintainer: bool, entity_id: EntityId, schema_id: u16, // Do not type alias u16!! - u16, - property_values: BTreeMap> + property_values: BTreeMap> ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; Self::do_add_schema_support_to_entity(&raw_origin, with_credential, as_entity_maintainer, entity_id, schema_id, property_values) @@ -838,7 +845,7 @@ decl_module! { with_credential: Option, as_entity_maintainer: bool, entity_id: EntityId, - property_values: BTreeMap> + property_values: BTreeMap> ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; Self::do_update_entity_property_values(&raw_origin, with_credential, as_entity_maintainer, entity_id, property_values) @@ -849,7 +856,7 @@ decl_module! { with_credential: Option, as_entity_maintainer: bool, entity_id: EntityId, - in_class_schema_property_id: u16 + in_class_schema_property_id: PropertyId ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; Self::do_clear_entity_property_vector(&raw_origin, with_credential, as_entity_maintainer, entity_id, in_class_schema_property_id) @@ -860,8 +867,8 @@ decl_module! { with_credential: Option, as_entity_maintainer: bool, entity_id: EntityId, - in_class_schema_property_id: u16, - index_in_property_vec: u32, + in_class_schema_property_id: PropertyId, + index_in_property_vec: VecMaxLength, nonce: T::Nonce ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; @@ -873,8 +880,8 @@ decl_module! { with_credential: Option, as_entity_maintainer: bool, entity_id: EntityId, - in_class_schema_property_id: u16, - index_in_property_vec: u32, + in_class_schema_property_id: PropertyId, + index_in_property_vec: VecMaxLength, property_value: PropertyValue, nonce: T::Nonce ) -> dispatch::Result { @@ -996,7 +1003,7 @@ impl Module { with_credential: Option, as_entity_maintainer: bool, entity_id: EntityId, - property_values: BTreeMap>, + property_values: BTreeMap>, ) -> dispatch::Result { let class_id = Self::get_class_id_by_entity_id(entity_id)?; @@ -1025,7 +1032,7 @@ impl Module { with_credential: Option, as_entity_maintainer: bool, entity_id: EntityId, - in_class_schema_property_id: u16, + in_class_schema_property_id: PropertyId, ) -> dispatch::Result { let class_id = Self::get_class_id_by_entity_id(entity_id)?; @@ -1055,8 +1062,8 @@ impl Module { with_credential: Option, as_entity_maintainer: bool, entity_id: EntityId, - in_class_schema_property_id: u16, - index_in_property_vec: u32, + in_class_schema_property_id: PropertyId, + index_in_property_vec: VecMaxLength, nonce: T::Nonce, ) -> dispatch::Result { let class_id = Self::get_class_id_by_entity_id(entity_id)?; @@ -1089,8 +1096,8 @@ impl Module { with_credential: Option, as_entity_maintainer: bool, entity_id: EntityId, - in_class_schema_property_id: u16, - index_in_property_vec: u32, + in_class_schema_property_id: PropertyId, + index_in_property_vec: VecMaxLength, property_value: PropertyValue, nonce: T::Nonce, ) -> dispatch::Result { @@ -1135,7 +1142,7 @@ impl Module { pub fn complete_entity_property_values_update( entity_id: EntityId, - new_property_values: BTreeMap>, + new_property_values: BTreeMap>, ) -> dispatch::Result { Self::ensure_known_entity_id(entity_id)?; @@ -1181,7 +1188,7 @@ impl Module { fn complete_entity_property_vector_cleaning( entity_id: EntityId, - in_class_schema_property_id: u16, + in_class_schema_property_id: PropertyId, ) -> dispatch::Result { Self::ensure_known_entity_id(entity_id)?; let entity = Self::entity_by_id(entity_id); @@ -1211,8 +1218,8 @@ impl Module { fn complete_remove_at_entity_property_vector( entity_id: EntityId, - in_class_schema_property_id: u16, - index_in_property_vec: u32, + in_class_schema_property_id: PropertyId, + index_in_property_vec: VecMaxLength, nonce: T::Nonce, ) -> dispatch::Result { Self::ensure_known_entity_id(entity_id)?; @@ -1244,8 +1251,8 @@ impl Module { fn complete_insert_at_entity_property_vector( entity_id: EntityId, - in_class_schema_property_id: u16, - index_in_property_vec: u32, + in_class_schema_property_id: PropertyId, + index_in_property_vec: VecMaxLength, property_value: PropertyValue, nonce: T::Nonce, ) -> dispatch::Result { @@ -1295,7 +1302,7 @@ impl Module { as_entity_maintainer: bool, entity_id: EntityId, schema_id: u16, - property_values: BTreeMap>, + property_values: BTreeMap>, ) -> dispatch::Result { // class id of the entity being updated let class_id = Self::get_class_id_by_entity_id(entity_id)?; @@ -1448,7 +1455,7 @@ impl Module { // the target entity and class exists and constraint allows it. fn ensure_internal_property_values_permitted( source_class_id: ClassId, - property_values: &BTreeMap>, + property_values: &BTreeMap>, ) -> dispatch::Result { for (in_class_index, property_value) in property_values.iter() { if let PropertyValue::Reference(ref target_entity_id) = property_value { @@ -1497,7 +1504,7 @@ impl Module { /// Returns an index of a newly added class schema on success. pub fn append_class_schema( class_id: ClassId, - existing_properties: Vec, + existing_properties: Vec, new_properties: Vec, ) -> Result { Self::ensure_known_class_id(class_id)?; @@ -1531,7 +1538,7 @@ impl Module { // Check that existing props are valid indices of class properties vector: let has_unknown_props = existing_properties .iter() - .any(|&prop_id| prop_id >= class.properties.len() as u16); + .any(|&prop_id| prop_id >= class.properties.len() as PropertyId); ensure!( !has_unknown_props, ERROR_CLASS_SCHEMA_REFERS_UNKNOWN_PROP_INDEX @@ -1571,7 +1578,7 @@ impl Module { pub fn add_entity_schema_support( entity_id: EntityId, schema_id: u16, - property_values: BTreeMap>, + property_values: BTreeMap>, ) -> dispatch::Result { Self::ensure_known_entity_id(entity_id)?; @@ -1702,9 +1709,9 @@ impl Module { pub fn ensure_index_in_property_vector_is_valid( value: &PropertyValue, - index_in_property_vec: u32, + index_in_property_vec: VecMaxLength, ) -> dispatch::Result { - fn is_valid_index(vec: &[T], index_in_property_vec: u32) -> bool { + fn is_valid_index(vec: &[T], index_in_property_vec: VecMaxLength) -> bool { (index_in_property_vec as usize) < vec.len() } @@ -1756,12 +1763,12 @@ impl Module { pub fn ensure_prop_value_can_be_inserted_at_prop_vec( value: &PropertyValue, entity_prop_value: &PropertyValue, - index_in_property_vec: u32, + index_in_property_vec: VecMaxLength, prop: &Property, ) -> dispatch::Result { Self::ensure_index_in_property_vector_is_valid(entity_prop_value, index_in_property_vec)?; - fn validate_prop_vec_len_after_value_insert(vec: &[T], max_len: u16) -> bool { + fn validate_prop_vec_len_after_value_insert(vec: &[T], max_len: VecMaxLength) -> bool { vec.len() < max_len as usize } @@ -1831,7 +1838,7 @@ impl Module { } } - pub fn validate_max_len_of_text(text: &[u8], max_len: u16) -> dispatch::Result { + pub fn validate_max_len_of_text(text: &[u8], max_len: TextMaxLength) -> dispatch::Result { if text.len() <= max_len as usize { Ok(()) } else { @@ -1843,7 +1850,7 @@ impl Module { value: &PropertyValue, prop: &Property, ) -> dispatch::Result { - fn validate_vec_len(vec: &[T], max_len: u16) -> bool { + fn validate_vec_len(vec: &[T], max_len: VecMaxLength) -> bool { vec.len() <= max_len as usize } diff --git a/runtime-modules/content-directory/src/mock.rs b/runtime-modules/content-directory/src/mock.rs index 9e5cf4e38a..0057ec38db 100644 --- a/runtime-modules/content-directory/src/mock.rs +++ b/runtime-modules/content-directory/src/mock.rs @@ -19,25 +19,25 @@ pub const MEMBER_TWO_WITH_CREDENTIAL_ONE: u64 = 103; pub const UNKNOWN_CLASS_ID: ClassId = 111; pub const UNKNOWN_ENTITY_ID: EntityId = 222; -pub const UNKNOWN_PROP_ID: u16 = 333; +pub const UNKNOWN_PROP_ID: PropertyId = 333; pub const UNKNOWN_SCHEMA_ID: u16 = 444; pub const SCHEMA_ID_0: u16 = 0; pub const SCHEMA_ID_1: u16 = 1; -pub const ZERO_NONCE: u64 = 0; -pub const FIRST_NONCE: u64 = 1; -pub const SECOND_NONCE: u64 = 2; +pub const ZERO_NONCE: ::Nonce = 0; +pub const FIRST_NONCE: ::Nonce = 1; +pub const SECOND_NONCE: ::Nonce = 2; -pub const VALID_PROPERTY_VEC_INDEX: u32 = 0; -pub const INVALID_PROPERTY_VEC_INDEX: u32 = 5; +pub const VALID_PROPERTY_VEC_INDEX: VecMaxLength = 0; +pub const INVALID_PROPERTY_VEC_INDEX: VecMaxLength = 5; -pub const PROP_ID_BOOL: u16 = 0; -pub const PROP_ID_REFERENCE_VEC: u16 = 1; -pub const PROP_ID_U32: u16 = 1; -pub const PROP_ID_REFERENCE: u16 = 2; -pub const PROP_ID_U32_VEC: u16 = 3; -pub const PROP_ID_U32_VEC_MAX_LEN: u16 = 20; +pub const PROP_ID_BOOL: PropertyId = 0; +pub const PROP_ID_REFERENCE_VEC: PropertyId = 1; +pub const PROP_ID_U32: PropertyId = 1; +pub const PROP_ID_REFERENCE: PropertyId = 2; +pub const PROP_ID_U32_VEC: PropertyId = 3; +pub const PROP_ID_U32_VEC_MAX_LEN: PropertyId = 20; pub const PRINCIPAL_GROUP_MEMBERS: [[u64; 2]; 2] = [ [ @@ -272,7 +272,7 @@ pub fn assert_class_props(class_id: ClassId, expected_props: Vec) { assert_eq!(class.properties, expected_props); } -pub fn assert_class_schemas(class_id: ClassId, expected_schema_prop_ids: Vec>) { +pub fn assert_class_schemas(class_id: ClassId, expected_schema_prop_ids: Vec>) { let class = TestModule::class_by_id(class_id); let schemas: Vec<_> = expected_schema_prop_ids .iter() @@ -294,7 +294,7 @@ pub fn simple_test_schema() -> Vec { }] } -pub fn simple_test_entity_property_values() -> BTreeMap> { +pub fn simple_test_entity_property_values() -> BTreeMap> { let mut property_values = BTreeMap::new(); property_values.insert(0, PropertyValue::Int64(1337)); property_values @@ -449,20 +449,20 @@ pub fn good_props() -> Vec { vec![good_prop_bool(), good_prop_u32()] } -pub fn good_prop_ids() -> Vec { +pub fn good_prop_ids() -> Vec { vec![0, 1] } -pub fn bool_prop_value() -> BTreeMap> { +pub fn bool_prop_value() -> BTreeMap> { let mut property_values = BTreeMap::new(); property_values.insert(0, PropertyValue::Bool(true)); property_values } pub fn prop_value( - index: u16, + index: PropertyId, value: PropertyValue, -) -> BTreeMap> { +) -> BTreeMap> { let mut property_values = BTreeMap::new(); property_values.insert(index, value); property_values diff --git a/runtime-modules/content-directory/src/permissions.rs b/runtime-modules/content-directory/src/permissions.rs index 8d2514ec4f..a038bca29e 100644 --- a/runtime-modules/content-directory/src/permissions.rs +++ b/runtime-modules/content-directory/src/permissions.rs @@ -1,9 +1,6 @@ use codec::{Decode, Encode}; use srml_support::dispatch; -#[cfg(feature = "std")] -use serde::{Deserialize, Serialize}; - use crate::constraint::*; use crate::credentials::*; diff --git a/runtime-modules/content-working-group/src/lib.rs b/runtime-modules/content-working-group/src/lib.rs index b0c6aeea93..970181b9b9 100755 --- a/runtime-modules/content-working-group/src/lib.rs +++ b/runtime-modules/content-working-group/src/lib.rs @@ -1191,7 +1191,7 @@ decl_module! { // Increment NextChannelId NextChannelId::::mutate(|id| *id += as One>::one()); - /// CREDENTIAL STUFF /// + // CREDENTIAL STUFF // // Dial out to membership module and inform about new role as channe owner. let registered_role = >::register_role_on_member(owner, &member_in_role).is_ok(); From 279e5da9bd3364294957f2bbb3c1981c25d8b32d Mon Sep 17 00:00:00 2001 From: iorveth Date: Wed, 22 Apr 2020 11:23:22 +0300 Subject: [PATCH 17/26] SchemaId type alias introduced --- .../content-directory/src/example.rs | 6 ++-- runtime-modules/content-directory/src/lib.rs | 33 ++++++++++--------- runtime-modules/content-directory/src/mock.rs | 10 +++--- .../content-directory/src/operations.rs | 8 ++--- 4 files changed, 29 insertions(+), 28 deletions(-) diff --git a/runtime-modules/content-directory/src/example.rs b/runtime-modules/content-directory/src/example.rs index 407b37350b..2497348771 100644 --- a/runtime-modules/content-directory/src/example.rs +++ b/runtime-modules/content-directory/src/example.rs @@ -354,7 +354,7 @@ fn create_podcast_class_schema() { b"A podcast channel".to_vec(), ),); - let channel_schema_id: u16 = 0; + let channel_schema_id: SchemaId = 0; assert_ok!( TestModule::append_class_schema(channel_class_id, vec![], channel_props), @@ -370,7 +370,7 @@ fn create_podcast_class_schema() { b"A podcast episode".to_vec(), ),); - let episode_schema_id: u16 = 0; + let episode_schema_id: SchemaId = 0; assert_ok!( TestModule::append_class_schema(episode_class_id, vec![], episode_props,), @@ -540,7 +540,7 @@ impl PropHelper { self.next_value(PropertyValue::Text(text)) } - fn get_property_values(self) -> BTreeMap> { + fn get_property_values(self) -> BTreeMap> { self.property_values } } diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index c70614fc0d..69d02eac1f 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -154,6 +154,7 @@ impl InputValidationLengthConstraint { pub type ClassId = u64; pub type EntityId = u64; pub type PropertyId = u16; +pub type SchemaId = u16; pub type VecMaxLength = u16; pub type TextMaxLength = u16; @@ -203,12 +204,12 @@ impl Class { } } - fn is_active_schema(&self, schema_index: u16) -> bool { + fn is_active_schema(&self, schema_index: SchemaId) -> bool { // Such indexing is safe, when length bounds were previously checked self.schemas[schema_index as usize].is_active } - fn update_schema_status(&mut self, schema_index: u16, schema_status: bool) { + fn update_schema_status(&mut self, schema_index: SchemaId, schema_status: bool) { // Such indexing is safe, when length bounds were previously checked self.schemas[schema_index as usize].is_active = schema_status; } @@ -242,7 +243,7 @@ pub struct Entity { /// What schemas under which this entity of a class is available, think /// v.2.0 Person schema for John, v3.0 Person schema for John /// Unlikely to be more than roughly 20ish, assuming schemas for a given class eventually stableize, or that very old schema are eventually removed. - pub supported_schemas: BTreeSet, // indices of schema in corresponding class + pub supported_schemas: BTreeSet, // indices of schema in corresponding class /// Values for properties on class that are used by some schema used by this entity! /// Length is no more than Class.properties. @@ -263,7 +264,7 @@ impl Default for Entity { impl Entity { fn new( class_id: ClassId, - supported_schemas: BTreeSet, + supported_schemas: BTreeSet, values: BTreeMap>, ) -> Self { Self { @@ -794,7 +795,7 @@ decl_module! { origin, with_credential: Option, class_id: ClassId, - schema_id: u16, // Do not type alias u16!! - u16, + schema_id: SchemaId, is_active: bool ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; @@ -833,7 +834,7 @@ decl_module! { with_credential: Option, as_entity_maintainer: bool, entity_id: EntityId, - schema_id: u16, // Do not type alias u16!! - u16, + schema_id: SchemaId, property_values: BTreeMap> ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; @@ -1129,7 +1130,7 @@ impl Module { pub fn complete_class_schema_status_update( class_id: ClassId, - schema_id: u16, // Do not type alias u16!! - u16, + schema_id: SchemaId, schema_status: bool, ) -> dispatch::Result { // Check that schema_id is a valid index of class schemas vector: @@ -1301,7 +1302,7 @@ impl Module { with_credential: Option, as_entity_maintainer: bool, entity_id: EntityId, - schema_id: u16, + schema_id: SchemaId, property_values: BTreeMap>, ) -> dispatch::Result { // class id of the entity being updated @@ -1506,7 +1507,7 @@ impl Module { class_id: ClassId, existing_properties: Vec, new_properties: Vec, - ) -> Result { + ) -> Result { Self::ensure_known_class_id(class_id)?; let non_empty_schema = !existing_properties.is_empty() || !new_properties.is_empty(); @@ -1556,13 +1557,13 @@ impl Module { // Use the current length of schemas in this class as an index // for the next schema that will be sent in a result of this function. - let schema_idx = class.schemas.len() as u16; + let schema_idx = class.schemas.len() as SchemaId; let mut schema = Schema::new(existing_properties); let mut updated_class_props = class.properties; new_properties.into_iter().for_each(|prop| { - let prop_id = updated_class_props.len() as u16; + let prop_id = updated_class_props.len() as PropertyId; updated_class_props.push(prop); schema.properties.push(prop_id); }); @@ -1577,7 +1578,7 @@ impl Module { pub fn add_entity_schema_support( entity_id: EntityId, - schema_id: u16, + schema_id: SchemaId, property_values: BTreeMap>, ) -> dispatch::Result { Self::ensure_known_entity_id(entity_id)?; @@ -1666,15 +1667,15 @@ impl Module { Ok(()) } - pub fn ensure_class_schema_id_exists(class: &Class, schema_id: u16) -> dispatch::Result { + pub fn ensure_class_schema_id_exists(class: &Class, schema_id: SchemaId) -> dispatch::Result { ensure!( - schema_id < class.schemas.len() as u16, + schema_id < class.schemas.len() as SchemaId, ERROR_UNKNOWN_CLASS_SCHEMA_ID ); Ok(()) } - pub fn ensure_class_schema_is_active(class: &Class, schema_id: u16) -> dispatch::Result { + pub fn ensure_class_schema_is_active(class: &Class, schema_id: SchemaId) -> dispatch::Result { ensure!( class.is_active_schema(schema_id), ERROR_CLASS_SCHEMA_NOT_ACTIVE @@ -1682,7 +1683,7 @@ impl Module { Ok(()) } - pub fn ensure_schema_id_is_not_added(entity: &Entity, schema_id: u16) -> dispatch::Result { + pub fn ensure_schema_id_is_not_added(entity: &Entity, schema_id: SchemaId) -> dispatch::Result { let schema_not_added = !entity.supported_schemas.contains(&schema_id); ensure!(schema_not_added, ERROR_SCHEMA_ALREADY_ADDED_TO_ENTITY); Ok(()) diff --git a/runtime-modules/content-directory/src/mock.rs b/runtime-modules/content-directory/src/mock.rs index 0057ec38db..77e26d19a6 100644 --- a/runtime-modules/content-directory/src/mock.rs +++ b/runtime-modules/content-directory/src/mock.rs @@ -20,10 +20,10 @@ pub const MEMBER_TWO_WITH_CREDENTIAL_ONE: u64 = 103; pub const UNKNOWN_CLASS_ID: ClassId = 111; pub const UNKNOWN_ENTITY_ID: EntityId = 222; pub const UNKNOWN_PROP_ID: PropertyId = 333; -pub const UNKNOWN_SCHEMA_ID: u16 = 444; +pub const UNKNOWN_SCHEMA_ID: SchemaId = 444; -pub const SCHEMA_ID_0: u16 = 0; -pub const SCHEMA_ID_1: u16 = 1; +pub const SCHEMA_ID_0: SchemaId = 0; +pub const SCHEMA_ID_1: SchemaId = 1; pub const ZERO_NONCE: ::Nonce = 0; pub const FIRST_NONCE: ::Nonce = 1; @@ -361,7 +361,7 @@ pub fn create_entity_with_schema_support() -> EntityId { entity_id } -pub fn create_class_with_schema() -> (ClassId, u16) { +pub fn create_class_with_schema() -> (ClassId, SchemaId) { let class_id = create_simple_class_with_default_permissions(); let schema_id = TestModule::append_class_schema( class_id, @@ -377,7 +377,7 @@ pub fn create_class_with_schema() -> (ClassId, u16) { (class_id, schema_id) } -pub fn create_class_with_schema_and_entity() -> (ClassId, u16, EntityId) { +pub fn create_class_with_schema_and_entity() -> (ClassId, SchemaId, EntityId) { let (class_id, schema_id) = create_class_with_schema(); let entity_id = create_entity_of_class(class_id); (class_id, schema_id, entity_id) diff --git a/runtime-modules/content-directory/src/operations.rs b/runtime-modules/content-directory/src/operations.rs index 9c8facefb1..a1a0cf2178 100644 --- a/runtime-modules/content-directory/src/operations.rs +++ b/runtime-modules/content-directory/src/operations.rs @@ -1,4 +1,4 @@ -use crate::{ClassId, EntityId, PropertyValue, Trait}; +use crate::{ClassId, EntityId, SchemaId, PropertyId, PropertyValue, Trait}; use codec::{Decode, Encode}; use rstd::collections::btree_map::BTreeMap; use rstd::prelude::*; @@ -24,7 +24,7 @@ pub enum ParameterizedEntity { #[derive(Encode, Decode, Eq, PartialEq, Clone, Debug)] pub struct ParametrizedClassPropertyValue { /// Index is into properties vector of class. - pub in_class_index: u16, + pub in_class_index: PropertyId, /// Value of property with index `in_class_index` in a given class. pub value: ParametrizedPropertyValue, @@ -44,7 +44,7 @@ pub struct UpdatePropertyValuesOperation { #[derive(Encode, Decode, Eq, PartialEq, Clone, Debug)] pub struct AddSchemaSupportToEntityOperation { pub entity_id: ParameterizedEntity, - pub schema_id: u16, + pub schema_id: SchemaId, pub parametrized_property_values: Vec>, } @@ -83,7 +83,7 @@ pub fn parametrized_entity_to_entity_id( pub fn parametrized_property_values_to_property_values( created_entities: &BTreeMap, parametrized_property_values: Vec>, -) -> Result>, &'static str> { +) -> Result>, &'static str> { let mut class_property_values = BTreeMap::new(); for parametrized_class_property_value in parametrized_property_values.into_iter() { From a1615330bd368066baa500177bc2f87200f09ec1 Mon Sep 17 00:00:00 2001 From: iorveth Date: Wed, 22 Apr 2020 11:43:27 +0300 Subject: [PATCH 18/26] Bunch of refactoring & optimizations --- runtime-modules/content-directory/src/lib.rs | 2 +- runtime-modules/content-directory/src/mock.rs | 7 +++---- runtime-modules/content-directory/src/operations.rs | 9 +++------ 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index 69d02eac1f..8ec472e15a 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -1595,7 +1595,7 @@ impl Module { Self::ensure_schema_id_is_not_added(&entity, schema_id)?; let class_schema_opt = class.schemas.get(schema_id as usize); - let schema_prop_ids = class_schema_opt.unwrap().properties.clone(); + let schema_prop_ids = &class_schema_opt.unwrap().properties; let current_entity_values = entity.values.clone(); let mut appended_entity_values = entity.values; diff --git a/runtime-modules/content-directory/src/mock.rs b/runtime-modules/content-directory/src/mock.rs index 77e26d19a6..a767ded4ce 100644 --- a/runtime-modules/content-directory/src/mock.rs +++ b/runtime-modules/content-directory/src/mock.rs @@ -260,10 +260,9 @@ pub fn with_test_externalities R>(f: F) -> R { } impl Property { - pub fn required(&self) -> Property { - let mut new_self = self.clone(); - new_self.required = true; - new_self + pub fn required(self) -> Self { + self.required = true; + self } } diff --git a/runtime-modules/content-directory/src/operations.rs b/runtime-modules/content-directory/src/operations.rs index a1a0cf2178..e8e5ebe755 100644 --- a/runtime-modules/content-directory/src/operations.rs +++ b/runtime-modules/content-directory/src/operations.rs @@ -70,8 +70,7 @@ pub fn parametrized_entity_to_entity_id( ParameterizedEntity::ExistingEntity(entity_id) => Ok(entity_id), ParameterizedEntity::InternalEntityJustAdded(op_index_u32) => { let op_index = op_index_u32 as usize; - if created_entities.contains_key(&op_index) { - let entity_id = created_entities.get(&op_index).unwrap(); + if let Some(entity_id) = created_entities.get(&op_index) { Ok(*entity_id) } else { Err("EntityNotCreatedByOperation") @@ -94,8 +93,7 @@ pub fn parametrized_property_values_to_property_values( ) => { // Verify that referenced entity was indeed created created let op_index = entity_created_in_operation_index as usize; - if created_entities.contains_key(&op_index) { - let entity_id = created_entities.get(&op_index).unwrap(); + if let Some(entity_id) = created_entities.get(&op_index) { PropertyValue::Reference(*entity_id) } else { return Err("EntityNotCreatedByOperation"); @@ -111,8 +109,7 @@ pub fn parametrized_property_values_to_property_values( entity_created_in_operation_index, ) => { let op_index = entity_created_in_operation_index as usize; - if created_entities.contains_key(&op_index) { - let entity_id = created_entities.get(&op_index).unwrap(); + if let Some(entity_id) = created_entities.get(&op_index) { entities.push(*entity_id); } else { return Err("EntityNotCreatedByOperation"); From 1d530215a250d96fb189520213936ded5660968c Mon Sep 17 00:00:00 2001 From: iorveth Date: Wed, 22 Apr 2020 11:44:38 +0300 Subject: [PATCH 19/26] Fix mut in required() method --- runtime-modules/content-directory/src/mock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime-modules/content-directory/src/mock.rs b/runtime-modules/content-directory/src/mock.rs index a767ded4ce..ed12611d25 100644 --- a/runtime-modules/content-directory/src/mock.rs +++ b/runtime-modules/content-directory/src/mock.rs @@ -260,7 +260,7 @@ pub fn with_test_externalities R>(f: F) -> R { } impl Property { - pub fn required(self) -> Self { + pub fn required(mut self) -> Self { self.required = true; self } From d53349157d251d17538d31449b9339c29bd8306e Mon Sep 17 00:00:00 2001 From: iorveth Date: Wed, 22 Apr 2020 23:17:02 +0300 Subject: [PATCH 20/26] Entity removal implementation, part 1 --- .../content-directory/src/errors.rs | 2 + runtime-modules/content-directory/src/lib.rs | 51 ++++++++++++++++++- .../content-directory/src/permissions.rs | 23 +++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/runtime-modules/content-directory/src/errors.rs b/runtime-modules/content-directory/src/errors.rs index 4b90625187..36e5f70add 100644 --- a/runtime-modules/content-directory/src/errors.rs +++ b/runtime-modules/content-directory/src/errors.rs @@ -50,3 +50,5 @@ pub const ERROR_PROP_VALUE_TYPE_DOESNT_MATCH_INTERNAL_ENTITY_VECTOR_TYPE: &str = "Propery value type does not match internal entity vector type"; pub const ERROR_INTERNAL_PROP_DOES_NOT_MATCH_ITS_CLASS: &str = "Internal property does not match its class"; +pub const ERROR_ENTITY_REFERENCE_COUNTER_DOES_NOT_EQUAL_TO_ZERO: &str = + "Entity removal can`t be completed, as there are some property values pointing to given entity"; diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index 8ec472e15a..664febd695 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -7,7 +7,7 @@ use rstd::prelude::*; use runtime_primitives::traits::{ MaybeSerialize, MaybeSerializeDeserialize, Member, One, SimpleArithmetic, Zero, }; -use srml_support::{decl_module, decl_storage, dispatch, ensure, traits::Get, Parameter}; +use srml_support::{decl_module, StorageMap, decl_storage, dispatch, ensure, traits::Get, Parameter}; #[cfg(feature = "std")] pub use serde::{Deserialize, Serialize}; @@ -249,6 +249,7 @@ pub struct Entity { /// Length is no more than Class.properties. pub values: BTreeMap>, // pub deleted: bool + pub reference_count: u32 } impl Default for Entity { @@ -257,6 +258,7 @@ impl Default for Entity { class_id: ClassId::default(), supported_schemas: BTreeSet::new(), values: BTreeMap::new(), + reference_count: 0 } } } @@ -271,6 +273,7 @@ impl Entity { class_id, supported_schemas, values, + ..Self::default() } } } @@ -825,10 +828,19 @@ decl_module! { class_id: ClassId ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; - let _entity_id = Self::do_create_entity(&raw_origin, with_credential, class_id)?; + Self::do_create_entity(&raw_origin, with_credential, class_id)?; Ok(()) } + pub fn remove_entity( + origin, + with_credential: Option, + entity_id: EntityId, + ) -> dispatch::Result { + let raw_origin = Self::ensure_root_or_signed(origin)?; + Self::do_remove_entity(&raw_origin, with_credential, entity_id) + } + pub fn add_schema_support_to_entity( origin, with_credential: Option, @@ -985,6 +997,26 @@ impl Module { ) } + fn do_remove_entity( + raw_origin: &system::RawOrigin, + with_credential: Option, + entity_id: EntityId, + ) -> dispatch::Result { + // class id of the entity being removed + let class_id = Self::get_class_id_by_entity_id(entity_id)?; + + Self::if_class_permissions_satisfied( + raw_origin, + with_credential, + None, + ClassPermissions::can_remove_entity, + class_id, + |_class_permissions, _access_level| { + Self::complete_entity_removal(entity_id) + } + ) + } + fn perform_entity_creation(class_id: ClassId) -> EntityId { let entity_id = NextEntityId::get(); @@ -1128,6 +1160,15 @@ impl Module { ) } + fn complete_entity_removal(entity_id: EntityId) -> dispatch::Result { + + // Ensure there is no property values pointing to given entity + Self::ensure_rc_is_zero(entity_id)?; + >::remove(entity_id); + >::remove(entity_id); + Ok(()) + } + pub fn complete_class_schema_status_update( class_id: ClassId, schema_id: SchemaId, @@ -1667,6 +1708,12 @@ impl Module { Ok(()) } + pub fn ensure_rc_is_zero(entity_id: EntityId) -> dispatch::Result { + let entity = Self::entity_by_id(entity_id); + ensure!(entity.reference_count == 0, ERROR_ENTITY_REFERENCE_COUNTER_DOES_NOT_EQUAL_TO_ZERO); + Ok(()) + } + pub fn ensure_class_schema_id_exists(class: &Class, schema_id: SchemaId) -> dispatch::Result { ensure!( schema_id < class.schemas.len() as SchemaId, diff --git a/runtime-modules/content-directory/src/permissions.rs b/runtime-modules/content-directory/src/permissions.rs index a038bca29e..4353a1140b 100644 --- a/runtime-modules/content-directory/src/permissions.rs +++ b/runtime-modules/content-directory/src/permissions.rs @@ -30,6 +30,9 @@ where /// Who can create new entities in the versioned store of this class pub create_entities: CredentialSet, + /// Who can remove entities from the versioned store of this class + pub remove_entities: CredentialSet, + /// The type of constraint on referencing the class from other entities. pub reference_constraint: ReferenceConstraint, @@ -123,6 +126,26 @@ where } } + pub fn can_remove_entity( + class_permissions: &Self, + access_level: &AccessLevel, + ) -> dispatch::Result { + match access_level { + AccessLevel::System => Ok(()), + AccessLevel::Credential(credential) => { + if !class_permissions.entities_can_be_created { + Err("EntitiesCannotBeRemoved") + } else if class_permissions.remove_entities.contains(credential) { + Ok(()) + } else { + Err("NotInRemoveEntitiesSet") + } + } + AccessLevel::Unspecified => Err("UnspecifiedActor"), + AccessLevel::EntityMaintainer => Err("AccessLevel::EntityMaintainer-UsedOutOfPlace"), + } + } + pub fn can_update_entity( class_permissions: &Self, access_level: &AccessLevel, From 3a4df8c45ce15d47d2ecaf9a3581bf213bd392af Mon Sep 17 00:00:00 2001 From: iorveth Date: Thu, 23 Apr 2020 13:30:35 +0300 Subject: [PATCH 21/26] Add additional equality check to forbid updating equal prop values --- runtime-modules/content-directory/src/lib.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index 664febd695..5f51d216c3 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -1202,14 +1202,16 @@ impl Module { if let Some(current_prop_value) = updated_values.get_mut(&id) { // Get class-level information about this property if let Some(class_prop) = class.properties.get(id as usize) { - // Validate a new property value against the type of this property - // and check any additional constraints like the length of a vector - // if it's a vector property or the length of a text if it's a text property. - Self::ensure_property_value_to_update_is_valid(&new_value, class_prop)?; - - // Update a current prop value in a mutable vector, if a new value is valid. - current_prop_value.update(new_value); - updated = true; + if new_value != *current_prop_value { + // Validate a new property value against the type of this property + // and check any additional constraints like the length of a vector + // if it's a vector property or the length of a text if it's a text property. + Self::ensure_property_value_to_update_is_valid(&new_value, class_prop)?; + + // Update a current prop value in a mutable vector, if a new value is valid. + current_prop_value.update(new_value); + updated = true; + } } } else { // Throw an error if a property was not found on entity From 21a344ee90c25ea1acee60c2493a1a77c95001b5 Mon Sep 17 00:00:00 2001 From: iorveth Date: Thu, 23 Apr 2020 20:39:24 +0300 Subject: [PATCH 22/26] Entity removal implementation, part 2 --- runtime-modules/content-directory/src/lib.rs | 145 +++++++++++++----- .../content-directory/src/operations.rs | 2 +- 2 files changed, 108 insertions(+), 39 deletions(-) diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index 5f51d216c3..e3c99d0a2f 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -7,7 +7,9 @@ use rstd::prelude::*; use runtime_primitives::traits::{ MaybeSerialize, MaybeSerializeDeserialize, Member, One, SimpleArithmetic, Zero, }; -use srml_support::{decl_module, StorageMap, decl_storage, dispatch, ensure, traits::Get, Parameter}; +use srml_support::{ + decl_module, decl_storage, dispatch, ensure, traits::Get, Parameter, StorageMap, +}; #[cfg(feature = "std")] pub use serde::{Deserialize, Serialize}; @@ -249,7 +251,7 @@ pub struct Entity { /// Length is no more than Class.properties. pub values: BTreeMap>, // pub deleted: bool - pub reference_count: u32 + pub reference_count: u32, } impl Default for Entity { @@ -258,7 +260,7 @@ impl Default for Entity { class_id: ClassId::default(), supported_schemas: BTreeSet::new(), values: BTreeMap::new(), - reference_count: 0 + reference_count: 0, } } } @@ -798,7 +800,7 @@ decl_module! { origin, with_credential: Option, class_id: ClassId, - schema_id: SchemaId, + schema_id: SchemaId, is_active: bool ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; @@ -846,7 +848,7 @@ decl_module! { with_credential: Option, as_entity_maintainer: bool, entity_id: EntityId, - schema_id: SchemaId, + schema_id: SchemaId, property_values: BTreeMap> ) -> dispatch::Result { let raw_origin = Self::ensure_root_or_signed(origin)?; @@ -1011,9 +1013,7 @@ impl Module { None, ClassPermissions::can_remove_entity, class_id, - |_class_permissions, _access_level| { - Self::complete_entity_removal(entity_id) - } + |_class_permissions, _access_level| Self::complete_entity_removal(entity_id), ) } @@ -1161,7 +1161,6 @@ impl Module { } fn complete_entity_removal(entity_id: EntityId) -> dispatch::Result { - // Ensure there is no property values pointing to given entity Self::ensure_rc_is_zero(entity_id)?; >::remove(entity_id); @@ -1171,7 +1170,7 @@ impl Module { pub fn complete_class_schema_status_update( class_id: ClassId, - schema_id: SchemaId, + schema_id: SchemaId, schema_status: bool, ) -> dispatch::Result { // Check that schema_id is a valid index of class schemas vector: @@ -1194,6 +1193,8 @@ impl Module { // so we can update them if new values provided present in new_property_values. let mut updated_values = entity.values; let mut updated = false; + let mut entities_rc_to_decrement_vec = vec![]; + let mut entities_rc_to_increment_vec = vec![]; // Iterate over a vector of new values and update corresponding properties // of this entity if new values are valid. for (id, new_value) in new_property_values.into_iter() { @@ -1207,7 +1208,24 @@ impl Module { // and check any additional constraints like the length of a vector // if it's a vector property or the length of a text if it's a text property. Self::ensure_property_value_to_update_is_valid(&new_value, class_prop)?; - + // Get unique entity ids to update rc + if let (Some(entities_rc_to_decrement), Some(entities_rc_to_increment)) = ( + Self::get_involved_entities(¤t_prop_value), + Self::get_involved_entities(&new_value), + ) { + let (entities_rc_to_decrement, entities_rc_to_increment): ( + Vec, + Vec, + ) = entities_rc_to_decrement + .into_iter() + .zip(entities_rc_to_increment.into_iter()) + .filter(|(entity_rc_to_decrement, entity_rc_to_increment)| { + entity_rc_to_decrement != entity_rc_to_increment + }) + .unzip(); + entities_rc_to_decrement_vec.push(entities_rc_to_decrement); + entities_rc_to_increment_vec.push(entities_rc_to_increment); + } // Update a current prop value in a mutable vector, if a new value is valid. current_prop_value.update(new_value); updated = true; @@ -1225,6 +1243,16 @@ impl Module { >::mutate(entity_id, |entity| { entity.values = updated_values; }); + entities_rc_to_decrement_vec + .iter() + .for_each(|entities_rc_to_decrement| { + Self::decrement_entities_rc(entities_rc_to_decrement); + }); + entities_rc_to_increment_vec + .iter() + .for_each(|entities_rc_to_increment| { + Self::increment_entities_rc(entities_rc_to_increment); + }) } Ok(()) @@ -1236,9 +1264,10 @@ impl Module { ) -> dispatch::Result { Self::ensure_known_entity_id(entity_id)?; let entity = Self::entity_by_id(entity_id); - - match entity.values.get(&in_class_schema_property_id) { - Some(current_prop_value) if current_prop_value.is_vec() => (), + let entities_rc_to_decrement = match entity.values.get(&in_class_schema_property_id) { + Some(current_prop_value) if current_prop_value.is_vec() => { + Self::get_involved_entities(¤t_prop_value) + } Some(_) => return Err(ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR), _ => // Throw an error if a property was not found on entity @@ -1246,7 +1275,7 @@ impl Module { { return Err(ERROR_UNKNOWN_ENTITY_PROP_ID) } - } + }; // Clear property value vector: >::mutate(entity_id, |entity| { @@ -1255,6 +1284,9 @@ impl Module { { current_property_value_vec.vec_clear(); } + if let Some(entities_rc_to_decrement) = entities_rc_to_decrement { + Self::decrement_entities_rc(&entities_rc_to_decrement); + } }); Ok(()) @@ -1269,19 +1301,22 @@ impl Module { Self::ensure_known_entity_id(entity_id)?; let entity = Self::entity_by_id(entity_id); - if let Some(current_prop_value) = entity.values.get(&in_class_schema_property_id) { - // Ensure property value vector nonces equality to avoid possible data races, - // when performing vector specific operations - Self::ensure_nonce_equality(current_prop_value, nonce)?; - Self::ensure_index_in_property_vector_is_valid( - current_prop_value, - index_in_property_vec, - )?; - } else { - // Throw an error if a property was not found on entity - // by an in-class index of a property. - return Err(ERROR_UNKNOWN_ENTITY_PROP_ID); - } + let involved_entity_id = + if let Some(current_prop_value) = entity.values.get(&in_class_schema_property_id) { + // Ensure property value vector nonces equality to avoid possible data races, + // when performing vector specific operations + Self::ensure_nonce_equality(current_prop_value, nonce)?; + Self::ensure_index_in_property_vector_is_valid( + current_prop_value, + index_in_property_vec, + )?; + Self::get_involved_entities(¤t_prop_value) + .map(|involved_entities| involved_entities[index_in_property_vec as usize]) + } else { + // Throw an error if a property was not found on entity + // by an in-class index of a property. + return Err(ERROR_UNKNOWN_ENTITY_PROP_ID); + }; // Remove property value vector >::mutate(entity_id, |entity| { @@ -1289,7 +1324,9 @@ impl Module { current_prop_value.vec_remove_at(index_in_property_vec) } }); - + if let Some(involved_entity_id) = involved_entity_id { + >::mutate(involved_entity_id, |entity| entity.reference_count += 1) + } Ok(()) } @@ -1332,6 +1369,9 @@ impl Module { // Insert property value into property value vector >::mutate(entity_id, |entity| { + if let Some(entities_rc_to_increment) = Self::get_involved_entities(&property_value) { + Self::increment_entities_rc(&entities_rc_to_increment); + } if let Some(current_prop_value) = entity.values.get_mut(&in_class_schema_property_id) { current_prop_value.vec_insert_at(index_in_property_vec, property_value) } @@ -1412,6 +1452,18 @@ impl Module { } } + fn increment_entities_rc(entity_ids: &[EntityId]) { + entity_ids.iter().for_each(|entity_id| { + >::mutate(entity_id, |entity| entity.reference_count += 1) + }); + } + + fn decrement_entities_rc(entity_ids: &[EntityId]) { + entity_ids.iter().for_each(|entity_id| { + >::mutate(entity_id, |entity| entity.reference_count -= 1) + }); + } + /// Returns the stored class if exist, error otherwise. fn ensure_class_exists(class_id: ClassId) -> Result, &'static str> { ensure!(>::exists(class_id), ERROR_CLASS_NOT_FOUND); @@ -1712,11 +1764,17 @@ impl Module { pub fn ensure_rc_is_zero(entity_id: EntityId) -> dispatch::Result { let entity = Self::entity_by_id(entity_id); - ensure!(entity.reference_count == 0, ERROR_ENTITY_REFERENCE_COUNTER_DOES_NOT_EQUAL_TO_ZERO); + ensure!( + entity.reference_count == 0, + ERROR_ENTITY_REFERENCE_COUNTER_DOES_NOT_EQUAL_TO_ZERO + ); Ok(()) } - pub fn ensure_class_schema_id_exists(class: &Class, schema_id: SchemaId) -> dispatch::Result { + pub fn ensure_class_schema_id_exists( + class: &Class, + schema_id: SchemaId, + ) -> dispatch::Result { ensure!( schema_id < class.schemas.len() as SchemaId, ERROR_UNKNOWN_CLASS_SCHEMA_ID @@ -1724,7 +1782,10 @@ impl Module { Ok(()) } - pub fn ensure_class_schema_is_active(class: &Class, schema_id: SchemaId) -> dispatch::Result { + pub fn ensure_class_schema_is_active( + class: &Class, + schema_id: SchemaId, + ) -> dispatch::Result { ensure!( class.is_active_schema(schema_id), ERROR_CLASS_SCHEMA_NOT_ACTIVE @@ -1732,7 +1793,10 @@ impl Module { Ok(()) } - pub fn ensure_schema_id_is_not_added(entity: &Entity, schema_id: SchemaId) -> dispatch::Result { + pub fn ensure_schema_id_is_not_added( + entity: &Entity, + schema_id: SchemaId, + ) -> dispatch::Result { let schema_not_added = !entity.supported_schemas.contains(&schema_id); ensure!(schema_not_added, ERROR_SCHEMA_ALREADY_ADDED_TO_ENTITY); Ok(()) @@ -1799,6 +1863,14 @@ impl Module { (entity, class) } + pub fn get_involved_entities(current_prop_value: &PropertyValue) -> Option> { + match current_prop_value { + PropertyValue::Reference(entity_id) => Some(vec![*entity_id]), + PropertyValue::ReferenceVec(entity_ids_vec, _) => Some(entity_ids_vec.clone()), + _ => None, + } + } + pub fn ensure_property_value_to_update_is_valid( value: &PropertyValue, prop: &Property, @@ -1889,11 +1961,8 @@ impl Module { } pub fn validate_max_len_of_text(text: &[u8], max_len: TextMaxLength) -> dispatch::Result { - if text.len() <= max_len as usize { - Ok(()) - } else { - Err(ERROR_TEXT_PROP_IS_TOO_LONG) - } + ensure!(text.len() <= max_len as usize, ERROR_TEXT_PROP_IS_TOO_LONG); + Ok(()) } pub fn validate_max_len_if_vec_prop( diff --git a/runtime-modules/content-directory/src/operations.rs b/runtime-modules/content-directory/src/operations.rs index e8e5ebe755..57d15120de 100644 --- a/runtime-modules/content-directory/src/operations.rs +++ b/runtime-modules/content-directory/src/operations.rs @@ -1,4 +1,4 @@ -use crate::{ClassId, EntityId, SchemaId, PropertyId, PropertyValue, Trait}; +use crate::{ClassId, EntityId, PropertyId, PropertyValue, SchemaId, Trait}; use codec::{Decode, Encode}; use rstd::collections::btree_map::BTreeMap; use rstd::prelude::*; From 1f8eec3f88fb1d7fb4076a33a6d71a8865a3319a Mon Sep 17 00:00:00 2001 From: iorveth Date: Thu, 23 Apr 2020 22:38:34 +0300 Subject: [PATCH 23/26] Fix add schema support not updating entity counter in case of reference --- runtime-modules/content-directory/src/lib.rs | 30 +++++++++++++------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index e3c99d0a2f..cfe028189d 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -1209,9 +1209,9 @@ impl Module { // if it's a vector property or the length of a text if it's a text property. Self::ensure_property_value_to_update_is_valid(&new_value, class_prop)?; // Get unique entity ids to update rc - if let (Some(entities_rc_to_decrement), Some(entities_rc_to_increment)) = ( - Self::get_involved_entities(¤t_prop_value), + if let (Some(entities_rc_to_increment), Some(entities_rc_to_decrement)) = ( Self::get_involved_entities(&new_value), + Self::get_involved_entities(¤t_prop_value), ) { let (entities_rc_to_decrement, entities_rc_to_increment): ( Vec, @@ -1223,8 +1223,8 @@ impl Module { entity_rc_to_decrement != entity_rc_to_increment }) .unzip(); - entities_rc_to_decrement_vec.push(entities_rc_to_decrement); entities_rc_to_increment_vec.push(entities_rc_to_increment); + entities_rc_to_decrement_vec.push(entities_rc_to_decrement); } // Update a current prop value in a mutable vector, if a new value is valid. current_prop_value.update(new_value); @@ -1243,16 +1243,16 @@ impl Module { >::mutate(entity_id, |entity| { entity.values = updated_values; }); + entities_rc_to_increment_vec + .iter() + .for_each(|entities_rc_to_increment| { + Self::increment_entities_rc(entities_rc_to_increment); + }); entities_rc_to_decrement_vec .iter() .for_each(|entities_rc_to_decrement| { Self::decrement_entities_rc(entities_rc_to_decrement); }); - entities_rc_to_increment_vec - .iter() - .for_each(|entities_rc_to_increment| { - Self::increment_entities_rc(entities_rc_to_increment); - }) } Ok(()) @@ -1325,7 +1325,7 @@ impl Module { } }); if let Some(involved_entity_id) = involved_entity_id { - >::mutate(involved_entity_id, |entity| entity.reference_count += 1) + >::mutate(involved_entity_id, |entity| entity.reference_count -= 1) } Ok(()) } @@ -1694,6 +1694,7 @@ impl Module { let current_entity_values = entity.values.clone(); let mut appended_entity_values = entity.values; + let mut entities_rc_to_increment_vec = vec![]; for prop_id in schema_prop_ids.iter() { if current_entity_values.contains_key(prop_id) { @@ -1707,14 +1708,16 @@ impl Module { // If a value was not povided for the property of this schema: if let Some(new_value) = property_values.get(prop_id) { Self::ensure_property_value_to_update_is_valid(new_value, class_prop)?; - + if let Some(entities_rc_to_increment) = Self::get_involved_entities(new_value) { + entities_rc_to_increment_vec.push(entities_rc_to_increment); + } appended_entity_values.insert(*prop_id, new_value.to_owned()); } else { // All required prop values should be are provided if class_prop.required { return Err(ERROR_MISSING_REQUIRED_PROP); } - // Add all missing non required schema prop values as PropertyValue::None + // Add all missing non required schema prop values as PropertyValue::Bool(false) else { appended_entity_values.insert(*prop_id, PropertyValue::Bool(false)); } @@ -1728,6 +1731,11 @@ impl Module { // Update entity values only if new properties have been added. if appended_entity_values.len() > entity.values.len() { entity.values = appended_entity_values; + entities_rc_to_increment_vec + .iter() + .for_each(|entities_rc_to_increment| { + Self::increment_entities_rc(entities_rc_to_increment); + }); } }); From 2067612f12125e900df3f4569df648d0c8854977 Mon Sep 17 00:00:00 2001 From: iorveth Date: Fri, 24 Apr 2020 12:34:53 +0300 Subject: [PATCH 24/26] Fix batch transaction test --- runtime-modules/content-directory/src/lib.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index cfe028189d..0286b3ccd6 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -1731,13 +1731,13 @@ impl Module { // Update entity values only if new properties have been added. if appended_entity_values.len() > entity.values.len() { entity.values = appended_entity_values; - entities_rc_to_increment_vec - .iter() - .for_each(|entities_rc_to_increment| { - Self::increment_entities_rc(entities_rc_to_increment); - }); } }); + entities_rc_to_increment_vec + .iter() + .for_each(|entities_rc_to_increment| { + Self::increment_entities_rc(entities_rc_to_increment); + }); Ok(()) } From 629ba85ecc32614e596cdc9c0261b59345294c12 Mon Sep 17 00:00:00 2001 From: iorveth Date: Fri, 24 Apr 2020 13:46:21 +0300 Subject: [PATCH 25/26] Bunch of matching trees refactoring --- runtime-modules/content-directory/src/lib.rs | 231 +++++++++--------- .../content-directory/src/operations.rs | 26 +- .../content-directory/src/permissions.rs | 23 +- 3 files changed, 133 insertions(+), 147 deletions(-) diff --git a/runtime-modules/content-directory/src/lib.rs b/runtime-modules/content-directory/src/lib.rs index 0286b3ccd6..e1537695ca 100755 --- a/runtime-modules/content-directory/src/lib.rs +++ b/runtime-modules/content-directory/src/lib.rs @@ -1193,6 +1193,7 @@ impl Module { // so we can update them if new values provided present in new_property_values. let mut updated_values = entity.values; let mut updated = false; + let mut entities_rc_to_decrement_vec = vec![]; let mut entities_rc_to_increment_vec = vec![]; // Iterate over a vector of new values and update corresponding properties @@ -1200,41 +1201,40 @@ impl Module { for (id, new_value) in new_property_values.into_iter() { // Try to find a current property value in the entity // by matching its id to the id of a property with an updated value. - if let Some(current_prop_value) = updated_values.get_mut(&id) { - // Get class-level information about this property - if let Some(class_prop) = class.properties.get(id as usize) { - if new_value != *current_prop_value { - // Validate a new property value against the type of this property - // and check any additional constraints like the length of a vector - // if it's a vector property or the length of a text if it's a text property. - Self::ensure_property_value_to_update_is_valid(&new_value, class_prop)?; - // Get unique entity ids to update rc - if let (Some(entities_rc_to_increment), Some(entities_rc_to_decrement)) = ( - Self::get_involved_entities(&new_value), - Self::get_involved_entities(¤t_prop_value), - ) { - let (entities_rc_to_decrement, entities_rc_to_increment): ( - Vec, - Vec, - ) = entities_rc_to_decrement - .into_iter() - .zip(entities_rc_to_increment.into_iter()) - .filter(|(entity_rc_to_decrement, entity_rc_to_increment)| { - entity_rc_to_decrement != entity_rc_to_increment - }) - .unzip(); - entities_rc_to_increment_vec.push(entities_rc_to_increment); - entities_rc_to_decrement_vec.push(entities_rc_to_decrement); - } - // Update a current prop value in a mutable vector, if a new value is valid. - current_prop_value.update(new_value); - updated = true; - } - } - } else { + let current_prop_value = updated_values + .get_mut(&id) // Throw an error if a property was not found on entity // by an in-class index of a property update. - return Err(ERROR_UNKNOWN_ENTITY_PROP_ID); + .ok_or(ERROR_UNKNOWN_ENTITY_PROP_ID)?; + // Get class-level information about this property + if let Some(class_prop) = class.properties.get(id as usize) { + if new_value != *current_prop_value { + // Validate a new property value against the type of this property + // and check any additional constraints like the length of a vector + // if it's a vector property or the length of a text if it's a text property. + Self::ensure_property_value_to_update_is_valid(&new_value, class_prop)?; + // Get unique entity ids to update rc + if let (Some(entities_rc_to_increment), Some(entities_rc_to_decrement)) = ( + Self::get_involved_entities(&new_value), + Self::get_involved_entities(¤t_prop_value), + ) { + let (entities_rc_to_decrement, entities_rc_to_increment): ( + Vec, + Vec, + ) = entities_rc_to_decrement + .into_iter() + .zip(entities_rc_to_increment.into_iter()) + .filter(|(entity_rc_to_decrement, entity_rc_to_increment)| { + entity_rc_to_decrement != entity_rc_to_increment + }) + .unzip(); + entities_rc_to_increment_vec.push(entities_rc_to_increment); + entities_rc_to_decrement_vec.push(entities_rc_to_decrement); + } + // Update a current prop value in a mutable vector, if a new value is valid. + current_prop_value.update(new_value); + updated = true; + } } } @@ -1264,18 +1264,20 @@ impl Module { ) -> dispatch::Result { Self::ensure_known_entity_id(entity_id)?; let entity = Self::entity_by_id(entity_id); - let entities_rc_to_decrement = match entity.values.get(&in_class_schema_property_id) { - Some(current_prop_value) if current_prop_value.is_vec() => { - Self::get_involved_entities(¤t_prop_value) - } - Some(_) => return Err(ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR), - _ => + let current_prop_value = entity + .values + .get(&in_class_schema_property_id) // Throw an error if a property was not found on entity // by an in-class index of a property. - { - return Err(ERROR_UNKNOWN_ENTITY_PROP_ID) - } - }; + .ok_or(ERROR_UNKNOWN_ENTITY_PROP_ID)?; + + // Ensure prop value under given class schema property id is vector + ensure!( + current_prop_value.is_vec(), + ERROR_PROP_VALUE_UNDER_GIVEN_INDEX_IS_NOT_A_VECTOR + ); + + let entities_rc_to_decrement = Self::get_involved_entities(¤t_prop_value); // Clear property value vector: >::mutate(entity_id, |entity| { @@ -1301,22 +1303,19 @@ impl Module { Self::ensure_known_entity_id(entity_id)?; let entity = Self::entity_by_id(entity_id); - let involved_entity_id = - if let Some(current_prop_value) = entity.values.get(&in_class_schema_property_id) { - // Ensure property value vector nonces equality to avoid possible data races, - // when performing vector specific operations - Self::ensure_nonce_equality(current_prop_value, nonce)?; - Self::ensure_index_in_property_vector_is_valid( - current_prop_value, - index_in_property_vec, - )?; - Self::get_involved_entities(¤t_prop_value) - .map(|involved_entities| involved_entities[index_in_property_vec as usize]) - } else { - // Throw an error if a property was not found on entity - // by an in-class index of a property. - return Err(ERROR_UNKNOWN_ENTITY_PROP_ID); - }; + let current_prop_value = entity + .values + .get(&in_class_schema_property_id) + // Throw an error if a property was not found on entity + // by an in-class index of a property. + .ok_or(ERROR_UNKNOWN_ENTITY_PROP_ID)?; + + // Ensure property value vector nonces equality to avoid possible data races, + // when performing vector specific operations + Self::ensure_nonce_equality(current_prop_value, nonce)?; + Self::ensure_index_in_property_vector_is_valid(current_prop_value, index_in_property_vec)?; + let involved_entity_id = Self::get_involved_entities(¤t_prop_value) + .map(|involved_entities| involved_entities[index_in_property_vec as usize]); // Remove property value vector >::mutate(entity_id, |entity| { @@ -1342,30 +1341,29 @@ impl Module { let (entity, class) = Self::get_entity_and_class(entity_id); // Get class-level information about this property - if let Some(class_prop) = class.properties.get(in_class_schema_property_id as usize) { - // Try to find a current property value in the entity - // by matching its id to the id of a property with an updated value. - if let Some(entity_prop_value) = entity.values.get(&in_class_schema_property_id) { - // Ensure property value vector nonces equality to avoid possible data races, - // when performing vector specific operations - Self::ensure_nonce_equality(entity_prop_value, nonce)?; - // Validate a new property value against the type of this property - // and check any additional constraints like the length of a vector - // if it's a vector property or the length of a text if it's a text property. - Self::ensure_prop_value_can_be_inserted_at_prop_vec( - &property_value, - entity_prop_value, - index_in_property_vec, - class_prop, - )?; - } - } else { + let class_prop = class + .properties + .get(in_class_schema_property_id as usize) // Throw an error if a property was not found on entity // by an in-class index of a property update. - { - return Err(ERROR_UNKNOWN_ENTITY_PROP_ID); - } - } + .ok_or(ERROR_UNKNOWN_ENTITY_PROP_ID)?; + + // Try to find a current property value in the entity + // by matching its id to the id of a property with an updated value. + if let Some(entity_prop_value) = entity.values.get(&in_class_schema_property_id) { + // Ensure property value vector nonces equality to avoid possible data races, + // when performing vector specific operations + Self::ensure_nonce_equality(entity_prop_value, nonce)?; + // Validate a new property value against the type of this property + // and check any additional constraints like the length of a vector + // if it's a vector property or the length of a text if it's a text property. + Self::ensure_prop_value_can_be_inserted_at_prop_vec( + &property_value, + entity_prop_value, + index_in_property_vec, + class_prop, + )?; + }; // Insert property value into property value vector >::mutate(entity_id, |entity| { @@ -1422,27 +1420,25 @@ impl Module { system::RawOrigin::Root => Ok(AccessLevel::System), system::RawOrigin::Signed(account_id) => { if let Some(credential) = with_credential { - if T::CredentialChecker::account_has_credential(&account_id, credential) { - if let Some(entity_id) = as_entity_maintainer { - // is entity maintained by system - ensure!( - >::exists(entity_id), - "NotEnityMaintainer" - ); - // ensure entity maintainer matches - match Self::entity_maintainer_by_entity_id(entity_id) { - Some(maintainer_credential) - if credential == maintainer_credential => - { - Ok(AccessLevel::EntityMaintainer) - } - _ => Err("NotEnityMaintainer"), + ensure!( + T::CredentialChecker::account_has_credential(&account_id, credential), + "OriginCannotActWithRequestedCredential" + ); + if let Some(entity_id) = as_entity_maintainer { + // is entity maintained by system + ensure!( + >::exists(entity_id), + "NotEnityMaintainer" + ); + // ensure entity maintainer matches + match Self::entity_maintainer_by_entity_id(entity_id) { + Some(maintainer_credential) if credential == maintainer_credential => { + Ok(AccessLevel::EntityMaintainer) } - } else { - Ok(AccessLevel::Credential(credential)) + _ => Err("NotEnityMaintainer"), } } else { - Err("OriginCannotActWithRequestedCredential") + Ok(AccessLevel::Credential(credential)) } } else { Ok(AccessLevel::Unspecified) @@ -1542,7 +1538,7 @@ impl Module { fn get_class_id_by_entity_id(entity_id: EntityId) -> Result { // use a utility method on versioned_store module - ensure!(>::exists(entity_id), "EntityNotFound"); + ensure!(>::exists(entity_id), ERROR_ENTITY_NOT_FOUND); let entity = Self::entity_by_id(entity_id); Ok(entity.class_id) } @@ -1567,14 +1563,14 @@ impl Module { Err("EntityCannotReferenceTargetEntity") } ReferenceConstraint::Restricted(permitted_properties) => { - if permitted_properties.contains(&PropertyOfClass { - class_id: source_class_id, - property_index: *in_class_index, - }) { - Ok(()) - } else { - Err("EntityCannotReferenceTargetEntity") - } + ensure!( + permitted_properties.contains(&PropertyOfClass { + class_id: source_class_id, + property_index: *in_class_index, + }), + "EntityCannotReferenceTargetEntity" + ); + Ok(()) } }?; } @@ -1714,13 +1710,9 @@ impl Module { appended_entity_values.insert(*prop_id, new_value.to_owned()); } else { // All required prop values should be are provided - if class_prop.required { - return Err(ERROR_MISSING_REQUIRED_PROP); - } + ensure!(!class_prop.required, ERROR_MISSING_REQUIRED_PROP); // Add all missing non required schema prop values as PropertyValue::Bool(false) - else { - appended_entity_values.insert(*prop_id, PropertyValue::Bool(false)); - } + appended_entity_values.insert(*prop_id, PropertyValue::Bool(false)); } } @@ -1823,10 +1815,10 @@ impl Module { entity.class_id == class_id, ERROR_INTERNAL_PROP_DOES_NOT_MATCH_ITS_CLASS ); - Ok(()) } - _ => Ok(()), + _ => (), } + Ok(()) } pub fn ensure_index_in_property_vector_is_valid( @@ -2021,11 +2013,8 @@ impl Module { _ => true, }; - if is_valid_len { - Ok(()) - } else { - Err(ERROR_VEC_PROP_IS_TOO_LONG) - } + ensure!(is_valid_len, ERROR_VEC_PROP_IS_TOO_LONG); + Ok(()) } pub fn ensure_prop_value_matches_its_type( diff --git a/runtime-modules/content-directory/src/operations.rs b/runtime-modules/content-directory/src/operations.rs index 57d15120de..b6702023e0 100644 --- a/runtime-modules/content-directory/src/operations.rs +++ b/runtime-modules/content-directory/src/operations.rs @@ -70,11 +70,9 @@ pub fn parametrized_entity_to_entity_id( ParameterizedEntity::ExistingEntity(entity_id) => Ok(entity_id), ParameterizedEntity::InternalEntityJustAdded(op_index_u32) => { let op_index = op_index_u32 as usize; - if let Some(entity_id) = created_entities.get(&op_index) { - Ok(*entity_id) - } else { - Err("EntityNotCreatedByOperation") - } + Ok(*created_entities + .get(&op_index) + .ok_or("EntityNotCreatedByOperation")?) } } } @@ -93,11 +91,10 @@ pub fn parametrized_property_values_to_property_values( ) => { // Verify that referenced entity was indeed created created let op_index = entity_created_in_operation_index as usize; - if let Some(entity_id) = created_entities.get(&op_index) { - PropertyValue::Reference(*entity_id) - } else { - return Err("EntityNotCreatedByOperation"); - } + let entity_id = created_entities + .get(&op_index) + .ok_or("EntityNotCreatedByOperation")?; + PropertyValue::Reference(*entity_id) } ParametrizedPropertyValue::InternalEntityVec(parametrized_entities) => { let mut entities: Vec = vec![]; @@ -109,11 +106,10 @@ pub fn parametrized_property_values_to_property_values( entity_created_in_operation_index, ) => { let op_index = entity_created_in_operation_index as usize; - if let Some(entity_id) = created_entities.get(&op_index) { - entities.push(*entity_id); - } else { - return Err("EntityNotCreatedByOperation"); - } + let entity_id = created_entities + .get(&op_index) + .ok_or("EntityNotCreatedByOperation")?; + entities.push(*entity_id); } } } diff --git a/runtime-modules/content-directory/src/permissions.rs b/runtime-modules/content-directory/src/permissions.rs index 4353a1140b..962befcd35 100644 --- a/runtime-modules/content-directory/src/permissions.rs +++ b/runtime-modules/content-directory/src/permissions.rs @@ -1,5 +1,5 @@ use codec::{Decode, Encode}; -use srml_support::dispatch; +use srml_support::{dispatch, ensure}; use crate::constraint::*; use crate::credentials::*; @@ -77,11 +77,11 @@ where match access_level { AccessLevel::System => Ok(()), AccessLevel::Credential(credential) => { - if class_permissions.add_schemas.contains(credential) { - Ok(()) - } else { - Err("NotInAddSchemasSet") - } + ensure!( + class_permissions.add_schemas.contains(credential), + "NotInAddSchemasSet" + ); + Ok(()) } AccessLevel::Unspecified => Err("UnspecifiedActor"), AccessLevel::EntityMaintainer => Err("AccessLevel::EntityMaintainer-UsedOutOfPlace"), @@ -90,16 +90,17 @@ where pub fn can_update_schema_status( class_permissions: &Self, + access_level: &AccessLevel, ) -> dispatch::Result { match access_level { AccessLevel::System => Ok(()), AccessLevel::Credential(credential) => { - if class_permissions.update_schemas_status.contains(credential) { - Ok(()) - } else { - Err("NotInUpdateSchemasStatusSet") - } + ensure!( + class_permissions.update_schemas_status.contains(credential), + "NotInUpdateSchemasStatusSet" + ); + Ok(()) } AccessLevel::Unspecified => Err("UnspecifiedActor"), AccessLevel::EntityMaintainer => Err("AccessLevel::EntityMaintainer-UsedOutOfPlace"), From 9725e55837746bea00e1d02b6b0dc356dfb55b88 Mon Sep 17 00:00:00 2001 From: iorveth Date: Fri, 24 Apr 2020 22:46:25 +0300 Subject: [PATCH 26/26] Entities removal test coverage added --- .../content-directory/src/tests.rs | 89 ++++++++++++++----- 1 file changed, 66 insertions(+), 23 deletions(-) diff --git a/runtime-modules/content-directory/src/tests.rs b/runtime-modules/content-directory/src/tests.rs index 53d8adf27c..3a91b50e32 100644 --- a/runtime-modules/content-directory/src/tests.rs +++ b/runtime-modules/content-directory/src/tests.rs @@ -1569,32 +1569,37 @@ fn cannot_complete_insert_at_entity_property_vector_when_nonce_does_not_match() }) } +fn create_entity_with_prop_value_referencing_another_entity() -> (EntityId, EntityId) { + let class_id = create_simple_class_with_default_permissions(); + let schema_id = TestModule::append_class_schema( + class_id, + vec![], + vec![ + good_prop_bool().required(), + new_reference_class_prop_vec(class_id), + ], + ) + .expect("This should not happen"); + let entity_id = create_entity_of_class(class_id); + let entity_id_2 = create_entity_of_class(class_id); + let mut property_values = BTreeMap::new(); + property_values.insert(PROP_ID_BOOL, PropertyValue::Bool(true)); + property_values.insert( + PROP_ID_REFERENCE_VEC, + PropertyValue::ReferenceVec(vec![entity_id_2], ::Nonce::default()), + ); + assert_ok!(TestModule::add_entity_schema_support( + entity_id, + schema_id, + property_values + )); + (entity_id, entity_id_2) +} + #[test] fn cannot_complete_insert_at_entity_property_vector_when_unknown_internal_entity_id() { with_test_externalities(|| { - let class_id = create_simple_class_with_default_permissions(); - let schema_id = TestModule::append_class_schema( - class_id, - vec![], - vec![ - good_prop_bool().required(), - new_reference_class_prop_vec(class_id), - ], - ) - .expect("This should not happen"); - let entity_id = create_entity_of_class(class_id); - let entity_id_2 = create_entity_of_class(class_id); - let mut property_values = BTreeMap::new(); - property_values.insert(PROP_ID_BOOL, PropertyValue::Bool(true)); - property_values.insert( - PROP_ID_REFERENCE_VEC, - PropertyValue::ReferenceVec(vec![entity_id_2], ::Nonce::default()), - ); - assert_ok!(TestModule::add_entity_schema_support( - entity_id, - schema_id, - property_values - )); + let (entity_id, _) = create_entity_with_prop_value_referencing_another_entity(); assert_err!( TestModule::complete_insert_at_entity_property_vector( entity_id, @@ -1608,6 +1613,44 @@ fn cannot_complete_insert_at_entity_property_vector_when_unknown_internal_entity }) } +// Remove entity +// -------------------------------------- + +#[test] +fn remove_entity_successfully() { + with_test_externalities(|| { + let (_, _, entity_id) = create_class_with_schema_and_entity(); + assert_ok!(TestModule::remove_entity(Origin::ROOT, None, entity_id)); + // Ensure entity related storage was cleared successfully. + assert_eq!( + TestModule::entity_by_id(entity_id), + Entity::::default() + ); + assert_eq!(TestModule::entity_maintainer_by_entity_id(entity_id), None); + }) +} + +#[test] +fn remove_entity_not_found() { + with_test_externalities(|| { + assert_err!( + TestModule::remove_entity(Origin::ROOT, None, UNKNOWN_ENTITY_ID), + ERROR_ENTITY_NOT_FOUND + ); + }) +} + +#[test] +fn remove_entity_reference_counter_does_not_equal_zero() { + with_test_externalities(|| { + let (_, entity_by_id_2) = create_entity_with_prop_value_referencing_another_entity(); + assert_err!( + TestModule::remove_entity(Origin::ROOT, None, entity_by_id_2), + ERROR_ENTITY_REFERENCE_COUNTER_DOES_NOT_EQUAL_TO_ZERO + ); + }) +} + // TODO test text max len // TODO test vec max len