Merge lp:~julian-edwards/gwacl/availability-response into lp:gwacl

Proposed by Julian Edwards on 2013-07-25
Status: Merged
Approved by: Julian Edwards on 2013-07-25
Approved revision: 207
Merged at revision: 200
Proposed branch: lp:~julian-edwards/gwacl/availability-response
Merge into: lp:gwacl
Diff against target: 192 lines (+142/-0)
4 files modified
management_base.go (+25/-0)
management_base_test.go (+87/-0)
xmlobjects.go (+12/-0)
xmlobjects_test.go (+18/-0)
To merge this branch: bzr merge lp:~julian-edwards/gwacl/availability-response
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) 2013-07-25 Approve on 2013-07-25
Review via email: mp+176855@code.launchpad.net

Commit message

Add CheckHostedServiceNameAvailability management call.

To post a comment you must log in.
Jeroen T. Vermeulen (jtv) wrote :

Well done, well tested. Testing against bad XML in the response, and against bugs in your test helper, is particularly thorough. It's a shame that test setup is always such a pain in Go.

.

The interface for CheckHostedServiceNameAvailability makes perfect sense to me, but I suspect part of its documentation was written before it became clear to what extent the Azure API's version deserved to be hidden:

16 +// CheckHostedServiceNameAvailability returns true if the supplied name is
17 +// available to use as a cloud service name. It returns nil if it is available
18 +// or an error containing the reason if it is not.

The function now returns nil if the name can be used, not true as that first comment line says.

By the way, it would help to hide a bit of unjustified specificity in Azure's API here. The focus on availability is misleading. A user might well decide that the only sensible way to check for name availability is to allocate it, and ignore this function — or worse, remove a call to this function from client code. But we also need this function to detect failures from name-based blocking.

Maybe you could say "acceptable" instead of "available," and add something like "a name might be unacceptable because it is already in use, or because it contains a pattern that is rejected by Azure policy, such as a trademarked or a controversial word."

I half wonder whether AddHostedService should use the new function to help diagnose Azure errors with status codes 400 and 409, just so we can return decently specific error objects. But programmatically identifiable error objects are just not part of Go idiom — or at least, combining them with helpful error strings isn't.

.

In TestAddHostedServiceWithBadName, you do:

96 + c.Check(recordedRequests, HasLen, 1)

No need for that. It's built into checkOneRequest.

review: Approve
207. By Julian Edwards on 2013-07-25

review comments applied

Julian Edwards (julian-edwards) wrote :

On 25/07/13 15:21, Jeroen T. Vermeulen wrote:
> Review: Approve
>
> Well done, well tested. Testing against bad XML in the response, and against bugs in your test helper, is particularly thorough. It's a shame that test setup is always such a pain in Go.
>
> .
>
> The interface for CheckHostedServiceNameAvailability makes perfect sense to me, but I suspect part of its documentation was written before it became clear to what extent the Azure API's version deserved to be hidden:
>
> 16 +// CheckHostedServiceNameAvailability returns true if the supplied name is
> 17 +// available to use as a cloud service name. It returns nil if it is available
> 18 +// or an error containing the reason if it is not.

Yes caught in my own headlights here. Fixed, thanks.

>
> The function now returns nil if the name can be used, not true as that first comment line says.
>
> By the way, it would help to hide a bit of unjustified specificity in Azure's API here. The focus on availability is misleading. A user might well decide that the only sensible way to check for name availability is to allocate it, and ignore this function — or worse, remove a call to this function from client code. But we also need this function to detect failures from name-based blocking.
>
> Maybe you could say "acceptable" instead of "available," and add something like "a name might be unacceptable because it is already in use, or because it contains a pattern that is rejected by Azure policy, such as a trademarked or a controversial word."
>
> I half wonder whether AddHostedService should use the new function to help diagnose Azure errors with status codes 400 and 409, just so we can return decently specific error objects. But programmatically identifiable error objects are just not part of Go idiom — or at least, combining them with helpful error strings isn't.

Possibly, but I would much rather make random IDs UUIDs instead.

>
> .
>
> In TestAddHostedServiceWithBadName, you do:
>
> 96 + c.Check(recordedRequests, HasLen, 1)
>
> No need for that. It's built into checkOneRequest.
>

