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

[Julian Edwards]

Add CheckHostedServiceNameAvailability management call.

[Jeroen T. Vermeulen (jtv)]

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

[lp:~julian-edwards/gwacl/availability-response]

207. By Julian Edwards on 2013-07-25

review comments applied

[Julian Edwards (julian-edwards)]

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.

=== modified file 'management_base.go'
--- management_base.go	2013-07-22 12:53:27 +0000
+++ management_base.go	2013-07-25 05:34:30 +0000
@@ -7,6 +7,7 @@
     "encoding/xml"
     "fmt"
     "net/http"
+    "strings"
     "time"
 )
 
@@ -179,6 +180,30 @@
     return api.blockUntilCompleted(response)
 }
 
+// CheckHostedServiceNameAvailability looks to see if the supplied name is
+// acceptable to use as a cloud service name. It returns nil if it is available
+// or an error containing the reason if it is not.  Names may not be acceptable
+// based on reserved words, trademarks and profanity.
+// See http://msdn.microsoft.com/en-us/library/windowsazure/jj154116.aspx
+func (api *ManagementAPI) CheckHostedServiceNameAvailability(name string) error {
+    var err error
+    response, err := api.session.get(
+        "services/hostedservices/operations/isavailable/"+name, "2012-03-01")
+    if err != nil {
+        return err
+    }
+
+    availability := &AvailabilityResponse{}
+    err = availability.Deserialize(response.Body)
+    if err != nil {
+        return err
+    }
+    if strings.ToLower(availability.Result) == "true" {
+        return nil
+    }
+    return fmt.Errorf(availability.Reason)
+}
+
 // DeleteHostedService deletes the named hosted service.
 // See http://msdn.microsoft.com/en-us/library/windowsazure/gg441305.aspx
 func (api *ManagementAPI) DeleteHostedService(serviceName string) error {

=== modified file 'management_base_test.go'
--- management_base_test.go	2013-07-22 12:53:27 +0000
+++ management_base_test.go	2013-07-25 05:34:30 +0000
@@ -549,6 +549,93 @@
     checkOneRequest(c, recordedRequests, expectedURL, "2012-03-01", expectedPayload, "POST")
 }
 
