From a3d29f51446a7103a2101a21135c5f920aa73dc4 Mon Sep 17 00:00:00 2001
From: Wiebe Cazemier <wiebe@halfgaar.net>
Date: Mon, 1 Aug 2022 19:34:57 +0200
Subject: [PATCH] Fix unnecessary QoS packet creation

---
 client.cpp             |  4 ++--
 publishcopyfactory.cpp | 13 +++++++------
 publishcopyfactory.h   |  2 +-
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/client.cpp b/client.cpp
index 3a42fde..27a98ae 100644
--- a/client.cpp
+++ b/client.cpp
@@ -251,14 +251,14 @@ void Client::writeMqttPacketAndBlameThisClient(PublishCopyFactory &copyFactory, 
 
     MqttPacket *p = copyFactory.getOptimumPacket(max_qos, this->protocolVersion, topic_alias, skip_topic);
 
-    assert(p->getQos() <= max_qos);
+    assert(static_cast<bool>(p->getQos()) == static_cast<bool>(max_qos));
 
     if (p->getQos() > 0)
     {
         // This may change the packet ID and QoS of the incoming packet for each subscriber, but because we don't store that packet anywhere,
         // that should be fine.
         p->setPacketId(packet_id);
-        p->setQos(max_qos);
+        p->setQos(copyFactory.getEffectiveQos(max_qos));
     }
 
     writeMqttPacketAndBlameThisClient(*p);
diff --git a/publishcopyfactory.cpp b/publishcopyfactory.cpp
index 5e274ff..9d962b8 100644
--- a/publishcopyfactory.cpp
+++ b/publishcopyfactory.cpp
@@ -19,31 +19,32 @@ PublishCopyFactory::PublishCopyFactory(Publish *publish) :
 
 MqttPacket *PublishCopyFactory::getOptimumPacket(const char max_qos, const ProtocolVersion protocolVersion, uint16_t topic_alias, bool skip_topic)
 {
+    const char actualQos = getEffectiveQos(max_qos);
+
     if (packet)
     {
         if (protocolVersion >= ProtocolVersion::Mqtt5 && (packet->containsClientSpecificProperties() || topic_alias > 0))
         {
             Publish newPublish(packet->getPublishData());
-            newPublish.qos = max_qos;
+            newPublish.qos = actualQos;
             newPublish.topicAlias = topic_alias;
             newPublish.skipTopic = skip_topic;
             this->oneShotPacket = std::make_unique<MqttPacket>(protocolVersion, newPublish);
             return this->oneShotPacket.get();
         }
 
-        if (packet->getProtocolVersion() == protocolVersion && orgQos == max_qos)
+        if (packet->getProtocolVersion() == protocolVersion && static_cast<bool>(orgQos) == static_cast<bool>(actualQos))
         {
-            assert(orgQos == packet->getQos());
             return packet;
         }
 
-        const int cache_key = (static_cast<uint8_t>(protocolVersion) * 10) + max_qos;
+        const int cache_key = (static_cast<uint8_t>(protocolVersion) * 10) + actualQos;
         std::unique_ptr<MqttPacket> &cachedPack = constructedPacketCache[cache_key];
 
         if (!cachedPack)
         {
             Publish newPublish(packet->getPublishData());
-            newPublish.qos = max_qos;
+            newPublish.qos = actualQos;
             cachedPack = std::make_unique<MqttPacket>(protocolVersion, newPublish);
         }
 
@@ -53,7 +54,7 @@ MqttPacket *PublishCopyFactory::getOptimumPacket(const char max_qos, const Proto
     // Getting an instance of a Publish object happens at least on retained messages, will messages and SYS topics. It's low traffic, anyway.
     assert(publish);
 
-    publish->qos = getEffectiveQos(max_qos);
+    publish->qos = actualQos;
 
     this->oneShotPacket = std::make_unique<MqttPacket>(protocolVersion, *publish);
     return this->oneShotPacket.get();
diff --git a/publishcopyfactory.h b/publishcopyfactory.h
index d7054e4..c80bc16 100644
--- a/publishcopyfactory.h
+++ b/publishcopyfactory.h
@@ -11,7 +11,7 @@
  * @brief The PublishCopyFactory class is for managing copies of an incoming publish, including sometimes not making copies at all.
  *
  * The idea is that certain incoming packets can just be written to the receiving client as-is, without constructing a new one. We do have to change the bytes
- * where the QoS is stored, so we keep track of the original.
+ * where the QoS is stored, so we keep track of the original QoS.
  *
  * Ownership info: object of this type are never copied or transferred, so the internal pointers come from (near) the same scope these objects
  * are created from.
--
libgit2 0.21.4