Merge lp:~rvb/gwacl/filtering into lp:gwacl

[Raphaël Badin]

Add the ability for ListSpecificHostedServices to filter out services if they don't match the given prefix.

[Gavin Panella (allenap)]

[1]

 type ListSpecificHostedServicesRequest struct {
-    ServiceNames []string
+    ServiceNames      []string
+    ServiceNamePrefix string
 }

This is kind of ugly. It's muddying the expected behaviour of
ListSpecificHostedServices. I think it also falls into the realms of
Jeroen's Swiss Army Knife antipattern.

How about allowing ServiceNames to contain globs? I don't think that
services can have glob pattern characters in them, so it should be
safe (and they can be escaped if necessary anyway).

See http://golang.org/pkg/path/#Match

Alternatively, you could split the prefix matching functionality out
into a separate ListHostedServicesWithPrefix function.

review: Needs Information

[Raphaël Badin (rvb)]

> [1]
>
> type ListSpecificHostedServicesRequest struct {
> -    ServiceNames []string
> +    ServiceNames      []string
> +    ServiceNamePrefix string
> }
>
> This is kind of ugly. It's muddying the expected behaviour of
> ListSpecificHostedServices. I think it also falls into the realms of
> Jeroen's Swiss Army Knife antipattern.
>
> How about allowing ServiceNames to contain globs? I don't think that
> services can have glob pattern characters in them, so it should be
> safe (and they can be escaped if necessary anyway).
>
> See http://golang.org/pkg/path/#Match
>
> Alternatively, you could split the prefix matching functionality out
> into a separate ListHostedServicesWithPrefix function.

I must say I started with a separate method but then I found that the new functionality was fitting nicely into the existing method. Can you elaborate a bit on what exactly is ugly here? The two constrains do not really step on each other's toes.

[Raphaël Badin (rvb)]

Maybe it's the name of the name that is not fitting any more… because if you look at that method as a filtering method which takes a set of constraints and spits out the list of services whos names match the constraints, it seems okay to me as it is in this branch. Don't you agree?

[Gavin Panella (allenap)]

> I must say I started with a separate method but then I found that the new
> functionality was fitting nicely into the existing method.  Can you elaborate
> a bit on what exactly is ugly here?  The two constrains do not really step on
> each other's toes.

The effect of specifying both ServiceNames and ServiceNamePrefix
overlaps. Without looking at the code or the docs, how does a
developer remember its behaviour when:

- both ServiceNames and ServiceNamePrefix are empty? Does this return
  nothing or everything?

- ServiceNames is empty, but ServiceNamePrefix is "foo"? Does this
  return nothing or everything beginning with "foo"?

- ...

If the two approaches are split into separate functions then the
behaviour becomes self-evident.

Likewise, if you were to stick to just ServiceNames and use glob
patterns the behaviour would be self-evident.

[Raphaël Badin (rvb)]

The glob pattern solution is the best solution to confuse people I think so I went with the original design and added a new method. I hate to add yet another ad-hoc method instead of having a generic filtering method but your point about the behavior being not self-evident without reading the doc is a good one.

[lp:~rvb/gwacl/filtering]

165. By Raphaël Badin on 2013-07-05

Add tListPrefixedHostedServices.

[Gavin Panella (allenap)]

Looks good. Thanks for splitting this out.

[1]

+    if request.ServiceNamePrefix != "" {
+        resServices := []HostedServiceDescriptor{}
+        for _, service := range services {
+            if strings.Index(service.ServiceName, request.ServiceNamePrefix) == 0 {
+                resServices = append(resServices, service)
+            }
+        }
+        services = resServices
+    }
+    return services, nil

This extra conditional is not really needed; it's optimisation only.
Keep it simple; at worst it will copy a slice, which is going to do as
much harm to this program as a fart would to a solar flare.

    resServices := []HostedServiceDescriptor{}
    for _, service := range services {
        if strings.HasPrefix(service.ServiceName, request.ServiceNamePrefix) {
            resServices = append(resServices, service)
        }
    }
    return resServices

Note that I've also changed it to use the HasPrefix function.

[2]

+    prefix := "prefix"
+    service1 := &HostedServiceDescriptor{ServiceName: prefix + "service1"}
+    service2 := makeHostedServiceDescriptor()

This goes back to the argument I was making about locality last week.
It's not obvious that makeHostedServiceDescriptor() creates a service
with a ServiceName that doesn't begin with `prefix`.

Also, consider dispensing with `prefix := "prefix"`. The variable is
used twice in the whole function. It would be clearer to choose a
distinctive prefix and type it in.

    service1 := &HostedServiceDescriptor{ServiceName: "foo123"}
    service2 := &HostedServiceDescriptor{ServiceName: "bar123"}
    ...
    request := &ListPrefixedHostedServicesRequest{
        ServiceNamePrefix: "foo",
    }

The term "prefix" appears eight times in that single test function as
it stands; with this change the term "foo" would appear in it exactly
twice, right where it's relevant, and it would be obvious how the two
services differ.

review: Approve

[lp:~rvb/gwacl/filtering]

166. By Raphaël Badin on 2013-07-08

Review fixes.

[Raphaël Badin (rvb)]

