Convert UserProperties to map[string]interface{}

[tonygunter]

I noticed that the most recent paho library for mqttv5 replaced the type for Publish.PublishProperties.User from map[string]string with []UserProperty. I'm assuming the reason for the change is the prevalence of use cases that require multiple values sharing a single key, but wouldn't it be more efficient to utilize the amqp style custom message header?

amqp uses amqp.Table where Table is map[string]interface{}, which seems to serve the same purpose (just use []string as your interface implementation).

Retrieving a slice of values that share a custom message header in amqp requires one line of code:

slice := message.Headers["key"]

Retrieving a slice of values that share a custom message header in paho requires a loop of string comparisons:

// GetAll returns a slice of all entries in the UserProperties
// that match key, or a nil slice if none were found.
func (u UserProperties) GetAll(key string) []string {
    var ret []string
    for _, v := range u {
        if v.Key == key {
            ret = append(ret, v.Value)
        }
    }

    return ret
}

[dambrisco]

The MQTT spec explicitly allows multiple values with the same "key", as brought up in #21 (which actually resulted in []UserProperty). Switching back to a map would mean it was in violation of the spec.

[tonygunter]

@dambrisco map[string]interface{} allows multiple values with the same key as well:

	switch  value := headers["key"].(type) {
	case []string:
		// handle slice
	case string:
		// handle string
	}

The difference is building the slices for multi-valued keys up front vs. building them upon request with GetAll(key). As implemented now, you're forcing the message recipient to know beforehand which keys have multiple values (GetAll) and which don't (Get). The alternative is to use GetAll every time, which is going to loop through the entire range of headers for every key performing a string comparison and a slice append on each iteration of the internal loop.

Not a huge deal, I was just hoping that the implementation would be more congruous with the amqp broker to make broker-agnostic code a little easier to write.

[tonygunter]

Here's a rough draft of what I'm talking about. This would be the way UserPropertiesFromPacketUser would package the properties up if they were in a map[string]interface{}

func NewUserPropertiesFromPacketUser(up []packets.User) map[string]interface{} {
	ret := make(map[string]interface{})
	for _, v := range up {
		existing := ret[v.Key]
		if existing == nil {
			ret[v.Key] = v.Value
		} else {
			switch entry := existing.(type) {
			case []string:
				entry = append(entry, v.Value)
				ret[v.Key] = entry
			case string:
				newEntry := make([]string, 2)
				newEntry[0] = entry
				newEntry[1] = v.Value
				ret[v.Key] = newEntry
			}
		}
	}
	return ret
}

When I run this with three entries, two of which share the same key, I get this as an output when I log the returned value:

INFO[0000] NEW PROPERTIES = map[key1:[value1 value2] key2:value1]

[alsm]

Thanks for the thought you've put into this, a couple of things; I'm not a fan of using interface{} unless I absolutely have to, and the performance hit of using them in an section that might get used frequently is high.
With just a single k/v in the user properties

pkg: github.com/eclipse/paho.golang/paho
BenchmarkUserProperties-8        5485357               212 ns/op             352 B/op          3 allocs/op
BenchmarkUserProperty-8         29049078                43.9 ns/op            32 B/op          1 allocs/op

With 10 k/v in the user properties and 5 being the same key

pkg: github.com/eclipse/paho.golang/paho
BenchmarkUserProperties-8         703861              1476 ns/op             784 B/op         15 allocs/op
BenchmarkUserProperty-8          6933013               158 ns/op             320 B/op          1 allocs/op

The first benchmark in each being the one using the code you suggested. I also looked at returning a map[string][]string but the performance on that is similar to yours. I'm not sure the removal of GetAll is an overall benefit given having to handle interface{} values and the performance impact; the current parsing and getall is still only a 3rd of the time and allocs in the 10 k/v with 5 the same test, and you pay the cost for all packets if using the map whether or not they have duplicate keys in.

[tonygunter]

@alsm Thanks! Didn't realize the performance was that terrible. I'm new to golang, what tool are you using for performance metrics? Instana?

[alsm]

No worries, I'm using the built in benchmark testing that go provides https://scene-si.org/2017/06/06/benchmarking-go-programs/