Problem with Unmarshalling Extensions

I wanted to add the port_security_enabled extension and ran into some problems...

The extension adds support for extra fields in the request and extra fields in the results. I wanted to tackle the latter first, thinking it would be as simple as the other extensions (volume tenants, server availability zones) that were recently added. Reconfiguring the networks package to account for the modified Extract functions/methods caused other network extension packages to break.

Things were going well with fixing the broken extensions until I ran into the provider extension which requires UnmarshalJSON to smooth over the SegmentationID field.

It looks like there's difficulties triggering UnmarshalJSON on anonymous/nested structs:

golang github issue 17877
this might also be relevant.

The result is that the following pattern no longer works:

type NetworkWithExt struct {
  networks.Network
  provider.NetworkProviderExt
}

UnmarshalJSON is triggered, but it strips out the Network struct. This can be worked around by embedding networks.Network in the provider.NetworkProviderExt struct, but that breaks the above pattern and might possibly break when someone wants to add another extension (portsbinding) to the mix.

The latest 4 commits of this branch are my attempt to cleanly show the problem. There are some notes in provider's unit tests with some additional information.

I'm not sure what the best solution would be here. I like the ability to compose structures with extended fields, but if it only works on extensions that don't require type modifications, that makes things a bit complicated.

I think the magical best solution would be if there was a way to retrieve a network or server result and it automatically included all of the extensions, but I have no idea how that would work. Each extension somehow register itself as an extension to its parent. When the parent is unmarshalled, it applies an extract on each registered extension. Since extract would only grab the fields configured in its struct, it would ignore all other fields, which is kind of what's done here. Of course, there's been the discovery of two resources that place arbitrary fields in the JSON body.

In any case, I'm stuck. It's entirely possible I'm overlooking something really simple, too.

I'm not sure when I'd have time to look into this. My initial thought is that defining UnmarshalJSON on Network should fix it:

func (r *Network) UnmarshalJSON(b []byte) error {
	return nil
}

I'm not sure when I'd have time to look into this.

No worries.

My initial thought is that defining UnmarshalJSON on Network should fix it:

When this is in place, UnmarshalJSON on the provider struct isn't triggered:

"json: cannot unmarshal number into Go struct field NetworkWithExt.provider:segmentation_id of type string"

Ah, I see. After reading the package documentation for UnmarshalJSON, it sounds like your findings align with the intended behavior (namely, only one of the UnmarshalJSON methods will get called). That is a bug (in Gophercloud), and I think it can be fixed by adding some logic in the gophercloud package's results.go file. Since this is something that I've claimed should (currently) be workable, I'll prioritize it for Monday.

Sounds good and no problem if you can't get to this any time soon.

I was fiddling around and came up with the following:

func RenderNetworkResult(r pagination.Page, v interface{}) {
  s := reflect.ValueOf(v)
  e := s.Elem()

  for i := 0; i < e.NumField(); i++ {
    f := e.Field(i)
    sliceOf := reflect.SliceOf(f.Type())
    slice := reflect.MakeSlice(sliceOf, 0, 0)

    x := reflect.New(slice.Type())
    x.Elem().Set(slice)
    results := x.Interface()

    ExtractNetworksInto(r, results)
    fmt.Printf("%#v\n", results)
  }
}

Which I was invoking by:

type NetworkWithExt struct {                                                         
  networks.Network                                                                   
  provider.NetworkProviderExt                                                        
}                                                                                    
                                                                                     
r := NetworkWithExt{}                                                                
                                                                                     
allPages, err := networks.List(fake.ServiceClient(), networks.ListOpts{}).AllPages() 
th.AssertNoErr(t, err)                                                               
                                                                                     
networks.RenderNetworkResult(allPages, &r)

I ran out of time and had to stop, but the idea was to inventory a struct, call Extract on each one, which should call UnmarshalJSON on each, and then somehow stitch everything together as one result.

Go issue #19194 sounds like it's close to (if not exactly) this.

I felt like taking a stab at this again tonight.