> Looks good. Thanks for splitting this out.
>
>
> [1]
>
> +    if request.ServiceNamePrefix != "" {
> +        resServices := []HostedServiceDescriptor{}
> +        for _, service := range services {
> +            if strings.Index(service.ServiceName, request.ServiceNamePrefix)
> == 0 {
> +                resServices = append(resServices, service)
> +            }
> +        }
> +        services = resServices
> +    }
> +    return services, nil
>
> This extra conditional is not really needed; it's optimisation only.
> Keep it simple; at worst it will copy a slice, which is going to do as
> much harm to this program as a fart would to a solar flare.
>
>    resServices := []HostedServiceDescriptor{}
>    for _, service := range services {
>        if strings.HasPrefix(service.ServiceName, request.ServiceNamePrefix) {
>            resServices = append(resServices, service)
>        }
>    }
>    return resServices
>
> Note that I've also changed it to use the HasPrefix function.

All right, fixed.

> [2]
>
> +    prefix := "prefix"
> +    service1 := &HostedServiceDescriptor{ServiceName: prefix + "service1"}
> +    service2 := makeHostedServiceDescriptor()
>
> This goes back to the argument I was making about locality last week.
> It's not obvious that makeHostedServiceDescriptor() creates a service
> with a ServiceName that doesn't begin with `prefix`.
>
> Also, consider dispensing with `prefix := "prefix"`. The variable is
> used twice in the whole function. It would be clearer to choose a
> distinctive prefix and type it in.
>
>    service1 := &HostedServiceDescriptor{ServiceName: "foo123"}
>    service2 := &HostedServiceDescriptor{ServiceName: "bar123"}
>    ...
>    request := &ListPrefixedHostedServicesRequest{
>        ServiceNamePrefix: "foo",
>    }
>
> The term "prefix" appears eight times in that single test function as
> it stands; with this change the term "foo" would appear in it exactly
> twice, right where it's relevant, and it would be obvious how the two
> services differ.

Yeah, well, we're going to have to agree to disagree on this one :). Also, grep -c prefix management_test.go -> 3 (!= 8)

[Gavin Panella (allenap)]

> Yeah, well, we're going to have to agree to disagree on this one :).

Yeah, I'm fine with that... I'll keep trying to convince you though :)

> Also, grep -c prefix management_test.go -> 3 (!= 8)

Okay, firstly, my mistake. The term "prefix" *without regard to case*
appears 8 times. Secondly, grep -c counts matching lines, not matches.

=== modified file 'management.go'
--- management.go	2013-07-05 09:28:09 +0000
+++ management.go	2013-07-08 07:03:25 +0000
@@ -5,6 +5,7 @@
 
 import (
     "sort"
+    "strings"
 )
 
 type ListInstancesRequest struct {
@@ -96,6 +97,27 @@
     return services, nil
 }
 
+type ListPrefixedHostedServicesRequest struct {
+    ServiceNamePrefix string
+}
+
+// ListPrefixedHostedServices returns a slice containing specific
+// HostedServiceDescriptor objects, insofar as they match the request.
+func (api *ManagementAPI) ListPrefixedHostedServices(request *ListPrefixedHostedServicesRequest) ([]HostedServiceDescriptor, error) {
+    services, err := api.ListHostedServices()
+    if err != nil {
+        return nil, err
+    }
+    resServices := []HostedServiceDescriptor{}
+    for _, service := range services {
+        if strings.HasPrefix(service.ServiceName, request.ServiceNamePrefix) {
+            resServices = append(resServices, service)
+        }
+    }
+    services = resServices
+    return services, nil
+}
+
 type DestroyDeploymentRequest struct {
     ServiceName    string
     DeploymentName string

=== modified file 'management_test.go'
--- management_test.go	2013-07-05 10:47:55 +0000
+++ management_test.go	2013-07-08 07:03:25 +0000
@@ -195,6 +195,33 @@
     c.Assert(recordedRequests, HasLen, 1)
 }
 
+func (suite *managementAPISuite) TestListPrefixedHostedServices(c *C) {
+    prefix := "prefix"
+    service1 := &HostedServiceDescriptor{ServiceName: prefix + "service1"}
+    service2 := makeHostedServiceDescriptor()
+    list := HostedServiceDescriptorList{HostedServices: []HostedServiceDescriptor{*service1, *service2}}
+    XML, err := list.Serialize()
+    c.Assert(err, IsNil)
+    fixedResponse := x509Response{
+        StatusCode: http.StatusOK,
+        Body:       []byte(XML),
+    }
+    rigFixedResponseDispatcher(&fixedResponse)
+    recordedRequests := make([]*X509Request, 0)
+    rigRecordingDispatcher(&recordedRequests)
+
+    api := makeAPI(c)
+    request := &ListPrefixedHostedServicesRequest{
+        ServiceNamePrefix: prefix,
+    }
+    descriptors, err := api.ListPrefixedHostedServices(request)
+
+    // Only the first service is returned.
+    c.Check(descriptors, DeepEquals, []HostedServiceDescriptor{*service1})
+    // Only one request to the API is made.
+    c.Assert(recordedRequests, HasLen, 1)
+}
+
 func (suite *managementAPISuite) TestListSpecificHostedServicesWithoutNamesReturnsNothing(c *C) {
     service1 := makeHostedServiceDescriptor()
     service2 := makeHostedServiceDescriptor()