QOS1+ Publish Packets incorrectly Packed

[MattBrittan]

Describe the bug
When unpacking QOS1+ PUBLISH packets the QOS level is not being set in the header.

To reproduce

// TestPublishPackUnpack packs and unpacks PUBLISH messages at each QOS level and confirms source and result are identical
func TestPublishPackUnpack(t *testing.T) {
	for qos := byte(0); qos < 3; qos++ {
		srcCp := NewControlPacket(PUBLISH)
		srcP := srcCp.Content.(*Publish)
		srcP.PacketID = 1
		srcP.QoS = qos
		srcP.Topic = "Test"
		srcP.Payload = []byte("Test")

		var b bytes.Buffer
		if _, err := srcCp.WriteTo(&b); err != nil {
			t.Error("failed to Write PUBLISH", err)
		}

		dstCp, err := ReadPacket(bytes.NewReader(b.Bytes()))
		if err != nil {
			t.Error("failed to Read PUBLISH", err)
		}
		dstP, ok := dstCp.Content.(*Publish)
		if !ok {
			t.Fatalf("readPacket did not return expected type (got %T)", dstCp.Content)
		}

		if (qos == 0 && dstP.PacketID != 0) || (qos == 1 && dstP.PacketID != 1) {
			t.Errorf("QOS %d unexpected Packet ID: %d", qos, dstP.PacketID)
		}
		if dstP.QoS != qos {
			t.Errorf("QOS %d unexpected QOS in decoded packet: %d", qos, dstP.QoS)
		}
		if dstP.Topic != "Test" {
			t.Errorf("QOS %d unexpected topic:%s", qos, dstP.Topic)
		}
		if bytes.Compare(dstP.Payload, []byte("Test")) != 0 {
			t.Errorf("QOS %d unexpected body: %s", qos, dstP.Payload)
		}
	}
}

Debug output

=== RUN   TestPublishPackUnpack
    publish_test.go:34: QOS 1 unexpected Packet ID: 0
    publish_test.go:37: QOS 1 unexpected QOS in decoded packet: 0
    publish_test.go:43: QOS 1 unexpected body: � Test
    publish_test.go:37: QOS 2 unexpected QOS in decoded packet: 0
    publish_test.go:43: QOS 2 unexpected body: � Test
--- FAIL: TestPublishPackUnpack (116.55s)

Expected behaviour
The test should pass

Software used:
@master

Additional context
Issue appears to be handling of FixedHeader.Flags in func (c *ControlPacket) WriteTo(w io.Writer) (int64, error). The Unpacker checks Flags and sets the QOS but WriteTo does not do the reverse.

Note: I am working on a fix.

[MattBrittan]

Looking into this further:

• Calling WriteTo on the ControlPacket type (srcCP in the test) fails
• Calling WriteTo on the Publish type (srcP in the test) is successful

This means (I think) that this is not currently causing any issues because a PUBLISH will always be sent via WriteTo on the Publish type. However, if ControlPacket is going to have a WriteTo then is should work for all packet types.

The way this currently works for other packets relies on a degree of duplication. For example in pubrel.go we write with &ControlPacket{FixedHeader: FixedHeader{Type: PUBREL, Flags: 2}} and Flags is also set to the required value (2) in packets.go with case PUBREL: cp.Flags = 2; cp.Content = &Pubrel{Properties: &Properties{}}. This works but the duplication is not ideal.

Looking at section 2.1.3 ("Flags") in the spec if looks like "PUBLISH" is the only packet with variable flags so I will implement a quick fix for now but it would be worth considering refactoring this in the future.

Anyway the PR fixes this by duplicating some code from publish.go into packets.go. I have also added a test which should ensure that both versions work (could probably be enhanced to check other flags).