interfaces: PlugInfo/SlotInfo/ConnectedPlug/ConnectedSlot attribute helpers

[stolowski]

Small helpers to ease with interfaces refactoring:

• Attr() getter for PlugInfo and SlotInfo that reflect ConnectdPlug/ConnectedSlot API. Once the transition is completed, the Attrs can be changed to private.
• AttrGettr interface to simplify change to some helper methods in interfaces (e.g. in content or dbus) where they need to read attributes in sanitize methods and connected plug/slot methods; with this simple interface they can read attr from PlugInfo/SlotInfo/ConnectedSlot/ConnectedPlug.
• Updated ConnectedPlug/ConnectedSlot Attr getters: applied Maciej's suggestion about using reflection.

[zyga]

LGTM

[zyga]

It would be great if you could just test this explicitly in a separate test method.

[zyga]

+1

[codecov-io]

Codecov Report

Merging #4301 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4301      +/-   ##
==========================================
+ Coverage   77.91%   77.95%   +0.04%     
==========================================
  Files         446      446              
  Lines       30820    30841      +21     
==========================================
+ Hits        24012    24043      +31     
+ Misses       4798     4789       -9     
+ Partials     2010     2009       -1

Impacted Files	Coverage Δ
interfaces/connection.go	75% <100%> (+9.31%)	⬆️
snap/info.go	83.55% <100%> (+1.17%)	⬆️
overlord/hookstate/hookmgr.go	72.83% <0%> (+1.15%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d9c50d...c14d2a7. Read the comment docs.

[bboozzoo]

One minor suggestion regarding the API

[bboozzoo]

I guess the caller will do a type assert because this is returning interface{}. In such case you may refactor this into

import "reflect"
...
func (plug *PlugInfo) Attr(key string, val interface{}) error {
	rt := reflect.TypeOf(val)
	if rt.Kind() != reflect.Ptr || val == nil {
		return errors.New("not assignable")
	}
	if v, ok := plug.Attrs[name]; ok {
		if reflect.TypeOf(v) != rt.Elem() {
			return errors.New("type mismatch")
		}
		rv := reflect.ValueOf(val)
		rv.Elem().Set(reflect.ValueOf(v))
		return nil
	}
	return errors.New("not found")
}

Then the call sites will be cleaner:

var foo string
if err := plug.Attr("foo", &foo); err != nil {
    // blah blah failed
}

A working example is right here: https://play.golang.org/p/tyvQ0RpeFA

[stolowski]

Thanks, I like that, it would simplify the code of interfaces a little bit. @pedronis , @niemeyer your opinion on that? If that's ok I'll update the ConnectedPlug/ConnectedSlot to match.

[niemeyer]

This should probably be Attrer or similar. Given typical conventions, it feels like an AttrGet method should exist.

That said, this looks like a left over. There's no usage of the interface in the whole PR, so I suggest just dropping it.

[stolowski]

Attrer, did you actually mean that? How about AttrReader?

The Attr method name needs to match the method of ConnectedPlug/Slot and PlugInfo/SlotInfo.
This interface is used in the followup branch, I just forgot to update it to match the changes to Attr methods. Fixed now.

[pedronis]

I think Gustavo did meant Attrer, I can see at least an example where in the stdlib they went for such blunt/ungrammatical naming https://golang.org/pkg/database/sql/driver/#Valuer

[stolowski]

I see. Ok, renamed.

[niemeyer]

The three lines above can go away and be replaced with just:

v, ok := dynamicAttrs[key]

This will work as you want if the map is nil.

[niemeyer]

For the same reason, this can be simply:

if !ok {

[niemeyer]

Instead of indenting everything and having to keep in mind what the condition was by the time we get to the last line, it's cleaner to invert the logic and repeat the above statement:

if !ok {
    return fmt.Errorf("attribute %q not found", key)
}
(... rest of the logic ...)

[niemeyer]

This would benefit from context as well. See the related review point.

[niemeyer]

This error message does not reflect the action being done. Neither the developer nor the user at the other end have asked to store an attribute. Should be something along the lines of:

"internal error: cannot get attribute with non-pointer value"

[niemeyer]

Similarly, we'd benefit from some context. Which interface type, which attribute. Otherwise we'll need to blindly hunt for the error when someone reports this.

[niemeyer]

Similarly, this message is going to surface to users due to errors from snap publishers. Neither of those need to be Go developers, so we don't want to expose strings such as *map[string]interface{} in an error message.

Also, this is lacking some context that would enable to user or publisher to tell where that attribute is.

Needs to be something similar to:

"snap %q has interface %q with invalid value type for %q attribute"

If you tweak the message further, let's please just preserve the hierarchy in the order of the arguments: snap > interface > attribute.

[stolowski]

@niemeyer Thanks for the review, I've addressed your points re error messages and code simplifications.

[pedronis]

lgtm, wondering about 2 error messages tough

[pedronis]

I find this error a bit too abstract,

"snap %q has interface %q with %q attribute value of invalid type %v, expected %v", snapName, ifaceName, key, reflect.TypeOf(v), rt.Elem()

[stolowski]

I included type names initially but changed this per earlier request from Gustavo:

"Similarly, this message is going to surface to users due to errors from snap publishers. Neither of those need to be Go developers, so we don't want to expose strings such as *map[string]interface{} in an error message.

Also, this is lacking some context that would enable to user or publisher to tell where that attribute is.

Needs to be something similar to:

"snap %q has interface %q with invalid value type for %q attribute"

[pedronis]

but 'invalid value type' is not very user friendly either, anyway my worry is that we will miss debug info, maybe we should log the message with types, and surface the one without:

"snap %q has interface %q with %q attribute value with unexpected type"

[stolowski]

Fair enough, I'll address this in a follow up PR. Thanks.

[pedronis]

same wondering about the error