Fix Subscribe.subscriptionID is optional

gregoryv · gregoryv · commit 0827966e0b0c · 2024-09-21T20:36:51.000+02:00
diff --git a/auth.go b/auth.go
@@ -95,10 +95,10 @@ func (p *Auth) properties(b []byte, i int) int {
 	return i - n
 }
 
-func (p *Auth) propertyMap() map[Ident]wireType {
-	return map[Ident]wireType{
-		AuthMethod:   &p.authMethod,
-		AuthData:     &p.authData,
-		ReasonString: &p.reasonString,
+func (p *Auth) propertyMap() map[Ident]func() wireType {
+	return map[Ident]func() wireType{
+		AuthMethod:   func() wireType { return &p.authMethod },
+		AuthData:     func() wireType { return &p.authData },
+		ReasonString: func() wireType { return &p.reasonString },
 	}
 }
diff --git a/buffer.go b/buffer.go
@@ -22,7 +22,7 @@ type buffer struct {
 // getAny reads all properties from the current offset starting with
 // the variable length.  fields map UserProp identity codes to wire
 // type fields and the addProp func is used for each user property.
-func (b *buffer) getAny(fields map[Ident]wireType, addProp func(UserProp)) {
+func (b *buffer) getAny(fields map[Ident]func() wireType, addProp func(UserProp)) {
 	if b.atEnd() {
 		return
 	}
@@ -39,7 +39,7 @@ func (b *buffer) getAny(fields map[Ident]wireType, addProp func(UserProp)) {
 		field, hasField := fields[id]
 		switch {
 		case hasField:
-			b.get(field)
+			b.get(field())
 
 		case id == UserProperty:
 			var p UserProp
diff --git a/buffer_test.go b/buffer_test.go
@@ -8,14 +8,14 @@ import (
 func Test_buffer(t *testing.T) {
 	{ // missing data
 		b := &buffer{}
-		b.getAny(map[Ident]wireType{}, func(UserProp) {})
+		b.getAny(map[Ident]func() wireType{}, func(UserProp) {})
 		if b.err != nil {
 			t.Error("getAny failes on empty data")
 		}
 	}
 	{ // unknown user property
 		b := &buffer{data: []byte{2, 0xff, 0}}
-		b.getAny(map[Ident]wireType{}, func(UserProp) {})
+		b.getAny(map[Ident]func() wireType{}, func(UserProp) {})
 		if b.err == nil {
 			t.Error("expect getAny to fail")
 		}
diff --git a/changelog.md b/changelog.md
@@ -6,6 +6,7 @@ and this project adheres to http://semver.org/spec/v2.0.0.html.
 
 ## [unreleased]
 
+- Fix Subscribe.subscriptionID is optional
 - Add func Unsubscribe.Filters
 - Update dependencies
 
diff --git a/connack.go b/connack.go
@@ -198,24 +198,24 @@ func (p *ConnAck) UnmarshalBinary(data []byte) error {
 	return b.err
 }
 
-func (p *ConnAck) propertyMap() map[Ident]wireType {
-	return map[Ident]wireType{
-		ReceiveMax:            &p.receiveMax,
-		SessionExpiryInterval: &p.sessionExpiryInterval,
-		MaxQoS:                &p.maxQoS,
-		RetainAvailable:       &p.retainAvailable,
-		MaxPacketSize:         &p.maxPacketSize,
-		AssignedClientID:      &p.assignedClientID,
-		TopicAliasMax:         &p.topicAliasMax,
-		ReasonString:          &p.reasonString,
-		WildcardSubAvailable:  &p.wildcardSubAvailable,
-		SubIDsAvailable:       &p.subIdentifiersAvailable,
-		SharedSubAvailable:    &p.sharedSubAvailable,
-		ServerKeepAlive:       &p.serverKeepAlive,
-		ResponseInformation:   &p.responseInformation,
-		ServerReference:       &p.serverReference,
-		AuthMethod:            &p.authMethod,
-		AuthData:              &p.authData,
+func (p *ConnAck) propertyMap() map[Ident]func() wireType {
+	return map[Ident]func() wireType{
+		ReceiveMax:            func() wireType { return &p.receiveMax },
+		SessionExpiryInterval: func() wireType { return &p.sessionExpiryInterval },
+		MaxQoS:                func() wireType { return &p.maxQoS },
+		RetainAvailable:       func() wireType { return &p.retainAvailable },
+		MaxPacketSize:         func() wireType { return &p.maxPacketSize },
+		AssignedClientID:      func() wireType { return &p.assignedClientID },
+		TopicAliasMax:         func() wireType { return &p.topicAliasMax },
+		ReasonString:          func() wireType { return &p.reasonString },
+		WildcardSubAvailable:  func() wireType { return &p.wildcardSubAvailable },
+		SubIDsAvailable:       func() wireType { return &p.subIdentifiersAvailable },
+		SharedSubAvailable:    func() wireType { return &p.sharedSubAvailable },
+		ServerKeepAlive:       func() wireType { return &p.serverKeepAlive },
+		ResponseInformation:   func() wireType { return &p.responseInformation },
+		ServerReference:       func() wireType { return &p.serverReference },
+		AuthMethod:            func() wireType { return &p.authMethod },
+		AuthData:              func() wireType { return &p.authData },
 	}
 }
 
diff --git a/connect.go b/connect.go
@@ -283,7 +283,7 @@ func (p *Connect) payload(b []byte, i int) int {
 			n := i
 
 			for id, v := range p.willPropertyMap() {
-				i += v.fillProp(b, i, id)
+				i += v().fillProp(b, i, id)
 			}
 			i += p.will.UserProperties.properties(b, i)
 
@@ -340,27 +340,27 @@ func (p *Connect) UnmarshalBinary(data []byte) error {
 
 	return buf.Err()
 }
-func (p *Connect) willPropertyMap() map[Ident]wireType {
-	return map[Ident]wireType{
-		WillDelayInterval:      &p.willDelayInterval,
-		PayloadFormatIndicator: &p.will.payloadFormat,
-		MessageExpiryInterval:  &p.will.messageExpiryInterval,
-		ContentType:            &p.will.contentType,
-		ResponseTopic:          &p.will.responseTopic,
-		CorrelationData:        &p.will.correlationData,
+func (p *Connect) willPropertyMap() map[Ident]func() wireType {
+	return map[Ident]func() wireType{
+		WillDelayInterval:      func() wireType { return &p.willDelayInterval },
+		PayloadFormatIndicator: func() wireType { return &p.will.payloadFormat },
+		MessageExpiryInterval:  func() wireType { return &p.will.messageExpiryInterval },
+		ContentType:            func() wireType { return &p.will.contentType },
+		ResponseTopic:          func() wireType { return &p.will.responseTopic },
+		CorrelationData:        func() wireType { return &p.will.correlationData },
 	}
 }
 
-func (p *Connect) propertyMap() map[Ident]wireType {
-	return map[Ident]wireType{
-		ReceiveMax:            &p.receiveMax,
-		SessionExpiryInterval: &p.sessionExpiryInterval,
-		MaxPacketSize:         &p.maxPacketSize,
-		TopicAliasMax:         &p.topicAliasMax,
-		RequestResponseInfo:   &p.requestResponseInfo,
-		RequestProblemInfo:    &p.requestProblemInfo,
-		AuthMethod:            &p.authMethod,
-		AuthData:              &p.authData,
+func (p *Connect) propertyMap() map[Ident]func() wireType {
+	return map[Ident]func() wireType{
+		ReceiveMax:            func() wireType { return &p.receiveMax },
+		SessionExpiryInterval: func() wireType { return &p.sessionExpiryInterval },
+		MaxPacketSize:         func() wireType { return &p.maxPacketSize },
+		TopicAliasMax:         func() wireType { return &p.topicAliasMax },
+		RequestResponseInfo:   func() wireType { return &p.requestResponseInfo },
+		RequestProblemInfo:    func() wireType { return &p.requestProblemInfo },
+		AuthMethod:            func() wireType { return &p.authMethod },
+		AuthData:              func() wireType { return &p.authData },
 	}
 }
 
diff --git a/disconnect.go b/disconnect.go
@@ -72,6 +72,6 @@ func (p *Disconnect) UnmarshalBinary(data []byte) error {
 	return b.err
 }
 
-func (p *Disconnect) propertyMap() map[Ident]wireType {
-	return map[Ident]wireType{}
+func (p *Disconnect) propertyMap() map[Ident]func() wireType {
+	return map[Ident]func() wireType{}
 }
diff --git a/puback.go b/puback.go
@@ -95,8 +95,8 @@ func (p *PubAck) UnmarshalBinary(data []byte) error {
 	return b.err
 }
 
-func (p *PubAck) propertyMap() map[Ident]wireType {
-	return map[Ident]wireType{
-		ReasonString: &p.reason,
+func (p *PubAck) propertyMap() map[Ident]func() wireType {
+	return map[Ident]func() wireType{
+		ReasonString: func() wireType { return &p.reason },
 	}
 }
diff --git a/pubcomp.go b/pubcomp.go
@@ -102,8 +102,8 @@ func (p *PubComp) UnmarshalBinary(data []byte) error {
 	return b.err
 }
 
-func (p *PubComp) propertyMap() map[Ident]wireType {
-	return map[Ident]wireType{
-		ReasonString: &p.reason,
+func (p *PubComp) propertyMap() map[Ident]func() wireType {
+	return map[Ident]func() wireType{
+		ReasonString: func() wireType { return &p.reason },
 	}
 }
diff --git a/publish.go b/publish.go
@@ -250,13 +250,13 @@ func (p *Publish) UnmarshalBinary(data []byte) error {
 	return buf.err
 }
 
-func (p *Publish) propertyMap() map[Ident]wireType {
-	return map[Ident]wireType{
-		PayloadFormatIndicator: &p.payloadFormat,
-		MessageExpiryInterval:  &p.messageExpiryInterval,
-		TopicAlias:             &p.topicAlias,
-		ResponseTopic:          &p.responseTopic,
-		CorrelationData:        &p.correlationData,
-		ContentType:            &p.contentType,
+func (p *Publish) propertyMap() map[Ident]func() wireType {
+	return map[Ident]func() wireType{
+		PayloadFormatIndicator: func() wireType { return &p.payloadFormat },
+		MessageExpiryInterval:  func() wireType { return &p.messageExpiryInterval },
+		TopicAlias:             func() wireType { return &p.topicAlias },
+		ResponseTopic:          func() wireType { return &p.responseTopic },
+		CorrelationData:        func() wireType { return &p.correlationData },
+		ContentType:            func() wireType { return &p.contentType },
 	}
 }
diff --git a/pubrec.go b/pubrec.go
@@ -102,8 +102,8 @@ func (p *PubRec) UnmarshalBinary(data []byte) error {
 	return b.err
 }
 
-func (p *PubRec) propertyMap() map[Ident]wireType {
-	return map[Ident]wireType{
-		ReasonString: &p.reason,
+func (p *PubRec) propertyMap() map[Ident]func() wireType {
+	return map[Ident]func() wireType{
+		ReasonString: func() wireType { return &p.reason },
 	}
 }
diff --git a/pubrel.go b/pubrel.go
@@ -97,8 +97,8 @@ func (p *PubRel) UnmarshalBinary(data []byte) error {
 	return b.err
 }
 
-func (p *PubRel) propertyMap() map[Ident]wireType {
-	return map[Ident]wireType{
-		ReasonString: &p.reason,
+func (p *PubRel) propertyMap() map[Ident]func() wireType {
+	return map[Ident]func() wireType{
+		ReasonString: func() wireType { return &p.reason },
 	}
 }
diff --git a/suback.go b/suback.go
@@ -79,7 +79,7 @@ func (p *SubAck) variableHeader(b []byte, i int) int {
 func (p *SubAck) properties(b []byte, i int) int {
 	n := i
 	for id, v := range p.propertyMap() {
-		i += v.fillProp(b, i, id)
+		i += v().fillProp(b, i, id)
 	}
 	i += p.UserProperties.properties(b, i)
 	return i - n
@@ -108,8 +108,8 @@ func (p *SubAck) UnmarshalBinary(data []byte) error {
 	return b.err
 }
 
-func (p *SubAck) propertyMap() map[Ident]wireType {
-	return map[Ident]wireType{
-		ReasonString: &p.reasonString,
+func (p *SubAck) propertyMap() map[Ident]func() wireType {
+	return map[Ident]func() wireType{
+		ReasonString: func() wireType { return &p.reasonString },
 	}
 }
diff --git a/subscribe.go b/subscribe.go
@@ -13,7 +13,7 @@ func NewSubscribe() *Subscribe {
 type Subscribe struct {
 	fixed          bits
 	packetID       wuint16
-	subscriptionID vbint
+	subscriptionID *vbint
 	UserProperties
 	filters []TopicFilter
 }
@@ -31,10 +31,7 @@ func (p *Subscribe) WellFormed() *Malformed {
 	if len(p.filters) == 0 {
 		return newMalformed(p, "filters", "no")
 	}
-	if p.subscriptionID == 0 {
-		return newMalformed(p, "sub ID", "missing")
-	}
-	if p.subscriptionID > 268_435_455 {
+	if v := p.subscriptionID; v != nil && *v > 268_435_455 {
 		return newMalformed(p, "sub ID", "too large")
 	}
 	for _, f := range p.filters {
@@ -47,7 +44,9 @@ func (p *Subscribe) WellFormed() *Malformed {
 
 func (p *Subscribe) dump(w io.Writer) {
 	fmt.Fprintf(w, "PacketID: %v\n", p.PacketID())
-	fmt.Fprintf(w, "SubscriptionID: %v\n", p.SubscriptionID())
+	if p.subscriptionID != nil {
+		fmt.Fprintf(w, "SubscriptionID: %v\n", p.SubscriptionID())
+	}
 
 	if len(p.filters) > 0 {
 		fmt.Fprintln(w, "Filters")
@@ -69,8 +68,18 @@ func (p *Subscribe) filterString() string {
 func (p *Subscribe) SetPacketID(v uint16) { p.packetID = wuint16(v) }
 func (p *Subscribe) PacketID() uint16     { return uint16(p.packetID) }
 
-func (p *Subscribe) SetSubscriptionID(v int) { p.subscriptionID = vbint(v) }
-func (p *Subscribe) SubscriptionID() int     { return int(p.subscriptionID) }
+func (p *Subscribe) SetSubscriptionID(v int) {
+	if p.subscriptionID == nil {
+		p.subscriptionID = new(vbint)
+	}
+	*p.subscriptionID = vbint(v)
+}
+func (p *Subscribe) SubscriptionID() int {
+	if p.subscriptionID == nil {
+		return -1
+	}
+	return int(*p.subscriptionID)
+}
 
 func (p *Subscribe) AddFilters(v ...TopicFilter) {
 	p.filters = append(p.filters, v...)
@@ -112,8 +121,12 @@ func (p *Subscribe) variableHeader(b []byte, i int) int {
 
 func (p *Subscribe) properties(b []byte, i int) int {
 	n := i
-	for id, v := range p.propertyMap() {
-		i += v.fillProp(b, i, id)
+	for id, v := range p.propertyMap(false) {
+		out := v()
+		if out == nil {
+			continue
+		}
+		i += out.fillProp(b, i, id)
 	}
 	i += p.UserProperties.properties(b, i)
 	return i - n
@@ -130,7 +143,7 @@ func (p *Subscribe) payload(b []byte, i int) int {
 func (p *Subscribe) UnmarshalBinary(data []byte) error {
 	b := &buffer{data: data}
 	b.get(&p.packetID)
-	b.getAny(p.propertyMap(), p.appendUserProperty)
+	b.getAny(p.propertyMap(true), p.appendUserProperty)
 
 	for {
 		var f TopicFilter
@@ -144,8 +157,16 @@ func (p *Subscribe) UnmarshalBinary(data []byte) error {
 	return b.err
 }
 
-func (p *Subscribe) propertyMap() map[Ident]wireType {
-	return map[Ident]wireType{
-		SubscriptionID: &p.subscriptionID,
+func (p *Subscribe) propertyMap(unmarshal bool) map[Ident]func() wireType {
+	return map[Ident]func() wireType{
+		SubscriptionID: func() wireType {
+			if unmarshal {
+				p.subscriptionID = new(vbint)
+			}
+			if p.subscriptionID == nil {
+				return nil
+			}
+			return p.subscriptionID
+		},
 	}
 }
diff --git a/subscribe_test.go b/subscribe_test.go
@@ -30,7 +30,6 @@ func ExampleSubscribe_Dump() {
 	Dump(os.Stdout, s)
 	// output:
 	// PacketID: 0
-	// SubscriptionID: 0
 	// Filters
 	//   0. a/b/c --r0pn2-
 	//   1. d/e --r0---1
@@ -39,8 +38,9 @@ func ExampleSubscribe_Dump() {
 }
 
 func ExampleSubscribe_malformed() {
-	// no filters
-	fmt.Println(NewSubscribe())
+	{// no filters
+		fmt.Println(NewSubscribe())
+	}
 	{ // bad qos
 		s := NewSubscribe()
 		s.SetSubscriptionID(1)
@@ -53,12 +53,6 @@ func ExampleSubscribe_malformed() {
 		s.AddFilters(NewTopicFilter("", OptQoS1))
 		fmt.Println(s)
 	}
-	{ // missing subscription id
-		s := NewSubscribe()
-		s.SetSubscriptionID(0)
-		s.AddFilters(NewTopicFilter("#", OptQoS1))
-		fmt.Println(s)
-	}
 	{ // too large subscription id
 		s := NewSubscribe()
 		s.SetSubscriptionID(268_435_455 + 1)
@@ -69,7 +63,6 @@ func ExampleSubscribe_malformed() {
 	// SUBSCRIBE --1- p0  5 bytes, malformed! no filters
 	// SUBSCRIBE --1- p0 a/b --r0--!! 13 bytes, malformed! invalid QoS
 	// SUBSCRIBE --1- p0  --r0---1 10 bytes, malformed! empty filter
-	// SUBSCRIBE --1- p0 # --r0---1 9 bytes, malformed! missing sub ID
 	// SUBSCRIBE --1- p0 # --r0---1 15 bytes, malformed! too large sub ID
 }
 
diff --git a/unsuback.go b/unsuback.go
diff --git a/wiretypes.go b/wiretypes.go

Original file line number	Diff line number	Diff line change
`@@ -95,10 +95,10 @@ func (p *Auth) properties(b []byte, i int) int {`
`95`	`95`	`return i - n`
`96`	`96`	`}`
`97`	`97`
`98`		`-func (p *Auth) propertyMap() map[Ident]wireType {`
`99`		`- return map[Ident]wireType{`
`100`		`- AuthMethod: &p.authMethod,`
`101`		`- AuthData: &p.authData,`
`102`		`- ReasonString: &p.reasonString,`
	`98`	`+func (p *Auth) propertyMap() map[Ident]func() wireType {`
	`99`	`+ return map[Ident]func() wireType{`
	`100`	`+ AuthMethod: func() wireType { return &p.authMethod },`
	`101`	`+ AuthData: func() wireType { return &p.authData },`
	`102`	`+ ReasonString: func() wireType { return &p.reasonString },`
`103`	`103`	`}`
`104`	`104`	`}`
Original file line number	Diff line number	Diff line change
`@@ -8,14 +8,14 @@ import (`
`8`	`8`	`func Test_buffer(t *testing.T) {`
`9`	`9`	`{ // missing data`
`10`	`10`	`b := &buffer{}`
`11`		`- b.getAny(map[Ident]wireType{}, func(UserProp) {})`
	`11`	`+ b.getAny(map[Ident]func() wireType{}, func(UserProp) {})`
`12`	`12`	`if b.err != nil {`
`13`	`13`	`t.Error("getAny failes on empty data")`
`14`	`14`	`}`
`15`	`15`	`}`
`16`	`16`	`{ // unknown user property`
`17`	`17`	`b := &buffer{data: []byte{2, 0xff, 0}}`
`18`		`- b.getAny(map[Ident]wireType{}, func(UserProp) {})`
	`18`	`+ b.getAny(map[Ident]func() wireType{}, func(UserProp) {})`
`19`	`19`	`if b.err == nil {`
`20`	`20`	`t.Error("expect getAny to fail")`
`21`	`21`	`}`
Original file line number	Diff line number	Diff line change
`@@ -72,6 +72,6 @@ func (p *Disconnect) UnmarshalBinary(data []byte) error {`
`72`	`72`	`return b.err`
`73`	`73`	`}`
`74`	`74`
`75`		`-func (p *Disconnect) propertyMap() map[Ident]wireType {`
`76`		`- return map[Ident]wireType{}`
	`75`	`+func (p *Disconnect) propertyMap() map[Ident]func() wireType {`
	`76`	`+ return map[Ident]func() wireType{}`
`77`	`77`	`}`
Original file line number	Diff line number	Diff line change
`@@ -95,8 +95,8 @@ func (p *PubAck) UnmarshalBinary(data []byte) error {`
`95`	`95`	`return b.err`
`96`	`96`	`}`
`97`	`97`
`98`		`-func (p *PubAck) propertyMap() map[Ident]wireType {`
`99`		`- return map[Ident]wireType{`
`100`		`- ReasonString: &p.reason,`
	`98`	`+func (p *PubAck) propertyMap() map[Ident]func() wireType {`
	`99`	`+ return map[Ident]func() wireType{`
	`100`	`+ ReasonString: func() wireType { return &p.reason },`
`101`	`101`	`}`
`102`	`102`	`}`
Original file line number	Diff line number	Diff line change
`@@ -102,8 +102,8 @@ func (p *PubComp) UnmarshalBinary(data []byte) error {`
`102`	`102`	`return b.err`
`103`	`103`	`}`
`104`	`104`
`105`		`-func (p *PubComp) propertyMap() map[Ident]wireType {`
`106`		`- return map[Ident]wireType{`
`107`		`- ReasonString: &p.reason,`
	`105`	`+func (p *PubComp) propertyMap() map[Ident]func() wireType {`
	`106`	`+ return map[Ident]func() wireType{`
	`107`	`+ ReasonString: func() wireType { return &p.reason },`
`108`	`108`	`}`
`109`	`109`	`}`