Merge lp:~rvb/gomaasapi/record-action-params into lp:gomaasapi

[Raphaël Badin]

Make the testservice record the url.Values posted when performing an operation (e.g. starting a node).

I chose to add another field named "nodeOperationRequestValues" instead of making the existing one (TestServer.nodeOperations) more complex because the new structure would have been a pain to manipulate in Go.

The main reason behind that change is so that we can check that the provider sends the correct "user data" when starting a node: https://code.launchpad.net/~rvb/juju-core/fix-test-start-instance/+merge/158116.

[lp:~rvb/gomaasapi/record-action-params]

39. By Raphaël Badin on 2013-04-10

Add check.

[Gavin Panella (allenap)]

[1]

+ nodeOperationRequestValues map[string][]url.Values

This needs a comment to explain what the string is meant to denote. Or
a type alias could be used (I guess; Go's probably got some gotcha up
it's sleeve to make that awkward).

[2]

+func (server *TestServer) NodeOperationRequestValues() map[string][]url.Values {
+ return server.nodeOperationRequestValues
+}

Why not make the field public in the first place?

If you want to ensure that this field is read-only field, then maybe
this'll work: it'll return by value (?) and so copy the returned map
(?), but will it copy the values in the map or references? In other
words, will the values be shared?

I think I hate pass by value.

[3]

+ operationRequestValues, _ := server.nodeOperationRequestValues[systemId]

Can you add a comment explaining why the second return value is being
ignored? I think I get it from reading the code (the `present` value
from the preceding line is sufficient) but it would be worth helping
future devs out.

[4]

+ if request.Body != nil {
...
+ requestValues, err = url.ParseQuery(string(body))

The condition here should be that there's a body *and* that the
Content-Type is application/x-www-form-encoded.

(I think the test service ought to be strict about what it accepts.)

[5]

+ nodeOperationRequestValues map[string][]url.Values

This expands to:

 nodeOperationRequestValues map[string][]map[string][]string

Which, having read around a bit, reads as:

  A mapping of system IDs to lists of mappings of query parameters.

Or, in Python syntax:

  {"sys1": [
      {"param1": ["value1", "value2"], "param2": ["value3"]},
      {...}
      ],
   "sys2": [
       ...
       ],
   ...
  }

Is that right?

review: Needs Information

[lp:~rvb/gomaasapi/record-action-params]

40. By Raphaël Badin on 2013-04-11

Check request's content-type.

[Raphaël Badin (rvb)]

> [1]

Done.

>
>
> [2]
>
> +func (server *TestServer) NodeOperationRequestValues()
> map[string][]url.Values {
> + return server.nodeOperationRequestValues
> +}
>
> Why not make the field public in the first place?
>
> If you want to ensure that this field is read-only field, then maybe
> this'll work: it'll return by value (?) and so copy the returned map
> (?), but will it copy the values in the map or references? In other
> words, will the values be shared?

It's not really a matter of making the field read-only (remember this is only the testservice code), I just want us to be able to change the underlying implementation without breaking the existing code.

> [3]
>
> + operationRequestValues, _ :=
> server.nodeOperationRequestValues[systemId]
>
> Can you add a comment explaining why the second return value is being
> ignored? I think I get it from reading the code (the `present` value
> from the preceding line is sufficient) but it would be worth helping
> future devs out.

That's the reason indeed. I've changed the code so that it will panic if the two variables (present and present2) don't have the same value.

> [4]
>
> + if request.Body != nil {
> ...
> + requestValues, err = url.ParseQuery(string(body))
>
> The condition here should be that there's a body *and* that the
> Content-Type is application/x-www-form-encoded.
>
> (I think the test service ought to be strict about what it accepts.)

Good point, done.

> [5]
>
> + nodeOperationRequestValues map[string][]url.Values
>
> This expands to:
>
> nodeOperationRequestValues map[string][]map[string][]string
>
> Which, having read around a bit, reads as:
>
>  A mapping of system IDs to lists of mappings of query parameters.
>
> Or, in Python syntax:
>
>  {"sys1": [
>      {"param1": ["value1", "value2"], "param2": ["value3"]},
>      {...}
>      ],
>   "sys2": [
>       ...
>       ],
>   ...
>  }
>
> Is that right?

That's right.

[Gavin Panella (allenap)]

review: Approve

=== modified file 'testservice.go'
--- testservice.go	2013-04-10 01:36:19 +0000
+++ testservice.go	2013-04-11 07:31:23 +0000
@@ -53,13 +53,17 @@
 // library.
 type TestServer struct {
 	*httptest.Server
-	serveMux       *http.ServeMux
-	client         Client
-	nodes          map[string]MAASObject
-	ownedNodes     map[string]bool
+	serveMux   *http.ServeMux
+	client     Client
+	nodes      map[string]MAASObject
+	ownedNodes map[string]bool
+	// mapping system_id -> list of operations performed.
 	nodeOperations map[string][]string
-	files          map[string]MAASObject
-	version        string
+	// mapping system_id -> list of Values passed when performing
+	// operations
+	nodeOperationRequestValues map[string][]url.Values
+	files                      map[string]MAASObject
+	version                    string
 }
 
 func getNodeURI(version, systemId string) string {
@@ -78,6 +82,7 @@
 	server.nodes = make(map[string]MAASObject)
 	server.ownedNodes = make(map[string]bool)
 	server.nodeOperations = make(map[string][]string)
+	server.nodeOperationRequestValues = make(map[string][]url.Values)
 	server.files = make(map[string]MAASObject)
 }
 
@@ -87,14 +92,40 @@
 	return server.nodeOperations
 }
 
-func (server *TestServer) addNodeOperation(systemId, operation string) {
+// NodeOperationRequestValues returns the map containing the list of the
+// url.Values extracted from the request used when performing operations
+// on nodes.
+func (server *TestServer) NodeOperationRequestValues() map[string][]url.Values {
+	return server.nodeOperationRequestValues
+}
+
+func (server *TestServer) addNodeOperation(systemId, operation string, request *http.Request) {
 	operations, present := server.nodeOperations[systemId]
+	operationRequestValues, present2 := server.nodeOperationRequestValues[systemId]
+	if present != present2 {
+		panic("inconsistent state: nodeOperations and nodeOperationRequestValues don't have the same keys.")
+	}
+	requestValues := url.Values{}
+	if request.Body != nil && request.Header.Get("Content-Type") == "application/x-www-form-urlencoded" {
+		defer request.Body.Close()
+		body, err := ioutil.ReadAll(request.Body)
+		if err != nil {
+			panic(err)
+		}
+		requestValues, err = url.ParseQuery(string(body))
+		if err != nil {
+			panic(err)
+		}
+	}
 	if !present {
 		operations = []string{operation}
+		operationRequestValues = []url.Values{requestValues}
 	} else {
 		operations = append(operations, operation)
+		operationRequestValues = append(operationRequestValues, requestValues)
 	}
 	server.nodeOperations[systemId] = operations
+	server.nodeOperationRequestValues[systemId] = operationRequestValues
 }
 
 // NewNode creates a MAAS node.  The provided string should be a valid json
@@ -245,7 +276,7 @@
 		// The only operations supported are "start", "stop" and "release".
 		if operation == "start" || operation == "stop" || operation == "release" {
 			// Record operation on node.
-			server.addNodeOperation(systemId, operation)
+			server.addNodeOperation(systemId, operation, r)
 
 			if operation == "release" {
 				delete(server.OwnedNodes(), systemId)

=== modified file 'testservice_test.go'
--- testservice_test.go	2013-03-12 16:27:02 +0000
+++ testservice_test.go	2013-04-11 07:31:23 +0000
@@ -97,26 +97,43 @@
 func (suite *TestServerSuite) TestClearClearsData(c *C) {
 	input := `{"system_id": "mysystemid"}`
 	suite.server.NewNode(input)
-	suite.server.addNodeOperation("mysystemid", "start")
+	suite.server.addNodeOperation("mysystemid", "start", &http.Request{})
 
 	suite.server.Clear()
 
 	c.Check(len(suite.server.nodes), Equals, 0)
 	c.Check(len(suite.server.nodeOperations), Equals, 0)
+	c.Check(len(suite.server.nodeOperationRequestValues), Equals, 0)
 }
 
 func (suite *TestServerSuite) TestAddNodeOperationPopulatesOperations(c *C) {
 	input := `{"system_id": "mysystemid"}`
 	suite.server.NewNode(input)
 
-	suite.server.addNodeOperation("mysystemid", "start")
-	suite.server.addNodeOperation("mysystemid", "stop")
+	suite.server.addNodeOperation("mysystemid", "start", &http.Request{})
+	suite.server.addNodeOperation("mysystemid", "stop", &http.Request{})
 
 	nodeOperations := suite.server.NodeOperations()
 	operations := nodeOperations["mysystemid"]
 	c.Check(operations, DeepEquals, []string{"start", "stop"})
 }
 
+func (suite *TestServerSuite) TestAddNodeOperationPopulatesOperationRequestValues(c *C) {
+	input := `{"system_id": "mysystemid"}`
+	suite.server.NewNode(input)
+	reader := strings.NewReader("key=value")
+	request, err := http.NewRequest("POST", "http://example.com/", reader)
+	request.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	c.Assert(err, IsNil)
+
+	suite.server.addNodeOperation("mysystemid", "start", request)
+
+	values := suite.server.NodeOperationRequestValues()
+	value := values["mysystemid"]
+	c.Check(len(value), Equals, 1)
+	c.Check(value[0], DeepEquals, url.Values{"key": []string{"value"}})
+}
+
 func (suite *TestServerSuite) TestNewNodeRequiresJSONString(c *C) {
 	input := `invalid:json`
 	defer func() {