river/encoding: fix issue where interface capsules were downcasted to empty objects #2437
Conversation
… empty objects This change is similar to the change introduced in grafana#2369, where calling the `Interface()` method on River values forced a type change from an interface-based capsule to an any-type capsule. The new `value.FromRaw` method allows constructing a River value from a reflect.Value directly, avoiding the need to convert to interface{}. As part of this change, a `!reflect.Value.IsZero()` check has been added for safety; this shouldn't happen in most circumstances unless a value.Null is trying to be encoded. Fixes grafana#2361.
| // FromRaw converts a reflect.Value into a River Value. It is useful to prevent | ||
| // downcasting a interface into an any. | ||
| func FromRaw(v reflect.Value) Value { | ||
| return makeValue(v) | ||
| } |
There was a problem hiding this comment.
The need to do this is probably the start of a wider discussion around whether interface types should be capsules. It's fairly hard to reason about in the internal code, and there's still some edge cases where a capsule can be downcasted like when providing it to a vm.Scope.
The alternative would be to stop magically trying to determine if specific types of interfaces can be capsules and require the marker interface instead (but channels could still default to capsules). This would be more work on component developers, but it's probably easier to reason about and harder to get wrong in the River implementation.
There could also be a registry of capsule types like we used to have, but I'm not a big fan of that solution.
cc @grafana/databases-agent
There was a problem hiding this comment.
When you say marker interface, do you mean interfaces would need to have the Capsule interface on them?
There was a problem hiding this comment.
Yeah, I mean to change the rules so that:
- Go interfaces are deferenced into their underlying type.
- Types which implement
river.Capsuleare forced capsule types. - Go types which have no River mapping (like channels) default to capsule types.
The only new rule here is the first one; currently interface types with at least one method are treated as capsules, though it's too easy for that information to be lost.
There was a problem hiding this comment.
Either way, I think making these changes should be something we do later, we still need this bug fix for now, especially since #2431 probably makes it happen for even more components.
github-actions
bot
added
the
frozen-due-to-age
This change is similar to the fix introduced in #2369, where calling the
Interface()method on River values forced a type change from an interface-based capsule to an any-type capsule. This caused the UI to crash when displaying an otelcol component (see #2361).The new
value.FromRawmethod allows constructing a River value from a reflect.Value directly, avoiding the need to convert to interface{}.As part of this change, a
!reflect.Value.IsZero()check has been added for safety; this shouldn't happen in most circumstances unless a value.Null is trying to be encoded.The CHANGELOG wasn't updated because this behavior only occurs when a component exports a capsule type, which was first introduced in the otelcol.* components which have yet to be released.
Fixes #2361.