From 7a264e9fb2991af93b47978600be3f2a18b442f3 Mon Sep 17 00:00:00 2001 From: jonathan-r-thorpe <64410119+jonathan-r-thorpe@users.noreply.github.com> Date: Mon, 14 Oct 2024 16:17:19 +0100 Subject: [PATCH] Fix enum sequence and struct sequence validation (#412) * Improve reporting and logging of errors * Fix constraint validation of enum sequences and struct sequences * Return NcMethosResultError instead of NcMethodResult --------- Co-authored-by: Simon Lo --- Development/nmos/control_protocol_methods.cpp | 41 ++- Development/nmos/control_protocol_utils.cpp | 254 +++++++++--------- 2 files changed, 158 insertions(+), 137 deletions(-) diff --git a/Development/nmos/control_protocol_methods.cpp b/Development/nmos/control_protocol_methods.cpp index d9d78129..0023c311 100644 --- a/Development/nmos/control_protocol_methods.cpp +++ b/Development/nmos/control_protocol_methods.cpp @@ -30,6 +30,7 @@ namespace nmos // unknown property utility::stringstream_t ss; ss << U("unknown property: ") << property_id.serialize() << U(" to do Get"); + slog::log(gate, SLOG_FLF) << ss.str(); return details::make_nc_method_result_error({ nc_method_status::property_not_implemented }, ss.str()); } @@ -82,15 +83,17 @@ namespace nmos } catch (const nmos::control_protocol_exception& e) { - slog::log(gate, SLOG_FLF) << "Set property: " << property_id.serialize() << " value: " << val.serialize() << " error: " << e.what(); - - return details::make_nc_method_result({ nc_method_status::parameter_error }); + utility::stringstream_t ss; + ss << "Set property: " << property_id.serialize() << " value: " << val.serialize() << " error: " << e.what(); + slog::log(gate, SLOG_FLF) << ss.str(); + return details::make_nc_method_result_error({ nc_method_status::parameter_error }, ss.str()); } } // unknown property utility::stringstream_t ss; ss << U("unknown property: ") << property_id.serialize() << " to do Set"; + slog::log(gate, SLOG_FLF) << ss.str(); return details::make_nc_method_result_error({ nc_method_status::property_not_implemented }, ss.str()); } @@ -126,12 +129,14 @@ namespace nmos // out of bound utility::stringstream_t ss; ss << U("property: ") << property_id.serialize() << U(" is outside the available range to do GetSequenceItem"); + slog::log(gate, SLOG_FLF) << ss.str(); return details::make_nc_method_result_error({ nc_method_status::index_out_of_bounds }, ss.str()); } // unknown property utility::stringstream_t ss; ss << U("unknown property: ") << property_id.serialize() << U(" to do GetSequenceItem"); + slog::log(gate, SLOG_FLF) << ss.str(); return details::make_nc_method_result_error({ nc_method_status::property_not_implemented }, ss.str()); } @@ -163,6 +168,7 @@ namespace nmos // property is not a sequence utility::stringstream_t ss; ss << U("property: ") << property_id.serialize() << U(" is not a sequence to do SetSequenceItem"); + slog::log(gate, SLOG_FLF) << ss.str(); return details::make_nc_method_result_error({ nc_method_status::invalid_request }, ss.str()); } @@ -190,21 +196,24 @@ namespace nmos } catch (const nmos::control_protocol_exception& e) { - slog::log(gate, SLOG_FLF) << "Set sequence item: " << property_id.serialize() << " index: " << index << " value: " << val.serialize() << " error: " << e.what(); - - return details::make_nc_method_result({ nc_method_status::parameter_error }); + utility::stringstream_t ss; + ss << "Set sequence item: " << property_id.serialize() << " index: " << index << " value: " << val.serialize() << " error: " << e.what(); + slog::log(gate, SLOG_FLF) << ss.str(); + return details::make_nc_method_result_error({ nc_method_status::parameter_error }, ss.str()); } } // out of bound utility::stringstream_t ss; ss << U("property: ") << property_id.serialize() << U(" is outside the available range to do SetSequenceItem"); + slog::log(gate, SLOG_FLF) << ss.str(); return details::make_nc_method_result_error({ nc_method_status::index_out_of_bounds }, ss.str()); } // unknown property utility::stringstream_t ss; ss << U("unknown property: ") << property_id.serialize() << U(" to do SetSequenceItem"); + slog::log(gate, SLOG_FLF) << ss.str(); return details::make_nc_method_result_error({ nc_method_status::property_not_implemented }, ss.str()); } @@ -266,15 +275,17 @@ namespace nmos } catch (const nmos::control_protocol_exception& e) { - slog::log(gate, SLOG_FLF) << "Add sequence item: " << property_id.serialize() << " value: " << val.serialize() << " error: " << e.what(); - - return details::make_nc_method_result({ nc_method_status::parameter_error }); + utility::stringstream_t ss; + ss << "Add sequence item: " << property_id.serialize() << " value: " << val.serialize() << " error: " << e.what(); + slog::log(gate, SLOG_FLF) << ss.str(); + return details::make_nc_method_result_error({ nc_method_status::parameter_error }, ss.str()); } } // unknown property utility::stringstream_t ss; ss << U("unknown property: ") << property_id.serialize() << U(" to do AddSequenceItem"); + slog::log(gate, SLOG_FLF) << ss.str(); return details::make_nc_method_result_error({ nc_method_status::property_not_implemented }, ss.str()); } @@ -299,6 +310,7 @@ namespace nmos // property is not a sequence utility::stringstream_t ss; ss << U("property: ") << property_id.serialize() << U(" is not a sequence to do RemoveSequenceItem"); + slog::log(gate, SLOG_FLF) << ss.str(); return details::make_nc_method_result_error({ nc_method_status::invalid_request }, ss.str()); } @@ -317,12 +329,14 @@ namespace nmos // out of bound utility::stringstream_t ss; ss << U("property: ") << property_id.serialize() << U(" is outside the available range to do RemoveSequenceItem"); + slog::log(gate, SLOG_FLF) << ss.str(); return details::make_nc_method_result_error({ nc_method_status::index_out_of_bounds }, ss.str()); } // unknown property utility::stringstream_t ss; ss << U("unknown property: ") << property_id.serialize() << U(" to do RemoveSequenceItem"); + slog::log(gate, SLOG_FLF) << ss.str(); return details::make_nc_method_result_error({ nc_method_status::property_not_implemented }, ss.str()); } @@ -346,6 +360,7 @@ namespace nmos // property is not a sequence utility::stringstream_t ss; ss << U("property: ") << property_id.serialize() << U(" is not a sequence to do GetSequenceLength"); + slog::log(gate, SLOG_FLF) << ss.str(); return details::make_nc_method_result_error({ nc_method_status::invalid_request }, ss.str()); } @@ -368,6 +383,7 @@ namespace nmos // null utility::stringstream_t ss; ss << U("property: ") << property_id.serialize() << " is a null sequence to do GetSequenceLength"; + slog::log(gate, SLOG_FLF) << ss.str(); return details::make_nc_method_result_error({ nc_method_status::invalid_request }, ss.str()); } } @@ -377,6 +393,7 @@ namespace nmos // unknown property utility::stringstream_t ss; ss << U("unknown property: ") << property_id.serialize() << " to do GetSequenceLength"; + slog::log(gate, SLOG_FLF) << ss.str(); return details::make_nc_method_result_error({ nc_method_status::property_not_implemented }, ss.str()); } @@ -444,13 +461,17 @@ namespace nmos // no role utility::stringstream_t ss; ss << U("role: ") << role.as_string() << U(" not found to do FindMembersByPath"); + slog::log(gate, SLOG_FLF) << ss.str(); return details::make_nc_method_result_error({ nc_method_status::parameter_error }, ss.str()); } } else { // no members - return details::make_nc_method_result_error({ nc_method_status::parameter_error }, U("no members to do FindMembersByPath")); + utility::stringstream_t ss; + ss << U("role: ") << role.as_string() << U(" has no members to do FindMembersByPath"); + slog::log(gate, SLOG_FLF) << ss.str(); + return details::make_nc_method_result_error({ nc_method_status::parameter_error }, ss.str()); } } diff --git a/Development/nmos/control_protocol_utils.cpp b/Development/nmos/control_protocol_utils.cpp index dd9afc31..46478f41 100644 --- a/Development/nmos/control_protocol_utils.cpp +++ b/Development/nmos/control_protocol_utils.cpp @@ -159,167 +159,167 @@ namespace nmos // See https://specs.amwa.tv/ms-05-02/branches/v1.0.x/docs/Constraints.html void datatype_constraints_validation(const web::json::value& data, const datatype_constraints_validation_parameters& params) { - // no constraints validation required - if (params.datatype_descriptor.is_null()) { return; } + auto parameter_constraints_validation = [¶ms](const web::json::value& value_) + { + // no constraints validation required + if (params.datatype_descriptor.is_null()) { return; } - const auto& datatype_type = nmos::fields::nc::type(params.datatype_descriptor); + const auto& datatype_type = nmos::fields::nc::type(params.datatype_descriptor); - // do NcDatatypeDescriptorPrimitive constraints validation - if (nc_datatype_type::Primitive == datatype_type) - { - // hmm, for the primitive type, it should not have datatype constraints specified via the datatype_descriptor but just in case - const auto& datatype_constraints = nmos::fields::nc::constraints(params.datatype_descriptor); - if (datatype_constraints.is_null()) + // do NcDatatypeDescriptorPrimitive constraints validation + if (nc_datatype_type::Primitive == datatype_type) { - auto primitive_validation = [](const nc_name& name, const web::json::value& value) - { - auto is_int16 = [](int32_t value) - { - return value >= (std::numeric_limits::min)() - && value <= (std::numeric_limits::max)(); - }; - auto is_uint16 = [](uint32_t value) - { - return value >= (std::numeric_limits::min)() - && value <= (std::numeric_limits::max)(); - }; - auto is_float32 = [](double value) - { - return value >= (std::numeric_limits::lowest)() - && value <= (std::numeric_limits::max)(); - }; - - if (U("NcBoolean") == name) { return value.is_boolean(); } - if (U("NcInt16") == name && value.is_number()) { return is_int16(value.as_number().to_int32()); } - if (U("NcInt32") == name && value.is_number()) { return value.as_number().is_int32(); } - if (U("NcInt64") == name && value.is_number()) { return value.as_number().is_int64(); } - if (U("NcUint16") == name && value.is_number()) { return is_uint16(value.as_number().to_uint32()); } - if (U("NcUint32") == name && value.is_number()) { return value.as_number().is_uint32(); } - if (U("NcUint64") == name && value.is_number()) { return value.as_number().is_uint64(); } - if (U("NcFloat32") == name && value.is_number()) { return is_float32(value.as_number().to_double()); } - if (U("NcFloat64") == name && value.is_number()) { return !value.as_number().is_integral(); } - if (U("NcString") == name) { return value.is_string(); } - - // invalid primitive type - return false; - }; - - // do primitive type constraints validation - const auto& name = nmos::fields::nc::name(params.datatype_descriptor); - if (data.is_array()) + // hmm, for the primitive type, it should not have datatype constraints specified via the datatype_descriptor but just in case + const auto& datatype_constraints = nmos::fields::nc::constraints(params.datatype_descriptor); + if (datatype_constraints.is_null()) { - for (const auto& value : data.as_array()) - { - if (!primitive_validation(name, value)) + auto primitive_validation = [](const nc_name& name, const web::json::value& value) { - throw control_protocol_exception("value is not a " + utility::us2s(name) + " type"); - } + auto is_int16 = [](int32_t value) + { + return value >= (std::numeric_limits::min)() + && value <= (std::numeric_limits::max)(); + }; + auto is_uint16 = [](uint32_t value) + { + return value >= (std::numeric_limits::min)() + && value <= (std::numeric_limits::max)(); + }; + auto is_float32 = [](double value) + { + return value >= (std::numeric_limits::lowest)() + && value <= (std::numeric_limits::max)(); + }; + + if (U("NcBoolean") == name) { return value.is_boolean(); } + if (U("NcInt16") == name && value.is_number()) { return is_int16(value.as_number().to_int32()); } + if (U("NcInt32") == name && value.is_number()) { return value.as_number().is_int32(); } + if (U("NcInt64") == name && value.is_number()) { return value.as_number().is_int64(); } + if (U("NcUint16") == name && value.is_number()) { return is_uint16(value.as_number().to_uint32()); } + if (U("NcUint32") == name && value.is_number()) { return value.as_number().is_uint32(); } + if (U("NcUint64") == name && value.is_number()) { return value.as_number().is_uint64(); } + if (U("NcFloat32") == name && value.is_number()) { return is_float32(value.as_number().to_double()); } + if (U("NcFloat64") == name && value.is_number()) { return !value.as_number().is_integral(); } + if (U("NcString") == name) { return value.is_string(); } + + // invalid primitive type + return false; + }; + + // do primitive type constraints validation + const auto& name = nmos::fields::nc::name(params.datatype_descriptor); + if (!primitive_validation(name, value_)) + { + throw control_protocol_exception("value is not a " + utility::us2s(name) + " type");; } } else { - if (!primitive_validation(name, data)) - { - throw control_protocol_exception("value is not a " + utility::us2s(name) + " type");; - } + constraints_validation(value_, datatype_constraints); } - } - else - { - constraints_validation(data, datatype_constraints); - } - - return; - } - // do NcDatatypeDescriptorTypeDef constraints validation - if (nc_datatype_type::Typedef == datatype_type) - { - // do the datatype constraints specified via the datatype_descriptor if presented - const auto& datatype_constraints = nmos::fields::nc::constraints(params.datatype_descriptor); - if (datatype_constraints.is_null()) - { - // do parent typename constraints validation - const auto& type_name = params.datatype_descriptor.at(nmos::fields::nc::parent_type); // parent type_name - datatype_constraints_validation(data, { details::get_datatype_descriptor(type_name, params.get_control_protocol_datatype_descriptor), params.get_control_protocol_datatype_descriptor }); + return; } - else + + // do NcDatatypeDescriptorTypeDef constraints validation + if (nc_datatype_type::Typedef == datatype_type) { - constraints_validation(data, datatype_constraints); - } + // do the datatype constraints specified via the datatype_descriptor if presented + const auto& datatype_constraints = nmos::fields::nc::constraints(params.datatype_descriptor); + if (datatype_constraints.is_null()) + { + // do parent typename constraints validation + const auto& type_name = params.datatype_descriptor.at(nmos::fields::nc::parent_type); // parent type_name + datatype_constraints_validation(value_, { details::get_datatype_descriptor(type_name, params.get_control_protocol_datatype_descriptor), params.get_control_protocol_datatype_descriptor }); + } + else + { + constraints_validation(value_, datatype_constraints); + } - return; - } + return; + } - // do NcDatatypeDescriptorEnum constraints validation - if (nc_datatype_type::Enum == datatype_type) - { - const auto& items = nmos::fields::nc::items(params.datatype_descriptor); - if (items.end() == std::find_if(items.begin(), items.end(), [&](const web::json::value& nc_enum_item_descriptor) { return nmos::fields::nc::value(nc_enum_item_descriptor) == data; })) + // do NcDatatypeDescriptorEnum constraints validation + if (nc_datatype_type::Enum == datatype_type) { - const auto& name = nmos::fields::nc::name(params.datatype_descriptor); - throw control_protocol_exception("value is not an enum " + utility::us2s(name) + " type"); - } + const auto& items = nmos::fields::nc::items(params.datatype_descriptor); + if (items.end() == std::find_if(items.begin(), items.end(), [&](const web::json::value& nc_enum_item_descriptor) { return nmos::fields::nc::value(nc_enum_item_descriptor) == value_; })) + { + const auto& name = nmos::fields::nc::name(params.datatype_descriptor); + throw control_protocol_exception("value is not an enum " + utility::us2s(name) + " type"); + } - return; - } + return; + } - // do NcDatatypeDescriptorStruct constraints validation - if (nc_datatype_type::Struct == datatype_type) - { - const auto& datatype_name = nmos::fields::nc::name(params.datatype_descriptor); - const auto& fields = nmos::fields::nc::fields(params.datatype_descriptor); - // NcFieldDescriptor - for (const web::json::value& nc_field_descriptor : fields) + // do NcDatatypeDescriptorStruct constraints validation + if (nc_datatype_type::Struct == datatype_type) { - const auto& field_name = nmos::fields::nc::name(nc_field_descriptor); - // is field in strurcture - if (!data.has_field(field_name)) { throw control_protocol_exception("missing " + utility::us2s(field_name) + " in " + utility::us2s(datatype_name)); } - - // is field nullable - if (nmos::fields::nc::is_nullable(nc_field_descriptor) != data.is_null()) { throw control_protocol_exception(utility::us2s(field_name) + " is not nullable"); } + const auto& datatype_name = nmos::fields::nc::name(params.datatype_descriptor); + const auto& fields = nmos::fields::nc::fields(params.datatype_descriptor); + // NcFieldDescriptor + for (const web::json::value& nc_field_descriptor : fields) + { + const auto& field_name = nmos::fields::nc::name(nc_field_descriptor); + // is field in strurcture + if (!value_.has_field(field_name)) { throw control_protocol_exception("missing " + utility::us2s(field_name) + " in " + utility::us2s(datatype_name)); } - // is field sequenceable - if (nmos::fields::nc::is_sequence(nc_field_descriptor) != data.is_array()) { throw control_protocol_exception(utility::us2s(field_name) + " is not sequenceable"); } + // is field nullable + if (nmos::fields::nc::is_nullable(nc_field_descriptor) != value_.is_null()) { throw control_protocol_exception(utility::us2s(field_name) + " is not nullable"); } - // check against field constraints if presented - const auto& constraints = nmos::fields::nc::constraints(nc_field_descriptor); - if (constraints.is_null()) - { - // no field constraints, move to check the constraints of its typeName - const auto& field_type_name = nc_field_descriptor.at(nmos::fields::nc::type_name); + // is field sequenceable + if (nmos::fields::nc::is_sequence(nc_field_descriptor) != value_.is_array()) { throw control_protocol_exception(utility::us2s(field_name) + " is not sequenceable"); } - if (!field_type_name.is_null()) + // check against field constraints if presented + const auto& constraints = nmos::fields::nc::constraints(nc_field_descriptor); + if (constraints.is_null()) { - auto value = data.at(field_name); + // no field constraints, move to check the constraints of its typeName + const auto& field_type_name = nc_field_descriptor.at(nmos::fields::nc::type_name); - if (value.is_array()) + if (!field_type_name.is_null()) { - for (const auto& val : value.as_array()) + auto value = value_.at(field_name); + + if (value.is_array()) + { + for (const auto& val : value.as_array()) + { + // do typename constraints validation + datatype_constraints_validation(val, { details::get_datatype_descriptor(field_type_name, params.get_control_protocol_datatype_descriptor), params.get_control_protocol_datatype_descriptor }); + } + } + else { // do typename constraints validation - datatype_constraints_validation(val, { details::get_datatype_descriptor(field_type_name, params.get_control_protocol_datatype_descriptor), params.get_control_protocol_datatype_descriptor }); + datatype_constraints_validation(value, { details::get_datatype_descriptor(field_type_name, params.get_control_protocol_datatype_descriptor), params.get_control_protocol_datatype_descriptor }); } } - else - { - // do typename constraints validation - datatype_constraints_validation(value, { details::get_datatype_descriptor(field_type_name, params.get_control_protocol_datatype_descriptor), params.get_control_protocol_datatype_descriptor }); - } + } + else + { + // do field constraints validation + const auto& value = value_.at(field_name); + constraints_validation(value, constraints); } } - else - { - // do field constraints validation - const auto& value = data.at(field_name); - constraints_validation(value, constraints); - } + // unsupported datatype_type, no validation is required + return; } + }; - return; + if (data.is_array()) + { + for (const auto& value : data.as_array()) + { + parameter_constraints_validation(value); + } + } + else + { + parameter_constraints_validation(data); } - - // unsupported datatype_type, no validation is required } // multiple levels of constraints validation, may throw nmos::control_protocol_exception