+func makeAvailabilityResponse(result, reason string) string {
+    return fmt.Sprintf(`
+        <?xml version="1.0" encoding="utf-8"?>
+        <AvailabilityResponse xmlns="http://schemas.microsoft.com/windowsazure">
+          <Result>%s</Result>
+          <Reason>%s</Reason>
+        </AvailabilityResponse>`, result, reason)
+}
+
+func (*managementBaseAPISuite) TestAddHostedServiceWithOKName(c *C) {
+    api := makeAPI(c)
+    body := makeAvailabilityResponse("true", "")
+    fixedResponse := x509Response{
+        StatusCode: http.StatusOK,
+        Body:       []byte(body),
+    }
+    rigFixedResponseDispatcher(&fixedResponse)
+    recordedRequests := make([]*X509Request, 0)
+    rigRecordingDispatcher(&recordedRequests)
+
+    serviceName := "service-name"
+    err := api.CheckHostedServiceNameAvailability(serviceName)
+
+    c.Assert(err, IsNil)
+    expectedURL := (AZURE_URL + api.session.subscriptionId +
+        "/services/hostedservices/operations/isavailable/" + serviceName)
+    checkOneRequest(c, &recordedRequests, expectedURL, "2012-03-01", nil, "GET")
+}
+
+func (*managementBaseAPISuite) TestAddHostedServiceWithBadName(c *C) {
+    api := makeAPI(c)
+    reason := "This is a false test response"
+    body := makeAvailabilityResponse("false", reason)
+    fixedResponse := x509Response{
+        StatusCode: http.StatusOK,
+        Body:       []byte(body),
+    }
+    rigFixedResponseDispatcher(&fixedResponse)
+    recordedRequests := make([]*X509Request, 0)
+    rigRecordingDispatcher(&recordedRequests)
+
+    serviceName := "service-name"
+    err := api.CheckHostedServiceNameAvailability(serviceName)
+
+    c.Assert(err, ErrorMatches, reason)
+    c.Check(recordedRequests, HasLen, 1)
+    expectedURL := (AZURE_URL + api.session.subscriptionId +
+        "/services/hostedservices/operations/isavailable/" + serviceName)
+    checkOneRequest(c, &recordedRequests, expectedURL, "2012-03-01", nil, "GET")
+}
+
+func (*managementBaseAPISuite) TestAddHostedServiceWithServerError(c *C) {
+    api := makeAPI(c)
+    fixedResponse := x509Response{
+        StatusCode: http.StatusBadRequest,
+    }
+    rigFixedResponseDispatcher(&fixedResponse)
+    recordedRequests := make([]*X509Request, 0)
+    rigRecordingDispatcher(&recordedRequests)
+
+    serviceName := "service-name"
+    err := api.CheckHostedServiceNameAvailability(serviceName)
+
+    c.Assert(err, ErrorMatches, ".*Bad Request.*")
+}
+
+func (*managementBaseAPISuite) TestAddHostedServiceWithBadXML(c *C) {
+    api := makeAPI(c)
+    body := `
+        <AvailabilityResponse>
+            <Result>foo</Result>
+            <Reason>unclosed tag
+        </AvailabilityResponse>`
+    fixedResponse := x509Response{
+        StatusCode: http.StatusOK,
+        Body:       []byte(body),
+    }
+    rigFixedResponseDispatcher(&fixedResponse)
+    recordedRequests := make([]*X509Request, 0)
+    rigRecordingDispatcher(&recordedRequests)
+
+    serviceName := "service-name"
+    err := api.CheckHostedServiceNameAvailability(serviceName)
+
+    c.Assert(err, ErrorMatches, ".*XML syntax error.*")
+}
+
 func assertDeleteHostedServiceRequest(c *C, api *ManagementAPI, serviceName string, httpRequest *X509Request) {
     expectedURL := fmt.Sprintf("%s%s/services/hostedservices/%s", AZURE_URL,
         api.session.subscriptionId, serviceName)

=== modified file 'xmlobjects.go'
--- xmlobjects.go	2013-07-18 10:51:19 +0000
+++ xmlobjects.go	2013-07-25 05:34:30 +0000
@@ -430,6 +430,18 @@
     return xml.Unmarshal(data, s)
 }
 
+// AvailabilityResponse is the reply from a Check Hosted Service Name
+// Availability operation.
+type AvailabilityResponse struct {
+    XMLNS  string `xml:"xmlns,attr"`
+    Result string `xml:"Result"`
+    Reason string `xml:"Reason"`
+}
+
+func (a *AvailabilityResponse) Deserialize(data []byte) error {
+    return xml.Unmarshal(data, a)
+}
+
 // UpdateHostedService contains the details necessary to call the
 // UpdateHostedService management API call.
 // See http://msdn.microsoft.com/en-us/library/windowsazure/gg441303.aspx

=== modified file 'xmlobjects_test.go'
--- xmlobjects_test.go	2013-07-18 01:45:02 +0000
+++ xmlobjects_test.go	2013-07-25 05:34:30 +0000
@@ -1717,6 +1717,24 @@
     c.Check(cssi.GeoReplicationEnabled, Equals, "false")
 }
 
+func (*xmlSuite) TestAvailabilityResponse(c *C) {
+    input := `
+        <?xml version="1.0" encoding="utf-8"?>
+        <AvailabilityResponse xmlns="http://schemas.microsoft.com/windowsazure">
+          <Result>name-availability</Result>
+          <Reason>reason</Reason>
+        </AvailabilityResponse>`
+    expected := &AvailabilityResponse{
+        XMLNS:  XMLNS,
+        Result: "name-availability",
+        Reason: "reason",
+    }
+    observed := &AvailabilityResponse{}
+    err := observed.Deserialize([]byte(input))
+    c.Assert(err, IsNil)
+    c.Assert(observed, DeepEquals, expected)
+}
+
 func makeUpdateHostedService(label, description string, property ExtendedProperty) string {
     template := dedent.Dedent(`
         <UpdateHostedService xmlns="http://schemas.microsoft.com/windowsazure">