Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RSDK-9299 - refactor get_resource_name to be a non-proto method #329

Merged
merged 4 commits into from
Nov 20, 2024
Merged

RSDK-9299 - refactor get_resource_name to be a non-proto method #329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6200d49
refactor get_resource_name to be a non-proto method
stuqdog Nov 19, 2024
995649d
make linter happy
stuqdog Nov 19, 2024
2120f7b
unit test remote name conversion
stuqdog Nov 20, 2024
e85455b
Merge branch 'main' into refactor-get-resource-name
stuqdog Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/viam/sdk/common/utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <unordered_map>
#include <vector>

#include <boost/algorithm/string.hpp>
#include <boost/blank.hpp>
#include <boost/log/core.hpp>
#include <boost/log/expressions.hpp>
Expand Down Expand Up @@ -183,6 +184,26 @@ bool from_dm_from_extra(const ProtoStruct& extra) {
}
return false;
}
std::pair<std::string, std::string> long_name_to_remote_and_short(const std::string& long_name) {
std::vector<std::string> 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(":"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: Why do we explicitly use fully qualified names like boost::split or std::something instead of bringing them into scope with using or using namespace in the code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question! I tend to like fully qualified names when they're not overly verbose. I appreciate the explicitness. And, a broader using line can add more than we need to scope and create potential for namespace collision.

Also in this case, boost::split and boost::is_any_of are only used once each in this file so using them would actually make the code (slightly) more verbose, not less.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it ever possible that long_name does not contain colons? in this case remote_name will be empty and name will be the entire long_name (right?) I am not sure if this is an edge case we need to worry about but if it is, is this behavior okay?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is possible. A long name is of form {remote}:{short_name} (or {remote1}:...{remoteN}:{short_name}. If there's no colon then we just have a short name so we just return the name as-is, which is ideal.

auto name = name_parts.back();
name_parts.pop_back();
auto remote_name = name_parts.empty()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should be checking name_parts.empty() above before calling back(), that's UB if the container is empty and I think since this is in a public header we might want to be slightly more cautious about inputs

Also could be good to add a unit test or two

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait just saw your reply to Julie above, so split will return a vector with one element if there's no separator?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait just saw your reply to Julie above, so split will return a vector with one element if there's no separator?

Yep! I've just added a test to highlight a couple cases just to be be explicit.

? ""
: 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
4 changes: 4 additions & 0 deletions src/viam/sdk/common/utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is a remote name and short name? and what purpose do they server as opposed to resource names?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a remote name is the name of a remote that the resource is part of, and the short name is just the name of the resource. so we could have remote1:my_arm as a long name, which is comprised of a remote name (remote1) and a short name my_arm).

/// exists) and short name
std::pair<std::string, std::string> 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();

Expand Down
17 changes: 4 additions & 13 deletions src/viam/sdk/components/component.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,17 @@

#include <string>

#include <google/protobuf/struct.pb.h>

#include <viam/api/common/v1/common.pb.h>

#include <viam/sdk/common/utils.hpp>
#include <viam/sdk/resource/resource.hpp>

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this function know what kComponent is? I cant seem to find it as a global variable anywhere...?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kComponent and kService are defined in utils.hpp, which is included in this file (and in service.cpp below).

}

} // namespace sdk
Expand Down
6 changes: 1 addition & 5 deletions src/viam/sdk/components/component.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,6 @@

#include <string>

#include <google/protobuf/struct.pb.h>

#include <viam/api/common/v1/common.pb.h>

#include <viam/sdk/resource/resource.hpp>

namespace viam {
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/viam/sdk/registry/registry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ Registry::registered_models() {
// NOLINTNEXTLINE(readability-convert-member-functions-to-static)
Status ModelRegistration::create_status(const std::shared_ptr<Resource>& 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;
}
Expand Down
20 changes: 9 additions & 11 deletions src/viam/sdk/resource/resource.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
#include <viam/sdk/resource/resource.hpp>

#include <grpcpp/support/status.h>

#include <viam/sdk/common/proto_value.hpp>
#include <viam/sdk/common/utils.hpp>
#include <viam/sdk/registry/registry.hpp>
Expand All @@ -10,23 +8,23 @@
namespace viam {
namespace sdk {

using common::v1::ResourceName;

Resource::~Resource() = default;
Resource::Resource(std::string name) : name_(std::move(name)) {}

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
Expand Down
10 changes: 5 additions & 5 deletions src/viam/sdk/resource/resource.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@

#include <unordered_map>

#include <grpcpp/impl/service_type.h>
#include <grpcpp/support/status.h>

#include <viam/sdk/common/proto_value.hpp>
#include <viam/sdk/config/resource.hpp>
#include <viam/sdk/resource/resource_api.hpp>
Expand All @@ -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 <>
Expand Down
21 changes: 5 additions & 16 deletions src/viam/sdk/resource/resource_api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand Down Expand Up @@ -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<std::string> 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) {
Expand Down
4 changes: 2 additions & 2 deletions src/viam/sdk/robot/service.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ std::vector<Status> RobotService_::generate_status_(
const std::shared_ptr<const ModelRegistration> 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;
Expand Down Expand Up @@ -195,7 +195,7 @@ ::grpc::Status RobotService_::StopAll(::grpc::ServerContext*,

for (const auto& r : resource_manager()->resources()) {
const std::shared_ptr<Resource> 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 {
Expand Down
11 changes: 3 additions & 8 deletions src/viam/sdk/services/service.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,16 @@

#include <string>

#include <viam/api/common/v1/common.pb.h>

#include <viam/sdk/common/utils.hpp>
#include <viam/sdk/resource/resource.hpp>

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as kComponent but where is kService coming from? how does the function know it exists

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

}

Service::Service() : Resource("service"){};
Service::Service() : Resource("service") {}

} // namespace sdk
} // namespace viam
6 changes: 2 additions & 4 deletions src/viam/sdk/services/service.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,18 @@

#include <string>

#include <viam/api/common/v1/common.pb.h>

#include <viam/sdk/resource/resource.hpp>

namespace viam {
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
Expand Down
17 changes: 17 additions & 0 deletions src/viam/sdk/tests/test_common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down