openconfig / ygot

A YANG-centric Go toolkit - Go/Protobuf Code Generation; Validation; Marshaling/Unmarshaling
Apache License 2.0
286 stars 107 forks source link

`ygotreflect` package for easier access to GoStruct metadata and manipulations #663

Open wenovus opened 2 years ago

wenovus commented 2 years ago

With the recent rename of Validate to ΛValidate, the ygot.ValidatedGoStruct interface has become less usable, not to mention this being a breaking change in order to allow nodes named validate to be created. This breaking change was unavoidable due to the definition of ygot.ValidatedGoStruct having to be static.

In the future, we might very well add new "metadata" methods such as these to GoStructs, some of which will be controlled by generation flags. In order for these methods to be accessible, the current way of using ValidatedGoStruct has two problems:

  1. It takes names away from the YANG namespace.
  2. It cannot contain any method that's not generated unconditionally.

A more flexible way would be to write functions for accessing these metadata in a separate gostruct package that would not only avoid awkward names, but also allow metadata generated conditionally to be accessible (e.g. gostruct.Validate(interface)). This, however, comes at the expense of discoverability due to these being functions rather than methods.

A way to get all these benefits without sacrificing discoverability is to provide some sort of "ygotreflect" package for traversing GoStructs. An API could look something like this (inspired by https://pkg.go.dev/google.golang.org/protobuf/reflect/protoreflect#Message):

interface := &oc.Interfaces_Interface{}
interface.YgotReflect().Validate()
interface.YgotReflect().Name() // a string
interface.YgotReflect().Descriptor().BelongingModule // a string
interface.YgotReflect().Interface() // returns the ValidatedGoStruct
interface.YgotReflect().Type().New()
interface.YgotReflect().Type().Parent().New()
interface.YgotReflect().Range() // returns a set of ygotreflect.FieldDescriptor values.

Such a set of utilities would not only easily provide the functionalities to solve the problem above, but is also highly-extensible, while having a low threat to backwards-incompatibility.

I'm planning to gather some input and explore whether this API would help improve the ease of user development.

robshakir commented 2 years ago

I think this is an interesting idea :-) The challenge of overlapping with potential YANG node names is always present for names that are user-friendly. Given that this is the case, one could observe that if I create a YANG model that has ygot-reflect as a name then this itself is going to overlap with fields.

I'm not quite sure that I see the full use case for a reflect like library -- what is the benefit to the user of having Interface() (especially chained onto a receiver that implements the interface itself)?

wenovus commented 2 years ago

I think this is an interesting idea :-) The challenge of overlapping with potential YANG node names is always present for names that are user-friendly. Given that this is the case, one could observe that if I create a YANG model that has ygot-reflect as a name then this itself is going to overlap with fields.

Good point, I think we can address this by giving users a flag to control the name of this accessor, exactly like how we're currently handling Validate.

I'm not quite sure that I see the full use case for a reflect like library -- what is the benefit to the user of having Interface() (especially chained onto a receiver that implements the interface itself)?

This allows fields' values to be accessed after iterating via Range, now I'm not saying this is useful, I also found this to be an interesting idea so I went ahead and started to apply the hammer to the nail. This is why I'm looking to research existing problems users have with ygot because the impact of this work is going to come from them and not from within ygot itself nor the similarities with proto.

I think the crux of this idea is that we can hide Validate(), or other helper functions, behind such an accessor, which solves the two problems listed in the OP. The more the "other helper functions" pool is, the better the argument I see in taking such an approach. I actually see ygot.GoStruct no longer being deprecated, and ygot.ValidatedGoStruct being deprecated by the new interface ygotreflect.Struct.

wenovus commented 2 years ago

Another idea inspired by someone else is to provide the schema, so

intf := &oc.Interfaces_Interface{}
intf.YgotReflect().Schema() // returns *yang.Entry