Adding MQTTv5 Properties (Publish) #2613
Conversation
RLeclair
commented
Jul 21, 2023
•
RLeclair
commented
- Adding MQTTv5 properties on publish. Current implementation works with Mosquitto.
- Renamed structs, functions, and variables to mqtt5.
- Added unit tests testing all relevant properties, tested against local broker.
- Excludes pre-existing files with low coverage.
RLeclair
requested review from
CIPop,
ericwolz,
ewertons,
jspaith,
vaavva,
ahsonkhan,
antkmsft,
RickWinter,
vhvb1989,
gearama and
LarryOsterman
as code owners
| * and they are subject to change in future versions of the SDK which would break your code. | ||
| */ | ||
|
|
||
| #ifndef _az_MQTT5_H |
There was a problem hiding this comment.
General about functions in this file: API documentation should describe both:
- usage of the API (within our SDK).
- instructions for developers that want to port the interface to another platform.
sdk/inc/azure/core/az_mqtt5.h
Outdated
| * | ||
| * @return The string value of the property. | ||
| */ | ||
| AZ_NODISCARD AZ_INLINE az_span az_mqtt5_property_string_get(az_mqtt5_property_string* prop_str) |
There was a problem hiding this comment.
If we've got a getter for this, should str be internal?
There was a problem hiding this comment.
The type az_mqtt5_property_string should be different for different MQTT implementations.
E.g. for ESP-MQTT it won't need two az_spans: instead, it will call into esp_mqtt5_client_get_user_property (example).
The struct in that case would need to hold on to the key/value pointers in order to allow freeing them later.
There was a problem hiding this comment.
Removed struct declaration for all az_mqtt5_property_*. They are now declared inside az_mqtt5_[platform implementation]. Removed inline attribute of the getter functions and they are also now declared in az_mqtt5_[platform implementation].
sdk/inc/azure/core/az_mqtt5.h
Outdated
| * @brief The string value of the property. | ||
| */ | ||
| az_span str; | ||
| } az_mqtt5_property_string; |
There was a problem hiding this comment.
why does this need to be a custom type instead of just an az_span (similar to the byte and int properties that don't have a special type)
There was a problem hiding this comment.
It is because we want the properties to be strongly typed, it will help distinguish between az_mqtt5_property_string and az_mqtt5_property_binarydata which are both az_spans but not the same from a MQTT perspective.
There was a problem hiding this comment.
It's actually because in some MQTT implementations these need to be freed and the primitive types do not.
In some cases freeing is done by libc/free(void*) but in many cases it is not somemqttstack_destroy(somemqttstack_string_type).
| { | ||
| (void)mqtt5; | ||
| property_bag->_internal.options | ||
| = options == NULL ? az_mqtt5_property_bag_options_default() : *options; |
There was a problem hiding this comment.
Should we even allow setting options for mosquitto if we aren't going to do anything with them?
There was a problem hiding this comment.
We're keeping the pattern of initializing with options which involves setting, though for a mosquitto specific case it would make sense to just allow options to be NULL. @CIPop thoughts? The above line is now:
property_bag->properties = options == NULL ? NULL : options->properties;
| cmocka_unit_test(test_az_mqtt5_policy_init_valid_success), | ||
| cmocka_unit_test(test_az_mqtt5_policy_outbound_connect_success), | ||
| cmocka_unit_test(test_az_mqtt5_policy_outbound_sub_success), | ||
| cmocka_unit_test(test_az_mqtt5_policy_outbound_pub_properties_success), |
There was a problem hiding this comment.
do we need a test without properties too?
There was a problem hiding this comment.
Added test w/o properties!
|
Will address rest of comments in a follow-up PR. Merging to unblock other work. |
| @@ -369,6 +379,17 @@ AZ_NODISCARD az_mqtt5_property_bag_options az_mqtt5_property_bag_options_default | |||
| }; | |||
| } | |||
|
|
|||
| AZ_NODISCARD az_result az_mqtt5_property_bag_empty(az_mqtt5_property_bag* property_bag) | |||
| { | |||
| _az_PRECONDITION_NOT_NULL(property_bag); | |||
There was a problem hiding this comment.
Do we want to fail if the property bag is null, or just not free?
There was a problem hiding this comment.
This is checking that the pointer to the property_bag struct is not null, in the case of mosquitto if we see that the internal pointer is null we know that it has already been freed and we should not do anything
There was a problem hiding this comment.
using _az_PRECONDITION_NOT_NULL will by default cause the entire program to hang instead of just skipping the free though.
There was a problem hiding this comment.
We never set the property_bag variable to NULL so there is no issue there unless the user mistakenly sets it to NULL
| /** | ||
| * @brief MQTT 5 property string. | ||
| * | ||
| * @note Depending on the MQTT 5 stack implementation, string may need to be freed. |
There was a problem hiding this comment.
Since we would be calling these in the hfsm, should the comment indicate that they always should be free'd or that the free method can always be safely called since some mqtt implementations do require it? (similar to what you say in line 313)
There was a problem hiding this comment.
Changed comment to "should always be freed". The method for freeing should always be safe.
| * | ||
| * @param[in] prop_str The MQTT 5 property string. | ||
| * | ||
| * @note After getting the string value, the property string must be freed using |
There was a problem hiding this comment.
Isn't this free required after the read/find, not the get? At the point the user is calling get, the property has already been allocated and needs to be free'd regardless
There was a problem hiding this comment.
Moved the comment to the read functions
| /** | ||
| * @brief MQTT 5 property binary data. | ||
| * | ||
| * @note Depending on the MQTT 5 stack implementation, string pair data may need to be freed. |
There was a problem hiding this comment.
| * @note Depending on the MQTT 5 stack implementation, string pair data may need to be freed. | |
| * @note Depending on the MQTT 5 stack implementation, binary data may need to be freed. |
| AZ_MQTT5_PROPERTY_TYPE_USER_PROPERTY = 38, | ||
| } az_mqtt5_property_type; | ||
|
|
||
| // Porting 3. The following functions must be implemented and will be called by the SDK to interact |
There was a problem hiding this comment.
It follows part 2 of the porting instructions in az_mqtt5.h, I added some clarification `// Porting az_mqtt5 ..."
| * @brief This header defines the MQTT 5 property bag and its related methods. The following | ||
| * structures and API are used by the SDK when sending and receiving MQTT5 properties. | ||
| * | ||
| * @note A property bag struct contains an internal property bag options struct. These options |
There was a problem hiding this comment.
nit: not internal anymore/not contained in the property bag(?)
There was a problem hiding this comment.
Removed, thanks for pointing that out!
