UMessage: validator and unit test #158
Conversation
|
@gregmedd Sorry I didn't see your message until this morning ... closed my browser. |
billpittman
force-pushed
the
feature/validator/umessage
branch
from
f1051c1
to
41237f1
Compare
Signed-off-by: Bill Pittman <[email protected]>
billpittman
force-pushed
the
feature/validator/umessage
branch
from
41237f1
to
374b2d9
Compare
billpittman
changed the title
There was a problem hiding this comment.
There are definitely some minor issues that could use clean-up in this PR. However, I don't know that any of the comments I'm adding are severe enough to block this change right now. I am considering capturing the feedback as issues and merging this code to unblock downstream work (e.g. up-transport-zenoh-cpp).
| return "The TTL, if present, indicates the ID has expired"; | ||
| case Reason::PRIORITY_OUT_OF_RANGE: | ||
| return "The Priority, if set, is not within the allowable range"; |
There was a problem hiding this comment.
Wouldn't these errors only be returned when the field is present?
| return "The TTL, if present, indicates the ID has expired"; | |
| case Reason::PRIORITY_OUT_OF_RANGE: | |
| return "The Priority, if set, is not within the allowable range"; | |
| return "The TTL indicates the ID has expired"; | |
| case Reason::PRIORITY_OUT_OF_RANGE: | |
| return "The Priority is not within the allowable range"; |
| { | ||
| auto [valid, reason] = isValidRpcRequest(umessage); | ||
| if (valid) { | ||
| return {true, std::nullopt}; |
There was a problem hiding this comment.
Minor: I'm pretty sure you can just do return {true, {}}, but I haven't tried it. It's not a big deal either way.
| if (!UPriority_IsValid(umessage.attributes().priority())) { | ||
| return {false, Reason::PRIORITY_OUT_OF_RANGE}; | ||
| } | ||
|
|
||
| if (!UPayloadFormat_IsValid(umessage.attributes().payload_format())) { | ||
| return {false, Reason::PAYLOAD_FORMAT_OUT_OF_RANGE}; | ||
| } |
There was a problem hiding this comment.
Thanks for suggestion. I have now used this in #161
| if (!response.attributes().has_reqid()) { | ||
| return {false, Reason::REQID_MISMATCH}; | ||
| } |
There was a problem hiding this comment.
Is this check necessary? I think isValidRpcResponse() already checks it.
| if (request.attributes().priority() != response.attributes().priority()) { | ||
| return {false, Reason::PRIORITY_MISMATCH}; | ||
| } | ||
|
|
||
| return {true, std::nullopt}; |
There was a problem hiding this comment.
It just occurred to me that there should be two more checks: The source of the request should match the sink of the response and the sink of the request should match the source of the request. A URI_MISMATCH reason could be added for this scenario.
| attributes.set_allocated_id(new UUID(id)); | ||
| attributes.set_allocated_source(new UUri(source)); | ||
| attributes.set_allocated_sink(new UUri(sink)); |
There was a problem hiding this comment.
Don't use set_allocated_ or new
| void SetUp() override { | ||
| source_.set_authority_name("10.0.0.1"); | ||
| source_.set_ue_id(0x00010001); | ||
| source_.set_ue_version_major(1); | ||
| source_.set_resource_id(1); | ||
|
|
||
| sink_.set_authority_name("10.0.0.2"); | ||
| sink_.set_ue_id(0x00010002); | ||
| sink_.set_ue_version_major(2); | ||
| sink_.set_resource_id(2); | ||
|
|
||
| reqId_.set_msb(0x1234); | ||
| reqId_.set_lsb(0x5678); | ||
| } |
There was a problem hiding this comment.
If these don't change, do they need to be initialized for each individual test?
| uint64_t timestamp = | ||
| std::chrono::duration_cast<std::chrono::milliseconds>( | ||
| std::chrono::system_clock::now().time_since_epoch()) | ||
| .count(); | ||
|
|
||
| id.set_msb((timestamp << 16) | (8ULL << 12) | | ||
| (0x123ULL)); // version 8 ; counter = 0x123 | ||
| id.set_lsb((2ULL << 62) | (0xFFFFFFFFFFFFULL)); // variant 10 |
There was a problem hiding this comment.
Could you just use the UUID builder here?
|
None of the changes I requested would prevent us from moving forward. I'll file an immediate follow-up PR to address these changes since Bill is not currently available. |
UMessage: Validator and unit test.