Ta.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'management_base.go'
2--- management_base.go 2013-07-22 12:53:27 +0000
3+++ management_base.go 2013-07-25 05:34:30 +0000
4@@ -7,6 +7,7 @@
5 "encoding/xml"
6 "fmt"
7 "net/http"
8+ "strings"
9 "time"
10 )
11
12@@ -179,6 +180,30 @@
13 return api.blockUntilCompleted(response)
14 }
15
16+// CheckHostedServiceNameAvailability looks to see if the supplied name is
17+// acceptable to use as a cloud service name. It returns nil if it is available
18+// or an error containing the reason if it is not. Names may not be acceptable
19+// based on reserved words, trademarks and profanity.
20+// See http://msdn.microsoft.com/en-us/library/windowsazure/jj154116.aspx
21+func (api *ManagementAPI) CheckHostedServiceNameAvailability(name string) error {
22+ var err error
23+ response, err := api.session.get(
24+ "services/hostedservices/operations/isavailable/"+name, "2012-03-01")
25+ if err != nil {
26+ return err
27+ }
28+
29+ availability := &AvailabilityResponse{}
30+ err = availability.Deserialize(response.Body)
31+ if err != nil {
32+ return err
33+ }
34+ if strings.ToLower(availability.Result) == "true" {
35+ return nil
36+ }
37+ return fmt.Errorf(availability.Reason)
38+}
39+
40 // DeleteHostedService deletes the named hosted service.
41 // See http://msdn.microsoft.com/en-us/library/windowsazure/gg441305.aspx
42 func (api *ManagementAPI) DeleteHostedService(serviceName string) error {
43
44=== modified file 'management_base_test.go'
45--- management_base_test.go 2013-07-22 12:53:27 +0000
46+++ management_base_test.go 2013-07-25 05:34:30 +0000
47@@ -549,6 +549,93 @@
48 checkOneRequest(c, recordedRequests, expectedURL, "2012-03-01", expectedPayload, "POST")
49 }
50
51+func makeAvailabilityResponse(result, reason string) string {
52+ return fmt.Sprintf(`
53+ <?xml version="1.0" encoding="utf-8"?>
54+ <AvailabilityResponse xmlns="http://schemas.microsoft.com/windowsazure">
55+ <Result>%s</Result>
56+ <Reason>%s</Reason>
57+ </AvailabilityResponse>`, result, reason)
58+}
59+
60+func (*managementBaseAPISuite) TestAddHostedServiceWithOKName(c *C) {
61+ api := makeAPI(c)
62+ body := makeAvailabilityResponse("true", "")
63+ fixedResponse := x509Response{
64+ StatusCode: http.StatusOK,
65+ Body: []byte(body),
66+ }
67+ rigFixedResponseDispatcher(&fixedResponse)
68+ recordedRequests := make([]*X509Request, 0)
69+ rigRecordingDispatcher(&recordedRequests)
70+
71+ serviceName := "service-name"
72+ err := api.CheckHostedServiceNameAvailability(serviceName)
73+
74+ c.Assert(err, IsNil)
75+ expectedURL := (AZURE_URL + api.session.subscriptionId +
76+ "/services/hostedservices/operations/isavailable/" + serviceName)
77+ checkOneRequest(c, &recordedRequests, expectedURL, "2012-03-01", nil, "GET")
78+}
79+
80+func (*managementBaseAPISuite) TestAddHostedServiceWithBadName(c *C) {
81+ api := makeAPI(c)
82+ reason := "This is a false test response"
83+ body := makeAvailabilityResponse("false", reason)
84+ fixedResponse := x509Response{
85+ StatusCode: http.StatusOK,
86+ Body: []byte(body),
87+ }
88+ rigFixedResponseDispatcher(&fixedResponse)
89+ recordedRequests := make([]*X509Request, 0)
90+ rigRecordingDispatcher(&recordedRequests)
91+
92+ serviceName := "service-name"
93+ err := api.CheckHostedServiceNameAvailability(serviceName)
94+
95+ c.Assert(err, ErrorMatches, reason)
96+ c.Check(recordedRequests, HasLen, 1)
97+ expectedURL := (AZURE_URL + api.session.subscriptionId +
98+ "/services/hostedservices/operations/isavailable/" + serviceName)
99+ checkOneRequest(c, &recordedRequests, expectedURL, "2012-03-01", nil, "GET")
100+}
101+
102+func (*managementBaseAPISuite) TestAddHostedServiceWithServerError(c *C) {
103+ api := makeAPI(c)
104+ fixedResponse := x509Response{
105+ StatusCode: http.StatusBadRequest,
106+ }
107+ rigFixedResponseDispatcher(&fixedResponse)
108+ recordedRequests := make([]*X509Request, 0)
109+ rigRecordingDispatcher(&recordedRequests)
110+
111+ serviceName := "service-name"
112+ err := api.CheckHostedServiceNameAvailability(serviceName)
113+
114+ c.Assert(err, ErrorMatches, ".*Bad Request.*")
115+}
116+
117+func (*managementBaseAPISuite) TestAddHostedServiceWithBadXML(c *C) {
118+ api := makeAPI(c)
119+ body := `
120+ <AvailabilityResponse>
121+ <Result>foo</Result>
122+ <Reason>unclosed tag
123+ </AvailabilityResponse>`
124+ fixedResponse := x509Response{
125+ StatusCode: http.StatusOK,
126+ Body: []byte(body),
127+ }
128+ rigFixedResponseDispatcher(&fixedResponse)
129+ recordedRequests := make([]*X509Request, 0)
130+ rigRecordingDispatcher(&recordedRequests)
131+
132+ serviceName := "service-name"
133+ err := api.CheckHostedServiceNameAvailability(serviceName)
134+
135+ c.Assert(err, ErrorMatches, ".*XML syntax error.*")
136+}
137+
138 func assertDeleteHostedServiceRequest(c *C, api *ManagementAPI, serviceName string, httpRequest *X509Request) {
139 expectedURL := fmt.Sprintf("%s%s/services/hostedservices/%s", AZURE_URL,
140 api.session.subscriptionId, serviceName)
141
142=== modified file 'xmlobjects.go'
143--- xmlobjects.go 2013-07-18 10:51:19 +0000
144+++ xmlobjects.go 2013-07-25 05:34:30 +0000
145@@ -430,6 +430,18 @@
146 return xml.Unmarshal(data, s)
147 }
148
149+// AvailabilityResponse is the reply from a Check Hosted Service Name
150+// Availability operation.
151+type AvailabilityResponse struct {
152+ XMLNS string `xml:"xmlns,attr"`
153+ Result string `xml:"Result"`
154+ Reason string `xml:"Reason"`
155+}
156+
157+func (a *AvailabilityResponse) Deserialize(data []byte) error {
158+ return xml.Unmarshal(data, a)
159+}
160+
161 // UpdateHostedService contains the details necessary to call the
162 // UpdateHostedService management API call.
163 // See http://msdn.microsoft.com/en-us/library/windowsazure/gg441303.aspx
164
165=== modified file 'xmlobjects_test.go'
166--- xmlobjects_test.go 2013-07-18 01:45:02 +0000
167+++ xmlobjects_test.go 2013-07-25 05:34:30 +0000
168@@ -1717,6 +1717,24 @@
169 c.Check(cssi.GeoReplicationEnabled, Equals, "false")
170 }
171
172+func (*xmlSuite) TestAvailabilityResponse(c *C) {
173+ input := `
174+ <?xml version="1.0" encoding="utf-8"?>
175+ <AvailabilityResponse xmlns="http://schemas.microsoft.com/windowsazure">
176+ <Result>name-availability</Result>
177+ <Reason>reason</Reason>
178+ </AvailabilityResponse>`
179+ expected := &AvailabilityResponse{
180+ XMLNS: XMLNS,
181+ Result: "name-availability",
182+ Reason: "reason",
183+ }
184+ observed := &AvailabilityResponse{}
185+ err := observed.Deserialize([]byte(input))
186+ c.Assert(err, IsNil)
187+ c.Assert(observed, DeepEquals, expected)
188+}
189+
190 func makeUpdateHostedService(label, description string, property ExtendedProperty) string {
191 template := dedent.Dedent(`
192 <UpdateHostedService xmlns="http://schemas.microsoft.com/windowsazure">

Subscribers

People subscribed via source and target branches

to all changes: