UpCloudLtd / upcloud-go-api

Go client for UpCloud's API
https://pkg.go.dev/github.com/UpCloudLtd/upcloud-go-api/v6
MIT License
42 stars 9 forks source link

Can we have yaml marshalling directives as well as xml #31

Closed james-nesbitt closed 4 years ago

james-nesbitt commented 7 years ago

All of the API related structs have excellent XML marshalling metadata directives attached, such as the following example:

type CreateServerRequest struct {
...
    AvoidHost  string `xml:"avoid_host,omitempty"`
    BootOrder  string `xml:"boot_order,omitempty"`
    CoreNumber int    `xml:"core_number,omitempty"`
...
}

Alternative encoding and decoding would be much simpler if the same metadata was provided for other marshalling/unmarshalling libraries such as json and yaml.

An example would be:

type CreateServerRequest struct {
...
    AvoidHost  string `xml:"avoid_host,omitempty" yaml:"AvoidHost,omitempty" json:"avoid_host,omitempty"`
    BootOrder  string `xml:"boot_order,omitempty" yaml:"BootOrder,omitempty" json:"boot_order,omitempty"`
    CoreNumber int    `xml:"core_number,omitempty" yaml:"CoreNumber,omitempty"  json:"core_number,omitempty"`
...
}
james-nesbitt commented 7 years ago

@Jalle19: I created this issue as a pointer for some colleagues, who told me that they were interested in contributing some effort.

Jalle19 commented 7 years ago

@james-nesbitt I started out developing this SDK with the JSON API exclusively in mind. I had to start using XML instead though because the API returns a very weird JSON structure which is hard to serialize/deserialize nicely. Here's an example:

{
  "account": {
    "credits": "10000",
    "username": "username"
  }
}

This means you can't have an Account object, because that would be serialized to this:

{
  "credits": "10000",
  "username": "username"
}

I asked UpCloud about this and they said it was due to the API using XML behind the scenes, and the JSON serializer doesn't really cope with that. They said they would potentially improve the situation in the next major API version (no release date whatsoever for that though).

I'm assuming the same situation would apply to YAML.

james-nesbitt commented 7 years ago

Yeah there are a lot of artifacts of XML usage inside their API.

While I am not suggesting that we use XML for the API->UpCloud processes, I would like the metadata added so that the structs used for creating requests can come from YAML.

Currently I am using your API, but feeding in data from YAML files. In order to get the yml to populate structs I have to duplicate your structs with metadata, and then use the duplicates to create your structs. Adding the metadata wouldn't break any existing functionality.

james-nesbitt commented 7 years ago

BTW the example that you gave can have an ugly solution by using the JSON UnMarshaller interface, but it would require a lot of silly mapping.

Jalle19 commented 7 years ago

Yes that's what I realized too while working on this. Do you think a YAML representation of structs will map correctly without custom marshaling logic? If so feel free to make a PR.

Generally I'm not very keen on adding YAML support since the API itself doesn't support it at all, but I guess it wouldn't hurt as long as it doesn't complicate things.

james-nesbitt commented 7 years ago

Right now I have servers starting and stopping using this approach:

https://github.com/james-nesbitt/kraut-project-template/blob/master/.kraut/upcloud.yml#L28

But I have to admit that I am having problems with the firewall rules (I keep getting response back saying that IPs and Ports are required.)

I think that the conversion code that I am using is not up to date in branch, but I will show you the effort that I have to go through to get YML converted to your structs:

https://github.com/james-nesbitt/kraut-handlers/blob/upcloud-manager/upcloud/factory_configyml.go#L368

(please don't be offended my very obtuse code, it is a POC, and a work in progress)

Jalle19 commented 7 years ago

You should add some debug print statements to the service class so you can see the XML that is generated for the request. You can also try the Postman collection that the SDK ships with too do further debugging if you're unsure what a request should look like.

james-nesbitt commented 7 years ago

sorry for delays here. Holidays got in the way. I will look at this again.

Jalle19 commented 7 years ago

Thanks!

PopoSensei commented 4 years ago

Hello again from 2020. One of the new updates to this API/SDK is using the JSON over XML. No yaml plans now nor in the future, so closing this issue. Please raise this issue again if there is still need for the support, so we can review the feature request again! :)