Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Marshal7951 for direct union leaf calls. #947

Merged
merged 2 commits into from
Jan 11, 2024
Merged

Conversation

wenovus
Copy link
Collaborator

@wenovus wenovus commented Jan 10, 2024

When jsonValue is called using the output of reflect.ValueOf() instead of reflect.StructField, any interface type is lost through re-packing to the empty interface (any). This means the reflect.Kind of a union field is no longer the union type, but instead its underlying concrete type. The current code doesn't handle this case, leading to runtime errors.

This handling code is now added, with a couple more fixes related to empty types.


Tested manually that the following ygnmi call is no longer affected by the original error:

sp := gnmi.OC().NetworkInstance("DEFAULT").Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_STATIC, "STATIC")
ygnmi.Replace(context.Background(), c, sp.Static("1.1.1.1/32").SetTag().Config(), oc.NetworkInstance_Protocol_Static_SetTag_Union(oc.UnionUint32(42)))

When `jsonValue` is called using the output of `reflect.ValueOf()` instead of
`reflect.StructField`, any interface type is lost through re-packing to the
empty interface (any). This means the `reflect.Kind` of a union field is no
longer the union type, but instead its underlying concrete type. The current
code doesn't handle this case, leading to runtime errors.

This handling code is now added, with a couple more fixes related to `empty`
types.

-----
Tested manually that the following ygnmi call is no longer affected by the
original error:
```go
sp := gnmi.OC().NetworkInstance("DEFAULT").Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_STATIC, "STATIC")
return ygnmi.Replace(context.Background(), c, sp.Static("1.1.1.1/32").SetTag().Config(), oc.NetworkInstance_Protocol_Static_SetTag_Union(oc.UnionUint32(42)))
```
@coveralls
Copy link

coveralls commented Jan 10, 2024

Coverage Status

coverage: 89.606% (-0.006%) from 89.612%
when pulling da103aa on marshal7951-union-bug
into dca67e2 on master.

Copy link
Contributor

@Ankur19 Ankur19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this Wen!

@wenovus
Copy link
Collaborator Author

wenovus commented Jan 11, 2024

Thanks for fixing this Wen!

Thanks for the review! and finding the bug!

@wenovus wenovus merged commit 12a8460 into master Jan 11, 2024
10 checks passed
@wenovus wenovus deleted the marshal7951-union-bug branch January 11, 2024 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants