Loss of data when marshaling model types

[viking66]

Bug Report

• import path: github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-05-01/network

• SDK version: v45.0.0

• go version: go version go1.14.6 darwin/amd64

• What happened?
  I'm working on a project that uses golang to call services within azure to collect data and then marshal that data and pass it to another service in the system which unmarshals that data and processes it. The data I get back from the azure service call contains all the fields I expect but I'm losing data when I marshal it to json.

• What did you expect or want to happen?
  I expect the marshaled data to include all the fields that were present in the service response.

• How can we reproduce it?
  Use golang to call network.ListAllComplete then marshal the data via json.Marshal and inspect the resulting json data. Lots of the fields from nested types are now missing.

• Anything we should know about your environment.
  There's nothing special about my environment.

• Root cause (from my perspective)
  Golang allows you to provide custom marshalers for your types by implementing a MarshalJSON() ([]byte, error) method on the type. Scanning through the code in https://github.com/Azure/azure-sdk-for-go/blob/master/services/network/mgmt/2020-05-01/network/models.go I see there's lots of custom marshalers that only include a subset of the fields from the type they're marshaling.

• Proposed solution
  All the types appear to include json:"foo..." tags which allows golang to automatically handle the json marshaling without having to define custom marshalers and unmarshalers. So as far as I can tell, you should be able to remove the custom (un)marshalers and the problem would be fixed.

• Alternative solution
  Update the custom (un)marshalers to include all the expected fields.

[ArcturusZhang]

Hi @viking66 thanks for this issue.

Sorry for the inconvenience but this is by our go SDK design.

In the definition of our REST API specs for Azure, there are a lot of read-only properties, which are not allowed to be in the request body. The custom marshaller functions are designed to serve the marshalling of the models into a request body, therefore it omits any property that is marked as read-only.

If you want to marshal everything to JSON, just create an alias type, like this:

func main() {
	type test compute.VirtualMachine
	vm := test{
		Name: to.StringPtr("test"),
		Location: to.StringPtr("eastus"),
	}
	b, err := json.Marshal(vm)
	if err != nil {
		panic(err)
	}
	fmt.Println(string(b))
}

You will get the result of

{"name":"test","location":"eastus","tags":null}

As a reference, you could check the custom marshaler definition here, which will ignore name.

This will by-pass golang's marshaller mechanism and gives you every property using the default marshaller.

Hope this helps, thanks

[viking66]

The alias trick works as long as the fields of the struct you alias don't also have custom marshalers. For example, if I try to marshal network.VirtualNetwork using an alias in v45.0.0 of the sdk I still lose data but if I use v44.0.0 I'm not losing the data I care about.

The difference between v44.0.0 and v45.0.0 is how network.VirtualNetwork's network.VirtualNetworkPropertiesFormat embedded type is marshaled. In v44.0.0 network.VirtualNetworkPropertiesFormat does not implement MarshalJSON but in v45.0.0 it does. As a result, in v45.0.0 a field like network.VirtualNetwork.Name gets dropped even when using an alias for VirtualNetwork because of the way go promotes the MarshalJSON method.

[ArcturusZhang]

@viking66 you are totally correct. I overlooked this could be recursive.

And the reason why we have to make this change is that some services do not accept a request body with read-only field assigned - to be specific this time it is the loadbalancer is causing trouble. Please refer to this issue and this issue for more information

@jhendrixMSFT do you have any good solution for this problem?

[jhendrixMSFT]

Our solution for track 2 is to add a tag to any read-only fields.

https://github.com/Azure/autorest.go/blob/track2/test/autorest/generated/complexgroup/models.go#L695

When creating the request, we recursively look for the tag, and if present, we clone the request object graph omitting all read-only fields.

https://github.com/Azure/azure-sdk-for-go/blob/master/sdk/azcore/request.go#L221

You could consider back-porting this to track 1. My only concern here is if any consumers have come to rely on the current behavior.

[ArcturusZhang]

Yeah, back-porting it to track 1 would be an elegant solution. For those are depending on the current behaviour, maybe they could also switch to the approach that are using the cloneWithoutReadOnlyFields method.

I will take a look if it is possible

[jhendrixMSFT]

By dependent on the old behavior, I meant that some customers might prefer that calls to json.Marshal exclude the R/O data. While I think this is unlikely, I'm just speculating based on the complaints we've had with the current behavior. We might need to provide a way to opt-in to this old behavior (I would prefer not to if possible).

[viking66]

Is there any ETA for a solution to this problem?

[AdallomRoy]

We are also experiencing this. Any ETA?
@viking66 as a workaround, you can first serialize the object using structs Map (https://github.com/fatih/structs) and then to json.

[lirenhe]

Our library for the new version will GA this year and we already have the preview https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/network/armnetwork. Please have a try on whether it could solve your problem. We won't fix it in old version of the SDK.

[ghost]

Hi, we're sending this friendly reminder because we haven't heard back from you in a while. We need more information about this issue to help address it. Please be sure to give us your input within the next 7 days. If we don't hear back from you within 14 days of this comment the issue will be automatically closed. Thank you!

[ghost]

Hi, we're sending this friendly reminder because we haven't heard back from you in a while. We need more information about this issue to help address it. Please be sure to give us your input within the next 7 days. If we don't hear back from you within 14 days of this comment the issue will be automatically closed. Thank you!