From e331f2c2033be01f1ab6f86ad42a648608002f23 Mon Sep 17 00:00:00 2001 From: Ethan Date: Wed, 20 Nov 2024 14:54:24 -0500 Subject: [PATCH] RSDK-9299 - refactor get_resource_name to be a non-proto method (#329) --- src/viam/sdk/common/utils.cpp | 21 +++++++++++++++++++++ src/viam/sdk/common/utils.hpp | 4 ++++ src/viam/sdk/components/component.cpp | 17 ++++------------- src/viam/sdk/components/component.hpp | 6 +----- src/viam/sdk/registry/registry.cpp | 2 +- src/viam/sdk/resource/resource.cpp | 20 +++++++++----------- src/viam/sdk/resource/resource.hpp | 10 +++++----- src/viam/sdk/resource/resource_api.cpp | 21 +++++---------------- src/viam/sdk/robot/service.cpp | 4 ++-- src/viam/sdk/services/service.cpp | 11 +++-------- src/viam/sdk/services/service.hpp | 6 ++---- src/viam/sdk/tests/test_common.cpp | 17 +++++++++++++++++ 12 files changed, 74 insertions(+), 65 deletions(-) diff --git a/src/viam/sdk/common/utils.cpp b/src/viam/sdk/common/utils.cpp index b3fec14c9..dbf1846fa 100644 --- a/src/viam/sdk/common/utils.cpp +++ b/src/viam/sdk/common/utils.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -183,6 +184,26 @@ bool from_dm_from_extra(const ProtoStruct& extra) { } return false; } +std::pair long_name_to_remote_and_short(const std::string& long_name) { + std::vector name_parts; + // boost::split causes a clang-tidy false positive, see + // https://bugs.llvm.org/show_bug.cgi?id=41141 + // + // NOLINTNEXTLINE + boost::split(name_parts, long_name, boost::is_any_of(":")); + auto name = name_parts.back(); + name_parts.pop_back(); + auto remote_name = name_parts.empty() + ? "" + : std::accumulate(std::next(name_parts.begin()), + name_parts.end(), + *name_parts.begin(), + [](const std::string& a, const std::string& b) { + return a + ":" + b; + }); + + return {std::move(remote_name), std::move(name)}; +} } // namespace sdk } // namespace viam diff --git a/src/viam/sdk/common/utils.hpp b/src/viam/sdk/common/utils.hpp index 20beca5ee..7a83e1929 100644 --- a/src/viam/sdk/common/utils.hpp +++ b/src/viam/sdk/common/utils.hpp @@ -61,6 +61,10 @@ class ClientContext { grpc::ClientContext wrapped_context_; }; +/// @brief Given a fully qualified resource name, returns remote name (or "" if no remote name +/// exists) and short name +std::pair long_name_to_remote_and_short(const std::string& long_name); + /// @brief Returns a new `ProtoStruct` with a random key for server-side debug logging ProtoStruct debug_map(); diff --git a/src/viam/sdk/components/component.cpp b/src/viam/sdk/components/component.cpp index c44bd91e9..f2676a8ac 100644 --- a/src/viam/sdk/components/component.cpp +++ b/src/viam/sdk/components/component.cpp @@ -2,26 +2,17 @@ #include -#include - -#include - #include -#include namespace viam { namespace sdk { -using viam::common::v1::ResourceName; - -Component::Component() : Resource("component"){}; +Component::Component() : Resource("component") {} -Component::Component(std::string name) : Resource(std::move(name)){}; +Component::Component(std::string name) : Resource(std::move(name)) {} -ResourceName Component::get_resource_name(std::string name) const { - auto r = this->Resource::get_resource_name(name); - *r.mutable_type() = kComponent; - return r; +Name Component::get_resource_name() const { + return Resource::get_resource_name(kComponent); } } // namespace sdk diff --git a/src/viam/sdk/components/component.hpp b/src/viam/sdk/components/component.hpp index 83830d2e4..c96628a4c 100644 --- a/src/viam/sdk/components/component.hpp +++ b/src/viam/sdk/components/component.hpp @@ -2,10 +2,6 @@ #include -#include - -#include - #include namespace viam { @@ -14,7 +10,7 @@ namespace sdk { class Component : public Resource { public: Component(); - viam::common::v1::ResourceName get_resource_name(std::string name) const override; + Name get_resource_name() const override; protected: explicit Component(std::string name); diff --git a/src/viam/sdk/registry/registry.cpp b/src/viam/sdk/registry/registry.cpp index e0676af4d..2667dcc7c 100644 --- a/src/viam/sdk/registry/registry.cpp +++ b/src/viam/sdk/registry/registry.cpp @@ -162,7 +162,7 @@ Registry::registered_models() { // NOLINTNEXTLINE(readability-convert-member-functions-to-static) Status ModelRegistration::create_status(const std::shared_ptr& resource) const { Status status; - *status.mutable_name() = resource->get_resource_name(resource->name()); + *status.mutable_name() = resource->get_resource_name().to_proto(); *status.mutable_status() = google::protobuf::Struct(); return status; } diff --git a/src/viam/sdk/resource/resource.cpp b/src/viam/sdk/resource/resource.cpp index 04b304679..60eff538d 100644 --- a/src/viam/sdk/resource/resource.cpp +++ b/src/viam/sdk/resource/resource.cpp @@ -1,7 +1,5 @@ #include -#include - #include #include #include @@ -10,8 +8,6 @@ namespace viam { namespace sdk { -using common::v1::ResourceName; - Resource::~Resource() = default; Resource::Resource(std::string name) : name_(std::move(name)) {} @@ -19,14 +15,16 @@ std::string Resource::name() const { return name_; } -ResourceName Resource::get_resource_name(std::string name) const { - ResourceName r; - *r.mutable_namespace_() = kRDK; - *r.mutable_type() = kResource; - *r.mutable_subtype() = this->api().resource_subtype(); - *r.mutable_name() = std::move(name); +Name Resource::get_resource_name(const std::string& type) const { + auto api_ = api(); + api_.set_resource_type(type); + + auto name_parts = long_name_to_remote_and_short(name_); + return {api_, name_parts.first, name_parts.second}; +} - return r; +Name Resource::get_resource_name() const { + return get_resource_name(kResource); } } // namespace sdk diff --git a/src/viam/sdk/resource/resource.hpp b/src/viam/sdk/resource/resource.hpp index 0d1d55eb7..dcf9f4363 100644 --- a/src/viam/sdk/resource/resource.hpp +++ b/src/viam/sdk/resource/resource.hpp @@ -2,9 +2,6 @@ #include -#include -#include - #include #include #include @@ -22,14 +19,17 @@ class Resource { /// @brief Returns the `API` associated with a particular resource. virtual API api() const = 0; - /// @brief Returns a `ResourceName` for a particular resource name. - virtual viam::common::v1::ResourceName get_resource_name(std::string name) const; + /// @brief Returns the `Name` for a particular resource. + virtual Name get_resource_name() const; /// @brief Return the resource's name. virtual std::string name() const; private: std::string name_; + + protected: + Name get_resource_name(const std::string& type) const; }; template <> diff --git a/src/viam/sdk/resource/resource_api.cpp b/src/viam/sdk/resource/resource_api.cpp index 4924b1bbe..3cca16d0e 100644 --- a/src/viam/sdk/resource/resource_api.cpp +++ b/src/viam/sdk/resource/resource_api.cpp @@ -32,7 +32,7 @@ std::string APIType::to_string() const { } APIType::APIType(std::string namespace_, std::string resource_type) - : namespace_(std::move(namespace_)), resource_type_(std::move(resource_type)){}; + : namespace_(std::move(namespace_)), resource_type_(std::move(resource_type)) {} std::string API::to_string() const { return APIType::to_string() + ":" + resource_subtype_; @@ -126,21 +126,10 @@ viam::common::v1::ResourceName Name::to_proto() const { } Name Name::from_proto(const viam::common::v1::ResourceName& proto) { - const API api(proto.namespace_(), proto.type(), proto.subtype()); - std::vector name_parts; - boost::split(name_parts, proto.name(), boost::is_any_of(":")); - auto name = name_parts.back(); - name_parts.pop_back(); - auto remote_name = name_parts.empty() - ? "" - : std::accumulate(std::next(name_parts.begin()), - name_parts.end(), - *name_parts.begin(), - [](const std::string& a, const std::string& b) { - return a + ":" + b; - }); - - return Name({proto.namespace_(), proto.type(), proto.subtype()}, remote_name, name); + auto name_parts = long_name_to_remote_and_short(proto.name()); + + return Name( + {proto.namespace_(), proto.type(), proto.subtype()}, name_parts.first, name_parts.second); }; Name Name::from_string(std::string name) { diff --git a/src/viam/sdk/robot/service.cpp b/src/viam/sdk/robot/service.cpp index 26004a939..20f5d62bb 100644 --- a/src/viam/sdk/robot/service.cpp +++ b/src/viam/sdk/robot/service.cpp @@ -82,7 +82,7 @@ std::vector RobotService_::generate_status_( const std::shared_ptr registration = kv.second; if (registration->api().resource_subtype() == resource->api().resource_subtype()) { bool resource_present = false; - const ResourceName name = resource->get_resource_name(resource->name()); + const ResourceName name = resource->get_resource_name().to_proto(); for (const auto& resource_name : resource_names) { if (name.SerializeAsString() == resource_name.SerializeAsString()) { resource_present = true; @@ -195,7 +195,7 @@ ::grpc::Status RobotService_::StopAll(::grpc::ServerContext*, for (const auto& r : resource_manager()->resources()) { const std::shared_ptr resource = r.second; - const ResourceName rn = resource->get_resource_name(resource->name()); + const ResourceName rn = resource->get_resource_name().to_proto(); const std::string rn_ = rn.SerializeAsString(); if (extra.find(rn_) != extra.end()) { try { diff --git a/src/viam/sdk/services/service.cpp b/src/viam/sdk/services/service.cpp index 739ab0d8d..ed207bc01 100644 --- a/src/viam/sdk/services/service.cpp +++ b/src/viam/sdk/services/service.cpp @@ -2,21 +2,16 @@ #include -#include - #include -#include namespace viam { namespace sdk { -common::v1::ResourceName Service::get_resource_name(std::string name) const { - auto r = this->Resource::get_resource_name(name); - *r.mutable_type() = kService; - return r; +Name Service::get_resource_name() const { + return Resource::get_resource_name(kService); } -Service::Service() : Resource("service"){}; +Service::Service() : Resource("service") {} } // namespace sdk } // namespace viam diff --git a/src/viam/sdk/services/service.hpp b/src/viam/sdk/services/service.hpp index 817304867..b9640b54b 100644 --- a/src/viam/sdk/services/service.hpp +++ b/src/viam/sdk/services/service.hpp @@ -2,8 +2,6 @@ #include -#include - #include namespace viam { @@ -11,11 +9,11 @@ namespace sdk { class Service : public Resource { public: - viam::common::v1::ResourceName get_resource_name(std::string name) const override; + Name get_resource_name() const override; Service(); protected: - explicit Service(std::string name) : Resource(std::move(name)){}; + explicit Service(std::string name) : Resource(std::move(name)) {} }; } // namespace sdk diff --git a/src/viam/sdk/tests/test_common.cpp b/src/viam/sdk/tests/test_common.cpp index 0d27d468c..6e4454b03 100644 --- a/src/viam/sdk/tests/test_common.cpp +++ b/src/viam/sdk/tests/test_common.cpp @@ -123,6 +123,23 @@ BOOST_AUTO_TEST_CASE(test_version_metadata) { BOOST_CHECK_EQUAL(version_constructed, version); } +BOOST_AUTO_TEST_CASE(test_name_conversion) { + std::string long_name1 = "foo:bar"; + auto res1 = long_name_to_remote_and_short(long_name1); + BOOST_CHECK_EQUAL(res1.first, "foo"); + BOOST_CHECK_EQUAL(res1.second, "bar"); + + std::string long_name2 = "foo:bar:baz"; + auto res2 = long_name_to_remote_and_short(long_name2); + BOOST_CHECK_EQUAL(res2.first, "foo:bar"); + BOOST_CHECK_EQUAL(res2.second, "baz"); + + std::string long_name3 = "foo"; + auto res3 = long_name_to_remote_and_short(long_name3); + BOOST_CHECK_EQUAL(res3.first, ""); + BOOST_CHECK_EQUAL(res3.second, "foo"); +} + BOOST_AUTO_TEST_SUITE_END() } // namespace sdktests