Provide a ListOpts helper

[pierreprinetti]

Here I propose a builder for List queries that allows "OR" clauses.

With query.New, it should be easier to list resources based on repeated properties. For example: get information about multiple ports by ID with a single call.

The properties that can be passed with the method .And(property string, values ...interface{}) are guarded by the fields tagged q: in the base filter. For example: when calling And() on query.New(nertworks.ListOpts{}), the And property name must be one of those defined in the network.ListOpts filter.

For example:

package main

import (
    "fmt"
    "os"

    "github.com/gophercloud/gophercloud/openstack/networking/v2/networks"
    "github.com/gophercloud/gophercloud/pagination"
    "github.com/gophercloud/utils/openstack/clientconfig"
    "github.com/gophercloud/utils/query"
)

func main() {
    networkClient, err := clientconfig.NewServiceClient("network", &clientconfig.ClientOpts{
        Cloud: os.Getenv("OS_CLOUD"),
    })
    if err != nil {
        panic(err)
    }

    q := query.New(networks.ListOpts{
        Status: "ACTIVE",
    }).
        And("name", "port-A", "port-B").
        And("tags", "tag1", "tag2")
        // .And("unexpected-field", "value") <-- this would generate an error when calling `List`

    fmt.Println("Query:", q) // Query: ?name=port-A&name=port-B&status=ACTIVE&tags=tag1&tags=tag2

    if err := networks.List(networkClient, q).EachPage(func(page pagination.Page) (bool, error) {
        n, err := networks.ExtractNetworks(page)
        for _, item := range n {
            fmt.Printf("Found %q\n", item.Name)
        }
        return true, err
    }); err != nil {
        panic(err)
    }
}

ListOpts is currently implemented for three Network resources:

• ports
• networks
• subnets

[mdbooth]

I think this give us exactly what we need. My only concern is that it sacrifices compile-type parameter checking, which is a substantial part of the value of gophercloud. I wonder if we could write something with strongly-typed arguments. If we were keen enough I'm pretty sure it could also be generated from base ListOpts argument.

[pierreprinetti]

It would be nice to be able to easily merge an existing ListOpts object with this

[pierreprinetti]

It would be nice to be able to easily merge an existing ListOpts object with this

or maybe that's a bad idea. Being able to merge a ListOpts with this would give the impression that you can have "this set of properties" OR "this property", while it will just merge the common properties instead.

[mdbooth]

I would be inclined to add a new struct explicitly for each resource which can use it, alongside the existing ListOpts. Maybe (ports|subnets|networks).ListOptsMulti? Little bit more boilerplate, but at least we wouldn't be accidentally adding support where we shouldn't.

[pierreprinetti]

we wouldn't be accidentally adding support where we shouldn't

Can you give me an example of what you mean?

[pierreprinetti]

I have added a bit of reflection. New now takes a struct (possibly, one of the existing ListOpts) and parses the field names in the q:"field-name" tags. And methods will reject fields that aren't defined in the base struct.

This gives a minimal (runtime) type check that may or may not add some safety.

[pierreprinetti]

also @mdbooth @dulek for review

[pierreprinetti]

and @spjmurray if you happen to pass by!

[dulek]

Yup, this looks pretty good to me.

[mdbooth]

we wouldn't be accidentally adding support where we shouldn't

Can you give me an example of what you mean?

Firstly I don't think we are adding support where we shouldn't because we need to implement the relevant interfaces for each point of use, specifically ToPortListQuery, ToNetworkListQuery, and ToSubnetListQuery. We can create one of these generic ListOpts, but we can't use it anywhere except these 3 specific places.

It feels a bit smelly to have this here. If we can successfully make it generic I would probably put it somewhere like openstack/networking/v2/networks/utils.go because it's specifically a neutron thing.

However, this is really an actual part of the API. I feel like it should live in https://github.com/gophercloud/gophercloud/blob/master/openstack/networking/v2/networks/requests.go alongside ListOpts. If we just created ListOptsMulti next to ListOpts which is identical except all its fields are slices instead of singletons then it would be compile-time type safe and very simple: basically just boilerplate. We could use something like this code to reduce the boilerplate and avoid implementation errors.

[spjmurray]

Ah yes, this is what we need!

Just for discussion purposes we could actually get this working today with something like:

import (
        "fmt"
        "net/url"

        "github.com/gophercloud/gophercloud"
        "github.com/gophercloud/gophercloud/openstack/networking/v2/ports"
)

type MyListOpts struct {
        ports.ListOpts

        Name []string `q:"name"`
}

func BuildCompositeQueryString(opts, base interface{}) (url.Values, error) {
        baseURL, err := gophercloud.BuildQueryString(base)
        if err != nil {
                return nil, err
        }

        optsURL, err := gophercloud.BuildQueryString(opts)
        if err != nil {
                return nil, err
        }

        baseQuery := baseURL.Query()

        for parameter, values := range optsURL.Query() {
                baseQuery.Del(parameter)

                for _, value := range values {
                        baseQuery.Add(parameter, value)
                }
        }

        return baseQuery, nil
}

func (opts MyListOpts) ToPortListQuery() (string, error) {
        values, err := BuildCompositeQueryString(opts, opts.ListOpts)
        return values.Encode(), err
}

func main() {
        opts := MyListOpts{
                ListOpts: ports.ListOpts{
                        Status: "detached",
                        Name: "ignored, hopefully",
                },
                Name: []string{
                        "port1",
                        "port2",
                },
        }

        query, err := opts.ToPortListQuery()
        if err != nil {
                fmt.Println("fail", err)
        }

        fmt.Println(query)
}

Kinda ugly though!

The obvious other way would be to have:

type MultiOpts struct {
   Name []string `q:"name"`
}

type ListOpts struct {
  Name string `q:"name"`

  MultiOpts MultiOpts
}