Repository: netty/netty
Issue: mqtt-codec: negative packet-ids

### Behavior
It looks like it's not possible to create an mqtt message (with packetId) through the provided _builder classes_ in `io.netty.handler.codec.mqtt.MqttMessageBuilders`

The reason why this is happening is because the internal representation of the packetId (in `PubAckBuilder`, `SubAckBuilder` and `UnsubAckBuilder`) is a `short`, not an `int`.
So this means that whatever value >32767 is converted to negative when entering the builder classes, and remains a negative `int` in the underlying `MqttMessageIdVariableHeader` of the message.

### Steps to reproduce
Just try to create an mqtt SubAck message using `io.netty.handler.codec.mqtt.SubAckBuilder` with `.packetId(36000)`
It will throw an exception saying the packetId needs to be between 1 and 65535.

### Minimal yet complete reproducer code (or URL to code)
https://github.com/netty/netty/pull/10930

### Netty version
4.1.58.Final

### JVM version (e.g. `java -version`)
```
openjdk version "1.8.0_275"
OpenJDK Runtime Environment (build 1.8.0_275-8u275-b01-0ubuntu1~20.04-b01)
OpenJDK 64-Bit Server VM (build 25.275-b01, mixed mode)
```

### OS version (e.g. `uname -a`)
```
Linux  5.8.0-36-generic #40~20.04.1-Ubuntu SMP  x86_64 x86_64 x86_64 GNU/Linux
```

Issue comments:

Pull request review comments:
I think we need to keep the old as well as otherwise it will be a breaking change. 
I think we need to keep the old method as well as otherwise this would be a breaking change
I think we need to keep the old method as well as otherwise this would be a breaking change

Patch that resolved the issue:
diff --git a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttMessageBuilders.java b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttMessageBuilders.java
index 8c854f0bf07..a2eea437740 100644
--- a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttMessageBuilders.java
+++ b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttMessageBuilders.java
@@ -524,7 +524,7 @@ public ConnAckPropertiesBuilder authenticationData(byte[] rawData) {
 
     public static final class PubAckBuilder {
 
-        private short packetId;
+        private int packetId;
         private byte reasonCode;
         private MqttProperties properties;
 
@@ -536,11 +536,19 @@ public PubAckBuilder reasonCode(byte reasonCode) {
             return this;
         }
 
-        public PubAckBuilder packetId(short packetId) {
+        public PubAckBuilder packetId(int packetId) {
             this.packetId = packetId;
             return this;
         }
 
+        /**
+         * @deprecated use {@link PubAckBuilder#packetId(int)} instead
+         */
+        @Deprecated
+        public PubAckBuilder packetId(short packetId) {
+            return packetId(packetId & 0xFFFF);
+        }
+
         public PubAckBuilder properties(MqttProperties properties) {
             this.properties = properties;
             return this;
@@ -557,18 +565,26 @@ public MqttMessage build() {
 
     public static final class SubAckBuilder {
 
-        private short packetId;
+        private int packetId;
         private MqttProperties properties;
         private final List<MqttQoS> grantedQoses = new ArrayList<MqttQoS>();
 
         SubAckBuilder() {
         }
 
-        public SubAckBuilder packetId(short packetId) {
+        public SubAckBuilder packetId(int packetId) {
             this.packetId = packetId;
             return this;
         }
 
+        /**
+         * @deprecated use {@link SubAckBuilder#packetId(int)} instead
+         */
+        @Deprecated
+        public SubAckBuilder packetId(short packetId) {
+            return packetId(packetId & 0xFFFF);
+        }
+
         public SubAckBuilder properties(MqttProperties properties) {
             this.properties = properties;
             return this;
@@ -604,18 +620,26 @@ public MqttSubAckMessage build() {
 
     public static final class UnsubAckBuilder {
 
-        private short packetId;
+        private int packetId;
         private MqttProperties properties;
         private final List<Short> reasonCodes = new ArrayList<Short>();
 
         UnsubAckBuilder() {
         }
 
-        public UnsubAckBuilder packetId(short packetId) {
+        public UnsubAckBuilder packetId(int packetId) {
             this.packetId = packetId;
             return this;
         }
 
+        /**
+         * @deprecated use {@link UnsubAckBuilder#packetId(int)} instead
+         */
+        @Deprecated
+        public UnsubAckBuilder packetId(short packetId) {
+            return packetId(packetId & 0xFFFF);
+        }
+
         public UnsubAckBuilder properties(MqttProperties properties) {
             this.properties = properties;
             return this;


Test patch:
diff --git a/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttMessageBuildersPacketIdTest.java b/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttMessageBuildersPacketIdTest.java
new file mode 100644
index 00000000000..c63e771f000
--- /dev/null
+++ b/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttMessageBuildersPacketIdTest.java
@@ -0,0 +1,115 @@
+/*
+ * Copyright 2020 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package io.netty.handler.codec.mqtt;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.util.Arrays;
+
+import static org.junit.Assert.*;
+
+@RunWith(value = Parameterized.class)
+public class MqttMessageBuildersPacketIdTest {
+    @Parameterized.Parameter
+    public Integer id;
+
+    @Parameterized.Parameters(name = "{index}: {0}")
+    public static Iterable<Integer> data() {
+        // we take a subset of valid packetIds
+        return Arrays.asList(
+                0x0001,
+                0x000F,
+                0x00FF,
+                0x0FFF,
+                0xFFFF
+        );
+    }
+
+    @Test
+    public void testUnsubAckMessageIdAsShort() {
+        final MqttUnsubAckMessage msg = MqttMessageBuilders.unsubAck()
+                .packetId(id.shortValue())
+                .build();
+
+        assertEquals(
+                id.intValue(),
+                msg.variableHeader().messageId()
+        );
+    }
+
+    @Test
+    public void testSubAckMessageIdAsShort() {
+        final MqttSubAckMessage msg = MqttMessageBuilders.subAck()
+                .packetId(id.shortValue())
+                .build();
+
+        assertEquals(
+                id.intValue(),
+                msg.variableHeader().messageId()
+        );
+    }
+
+    @Test
+    public void testPubAckMessageIdAsShort() {
+        final MqttMessage msg = MqttMessageBuilders.pubAck()
+                .packetId(id.shortValue())
+                .build();
+
+        assertEquals(
+                id.intValue(),
+                ((MqttMessageIdVariableHeader) msg.variableHeader()).messageId()
+        );
+    }
+
+    @Test
+    public void testUnsubAckMessageIdAsInt() {
+        final MqttUnsubAckMessage msg = MqttMessageBuilders.unsubAck()
+                .packetId(id)
+                .build();
+
+        assertEquals(
+                id.intValue(),
+                msg.variableHeader().messageId()
+        );
+    }
+
+    @Test
+    public void testSubAckMessageIdAsInt() {
+        final MqttSubAckMessage msg = MqttMessageBuilders.subAck()
+                .packetId(id)
+                .build();
+
+        assertEquals(
+                id.intValue(),
+                msg.variableHeader().messageId()
+        );
+    }
+
+    @Test
+    public void testPubAckMessageIdAsInt() {
+        final MqttMessage msg = MqttMessageBuilders.pubAck()
+                .packetId(id)
+                .build();
+
+        assertEquals(
+                id.intValue(),
+                ((MqttMessageIdVariableHeader) msg.variableHeader()).messageId()
+        );
+    }
+}