Merge lp:~wallyworld/gwacl/prefix-service-match into lp:gwacl

Proposed by Ian Booth on 2014-12-04
Status: Rejected
Rejected by: Ian Booth on 2014-12-04
Proposed branch: lp:~wallyworld/gwacl/prefix-service-match
Merge into: lp:gwacl
Diff against target: 119 lines (+52/-17)
2 files modified
management.go (+14/-2)
management_test.go (+38/-15)
To merge this branch: bzr merge lp:~wallyworld/gwacl/prefix-service-match
Reviewer Review Type Date Requested Status
Andrew Wilkins (community) 2014-12-04 Approve on 2014-12-04
Review via email: mp+243620@code.launchpad.net

Commit message

When finding services using a prefix match, it was possible to accidentally include additional unwanted services in the result. This branch allows a separator to be specified when invoking the ListPrefixedHostedServices() API. The separator is considered to sit between the prefix and the rest of the service name.
Thus if a separator of "-" is specified with a prefix of "azure", then:

azure-service will match

azure-1-service will not match

Description of the change

When finding services using a prefix match, it was possible to accidentally include additional unwanted services in the result. This branch allows a separator to be specified when invoking the ListPrefixedHostedServices() API. The separator is considered to sit between the prefix and the rest of the service name.
Thus if a separator of "-" is specified with a prefix of "azure", then:

azure-service will match

azure-1-service will not match

To post a comment you must log in.
Andrew Wilkins (axwalk) wrote :

Feels like kind of a weird overloading; I kinda think we'd be better off just ditching the use of ListPrefixedHostedServices and use ListHostedServices and regex match in Juju.

But... LGTM, I don't want to hold up the fix.

review: Approve

Unmerged revisions

241. By Ian Booth on 2014-12-04

Allow prefixed searches to specify a separator

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'management.go'
2--- management.go 2014-12-02 00:36:45 +0000
3+++ management.go 2014-12-04 06:21:52 +0000
4@@ -5,8 +5,8 @@
5
6 import (
7 "fmt"
8+ "regexp"
9 "sort"
10- "strings"
11 )
12
13 type ListInstancesRequest struct {
14@@ -100,6 +100,7 @@
15
16 type ListPrefixedHostedServicesRequest struct {
17 ServiceNamePrefix string
18+ PrefixSeparator string
19 }
20
21 // ListPrefixedHostedServices returns a slice containing specific
22@@ -109,9 +110,20 @@
23 if err != nil {
24 return nil, err
25 }
26+ // Service names are prefixed with request.ServiceNamePrefix, optionally followed by a separator.
27+ // If a separator is specified, we must be careful not to include services where the specified prefix
28+ // is a substring of the true prefix. ie we mustn't allow "azure" to match "azure-1".
29+ var prefixMatch *regexp.Regexp
30+ if request.PrefixSeparator != "" {
31+ prefixMatch = regexp.MustCompile(
32+ fmt.Sprintf("^%s%s[^%s]*$", request.ServiceNamePrefix, request.PrefixSeparator, request.PrefixSeparator),
33+ )
34+ } else {
35+ prefixMatch = regexp.MustCompile(fmt.Sprintf("^%s.*$", request.ServiceNamePrefix))
36+ }
37 resServices := []HostedServiceDescriptor{}
38 for _, service := range services {
39- if strings.HasPrefix(service.ServiceName, request.ServiceNamePrefix) {
40+ if prefixMatch.Match([]byte(service.ServiceName)) {
41 resServices = append(resServices, service)
42 }
43 }
44
45=== modified file 'management_test.go'
46--- management_test.go 2014-12-02 00:36:45 +0000
47+++ management_test.go 2014-12-04 06:21:52 +0000
48@@ -193,33 +193,56 @@
49 c.Assert(recordedRequests, HasLen, 1)
50 }
51
52-func (suite *managementAPISuite) TestListPrefixedHostedServices(c *C) {
53- prefix := "prefix"
54- service1 := &HostedServiceDescriptor{ServiceName: prefix + "service1"}
55- service2 := makeHostedServiceDescriptor()
56- list := HostedServiceDescriptorList{HostedServices: []HostedServiceDescriptor{*service1, *service2}}
57- XML, err := list.Serialize()
58- c.Assert(err, IsNil)
59- fixedResponse := x509Response{
60- StatusCode: http.StatusOK,
61- Body: []byte(XML),
62- }
63- rigFixedResponseDispatcher(&fixedResponse)
64+func (suite *managementAPISuite) checkPrefixedHostedServices(c *C, prefix, separator string, expected *HostedServiceDescriptor) {
65 recordedRequests := make([]*X509Request, 0)
66 rigRecordingDispatcher(&recordedRequests)
67-
68 api := makeAPI(c)
69 request := &ListPrefixedHostedServicesRequest{
70 ServiceNamePrefix: prefix,
71+ PrefixSeparator: separator,
72 }
73 descriptors, err := api.ListPrefixedHostedServices(request)
74+ c.Assert(err, IsNil)
75
76- // Only the first service is returned.
77- c.Check(descriptors, DeepEquals, []HostedServiceDescriptor{*service1})
78+ // Only the expected service is returned.
79+ c.Check(descriptors, DeepEquals, []HostedServiceDescriptor{*expected})
80 // Only one request to the API is made.
81 c.Assert(recordedRequests, HasLen, 1)
82 }
83
84+func (suite *managementAPISuite) TestListPrefixedHostedServicesNoSeparator(c *C) {
85+ prefix := "prefix_abc"
86+ service1 := &HostedServiceDescriptor{ServiceName: prefix + "-service1"}
87+ service2 := makeHostedServiceDescriptor()
88+ list := HostedServiceDescriptorList{HostedServices: []HostedServiceDescriptor{*service1, *service2}}
89+ XML, err := list.Serialize()
90+ c.Assert(err, IsNil)
91+ fixedResponse := x509Response{
92+ StatusCode: http.StatusOK,
93+ Body: []byte(XML),
94+ }
95+ rigFixedResponseDispatcher(&fixedResponse)
96+
97+ suite.checkPrefixedHostedServices(c, prefix, "", service1)
98+}
99+
100+func (suite *managementAPISuite) TestListPrefixedHostedServicesWithSeparator(c *C) {
101+ prefix := "prefix_abc"
102+ service1 := &HostedServiceDescriptor{ServiceName: prefix + "-service1"}
103+ service2 := &HostedServiceDescriptor{ServiceName: prefix + "-1-service2"}
104+ service3 := makeHostedServiceDescriptor()
105+ list := HostedServiceDescriptorList{HostedServices: []HostedServiceDescriptor{*service1, *service2, *service3}}
106+ XML, err := list.Serialize()
107+ c.Assert(err, IsNil)
108+ fixedResponse := x509Response{
109+ StatusCode: http.StatusOK,
110+ Body: []byte(XML),
111+ }
112+ rigFixedResponseDispatcher(&fixedResponse)
113+
114+ suite.checkPrefixedHostedServices(c, prefix, "-", service1)
115+}
116+
117 func (suite *managementAPISuite) TestListSpecificHostedServicesWithoutNamesReturnsNothing(c *C) {
118 service1 := makeHostedServiceDescriptor()
119 service2 := makeHostedServiceDescriptor()

Subscribers

People subscribed via source and target branches

to all changes: