Repository: netty/netty
Issue: Add support for "session present" flag in io.netty.handler.codec.mqtt.MqttConnAckVariableHeader
Labels: improvement

Netty version: 4.1.0.Beta5

Context:
I'm implementing an MQTT broker based on netty using the classes from the package io.netty.handler.codec.mqtt, in the version 3.1.1 of the MQTT protocol it has been added a new flag in the variable header of the CONNACK message called "session present": http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Toc398718035, it would be great if that flag was supported in io.netty.handler.codec.mqtt.MqttConnAckVariableHeader.


Issue comments:
We love contributions. Maybe care to submit a PR?

> Am 29.07.2015 um 11:23 schrieb Jorge Castellanos Solaz notifications@github.com:
> 
> Netty version: 4.1.0.Beta5
> 
> Context:
> I'm implementing an MQTT broker based on netty using the classes from the package io.netty.handler.codec.mqtt, in the version 3.1.1 of the MQTT protocol it has been added a new flag in the variable header of the CONNACK message called "session present": http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Toc398718035, it would be great if that flag was supported in io.netty.handler.codec.mqtt.MqttConnAckVariableHeader.
> 
> —
> Reply to this email directly or view it on GitHub.

@normanmaurer I tried to do it in PR. https://github.com/netty/netty/pull/4042

but I am not sure if it's OK to do it against 4.1 (the branch we're using) or if I should do it against master and you guys (or me?) take care of the cherry-pick

Also I just noticed now that I didn't format the commit properly, but it's so trivial I'm not sure it's worth it...

@flozano - Thanks for the PR! Doing against 4.1 should be fine. We will take care of cherry-picking into other branches.

@flozano thanks.. Adjusted commit message and cherry-picked into 4.1(a91e64850505938cfe3a2079e1ac2c247df6532b)  and master (6ff66fe967ed1aa45f2e87d263463f35c959b5cf).


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttConnAckVariableHeader.java b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttConnAckVariableHeader.java
index 7dfa3094837..a1b73303d1d 100644
--- a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttConnAckVariableHeader.java
+++ b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttConnAckVariableHeader.java
@@ -25,19 +25,25 @@ public class MqttConnAckVariableHeader {
 
     private final MqttConnectReturnCode connectReturnCode;
 
-    public MqttConnAckVariableHeader(MqttConnectReturnCode connectReturnCode) {
+    private final boolean sessionPresent;
+
+    public MqttConnAckVariableHeader(MqttConnectReturnCode connectReturnCode, boolean sessionPresent) {
         this.connectReturnCode = connectReturnCode;
+        this.sessionPresent = sessionPresent;
     }
 
     public MqttConnectReturnCode connectReturnCode() {
         return connectReturnCode;
     }
 
+    public boolean sessionPresent() { return sessionPresent; }
+
     @Override
     public String toString() {
         return new StringBuilder(StringUtil.simpleClassName(this))
             .append('[')
             .append("connectReturnCode=").append(connectReturnCode)
+            .append(", sessionPresent=").append(sessionPresent)
             .append(']')
             .toString();
     }
diff --git a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java
index bcb770a354a..70f0a41e7ba 100644
--- a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java
+++ b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java
@@ -236,11 +236,11 @@ private static Result<MqttConnectVariableHeader> decodeConnectionVariableHeader(
     }
 
     private static Result<MqttConnAckVariableHeader> decodeConnAckVariableHeader(ByteBuf buffer) {
-        buffer.readUnsignedByte(); // reserved byte
+        final boolean sessionPresent = (buffer.readUnsignedByte() & 0x01) == 0x01;
         byte returnCode = buffer.readByte();
         final int numberOfBytesConsumed = 2;
         final MqttConnAckVariableHeader mqttConnAckVariableHeader =
-                new MqttConnAckVariableHeader(MqttConnectReturnCode.valueOf(returnCode));
+                new MqttConnAckVariableHeader(MqttConnectReturnCode.valueOf(returnCode), sessionPresent);
         return new Result<MqttConnAckVariableHeader>(mqttConnAckVariableHeader, numberOfBytesConsumed);
     }
 
diff --git a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttEncoder.java b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttEncoder.java
index db2d871f9d9..eda2a8dde2c 100644
--- a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttEncoder.java
+++ b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttEncoder.java
@@ -192,7 +192,7 @@ private static ByteBuf encodeConnAckMessage(
         ByteBuf buf = byteBufAllocator.buffer(4);
         buf.writeByte(getFixedHeaderByte1(message.fixedHeader()));
         buf.writeByte(2);
-        buf.writeByte(0);
+        buf.writeByte(message.variableHeader().sessionPresent() ? 0x01 : 0x00);
         buf.writeByte(message.variableHeader().connectReturnCode().byteValue());
 
         return buf;


Test patch:
diff --git a/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java b/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java
index f16602a64e6..705d071bfd8 100644
--- a/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java
+++ b/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java
@@ -289,7 +289,7 @@ private static MqttConnAckMessage createConnAckMessage() {
         MqttFixedHeader mqttFixedHeader =
                 new MqttFixedHeader(MqttMessageType.CONNACK, false, MqttQoS.AT_MOST_ONCE, false, 0);
         MqttConnAckVariableHeader mqttConnAckVariableHeader =
-                new MqttConnAckVariableHeader(MqttConnectReturnCode.CONNECTION_ACCEPTED);
+                new MqttConnAckVariableHeader(MqttConnectReturnCode.CONNECTION_ACCEPTED, true);
         return new MqttConnAckMessage(mqttFixedHeader, mqttConnAckVariableHeader);
     }