Inconsistent Handling of Input Arguments in List Function about leaseweb-go-sdk HOT 7 CLOSED

Completed in #83

from leaseweb-go-sdk.

The issue is not only related to DedicatedServerApi.List function but to all the functions which accepts a args ...interface{} parameter.

To explain the issue with the above example of DedicatedServerApi.List, it is currently impossible to send a request with only a site parameter for example.

Since this parameter is expected to be in the 5th position, we have to specify before "empty" parameters. But this then results in a request with all the parameters being added to the request ?offset=""&limit=""&macAddress=""&ip=""&site=AMS-01 which will have a different behavior than what we want to send ?site=AMS-01

Besides this, it's not really readable for clients to have calls like this DedicatedServerApi{}.List("", "", "", "", site) and is very error-prone.

from leaseweb-go-sdk.

The solution proposed above is unfortunately not really solving the issue since there it is impossible to send a request with onlyprivateNetworkCapable=false for example
(also the signature of the function should be List(opts ListOptions) instead of List(opts ...ListOptions))

from leaseweb-go-sdk.

I found two options which I think could be valid solutions

Option 1

Don't hardcode the parameters in the SDK and require the client to send only the list of parameters it needs.

func (dsa DedicatedServerApi) List(args map[string]string)

params = map[string]string{
	"site": "AMS-01",
	"privateNetworkCapable": "false",
}
DedicatedServerApi{}.List(params)

This makes the SDK very simple as there's no need to hardcode and maintain over time a list of parameters for each request.

As a downside, it requires the developer to rely a bit more on the API doc to figure out the list of parameters accepted and how to send them (boolean -> "true"/"false"), as there won't be any word completion in the editor or compilation check for them.

But to me, checking the API doc is always needed anyway because even now there is no description of what each parameter do precisely (it would need to be added as a go documentation but then we end up copy pasting the whole API doc in the SDK again).

Option 2

Similar to the option proposed initially but each field needs to be an pointer so that it can be nil to indicate its absence.

type ListOptions struct {
	OffSet *int
	Limit *int
	IP *string
	MacAddress *string
	Site *string
	PrivateRackID *string
	PrivateNetworkCapable *bool
	PrivateNetworkEnabled *bool
}

The aws-sdk-go seems to be doing something similar (see for example https://docs.aws.amazon.com/sdk-for-go/api/service/s3/s3manager/#UploadInput) and hetzner too (see for example https://github.com/hetznercloud/hcloud-go/blob/main/hcloud/server.go#L306)

But then in usage it gets more complex because calling it the following way is not allowed as we can't take the address of a constant

opts = ListOptions{
	Site: &"AMS-01",
	PrivateNetworkCapable: &false,
}
DedicatedServerApi{}.List(opts)

so the variable needs to be defined beforehand like

site = "AMS-01"
pnCapable = false
opts = ListOptions{
	Site: &site,
	PrivateNetworkCapable: &pnCapable,
}

or there must be some wrapper around the types (like AWS is doing https://github.com/aws/aws-sdk-go/blob/main/aws/convert_types.go#L6)

s3manager.UploadInput {
	Key: aws.String("key"),
	Bucket: aws.String("bucket"),
}

from leaseweb-go-sdk.

Hello @tardypad @msslr, and thanks for reporting this issue and came up with the solution!
Having multiple arguments of different types and lengths for the list function and similar ones was a temporary solution, and now it's good to decide about the upcoming changes and see what direction we want to go with this!
I appreciate the 2 options which @tardypad provided as a solution!

The downside of the first option as @tardypad mentioned is not clear to use (always needed to check api-docs) which is not the goal of this SDK at the moment! and for the second option, for now I would not prefer to add more complexity to just define arguments!

I came up with a similar thing which still need some consideration! For List function (and similar) of DedicatedServerApi we can do:

import "github.com/google/go-querystring/query"

type DedicatedServerListOption struct {
	Offset                int    `url:"offset"`
	Limit                 int    `url:"limit"`
	Ip                    string `url:"ip"`
	MacAddress            string `url:"macAddress"`
	Site                  string `url:"site"`
	PrivateRackId         string `url:"privateRackId"`
	PrivateNetworkCapable bool   `url:"privateNetworkCapable"`
	PrivateNetworkEnabled bool   `url:"privateNetworkEnabled"`
}

func (dsa DedicatedServerApi) List(listOption ...DedicatedServerListOption) (*DedicatedServers, error) {
	queryValue := getQueryStringByOption(listOption)
	path := dsa.getPath("/servers?" + queryValue)
	result := &DedicatedServers{}
	if err := doRequest(http.MethodGet, path, result); err != nil {
		return nil, err
	}
	return result, nil
}

func getQueryStringByOption(option ...interface{}) string {
	if len(option) == 0 {
		return ""
	}
	q, err := query.Values(option[0])
	if err != nil {
		return ""
	}
	return q.Encode()
}

Pros:

1. It's an optional parameter! In case of need, you can pass the struct as argument!
2. the data types and the names of the attributes are documented, and it's easy to use for developers!

Cons:

1. Hard to maintain (change in API, long naming for option structs)
2. Allowing List function to receive more than 1 parameter, which ATM I think is better than passing nil or empty struct when we don't want to pass anything!

I would like to know your option about this solution, so we can decide next steps!

Thanks @tardypad @msslr

from leaseweb-go-sdk.

Unfortunately the new proposed solution has the same issue as the initial one: when the struct is created all the fields are initialized with their default values and they end up in the query parameters.

To confirm it, I have built the code and tried it.

The code above is not correct, something seems wrong with the use of variadic functions.
Here is the simpler code I tried

type DedicatedServerListOption struct {
	Offset                int    `url:"offset"`
	Limit                 int    `url:"limit"`
	Ip                    string `url:"ip"`
	MacAddress            string `url:"macAddress"`
	Site                  string `url:"site"`
	PrivateRackId         string `url:"privateRackId"`
	PrivateNetworkCapable bool   `url:"privateNetworkCapable"`
	PrivateNetworkEnabled bool   `url:"privateNetworkEnabled"`
}

func (dsa DedicatedServerApi) List(listOption DedicatedServerListOption) (*DedicatedServers, error) {
	q, err := query.Values(listOption)
	if err != nil {
		return nil, err
	}

	path := dsa.getPath("/servers?" + q.Encode())
	result := &DedicatedServers{}
	if err := doRequest(http.MethodGet, path, result); err != nil {
		return nil, err
	}
	return result, nil
}

Which I've then used with the following call

result, err := LSW.DedicatedServerApi{}.List(LSW.DedicatedServerListOption{
	Offset: 0,
	Limit:  20,
})

Monitoring the call made this is what I see
https://api.leaseweb.com/bareMetals/v2/servers?ip=&limit=20&macAddress=&offset=0&privateNetworkCapable=false&privateNetworkEnabled=false&privateRackId=&site=

from leaseweb-go-sdk.

Thank you to everyone who contributed to resolving this issue. We have fixed it with the pull request 83 and I closed this issue now. If any further issues arise, please don't hesitate to open a new one. Thanks again.

from leaseweb-go-sdk.