1837 daps refactor common library to use proto3#1842
Open
JoshuaSBrown wants to merge 13 commits intodevelfrom
Open
1837 daps refactor common library to use proto3#1842JoshuaSBrown wants to merge 13 commits intodevelfrom
JoshuaSBrown wants to merge 13 commits intodevelfrom
Conversation
…hat was there before, update INCLUDE directive in CMakeLists.txt from proto to proto3
…e consistent and the addition of files to CMake Variable
…g out includes to use new proto3
Contributor
Reviewer's GuideRefactors the common library to use a new proto3-based, envelope-centric messaging scheme, simplifying Frame metadata, centralizing message type lookup on the Envelope descriptor, and updating ZeroMQ send/receive paths and tests to operate on inner messages wrapped in SDMS::Envelope instead of legacy Anon/Auth protocol IDs. Sequence diagram for sending a message with proto3 Envelope over ZeroMQsequenceDiagram
participant App as ClientApp
participant GMsg as GoogleProtoMessage
participant FFactory as FrameFactory
participant PMap as ProtoBufMap
participant ZMQ as ZeroMQCommunicator
participant Sock as ZeroMQSocket
participant Buf as Buffer
participant Env as Envelope
App->>GMsg: setPayload(unique_ptr<Message> inner)
activate GMsg
GMsg->>FFactory: create(inner, PMap)
activate FFactory
FFactory->>PMap: getMessageType(inner)
activate PMap
PMap-->>FFactory: uint16_t msg_type
deactivate PMap
FFactory-->>GMsg: Frame frame(msg_type, size=inner.ByteSizeLong())
deactivate FFactory
GMsg->>PMap: wrapInEnvelope(inner)
activate PMap
PMap-->>GMsg: unique_ptr<Envelope> temp_envelope
deactivate PMap
GMsg->>GMsg: m_dyn_attributes[FRAME_SIZE] = temp_envelope.ByteSizeLong()
GMsg->>GMsg: m_dyn_attributes[MSG_TYPE] = frame.msg_type
deactivate GMsg
App->>ZMQ: send(msg)
activate ZMQ
ZMQ->>ZMQ: sendFrame(msg, Sock)
ZMQ->>FFactory: create(msg)
activate FFactory
FFactory-->>ZMQ: Frame frame(msg_type, size)
deactivate FFactory
ZMQ->>Sock: zmq_msg_send(frame header)
ZMQ->>ZMQ: sendBody(msg, Buf, Sock)
ZMQ->>GMsg: getPayload()
GMsg-->>ZMQ: proto::Message* inner
ZMQ->>PMap: wrapInEnvelope(*inner)
activate PMap
PMap-->>ZMQ: unique_ptr<Envelope> Env
deactivate PMap
ZMQ->>Buf: copyToBuffer(Env, size)
ZMQ->>Sock: zmq_msg_send(body bytes)
deactivate ZMQ
Sequence diagram for receiving a proto3 Envelope message over ZeroMQsequenceDiagram
participant Sock as ZeroMQSocket
participant ZMQ as ZeroMQCommunicator
participant Buf as Buffer
participant FFactory as FrameFactory
participant PMap as ProtoBufMap
participant Env as Envelope
participant PFac as ProtoBufFactory
participant GMsg as GoogleProtoMessage
Sock-->>ZMQ: zmq_msg_recv(frame header)
activate ZMQ
ZMQ->>FFactory: create(zmq_msg)
activate FFactory
FFactory-->>ZMQ: Frame frame(size, msg_type)
deactivate FFactory
ZMQ->>GMsg: set(FRAME_SIZE, frame.size)
ZMQ->>GMsg: set(MSG_TYPE, frame.msg_type)
alt frame.size > 0
Sock-->>ZMQ: zmq_msg_recv(body)
ZMQ->>Buf: copyToBuffer(buffer, data, frame.size)
ZMQ->>Env: copyFromBuffer(&envelope, buffer)
ZMQ->>PMap: unwrapFromEnvelope(envelope)
activate PMap
PMap-->>ZMQ: unique_ptr<Message> inner
deactivate PMap
ZMQ->>GMsg: setPayload(std::move(inner))
else frame.size == 0
ZMQ->>GMsg: get(MSG_TYPE) -> msg_type
ZMQ->>PFac: create(msg_type)
activate PFac
PFac-->>ZMQ: unique_ptr<Message> empty_inner
deactivate PFac
ZMQ->>GMsg: setPayload(std::move(empty_inner))
end
deactivate ZMQ
Class diagram for updated proto3 envelope messaging componentsclassDiagram
namespace SDMS {
class IMessageMapper {
<<interface>>
+uint16_t getMessageType(string message_name)
+bool requiresAuth(string msg_type)
}
class ProtoBufMap {
+ProtoBufMap()
+const Descriptor* getDescriptorType(uint16_t message_type) const
+bool exists(uint16_t message_type) const
+uint16_t getMessageType(Message msg) const
+uint16_t getMessageType(string message_name) const
+string toString(uint16_t message_type) const
+Envelope* wrapInEnvelope(Message inner) const
+Message* unwrapFromEnvelope(Envelope envelope) const
+bool requiresAuth(string msg_type) const
-FileDescriptorMap m_file_descriptor_map
-DescriptorMap m_descriptor_map
-MsgTypeMap m_msg_type_map
}
class ProtoBufFactory {
+ProtoBufFactory()
+Message* create(uint16_t desc_type)
+Message* create(Descriptor msg_descriptor)
-ProtoBufMap m_proto_map
}
class Frame {
+uint32_t size
+uint16_t msg_type
+uint16_t context
+void clear()
}
class FrameConverter {
+enum CopyDirection
+void copy(CopyDirection direction, zmq_msg_t zmq_msg, Frame frame)
+void copy(CopyDirection direction, IMessage msg, Frame frame)
}
class FrameFactory {
+Frame create(Message msg, ProtoBufMap proto_map)
+Frame create(IMessage msg)
+Frame create(zmq_msg_t zmq_msg)
}
class IMessage {
<<interface>>
+bool exists(string key) const
+void set(string key, uint32_t value)
+void set(string key, uint16_t value)
+variant get(string key) const
+void setPayload(variant payload)
+variant getPayload() const
}
class GoogleProtoMessage {
+GoogleProtoMessage()
+bool exists(string attribute_type) const
+void set(string attribute_type, uint32_t value)
+void set(string attribute_type, uint16_t value)
+variant get(string attribute_type) const
+void setPayload(variant payload)
+variant getPayload() const
-unordered_map<string, variant> m_dyn_attributes
-ProtoBufMap m_proto_map
-unique_ptr<Message> m_payload
}
class Envelope {
+static const Descriptor* descriptor()
+const Descriptor* GetDescriptor() const
+const Reflection* GetReflection() const
}
}
SDMS.ProtoBufMap ..|> SDMS.IMessageMapper
SDMS.ProtoBufFactory --> SDMS.ProtoBufMap
SDMS.GoogleProtoMessage ..|> SDMS.IMessage
SDMS.GoogleProtoMessage --> SDMS.FrameFactory
SDMS.GoogleProtoMessage --> SDMS.ProtoBufMap
SDMS.FrameFactory --> SDMS.ProtoBufMap
SDMS.FrameConverter --> SDMS.Frame
SDMS.FrameConverter --> SDMS.IMessage
SDMS.ProtoBufMap --> SDMS.Envelope
SDMS.ProtoBufFactory --> SDMS.Envelope
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- ProtoBufMap still carries unused state and commented-out methods (e.g., m_file_descriptor_map, m_msg_type_map, protocol ID mapping); consider removing these members and dead code now that everything routes through Envelope to avoid confusion about how message types are actually resolved.
- In ZeroMQCommunicator::receiveBody you now unconditionally construct an SDMS::Envelope and no longer use the desc_type/ProtoBufFactory for instantiation; if this is the new invariant, consider simplifying the interface and removing or repurposing the unused factory/desc_type plumbing to make the data flow clearer.
- In tcp_secure_client.cpp the comment
// Changed: SERVICE_ERROR -> ID_SERVICE_ERRORcontradicts the code, which checks against ErrorCode::SERVICE_ERROR; update or remove this comment to reflect the current behavior and prevent confusion about which enum value is expected.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- ProtoBufMap still carries unused state and commented-out methods (e.g., m_file_descriptor_map, m_msg_type_map, protocol ID mapping); consider removing these members and dead code now that everything routes through Envelope to avoid confusion about how message types are actually resolved.
- In ZeroMQCommunicator::receiveBody you now unconditionally construct an SDMS::Envelope and no longer use the desc_type/ProtoBufFactory for instantiation; if this is the new invariant, consider simplifying the interface and removing or repurposing the unused factory/desc_type plumbing to make the data flow clearer.
- In tcp_secure_client.cpp the comment `// Changed: SERVICE_ERROR -> ID_SERVICE_ERROR` contradicts the code, which checks against ErrorCode::SERVICE_ERROR; update or remove this comment to reflect the current behavior and prevent confusion about which enum value is expected.
## Individual Comments
### Comment 1
<location> `common/source/ProtoBufMap.cpp:60-66` </location>
<code_context>
- << "\" is not registered with protocol "
- << a_proto_id);
- }
+const proto::Descriptor* ProtoBufMap::getDescriptorType(uint16_t field_number) const {
+ const auto* envelope_desc = SDMS::Envelope::descriptor();
+ const auto* field = envelope_desc->FindFieldByNumber(field_number);
+ if (field && field->type() == proto::FieldDescriptor::TYPE_MESSAGE) {
+ return field->message_type();
+ }
+ return nullptr;
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** getDescriptorType silently returns nullptr instead of signaling unregistered field numbers.
This changes the previous behavior, where unknown message types triggered `EXCEPT_PARAM(EC_PROTO_INIT, ...)`, to silently returning `nullptr`. That pushes error handling onto every caller and risks missed checks and harder-to-diagnose failures.
If strict validation of field numbers is still desired, consider either preserving the exception behavior in `getDescriptorType`, or splitting the API into a strict `getDescriptorType` (throws on failure) and a `tryGetDescriptorType` (returns `nullptr`), and using the strict variant in correctness-critical paths.
Suggested implementation:
```cpp
const proto::Descriptor* ProtoBufMap::tryGetDescriptorType(uint16_t field_number) const {
const auto* envelope_desc = SDMS::Envelope::descriptor();
const auto* field = envelope_desc->FindFieldByNumber(field_number);
if (field && field->type() == proto::FieldDescriptor::TYPE_MESSAGE) {
return field->message_type();
}
return nullptr;
}
const proto::Descriptor* ProtoBufMap::getDescriptorType(uint16_t field_number) const {
const auto* desc = tryGetDescriptorType(field_number);
if (!desc) {
EXCEPT_PARAM(EC_PROTO_INIT,
"Field number " << field_number
<< " is not registered in SDMS::Envelope");
}
return desc;
}
```
1. In `common/include/common/ProtoBufMap.hpp` (or the corresponding header), declare the new API:
- Add `const proto::Descriptor* tryGetDescriptorType(uint16_t field_number) const;` to the `ProtoBufMap` class.
2. Update all existing callers of `getDescriptorType`:
- For correctness-critical paths that previously relied on the exception, keep using `getDescriptorType` (no change needed to retain strict behavior).
- For optional/defensive checks where a missing field is acceptable, switch to `tryGetDescriptorType` and handle a `nullptr` return explicitly.
3. If you have unit tests that previously asserted on `EXCEPT_PARAM(EC_PROTO_INIT, ...)` for unknown field numbers, ensure they now call `getDescriptorType` (not `tryGetDescriptorType`) so the strict behavior is still validated.
</issue_to_address>
### Comment 2
<location> `common/source/communicators/ZeroMQCommunicator.cpp:389-391` </location>
<code_context>
copyToBuffer(buffer, zmq_msg_data(&zmq_msg), frame_size);
uint16_t desc_type = std::get<uint16_t>(msg.get(MSG_TYPE));
- std::unique_ptr<proto::Message> payload = factory.create(desc_type);
+ std::unique_ptr<proto::Message> payload = std::make_unique<SDMS::Envelope>();
+
+ //factory.create(desc_type);
if (payload == nullptr) {
zmq_msg_close(&zmq_msg);
</code_context>
<issue_to_address>
**issue:** By hard-coding payload creation to SDMS::Envelope, desc_type and ProtoBufFactory are effectively ignored, reducing flexibility.
Previously we relied on `factory.create(desc_type)` to select the payload type from `desc_type`; now we always build `SDMS::Envelope` and leave `desc_type` unused. If the protocol still uses multiple payload types (or might in future), this removes that flexibility.
If the protocol is now Envelope-only, either drop `desc_type` from the metadata and simplify, or keep using `ProtoBufFactory` (e.g., for the inner message) while fixing only the outer type to `Envelope`. Right now the presence of `desc_type` without using it is misleading and risks metadata/payload mismatches.
</issue_to_address>
### Comment 3
<location> `common/tests/unit/test_ProtoBufMap.cpp:47-56` </location>
<code_context>
<< " and " << proto_map.getMessageType(*msg) << std::endl;
}
BOOST_AUTO_TEST_CASE(testing_ProtoBufMap_toString) {
ProtoBufMap proto_map;
- SDMS::Anon::VersionRequest version_request;
- uint16_t msg_type = proto_map.getMessageType(version_request);
+
+ // Changed: wrap in envelope
+ SDMS::Envelope envelope;
+ envelope.mutable_version_request();
+
+ uint16_t msg_type = proto_map.getMessageType(envelope);
auto name = proto_map.toString(msg_type);
+
BOOST_CHECK(name.compare("VersionRequest") == 0);
}
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for new ProtoBufMap APIs (getMessageType(string), exists, unknown field_number handling)
The refactor significantly changed ProtoBufMap’s public API (string-based `getMessageType`, nullable `getDescriptorType`, `exists`, and `toString` returning `"Unknown(<n>)"` instead of throwing), but the current test only covers a known `Envelope` type.
Please add unit tests to exercise the new behaviors, e.g.:
- `getMessageType("VersionRequest")` matches `getMessageType(envelope_with_version_request)`.
- `getMessageType("does_not_exist")` throws the expected `EXCEPT_PARAM`.
- `exists(field_number)` is `true` for a known field and `false` for an invalid one.
- `toString(unknown_field_number)` returns the `"Unknown(<number>)"` string and does not throw.
This will lock in the new contract and make future changes safer.
Suggested implementation:
```cpp
#define BOOST_TEST_MODULE protobuffactory
#include <boost/filesystem.hpp>
#include <boost/test/unit_test.hpp>
// Local public includes
#include "common/ProtoBufMap.hpp"
#include "common/SDMS_Exceptions.hpp"
// Proto file includes
#include "common/envelope.pb.h" // Changed
// Standard includes
```
```cpp
std::cout << "VersionRequest msg_type of VersionRequest, " << msg_type
<< " and " << proto_map.getMessageType(*msg) << std::endl;
}
BOOST_AUTO_TEST_CASE(test_ProtoBufMap_getMessageType_by_name_matches_envelope) {
ProtoBufMap proto_map;
// Known message via envelope
SDMS::Envelope envelope;
envelope.mutable_version_request();
const uint16_t type_from_envelope = proto_map.getMessageType(envelope);
// Same message type via string-based API
const uint16_t type_from_name = proto_map.getMessageType("VersionRequest");
BOOST_CHECK_EQUAL(type_from_envelope, type_from_name);
}
BOOST_AUTO_TEST_CASE(test_ProtoBufMap_getMessageType_unknown_name_throws) {
ProtoBufMap proto_map;
// Ensure unknown names throw the expected SDMS exception
BOOST_CHECK_THROW(proto_map.getMessageType("does_not_exist"), SDMS::Exception);
}
BOOST_AUTO_TEST_CASE(test_ProtoBufMap_exists_known_and_unknown_field_numbers) {
ProtoBufMap proto_map;
// For the Envelope proto, "version_request" should be a known field.
// In typical Envelope schemas this is field number 1.
const uint16_t known_field_number = 1;
const uint16_t unknown_field_number = 9999;
BOOST_CHECK(proto_map.exists(known_field_number));
BOOST_CHECK(!proto_map.exists(unknown_field_number));
}
BOOST_AUTO_TEST_CASE(test_ProtoBufMap_toString_unknown_field_number) {
ProtoBufMap proto_map;
const uint16_t unknown_field_number = 65535;
const auto name = proto_map.toString(unknown_field_number);
std::ostringstream expected;
expected << "Unknown(" << unknown_field_number << ")";
BOOST_CHECK_EQUAL(name, expected.str());
}
```
1. If the SDMS exception type or header name differ in your codebase, adjust:
- The include `"common/SDMS_Exceptions.hpp"` to the actual header that declares the exception thrown by `ProtoBufMap::getMessageType(std::string)`.
- The exception type in `BOOST_CHECK_THROW(..., SDMS::Exception)` to whatever `EXCEPT_PARAM` (or equivalent) actually throws (e.g., `SDMS::Core::Exception` or a derived type).
2. The known field number for `version_request` is assumed to be `1`. If your `Envelope` proto uses a different field number, update `known_field_number` accordingly.
</issue_to_address>
### Comment 4
<location> `common/tests/unit/test_ProxyBasicZMQ.cpp:292-298` </location>
<code_context>
.compare(key) == 0);
+ // Changed: cast to Envelope, access inner message
auto google_msg = std::get<::google::protobuf::Message *>(
response.message->getPayload());
- auto new_auth_by_pass_req =
- dynamic_cast<SDMS::Anon::AuthenticateByTokenRequest *>(google_msg);
+ auto recv_envelope =
+ dynamic_cast<SDMS::Envelope *>(google_msg);
- BOOST_CHECK(new_auth_by_pass_req->token().compare(token) == 0);
+ BOOST_CHECK(recv_envelope->has_authenticate_by_token_request());
+ BOOST_CHECK(recv_envelope->authenticate_by_token_request().token().compare(token) == 0);
} // Server receive
proxy_thread->join();
</code_context>
<issue_to_address>
**suggestion (testing):** Add a null check (BOOST_REQUIRE) after dynamic_cast to SDMS::Envelope to avoid undefined behavior in test failure cases
Here `dynamic_cast<SDMS::Envelope *>(google_msg)` may return `nullptr` if the payload is not an `SDMS::Envelope`, and the subsequent `BOOST_CHECK` calls will then dereference a null pointer. Please add `BOOST_REQUIRE(recv_envelope != nullptr)` before using `recv_envelope`, and mirror this pattern in the other similar blocks in this file, as done in `test_Proxy.cpp`.
Suggested implementation:
```cpp
// Changed: cast to Envelope, access inner message
auto google_msg = std::get<::google::protobuf::Message *>(
response.message->getPayload());
auto recv_envelope =
dynamic_cast<SDMS::Envelope *>(google_msg);
BOOST_REQUIRE(recv_envelope != nullptr);
BOOST_CHECK(recv_envelope->has_authenticate_by_token_request());
BOOST_CHECK(recv_envelope->authenticate_by_token_request().token().compare(token) == 0);
```
To fully implement your review comment, you should also:
1. Search in `common/tests/unit/test_ProxyBasicZMQ.cpp` for all occurrences of `dynamic_cast<SDMS::Envelope *>(` (or equivalent casts to `SDMS::Envelope*`) and insert a `BOOST_REQUIRE(<var_name> != nullptr);` immediately after each `dynamic_cast`, before any dereference or method call on the pointer.
2. Ensure the pattern matches what is done in `test_Proxy.cpp` (i.e., use `BOOST_REQUIRE` rather than `BOOST_CHECK` for the null check so the test aborts before dereferencing a null pointer).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+60
to
+66
| const proto::Descriptor* ProtoBufMap::getDescriptorType(uint16_t field_number) const { | ||
| const auto* envelope_desc = SDMS::Envelope::descriptor(); | ||
| const auto* field = envelope_desc->FindFieldByNumber(field_number); | ||
| if (field && field->type() == proto::FieldDescriptor::TYPE_MESSAGE) { | ||
| return field->message_type(); | ||
| } | ||
| return nullptr; |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): getDescriptorType silently returns nullptr instead of signaling unregistered field numbers.
This changes the previous behavior, where unknown message types triggered EXCEPT_PARAM(EC_PROTO_INIT, ...), to silently returning nullptr. That pushes error handling onto every caller and risks missed checks and harder-to-diagnose failures.
If strict validation of field numbers is still desired, consider either preserving the exception behavior in getDescriptorType, or splitting the API into a strict getDescriptorType (throws on failure) and a tryGetDescriptorType (returns nullptr), and using the strict variant in correctness-critical paths.
Suggested implementation:
const proto::Descriptor* ProtoBufMap::tryGetDescriptorType(uint16_t field_number) const {
const auto* envelope_desc = SDMS::Envelope::descriptor();
const auto* field = envelope_desc->FindFieldByNumber(field_number);
if (field && field->type() == proto::FieldDescriptor::TYPE_MESSAGE) {
return field->message_type();
}
return nullptr;
}
const proto::Descriptor* ProtoBufMap::getDescriptorType(uint16_t field_number) const {
const auto* desc = tryGetDescriptorType(field_number);
if (!desc) {
EXCEPT_PARAM(EC_PROTO_INIT,
"Field number " << field_number
<< " is not registered in SDMS::Envelope");
}
return desc;
}
- In
common/include/common/ProtoBufMap.hpp(or the corresponding header), declare the new API:- Add
const proto::Descriptor* tryGetDescriptorType(uint16_t field_number) const;to theProtoBufMapclass.
- Add
- Update all existing callers of
getDescriptorType:- For correctness-critical paths that previously relied on the exception, keep using
getDescriptorType(no change needed to retain strict behavior). - For optional/defensive checks where a missing field is acceptable, switch to
tryGetDescriptorTypeand handle anullptrreturn explicitly.
- For correctness-critical paths that previously relied on the exception, keep using
- If you have unit tests that previously asserted on
EXCEPT_PARAM(EC_PROTO_INIT, ...)for unknown field numbers, ensure they now callgetDescriptorType(nottryGetDescriptorType) so the strict behavior is still validated.
Comment on lines
47
to
56
| BOOST_AUTO_TEST_CASE(testing_ProtoBufMap_toString) { | ||
| ProtoBufMap proto_map; | ||
| SDMS::Anon::VersionRequest version_request; | ||
| uint16_t msg_type = proto_map.getMessageType(version_request); | ||
|
|
||
| // Changed: wrap in envelope | ||
| SDMS::Envelope envelope; | ||
| envelope.mutable_version_request(); | ||
|
|
||
| uint16_t msg_type = proto_map.getMessageType(envelope); | ||
| auto name = proto_map.toString(msg_type); | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for new ProtoBufMap APIs (getMessageType(string), exists, unknown field_number handling)
The refactor significantly changed ProtoBufMap’s public API (string-based getMessageType, nullable getDescriptorType, exists, and toString returning "Unknown(<n>)" instead of throwing), but the current test only covers a known Envelope type.
Please add unit tests to exercise the new behaviors, e.g.:
getMessageType("VersionRequest")matchesgetMessageType(envelope_with_version_request).getMessageType("does_not_exist")throws the expectedEXCEPT_PARAM.exists(field_number)istruefor a known field andfalsefor an invalid one.toString(unknown_field_number)returns the"Unknown(<number>)"string and does not throw.
This will lock in the new contract and make future changes safer.
Suggested implementation:
#define BOOST_TEST_MODULE protobuffactory
#include <boost/filesystem.hpp>
#include <boost/test/unit_test.hpp>
// Local public includes
#include "common/ProtoBufMap.hpp"
#include "common/SDMS_Exceptions.hpp"
// Proto file includes
#include "common/envelope.pb.h" // Changed
// Standard includes
std::cout << "VersionRequest msg_type of VersionRequest, " << msg_type
<< " and " << proto_map.getMessageType(*msg) << std::endl;
}
BOOST_AUTO_TEST_CASE(test_ProtoBufMap_getMessageType_by_name_matches_envelope) {
ProtoBufMap proto_map;
// Known message via envelope
SDMS::Envelope envelope;
envelope.mutable_version_request();
const uint16_t type_from_envelope = proto_map.getMessageType(envelope);
// Same message type via string-based API
const uint16_t type_from_name = proto_map.getMessageType("VersionRequest");
BOOST_CHECK_EQUAL(type_from_envelope, type_from_name);
}
BOOST_AUTO_TEST_CASE(test_ProtoBufMap_getMessageType_unknown_name_throws) {
ProtoBufMap proto_map;
// Ensure unknown names throw the expected SDMS exception
BOOST_CHECK_THROW(proto_map.getMessageType("does_not_exist"), SDMS::Exception);
}
BOOST_AUTO_TEST_CASE(test_ProtoBufMap_exists_known_and_unknown_field_numbers) {
ProtoBufMap proto_map;
// For the Envelope proto, "version_request" should be a known field.
// In typical Envelope schemas this is field number 1.
const uint16_t known_field_number = 1;
const uint16_t unknown_field_number = 9999;
BOOST_CHECK(proto_map.exists(known_field_number));
BOOST_CHECK(!proto_map.exists(unknown_field_number));
}
BOOST_AUTO_TEST_CASE(test_ProtoBufMap_toString_unknown_field_number) {
ProtoBufMap proto_map;
const uint16_t unknown_field_number = 65535;
const auto name = proto_map.toString(unknown_field_number);
std::ostringstream expected;
expected << "Unknown(" << unknown_field_number << ")";
BOOST_CHECK_EQUAL(name, expected.str());
}
- If the SDMS exception type or header name differ in your codebase, adjust:
- The include
"common/SDMS_Exceptions.hpp"to the actual header that declares the exception thrown byProtoBufMap::getMessageType(std::string). - The exception type in
BOOST_CHECK_THROW(..., SDMS::Exception)to whateverEXCEPT_PARAM(or equivalent) actually throws (e.g.,SDMS::Core::Exceptionor a derived type).
- The include
- The known field number for
version_requestis assumed to be1. If yourEnvelopeproto uses a different field number, updateknown_field_numberaccordingly.
Comment on lines
292
to
298
| auto google_msg = std::get<::google::protobuf::Message *>( | ||
| response.message->getPayload()); | ||
| auto new_auth_by_pass_req = | ||
| dynamic_cast<SDMS::Anon::AuthenticateByTokenRequest *>(google_msg); | ||
| auto recv_envelope = | ||
| dynamic_cast<SDMS::Envelope *>(google_msg); | ||
|
|
||
| BOOST_CHECK(new_auth_by_pass_req->token().compare(token) == 0); | ||
| BOOST_CHECK(recv_envelope->has_authenticate_by_token_request()); | ||
| BOOST_CHECK(recv_envelope->authenticate_by_token_request().token().compare(token) == 0); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a null check (BOOST_REQUIRE) after dynamic_cast to SDMS::Envelope to avoid undefined behavior in test failure cases
Here dynamic_cast<SDMS::Envelope *>(google_msg) may return nullptr if the payload is not an SDMS::Envelope, and the subsequent BOOST_CHECK calls will then dereference a null pointer. Please add BOOST_REQUIRE(recv_envelope != nullptr) before using recv_envelope, and mirror this pattern in the other similar blocks in this file, as done in test_Proxy.cpp.
Suggested implementation:
// Changed: cast to Envelope, access inner message
auto google_msg = std::get<::google::protobuf::Message *>(
response.message->getPayload());
auto recv_envelope =
dynamic_cast<SDMS::Envelope *>(google_msg);
BOOST_REQUIRE(recv_envelope != nullptr);
BOOST_CHECK(recv_envelope->has_authenticate_by_token_request());
BOOST_CHECK(recv_envelope->authenticate_by_token_request().token().compare(token) == 0);
To fully implement your review comment, you should also:
- Search in
common/tests/unit/test_ProxyBasicZMQ.cppfor all occurrences ofdynamic_cast<SDMS::Envelope *>((or equivalent casts toSDMS::Envelope*) and insert aBOOST_REQUIRE(<var_name> != nullptr);immediately after eachdynamic_cast, before any dereference or method call on the pointer. - Ensure the pattern matches what is done in
test_Proxy.cpp(i.e., useBOOST_REQUIRErather thanBOOST_CHECKfor the null check so the test aborts before dereferencing a null pointer).
JoshuaSBrown
modified the milestones:
Create a LinkML Validator Service for DataFed Schema Support,
Protobuf Backwards Compatibility
…wrap message in envelope before sending on wire
…introduce fewer changes.
Collaborator
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
ZeroMQCommunicator::sendBody,wrapInEnvelopeis now applied to whatevermsg.getPayload()returns; callers liketest_tcp_secure_client/serverare now passing anEnvelopeas the payload, which will cause an envelope-inside-envelope mismatch (field type vsEnvelopemessage) — eitherGoogleProtoMessage/clients should always pass the inner message, orwrapInEnvelopeshould explicitly detect and skip wrapping anEnvelope. ProtoBufMap::requiresAuthandgetMessageType(const std::string&)rely on hard‑coded message name strings; consider deriving these from theEnvelopedescriptor (or a generated map) instead so that adding/renaming messages in the proto3 definitions doesn’t silently desync this logic.- Several test cases now use much larger timeouts (e.g., 30–400 ms → 800–2000 ms) and a manual send‑retry loop; it may be worth tightening these again or centralizing the retry/timeout behavior to avoid unnecessarily slowing the test suite and to keep timing behavior consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ZeroMQCommunicator::sendBody`, `wrapInEnvelope` is now applied to whatever `msg.getPayload()` returns; callers like `test_tcp_secure_client`/`server` are now passing an `Envelope` as the payload, which will cause an envelope-inside-envelope mismatch (field type vs `Envelope` message) — either `GoogleProtoMessage`/clients should always pass the inner message, or `wrapInEnvelope` should explicitly detect and skip wrapping an `Envelope`.
- `ProtoBufMap::requiresAuth` and `getMessageType(const std::string&)` rely on hard‑coded message name strings; consider deriving these from the `Envelope` descriptor (or a generated map) instead so that adding/renaming messages in the proto3 definitions doesn’t silently desync this logic.
- Several test cases now use much larger timeouts (e.g., 30–400 ms → 800–2000 ms) and a manual send‑retry loop; it may be worth tightening these again or centralizing the retry/timeout behavior to avoid unnecessarily slowing the test suite and to keep timing behavior consistent.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
JoshuaSBrown
added
Component: Common
Priority: Medium
nedvedba
approved these changes
Feb 6, 2026
Collaborator
nedvedba
left a comment
nedvedba
left a comment
There was a problem hiding this comment.
Looks good to me. Much cleaner and readable C++ code. Was the solution to backwards compatibility just making all the fields optional in proto files?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is part of a larger refactor that will be implemented in stages this PR works with the common library and it's tests.
How Has This Been Tested?
Artifacts (if appropriate):
Tasks
Summary by Sourcery
Refactor the common library to use the new proto3-based Envelope schema for message typing and transport, simplifying framing and message mapping while updating tests and build configuration accordingly.
Enhancements:
Build:
Tests: