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

Proposed by Ian Booth
Status: Rejected
Rejected by: Ian Booth
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) Approve
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.
Revision history for this message
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

Allow prefixed searches to specify a separator

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'management.go'
--- management.go 2014-12-02 00:36:45 +0000
+++ management.go 2014-12-04 06:21:52 +0000
@@ -5,8 +5,8 @@
55
6import (6import (
7 "fmt"7 "fmt"
8 "regexp"
8 "sort"9 "sort"
9 "strings"
10)10)
1111
12type ListInstancesRequest struct {12type ListInstancesRequest struct {
@@ -100,6 +100,7 @@
100100
101type ListPrefixedHostedServicesRequest struct {101type ListPrefixedHostedServicesRequest struct {
102 ServiceNamePrefix string102 ServiceNamePrefix string
103 PrefixSeparator string
103}104}
104105
105// ListPrefixedHostedServices returns a slice containing specific106// ListPrefixedHostedServices returns a slice containing specific
@@ -109,9 +110,20 @@
109 if err != nil {110 if err != nil {
110 return nil, err111 return nil, err
111 }112 }
113 // Service names are prefixed with request.ServiceNamePrefix, optionally followed by a separator.
114 // If a separator is specified, we must be careful not to include services where the specified prefix
115 // is a substring of the true prefix. ie we mustn't allow "azure" to match "azure-1".
116 var prefixMatch *regexp.Regexp
117 if request.PrefixSeparator != "" {
118 prefixMatch = regexp.MustCompile(
119 fmt.Sprintf("^%s%s[^%s]*$", request.ServiceNamePrefix, request.PrefixSeparator, request.PrefixSeparator),
120 )
121 } else {
122 prefixMatch = regexp.MustCompile(fmt.Sprintf("^%s.*$", request.ServiceNamePrefix))
123 }
112 resServices := []HostedServiceDescriptor{}124 resServices := []HostedServiceDescriptor{}
113 for _, service := range services {125 for _, service := range services {
114 if strings.HasPrefix(service.ServiceName, request.ServiceNamePrefix) {126 if prefixMatch.Match([]byte(service.ServiceName)) {
115 resServices = append(resServices, service)127 resServices = append(resServices, service)
116 }128 }
117 }129 }
118130
=== modified file 'management_test.go'
--- management_test.go 2014-12-02 00:36:45 +0000
+++ management_test.go 2014-12-04 06:21:52 +0000
@@ -193,33 +193,56 @@
193 c.Assert(recordedRequests, HasLen, 1)193 c.Assert(recordedRequests, HasLen, 1)
194}194}
195195
196func (suite *managementAPISuite) TestListPrefixedHostedServices(c *C) {196func (suite *managementAPISuite) checkPrefixedHostedServices(c *C, prefix, separator string, expected *HostedServiceDescriptor) {
197 prefix := "prefix"
198 service1 := &HostedServiceDescriptor{ServiceName: prefix + "service1"}
199 service2 := makeHostedServiceDescriptor()
200 list := HostedServiceDescriptorList{HostedServices: []HostedServiceDescriptor{*service1, *service2}}
201 XML, err := list.Serialize()
202 c.Assert(err, IsNil)
203 fixedResponse := x509Response{
204 StatusCode: http.StatusOK,
205 Body: []byte(XML),
206 }
207 rigFixedResponseDispatcher(&fixedResponse)
208 recordedRequests := make([]*X509Request, 0)197 recordedRequests := make([]*X509Request, 0)
209 rigRecordingDispatcher(&recordedRequests)198 rigRecordingDispatcher(&recordedRequests)
210
211 api := makeAPI(c)199 api := makeAPI(c)
212 request := &ListPrefixedHostedServicesRequest{200 request := &ListPrefixedHostedServicesRequest{
213 ServiceNamePrefix: prefix,201 ServiceNamePrefix: prefix,
202 PrefixSeparator: separator,
214 }203 }
215 descriptors, err := api.ListPrefixedHostedServices(request)204 descriptors, err := api.ListPrefixedHostedServices(request)
205 c.Assert(err, IsNil)
216206
217 // Only the first service is returned.207 // Only the expected service is returned.
218 c.Check(descriptors, DeepEquals, []HostedServiceDescriptor{*service1})208 c.Check(descriptors, DeepEquals, []HostedServiceDescriptor{*expected})
219 // Only one request to the API is made.209 // Only one request to the API is made.
220 c.Assert(recordedRequests, HasLen, 1)210 c.Assert(recordedRequests, HasLen, 1)
221}211}
222212
213func (suite *managementAPISuite) TestListPrefixedHostedServicesNoSeparator(c *C) {
214 prefix := "prefix_abc"
215 service1 := &HostedServiceDescriptor{ServiceName: prefix + "-service1"}
216 service2 := makeHostedServiceDescriptor()
217 list := HostedServiceDescriptorList{HostedServices: []HostedServiceDescriptor{*service1, *service2}}
218 XML, err := list.Serialize()
219 c.Assert(err, IsNil)
220 fixedResponse := x509Response{
221 StatusCode: http.StatusOK,
222 Body: []byte(XML),
223 }
224 rigFixedResponseDispatcher(&fixedResponse)
225
226 suite.checkPrefixedHostedServices(c, prefix, "", service1)
227}
228
229func (suite *managementAPISuite) TestListPrefixedHostedServicesWithSeparator(c *C) {
230 prefix := "prefix_abc"
231 service1 := &HostedServiceDescriptor{ServiceName: prefix + "-service1"}
232 service2 := &HostedServiceDescriptor{ServiceName: prefix + "-1-service2"}
233 service3 := makeHostedServiceDescriptor()
234 list := HostedServiceDescriptorList{HostedServices: []HostedServiceDescriptor{*service1, *service2, *service3}}
235 XML, err := list.Serialize()
236 c.Assert(err, IsNil)
237 fixedResponse := x509Response{
238 StatusCode: http.StatusOK,
239 Body: []byte(XML),
240 }
241 rigFixedResponseDispatcher(&fixedResponse)
242
243 suite.checkPrefixedHostedServices(c, prefix, "-", service1)
244}
245
223func (suite *managementAPISuite) TestListSpecificHostedServicesWithoutNamesReturnsNothing(c *C) {246func (suite *managementAPISuite) TestListSpecificHostedServicesWithoutNamesReturnsNothing(c *C) {
224 service1 := makeHostedServiceDescriptor()247 service1 := makeHostedServiceDescriptor()
225 service2 := makeHostedServiceDescriptor()248 service2 := makeHostedServiceDescriptor()

Subscribers

People subscribed via source and target branches

to all changes: