ARM model struct-type fields should be pointer-to-struct

[axw]

I've just debugged an issue with creating subnets using the ARM APIs. I've successfully created a vnet, and then attempted to create a subnet for it, filling in only the address prefix field. This fails with "400 Bad Request", no further explanation.

Tracing the request, I see the body is serialised as:

{"properties":{"addressPrefix":"10.0.0.0/8","networkSecurityGroup":{}}}

I had previously noticed that struct-type fields of ARM model types (e.g. arm/network/Subnet.Properties, or, as in this case, arm/network/Subnet.Properties.NetworkSecurityGroup) specify "omitempty". This is ineffective for struct-types; you have to use pointer-to-struct.

So I modified the code by hand, changing the NetworkSecurityGroup field to look like:

type Subnet struct {
    ...
    NetworkSecurityGroup *struct {
        Id string `json:"id,omitempty"`
    } `json:"networkSecurityGroup,omitempty"`
    ...
}

and with that, attempting to create the subnet succeeds.

I think this change should be applied to all struct-type fields of all generated model types.

[axw]

Ah... but that poses another problem. The field types are anonymous, so they'll then be nil-pointers, and you'll have no way to set them (no name to refer to). I was going to suggest separately that fields not be anonymous, because it makes constructing parameters a bit tedious. You can't initialise struct-type (or pointer-to-struct-type) fields in a struct literal if the type is anonymous.

I'm not sure how that should look, but first suggestion would just be to concatenate the field name with the enclosing type to create a type name. e.g.

type Subnet struct {
   ...
    Properties *SubnetProperties `json:"properties,omitempty"`
   ...
}

type SubnetProperties struct {
    ...
    NetworkSecurityGroup *SubnetPropertiesNetworkSecurityGroup `json:"networkSecurityGroup,omitempty"`
    ...
}

etc. A bit noisier, I guess, but I'm not sure there's a better way...

[brendandixon]

@axw Let me give this some thought.

[ahmetb]

@axw +1. We use this practice in management package a lot. Also one of widely adopted use cases of pointers in Go especially for designing REST API clients and servers.

[ahmetb]

I just recalled this article https://willnorris.com/2014/05/go-rest-apis-and-pointers

It's also one of the challenges I observed in aws-sdk-go:

• aws.String("xxx") ? aws/aws-sdk-go#363
• []*Type vs. []Type aws/aws-sdk-go#284
• Request Input/Output pointer usage. aws/aws-sdk-go#294

[axw]

@ahmetalpbalkan I personally find the "everything must be a pointer" thing painful, but as was brought up in the aws-sdk-go comments, it's necessary if omitting means something other than the zero value in Go. I don't know the API all that well, so I'll leave it in capable hands :)

[ahmetb]

@axw agreed. Everything is a pointer is one of the solutions to PATCH (merge) requests problem in the article I sent above... We probably don't need that here in the most calls. It seems like what we need here is just omitemtpy on strings and structs. I'll let @brendandixon come up with a solution.

[brendandixon]

@axw The latest update converts the structures to use pointer fields and introduces (via the go-autorest package) helpers to ease manipulating them. Why not take a look and let me know what you think.

[brendandixon]

Changes merged from PR #221

[axw]

@brendandixon Thanks, the changes look good. I'm travelling next week so might not get to test it thoroughly for a little while.

I'm curious to know what the reason for not making enumerator fields pointers. Just because it's a pain to set the value? It doesn't matter as long as they all have an underlying string type, because "" will be omitted anyway; just curious.

[brendandixon]

@axw I keep enums for two reasons: First, if they're truly an enumerated type, then the set of values is all the type can legally hold. Second, Go's strict typing makes creating an easy-to-use helper, as we now have for strings and such, a pain (you'd need a lot of them). Though Go does allow taking the address of the string constant. I don't have strong feelings about this. It felt better than the alternative.

[axw]

@brendandixon Fair enough. Thanks!

[axw]

@brendandixon Apologies for the delay in getting back to you. I've updated my code with the latest azure-sdk-for-go, and things are workable now. Thank you.

My only concern is that the use of pointers for all types makes things a bit clumsy and error-prone in some circumstances; namely when it comes to maps and slices. The results of "List" calls now return pointer-to-slice, so you have to do a nil-check before a "for ... range", which would be unnecessary if you have a nil-slice. I can see people just blindly dereferencing the pointer to get at the slice (same deal for map). This might be fine if Azure always returns "[]" or "{}" for those, but that's an assumption waiting to be tested.

My suggestion, take it or leave it, is to only use pointers for things other than enums, slices and maps.

[brendandixon]

@axw Thanks for the comment. I struggled with that as well. I decided to make them pointers so that the entire slice could be removed. The case I considered was removing all tags from a resource. Perhaps pushing an empty slice would suffice? Perhaps you could experiment and let me know?

[axw]

@brendandixon Right, I see. omitempty will omit both a nil and a non-nil but empty map. That's a pain.

I think the only real alternative would be to write your own marshalling. e.g.:

    type VirtualMachine struct {
        ...
        Tags map[string]*string
        ...
    }

    type virtualMachineWire struct {
        ...
        Tags *map[string]*string `json:"tags,omitempty"`
        ...
    }

    func (v *VirtualMachine) MarshalJSON() ([]byte, error) {
        var wire virtualMachineWire
        ...
        if v.Tags != nil {
            wire.Tags = &v.Tags
        }
        ...
        return json.Marshal(&wire)
    }

That's possible going too far. I can suck it up.

[axw]

@brendandixon The core issue is resolved anyway, so please close this issue at your discretion.

[brendandixon]

@axw All right. Closing.