I came up with https://play.golang.org/p/5R7IJQaBWs and was floored when it actually worked. So I started reviewing #304 to see if I could implement it there. I backtracked to my branch here and changed extractIntoPtr to this:

func (r Result) extractIntoPtr(to interface{}, label string) error {
  if label == "" {
    return r.ExtractInto(&to)
  }

  var m map[string]interface{}
  err := r.ExtractInto(&m)
  if err != nil {
    return err
  }

  b, err := json.Marshal(m[label])
  if err != nil {
    return err
  }

  toValue := reflect.ValueOf(to)
  if toValue.Kind() == reflect.Ptr {
    toValue = toValue.Elem()
  }

  if toValue.Kind() == reflect.Struct {
    for i := 0; i < toValue.NumField(); i++ {
      toField := toValue.Field(i)

      if toField.Kind() == reflect.Struct {
        s := toField.Addr().Interface()
        err = json.NewDecoder(bytes.NewReader(b)).Decode(s)
        if err != nil {
          return err
        }
      }
    }
  }

  err = json.Unmarshal(b, &to)
  return err
}

And the networking/v2/extensions/provider TestGet, TestCreate, and TestUpdate tests passed!

I haven't run a full suite of tests yet. I need to look at the slice stuff, do a full review of that networkv2-extensions-refactor branch, probably rebase it, etc. But maybe this is going in the right direction?

Maybe! See where it takes you :)

I got it! But I need some help.

Handling slices of a "composed" struct was much harder than a plain composed struct.

The core issue I came upon is that the pagination code triggers IsEmpty, which (in the case of Networks), triggers ExtractNetworks, which triggers ExtractNetworksInto.

ExtractNetworksInto is what we want to explicitly call when extracting a result into a composed slice struct.

So pagination is sending the following to ExtractNetworksInto:

[]networks.Network

While we want to also send:

type NetworkWithExt struct {  
  networks.Network            
  provider.NetworkProviderExt 
}                             
var actual []NetworkWithExt

When you start stripping the layers away to iterate on the fields, the former will iterate on the actual networks.Network fields and try to unmarshal the result into, say networks.Network.ID. What we want is to ignore that and only iterate on our composed struct.

I couldn't think of a clean way to do that, so I hacked in a boolean just as a proof-of-concept.

Does that make sense?

I made a new commit to this branch.

You can check it out and do:

$ cd openstack/networking/v2/extensions/provider
$ go test -v ./testing

All tests will pass. It's now a matter of cleaning the implementation up.

Does that make sense?

Maybe? It sounds like the part you want help with is getting rid of the compose parameter (since leaving it in is probably a non-starter). To check for embedded fields, you can try something like this:
https://github.com/gophercloud/gophercloud/pull/304/files#diff-6ccb1cf4a35d1aa16270b9ffeed1e74eR118

Yes, exactly. And that was perfect. I just clobbered my last commit with a new solution using the Anonymous check.

All unit tests pass, but I realized this implementation is simply luck. The idea of using NewDecoder came some example in the reflect package docs. It seems to be doing some kind of deferred unmarshalling or something. If I attempt to remove line 128, things blow up.

I also got very tangled in reflect-land. typeOfV was most likely the result of throwing things at this until something stuck.

In short, I have no idea what I just came up with :)

edit: bah. looks like there's one test in all of Gophercloud that fails... I'll look into that.

Fixed by #443

Vpnaas: VPN service resource (gophercloud#300)

Navigation Menu

Repository navigation

Problem with Unmarshalling Extensions #300

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Problem with Unmarshalling Extensions #300

Description

Activity

jrperritt commented on Mar 18, 2017

jtopjian commented on Mar 18, 2017

jrperritt commented on Mar 18, 2017

jtopjian commented on Mar 18, 2017

jrperritt commented on Mar 22, 2017

jtopjian commented on Aug 3, 2017

jrperritt commented on Aug 3, 2017

jtopjian commented on Aug 4, 2017

jrperritt commented on Aug 4, 2017

jtopjian commented on Aug 4, 2017

jtopjian commented on Aug 7, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions