Merged
Merge Request #21
·
created by
fix/pgroen/topic length limitation
hasWildcard had an invalid check on the last character. When the topic size is 36 and we check the size>() -1 (35) it is interpreted as an '#' which in turn is a wildcard in the topic making sure it is not published.
We now check the actual character of the last entry in the string.
From
fix/pgroen/topic_length_limitation
into
master
-
mentioned in commit 078653bde8059dc973fca91f91f5c4bb660046ca
-
Status changed to merged
Showing
6 changed files
CMakeLists.txt
@@ -13,5 +13,7 @@ add_subdirectory(examples/pub) | @@ -13,5 +13,7 @@ add_subdirectory(examples/pub) | ||
13 | add_subdirectory(examples/sub) | 13 | add_subdirectory(examples/sub) |
14 | add_subdirectory(examples/subunsub) | 14 | add_subdirectory(examples/subunsub) |
15 | 15 | ||
16 | +add_subdirectory(test) | ||
17 | + | ||
16 | include(packaging) | 18 | include(packaging) |
17 | package_component() | 19 | package_component() |
src/CMakeLists.txt
@@ -13,8 +13,8 @@ include_directories( | @@ -13,8 +13,8 @@ include_directories( | ||
13 | ) | 13 | ) |
14 | 14 | ||
15 | set(SRC_LIST | 15 | set(SRC_LIST |
16 | - ${CMAKE_CURRENT_SOURCE_DIR}/log.cpp | ||
17 | - ${CMAKE_CURRENT_SOURCE_DIR}/threadcontext.cpp | 16 | + ${CMAKE_CURRENT_SOURCE_DIR}/log.cpp |
17 | + ${CMAKE_CURRENT_SOURCE_DIR}/threadcontext.cpp | ||
18 | ${CMAKE_CURRENT_SOURCE_DIR}/mqttpublisherbase.cpp | 18 | ${CMAKE_CURRENT_SOURCE_DIR}/mqttpublisherbase.cpp |
19 | ${CMAKE_CURRENT_SOURCE_DIR}/mqttsubscriberbase.cpp | 19 | ${CMAKE_CURRENT_SOURCE_DIR}/mqttsubscriberbase.cpp |
20 | ${CMAKE_CURRENT_SOURCE_DIR}/clientpaho.cpp | 20 | ${CMAKE_CURRENT_SOURCE_DIR}/clientpaho.cpp |
src/mqttclient.cpp
@@ -215,9 +215,17 @@ void MqttClient::disconnect() | @@ -215,9 +215,17 @@ void MqttClient::disconnect() | ||
215 | 215 | ||
216 | Token MqttClient::publish(const MqttMessage& message, int qos) | 216 | Token MqttClient::publish(const MqttMessage& message, int qos) |
217 | { | 217 | { |
218 | - if (hasWildcard(message.topic()) || !isValidTopic(message.topic())) { | 218 | + if (hasWildcard(message.topic())) |
219 | + { | ||
220 | + LogDebug("[MqttClient::publish]","Topic has wildcard : " + message.topic()); | ||
219 | return Token(m_clientId, -1); | 221 | return Token(m_clientId, -1); |
220 | } | 222 | } |
223 | + else if(!isValidTopic(message.topic())) | ||
224 | + { | ||
225 | + LogDebug("[MqttClient::publish]","Topic is invalid : " + message.topic()); | ||
226 | + return Token(m_clientId, -1); | ||
227 | + } | ||
228 | + | ||
221 | OSDEV_COMPONENTS_LOCKGUARD(m_interfaceMutex); | 229 | OSDEV_COMPONENTS_LOCKGUARD(m_interfaceMutex); |
222 | IMqttClientImpl* client(nullptr); | 230 | IMqttClientImpl* client(nullptr); |
223 | { | 231 | { |
@@ -239,6 +247,13 @@ Token MqttClient::publish(const MqttMessage& message, int qos) | @@ -239,6 +247,13 @@ Token MqttClient::publish(const MqttMessage& message, int qos) | ||
239 | } | 247 | } |
240 | client = m_principalClient.get(); | 248 | client = m_principalClient.get(); |
241 | } | 249 | } |
250 | + | ||
251 | + if(!client) | ||
252 | + { | ||
253 | + LogDebug("[MqttClient::publish]", "Invalid pointer to IMqttClient retrieved."); | ||
254 | + return Token(m_clientId, -1); | ||
255 | + } | ||
256 | + | ||
242 | return Token(client->clientId(), client->publish(message, qos)); | 257 | return Token(client->clientId(), client->publish(message, qos)); |
243 | } | 258 | } |
244 | 259 |
src/mqttutil.cpp
@@ -55,7 +55,12 @@ bool isValidTopic( const std::string &topic ) | @@ -55,7 +55,12 @@ bool isValidTopic( const std::string &topic ) | ||
55 | 55 | ||
56 | bool hasWildcard( const std::string &topic ) | 56 | bool hasWildcard( const std::string &topic ) |
57 | { | 57 | { |
58 | - return ( topic.size() > 0 && (topic.find( '+' ) != std::string::npos || topic.size() - 1 == '#' ) ); | 58 | + return ( topic.size() > 0 && |
59 | + ( | ||
60 | + topic.find( '+' ) != std::string::npos || | ||
61 | + topic.back() == '#' | ||
62 | + ) | ||
63 | + ); | ||
59 | } | 64 | } |
60 | 65 | ||
61 | bool testForOverlap( const std::string &existingTopic, const std::string &newTopic ) | 66 | bool testForOverlap( const std::string &existingTopic, const std::string &newTopic ) |
test/CMakeLists.txt
0 → 100644
1 | +#**************************************************************************** | ||
2 | +#* Copyright (c) 2023 Open Systems Development B.V. | ||
3 | +#***************************************************************************** | ||
4 | +# | ||
5 | +# Don't call this file directly from cmake. | ||
6 | +# This file is included from the upper directory. | ||
7 | +# | ||
8 | +# Build rules for the MQTT Library | ||
9 | + | ||
10 | +add_executable(topictest | ||
11 | + TopicLengthTest.cpp | ||
12 | +) | ||
13 | + | ||
14 | +target_include_directories(topictest PRIVATE | ||
15 | + ${CMAKE_CIRRENT_SOURECE_DIR} | ||
16 | + ../include | ||
17 | +) | ||
18 | + | ||
19 | +target_link_libraries(topictest PRIVATE | ||
20 | + gmock_main | ||
21 | + gmock | ||
22 | + gtest | ||
23 | + mqtt-cpp | ||
24 | +) | ||
25 | + | ||
26 | +add_test(NAME topictest COMMAND topictest) | ||
27 | + | ||
28 | +set_tests_properties(topictest PROPERTIES | ||
29 | + WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}" | ||
30 | +) |
test/TopicLengthTest.cpp
0 → 100644
1 | +/**************************************************************************** | ||
2 | + * COpyright (c) 2023 Open Systems Development B.V. | ||
3 | + ****************************************************************************/ | ||
4 | + | ||
5 | +#include <gmock/gmock.h> | ||
6 | +#include <gtest/gtest.h> | ||
7 | +#include <string> | ||
8 | +#include <memory> | ||
9 | + | ||
10 | +#include "mqttclient.h" | ||
11 | + | ||
12 | +using namespace osdev::components::mqtt; | ||
13 | +using namespace osdev::components::log; | ||
14 | + | ||
15 | +static const std::string main_topic = "test/"; | ||
16 | + | ||
17 | +/**************************************************************************** | ||
18 | + * H E L P E R C L A S S E S | ||
19 | + ****************************************************************************/ | ||
20 | +class Publisher | ||
21 | +{ | ||
22 | +public: | ||
23 | + Publisher() : m_mqtt_client("TopicTester"){} | ||
24 | + virtual ~Publisher() {} | ||
25 | + | ||
26 | + void connect(const std::string &hostname, | ||
27 | + int portnumber = 1883, | ||
28 | + const std::string &username = std::string(), | ||
29 | + const std::string &password = std::string(), | ||
30 | + const std::string &lwt_topic = std::string(), | ||
31 | + const std::string &lwt_message = std::string() | ||
32 | + ) | ||
33 | + { | ||
34 | + m_mqtt_client.connect(hostname, portnumber, | ||
35 | + osdev::components::mqtt::Credentials(username, password), | ||
36 | + osdev::components::mqtt::mqtt_LWT(lwt_topic, lwt_message), | ||
37 | + true, | ||
38 | + osdev::components::log::LogSettings | ||
39 | + { | ||
40 | + osdev::components::log::LogLevel::Debug, | ||
41 | + osdev::components::log::LogMask::None | ||
42 | + }); | ||
43 | + } | ||
44 | + | ||
45 | + void publish(const std::string &message_topic, const std::string &message_payload) | ||
46 | + { | ||
47 | + osdev::components::mqtt::MqttMessage message(message_topic, true, false, message_payload); | ||
48 | + osdev::components::mqtt::Token t_result = m_mqtt_client.publish(message, 0); | ||
49 | + } | ||
50 | + | ||
51 | +private: | ||
52 | + osdev::components::mqtt::MqttClient m_mqtt_client; | ||
53 | +}; | ||
54 | + | ||
55 | +/// @brief class to generate a cumulative topic.. | ||
56 | +class TopicTester | ||
57 | +{ | ||
58 | + public: | ||
59 | + TopicTester(std::shared_ptr<Publisher> publisher) : m_publisher(publisher){} | ||
60 | + virtual ~TopicTester(){} | ||
61 | + | ||
62 | + void RunTopicTester(int max_number_of_chars) | ||
63 | + { | ||
64 | + for(int nCount = 1; nCount < max_number_of_chars; nCount++) | ||
65 | + { | ||
66 | + std::string subtopic(nCount, 'a'); | ||
67 | + std::string topic = std::string(main_topic + subtopic); | ||
68 | + std::string message(std::to_string(topic.size()) + " (" + std::to_string(subtopic.size()) + ")"); | ||
69 | + | ||
70 | + m_publisher->publish(topic, message); | ||
71 | + } | ||
72 | + } | ||
73 | + | ||
74 | + private: | ||
75 | + std::shared_ptr<Publisher> m_publisher; | ||
76 | +}; | ||
77 | + | ||
78 | +/***************************************************************************** | ||
79 | + * T H E A C T U A L T E S T S | ||
80 | + *****************************************************************************/ | ||
81 | +/// TopicTester | ||
82 | +TEST(topictest, TopicLengthTest) | ||
83 | +{ | ||
84 | + std::shared_ptr<Publisher> pPublisher = std::make_shared<Publisher>(); | ||
85 | + pPublisher->connect("127.0.0.1", 1883); | ||
86 | + | ||
87 | + TopicTester oTester(pPublisher); | ||
88 | + | ||
89 | + oTester.RunTopicTester(101); | ||
90 | +} |