ygot icon indicating copy to clipboard operation
ygot copied to clipboard

Unable to oc.Unmarshal the Annotation type (should I be able to?) to OC structs

Open sysbot opened this issue 5 years ago • 6 comments

My understand by looking at [1], it's assumed that the ygot.Annotation type are generated per the oc structs. So as long as the custom Annotation implementations implemented with MarshalJSON/UnmarshalJSON then Unmarshaling should work. However, with the following example, I will get this error

panic: parent container device (type *exampleoc.Device): JSON contains unexpected field @system

Which is expected because there's no mapping. My question is how would one map the ΛSystem which represented as @system when marshaled into JSON back into the OC struct? One example structs below from examples in [2]

type ExampleAnnotation struct {
	ConfigSource string `json:"cfg-source"`
}

// MarshalJSON marshals the ExampleAnnotation receiver to JSON.
func (e *ExampleAnnotation) MarshalJSON() ([]byte, error) {
	return json.Marshal(*e)
}

// UnmarshalJSON ensures that ExampleAnnotation implements the ygot.Annotation
// interface. It is stubbed out and unimplemented.
func (e *ExampleAnnotation) UnmarshalJSON(b []byte) error {
	return fmt.Errorf("this is called")
}

Main example.

package main

import (
	"encoding/json"
	"fmt"

	oc "github.com/openconfig/ygot/exampleoc"

	"github.com/kylelemons/godebug/pretty"
)

func main() {
	x := &oc.Device{}

	err := oc.Unmarshal([]byte(`{
	"@system": [
		{
		  "name": "demodemodemo"
		}
	],
	"system": {
		"config": {
		  "domain-name": "rtcdn.caffeine.tv",
		  "hostname": "local-0-0"
		}
	}
}`), x)
	if err != nil {
		panic(err)
	}
	fmt.Printf("%v\n", pretty.Sprint(x))
}

In another example I marshall this into JSON

	d := &oc.Device{
		ΛMetadata: []ygot.Annotation{
			&HostAnnotation{Name: "demodemodemo"},
		},
		System: &oc.System{
			Hostname: ygot.String("rtr02.pop44"),
			ΛHostname: []ygot.Annotation{
				&ExampleAnnotation{ConfigSource: "devicedemo"},
			},
		},
	}

Which results in

{
 "@": []
}

oc.Unmarshal() will unable to read that @ as a known and mappable field.

[1] https://raw.githubusercontent.com/openconfig/ygot/master/exampleoc/oc.go [2] https://github.com/openconfig/ygot/blob/master/demo/device/devicedemo.go#L50

sysbot avatar Aug 27 '19 16:08 sysbot

I see this: "// TODO(robjs): Implement unmarshalling annotations" @robshakir

wenovus avatar Aug 27 '19 21:08 wenovus

Oh wow @wenovus that's a great find. Thank you.

sysbot avatar Aug 27 '19 21:08 sysbot

Yes, as of today we don't have an implementation for unmarshalling annotations - since the initial user that we implemented this for didn't require this functionality. We have an option to be able to ignore additional fields so that the output can be unmarshalled, ignoring the metadata.

Adding unmarshalling of these types is a case of:

  • Determining a way whereby a set of structs that the annotation could be are handed to the Unmarshal function. This is because the annotations are, by design, opaque to the generated code, so we don't necessarily know in ytypes what types we might receive there.
  • Adding a check in checkDataTreeAgainstPaths in ytypes to be able to tell that the field is an annotation (we have a utility function for this). We don't necessarily need to use the @ name to identify this, because we can do it from the struct field type, which is annotated with the @name path in the tag.
  • If it is an annotation, comparing this against the set of types that we were provided as potential annotations - if the data can be unmarshalled into one of the types, then we would create it and repopulate the annotation field.

This should be relatively easy to implement, and we're more than happy to review designs or PRs to add this if this is something that you need!

Cheers, r.

robshakir avatar Aug 27 '19 22:08 robshakir

Thanks @robshakir. I'm happy to help with this (not at the moment but as soon as I get some time) and will study the original implementation for adding Marshaling of Annotation. Appreciate the pointers on what needs to be done but I'm still really new to the codebase so navigation help would be appreciated.

What's the option currently for ignoring the metadata?

  1. I'm assuming the determination whereby a set of structs that the annotation could are handle at [1]. When looking at [2], what are the Tag values in this case? Are those the @ and @name that are read from the JSON? How are these related to fields? They seemed to be use interchangeably here.

  2. In [3], the tag check using [4], it seemed that the top level annotation would not get split correctly with

   "@": [
      {
         "name": "demodemodemo"
      }
   ],

due it attempting to split the : in the field/tag.

How to made the mapping between "@" to struct field type at this point reading in the JSON?

  1. types that we were provided do you have pointers to this code path? Thanks.

Thanks,

[1] https://github.com/openconfig/ygot/blob/2896b11e7f14c6d5615b7be53c9d8663493712df/ytypes/container.go#L146 [2] https://github.com/openconfig/ygot/blob/2896b11e7f14c6d5615b7be53c9d8663493712df/ytypes/container.go#L157 [3] https://github.com/openconfig/ygot/blob/2896b11e7f14c6d5615b7be53c9d8663493712df/ytypes/util_schema.go#L219 [4] https://github.com/openconfig/ygot/blob/2896b11e7f14c6d5615b7be53c9d8663493712df//util/path.go#L256:6

sysbot avatar Aug 27 '19 23:08 sysbot

@robshakir I managed to review the code for ygot a bit more today and I have a better sense of things now.

My proposal for getting the structs that would be handled by Unmarshal in ygot can be done via a global map similar to Gob's Register function (https://golang.org/pkg/encoding/gob/#Register). So before calling Unmarshal, one need to register the types with for example ygot.RegisterAnnotation(&map[string]interface{}{}) then accessing this global var from within the unmarshal functions. The rest is to map the Annotation fields into these structs.

sysbot avatar Aug 30 '19 00:08 sysbot

Thanks @sysbot for looking into this. I'm also relatively new to this codebase; however, I do see this working and I probably would go about it the same way. Sorry it took so long for me to get to this, but if you're interested in this feature still, @robshakir or I would be happy to review.

Assuming the code iterates through the Annotation types, and if any UnmarshalJSON succeeds, then the field is set, I have a couple general questions about what what should be done:

  • What if it's unmarshallable to multiple Annotation implementations?
  • What if it's not unmarshallable to any Annotation implementation?

wenovus avatar Jun 02 '20 01:06 wenovus