|
|
DescriptionSecond phase of nova testing service: HTTP API.
Implemented common HTTP API handling as well as flavors API.
This is the first step towards the rest of the HTTP API implementation.
https://code.launchpad.net/~dimitern/goose/nova-testing-service-http-flavors/+merge/139089
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 50
Patch Set 2 : Second phase of nova testing service: HTTP aPI. #
Total comments: 52
Patch Set 3 : Second phase of nova testing service: HTTP API. #
Total comments: 3
MessagesTotal messages: 10
Please take a look.
Sign in to reply to this message.
Not yet got the full logic but so far some first remarks. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:47: ep := strings.TrimRight(n.hostname, "/") + "/" Are multiple trailing slashes common? I found it several times so maybe a unifyHostname() makes sense. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:75: `{"choices": [{"status": "CURRENT", "media-types": [{"base": "application/xml", "type": "application/vnd.openstack.compute+xml;version=2"}, {"base": "application/json", "type": "application/vnd.openstack.compute+json;version=2"}], "id": "v2.0", "links": [{"href": "$ENDPOINT$$URL$", "rel": "self"}]}]}`, Split into multiline with ``+ ``+ ``. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:81: `{"versions": [{"status": "CURRENT", "updated": "2011-01-21T11:33:21Z", "id": "v2.0", "links": [{"href": "$ENDPOINT$", "rel": "self"}]}]}`, Ditto. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:87: `{"version": {"status": "CURRENT", "updated": "2011-01-21T11:33:21Z", "media-types": [{"base": "application/xml", "type": "application/vnd.openstack.compute+xml;version=2"}, {"base": "application/json", "type": "application/vnd.openstack.compute+json;version=2"}], "id": "v2.0", "links": [{"href": "$ENDPOINT$", "rel": "self"}, {"href": "http://docs.openstack.org/api/openstack-compute/1.1/os-compute-devguide-1.1.pdf", "type": "application/pdf", "rel": "describedby"}, {"href": "http://docs.openstack.org/api/openstack-compute/1.1/wadl/os-compute-1.1.wadl", "type": "application/vnd.sun.wadl+xml", "rel": "describedby"}]}}`, Ditto.
Sign in to reply to this message.
generally looks good. here's a first round of comments - i've only had a brief glance at the tests so far. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:122: ep := strings.TrimRight(n.hostname, "/") + "/" i hadn't noticed there was so much duplication between this and buildFlavorLinks. how about a common function or method, say Nova.links(path, id string) []nova.Link, that encapsulates the identical code they both share? so this function would be: func (n *Nova) buildServerLinks(server *nova.ServerDetail) { server.Links = n.links("/servers/", server.Id) } https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:16: var tenantId = "tenant_id" // tenant ID part of the endpoint URL any particular reason these are var not const? https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:26: // verbatim real Nova responses perhaps put these all in a single var block to group them? https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:115: body = strings.Replace(body, "$ENDPOINT$", endpoint(), 1) it might be more usual to use templates to do this. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:129: // workaround https://code.google.com/p/go/issues/detail?id=4454 s/workaround/work around/ or s/workaround/workaround for/ i'm not sure you need this workaround in both send and sendJSON. why not put it in the one place it's actually needed - where we have a StatusNoContent code (you could do that in sendJSON, checking the code there, or in handleFlavors itself). alternatively, you could always set the content length to the correct length, which is trivial because you know the body length in both send and sendJSON https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:142: // handling the error message. // sendError responds with the given error to the // given http request. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:151: data := []byte{} var data []byte https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:177: body = nil does r.Body.Read return an error if there's no body? ISTM that you might be ignoring other errors when there is actually a valid body. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:179: defer r.Body.Close() s/defer// (you don't use r.Body after this, do you?) https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:181: case strings.HasPrefix(cmd, "flavors/detail"): i'm a bit concerned that this switch statement might grow to enormous proportions. it's probably ok at the moment, because it's not clear what the regularities will be yet, you will probably want to consider some kind of table-driven approach to make the code less monololithic. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:231: fallthrough the PUT above gives a JSON response. why is flavors different from flavors/detail in this respect? https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:233: notFoundResponse.send(w, r) not forbidden? https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:248: path := strings.TrimLeft(r.URL.Path, "/") i think this logic could perhaps be made a little clearer by splitting the path up front. but, thinking about it, couldn't we use http's ServeMux to make this logic more straightforward? for instance, i think the following handlers might do a similar thing to your logic here: mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request){ if r.URL.Path == "/" { noVersionResponse.send(w, r) } else { multipleChoicesResponse.send(w, r) } } mux.Handle("/v2/", badRequestResponse) mux.Handle("/v2/tenant_id/", notFoundResponse) mux.Handle("/v2/tenant_id/flavors/", n.handler("/v2/tenant_id/", (*Nova).handleFlavors)) func (n *Nova) handle(prefix string, hf func(*Nova, w http.ResponseWriter, r *http.Request) http.Handler { h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { hf(n, w, r) }) return http.StripPrefix(prefix, h) } I've assumed you've defined a ServeHTTP method on your response type, and that you remove the first two (redundant) arguments from handleFlavors. The one thing this does not do is check that the path doesn't end in /, but i think that's probably reasonable to delegate to the handlers themselves. I may well have missed some crucial detail here! https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:258: if len(urlparts) >= 2 && urlparts[1] != tenantId { s/>= 2/> 1/ (for consistency with the above test) https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_test.go (right): https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:25: ep := strings.TrimRight(hostname, "/") + "/" i'm not sure the trimming is necessary here - we can see that the host name has the right number of slashes just above.
Sign in to reply to this message.
Overall I like where this is at. I haven't reviewed everything, but I figure some feedback is better than none as I get pulled away to something else. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:107: } doesn't this make it a 'versionSuffix'? infix? https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:111: // returns the result as a string. It isn't really "any", for that I would think we would parse the body, and then use a map. Perhaps better stated as: // replaceVars replaces $ENDPOINT$ and $URL$ in resp.body with values taken from the original request, and returns the result as a string. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:180: switch { It feels like we are close to wanting a more data-descriptive approach to writing these switch statements, but this is sufficient for now. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:258: if len(urlparts) >= 2 && urlparts[1] != tenantId { tenantId here looks to be a global. It *really* seems like it should be a member of the Nova service. We may want to leave it as a global for transition purposes, but I'd like to see us switch away from globals as much as possible. So can we add it to Nova without removing the global value for now? https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:49: c.Assert(err, IsNil) I don't know if this is quite as strict as we would want it to be. I'm pretty sure that json.Unmarshal is happy to discard things that don't fit. (I'm sure that it just skips struct fields that aren't present in the JSON, I would guess it does the converse and ignores entries in the JSON that aren't in the struct.) So I don't quite know how to be stricter that the response really matches expected structure without passing in the actual expected content, and then using some sort of reflect.Type magic to unmarshal and then c.Assert(parsed, Equals, expected) https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:78: if len(body) == 0 { it seems a little strange that sometimes body is a 'string' and sometimes body is a []byte. I wonder if we would have a better way to clarify when we have a specific type.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:47: ep := strings.TrimRight(n.hostname, "/") + "/" On 2012/12/11 08:45:01, TheMue wrote: > Are multiple trailing slashes common? I found it several times so maybe a > unifyHostname() makes sense. I changed and simplified this logic. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:122: ep := strings.TrimRight(n.hostname, "/") + "/" On 2012/12/11 10:31:42, rog wrote: > i hadn't noticed there was so much duplication between this and > buildFlavorLinks. > how about a common function or method, say Nova.links(path, id string) > []nova.Link, that encapsulates the identical code they both share? > > so this function would be: > func (n *Nova) buildServerLinks(server *nova.ServerDetail) { > server.Links = n.links("/servers/", server.Id) > } Done. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:16: var tenantId = "tenant_id" // tenant ID part of the endpoint URL On 2012/12/11 10:31:42, rog wrote: > any particular reason these are var not const? Well, not really, I changed the logic and made them const. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:26: // verbatim real Nova responses On 2012/12/11 10:31:42, rog wrote: > perhaps put these all in a single var block to group them? Done. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:75: `{"choices": [{"status": "CURRENT", "media-types": [{"base": "application/xml", "type": "application/vnd.openstack.compute+xml;version=2"}, {"base": "application/json", "type": "application/vnd.openstack.compute+json;version=2"}], "id": "v2.0", "links": [{"href": "$ENDPOINT$$URL$", "rel": "self"}]}]}`, On 2012/12/11 08:45:01, TheMue wrote: > Split into multiline with ``+ ``+ ``. Done. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:81: `{"versions": [{"status": "CURRENT", "updated": "2011-01-21T11:33:21Z", "id": "v2.0", "links": [{"href": "$ENDPOINT$", "rel": "self"}]}]}`, On 2012/12/11 08:45:01, TheMue wrote: > Ditto. Done. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:87: `{"version": {"status": "CURRENT", "updated": "2011-01-21T11:33:21Z", "media-types": [{"base": "application/xml", "type": "application/vnd.openstack.compute+xml;version=2"}, {"base": "application/json", "type": "application/vnd.openstack.compute+json;version=2"}], "id": "v2.0", "links": [{"href": "$ENDPOINT$", "rel": "self"}, {"href": "http://docs.openstack.org/api/openstack-compute/1.1/os-compute-devguide-1.1.pdf", "type": "application/pdf", "rel": "describedby"}, {"href": "http://docs.openstack.org/api/openstack-compute/1.1/wadl/os-compute-1.1.wadl", "type": "application/vnd.sun.wadl+xml", "rel": "describedby"}]}}`, On 2012/12/11 08:45:01, TheMue wrote: > Ditto. Done. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:107: } On 2012/12/11 11:20:20, jameinel wrote: > doesn't this make it a 'versionSuffix'? infix? I removed this and changed the logic. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:111: // returns the result as a string. On 2012/12/11 11:20:20, jameinel wrote: > It isn't really "any", for that I would think we would parse the body, and then > use a map. Perhaps better stated as: > // replaceVars replaces $ENDPOINT$ and $URL$ in resp.body with values taken from > the original request, and returns the result as a string. Done. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:115: body = strings.Replace(body, "$ENDPOINT$", endpoint(), 1) On 2012/12/11 10:31:42, rog wrote: > it might be more usual to use templates to do this. I was thinking of it, but was not sure it'll work, since the {var} replacements could interfere with the JSON data. After discussion on IRC I realized I can change the delimiters to e.g. ${ and }$ and still use the template package, but the code looked ugly and too much for this simple task. So I decided to leave it like before. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:129: // workaround https://code.google.com/p/go/issues/detail?id=4454 On 2012/12/11 10:31:42, rog wrote: > s/workaround/work around/ > or > s/workaround/workaround for/ > > i'm not sure you need this workaround in both send and sendJSON. why not put it > in the one place it's actually needed - where we have a StatusNoContent code > (you could do that in sendJSON, checking the code there, or in handleFlavors > itself). > > alternatively, you could always set the content length to the correct length, > which is trivial because you know the body length in both send and sendJSON Changed to set the Content-Length always. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:142: // handling the error message. On 2012/12/11 10:31:42, rog wrote: > // sendError responds with the given error to the > // given http request. Done. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:151: data := []byte{} On 2012/12/11 10:31:42, rog wrote: > var data []byte Done. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:177: body = nil On 2012/12/11 10:31:42, rog wrote: > does r.Body.Read return an error if there's no body? ISTM that you might be > ignoring other errors when there is actually a valid body. I changed that to check if there is a body explicitly before reading it. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:179: defer r.Body.Close() On 2012/12/11 10:31:42, rog wrote: > s/defer// > (you don't use r.Body after this, do you?) Done. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:180: switch { On 2012/12/11 11:20:20, jameinel wrote: > It feels like we are close to wanting a more data-descriptive approach to > writing these switch statements, but this is sufficient for now. I'm simplifying this a bit. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:181: case strings.HasPrefix(cmd, "flavors/detail"): On 2012/12/11 10:31:42, rog wrote: > i'm a bit concerned that this switch statement might grow to enormous > proportions. it's probably ok at the moment, because it's not clear what the > regularities will be yet, you will probably want to consider some kind of > table-driven approach to make the code less monololithic. It was looking ugly to me already, but was not sure a bunch of ifs will be better. I'll try using multiple ServerMux.Handle calls, as discussed on IRC. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:231: fallthrough On 2012/12/11 10:31:42, rog wrote: > the PUT above gives a JSON response. why is flavors different from > flavors/detail in this respect? I don't know but it's a different response (as sniffed). From looking at the nova source, there is more to it that's documented. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:233: notFoundResponse.send(w, r) On 2012/12/11 10:31:42, rog wrote: > not forbidden? Amazingly, no. But I had issues with canonistack yesterday and it seemed broken, sending nonsense responses for flavors in general. I'll look more into it today. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:248: path := strings.TrimLeft(r.URL.Path, "/") On 2012/12/11 10:31:42, rog wrote: > i think this logic could perhaps be made a little clearer by splitting the path > up front. but, thinking about it, couldn't we use http's ServeMux to make this > logic more straightforward? > > for instance, i think the following handlers might do a similar thing to your > logic here: > > mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request){ > if r.URL.Path == "/" { > noVersionResponse.send(w, r) > } else { > multipleChoicesResponse.send(w, r) > } > } > mux.Handle("/v2/", badRequestResponse) > mux.Handle("/v2/tenant_id/", notFoundResponse) > mux.Handle("/v2/tenant_id/flavors/", n.handler("/v2/tenant_id/", > (*Nova).handleFlavors)) > > > func (n *Nova) handle(prefix string, hf func(*Nova, w http.ResponseWriter, r > *http.Request) http.Handler { > h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { > hf(n, w, r) > }) > return http.StripPrefix(prefix, h) > } > > I've assumed you've defined a ServeHTTP method on your response type, and that > you > remove the first two (redundant) arguments from handleFlavors. > > The one thing this does not do is check that the path doesn't end in /, but i > think that's probably reasonable to delegate to the handlers themselves. > > I may well have missed some crucial detail here! Looks reasonable, I'll see how to change it so it fits better. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:258: if len(urlparts) >= 2 && urlparts[1] != tenantId { On 2012/12/11 10:31:42, rog wrote: > s/>= 2/> 1/ > (for consistency with the above test) I changed that altogether, now using mux.Handle(). https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:258: if len(urlparts) >= 2 && urlparts[1] != tenantId { On 2012/12/11 11:20:20, jameinel wrote: > tenantId here looks to be a global. It *really* seems like it should be a member > of the Nova service. We may want to leave it as a global for transition > purposes, but I'd like to see us switch away from globals as much as possible. > So can we add it to Nova without removing the global value for now? Done. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:49: c.Assert(err, IsNil) On 2012/12/11 11:20:20, jameinel wrote: > I don't know if this is quite as strict as we would want it to be. > I'm pretty sure that json.Unmarshal is happy to discard things that don't fit. > (I'm sure that it just skips struct fields that aren't present in the JSON, I > would guess it does the converse and ignores entries in the JSON that aren't in > the struct.) > > So I don't quite know how to be stricter that the response really matches > expected structure without passing in the actual expected content, and then > using some sort of reflect.Type magic to unmarshal and then > c.Assert(parsed, Equals, expected) My idea here is, if we manage to unmarshal the response to the expected struct, it should be sufficient, because `expected` contains the necessary fields of the response. It 's up to the caller to pass an appropriate `expected` type for the response, thus asserting the JSON structure is at least valid. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:78: if len(body) == 0 { On 2012/12/11 11:20:20, jameinel wrote: > it seems a little strange that sometimes body is a 'string' and sometimes body > is a []byte. I wonder if we would have a better way to clarify when we have a > specific type. Since it's used for tests, I don't feel this to be a big issue - both types are essentially the same with regards to the end result. But fair enough, I'll unify these to be []byte everywhere. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_test.go (right): https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:25: ep := strings.TrimRight(hostname, "/") + "/" On 2012/12/11 10:31:42, rog wrote: > i'm not sure the trimming is necessary here - we can see that the host name has > the right number of slashes just above. Yes, but since it's a global var, it gets changed once the server is started and I did it just to be on the safe side. Anyway, as TheMue also suggested I'll unify these better.
Sign in to reply to this message.
LGTM with some minor comments below. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:28: // endpoint URL from the passed path. s/passed/given/ ? https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:30: This server could not verify that you are authorized to access the ` + if you're going to use + to concatenate parts of the string, it's probably worth doing it for all of it. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:178: if len(data) == 0 { why not just always set content length, as above? you could even make a function so it's the same code used by both places: func writeResponseBody(w http.ResponseWriter, data []byte) { w.Header().Set("Content-Length", strconv.Itoa(len(data))) w.Write(data) } https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:226: if hasSlash { i don't think this can ever happen, because the handler path passed to the ServeMux doesn't have a trailing /, so won't be triggered for sub-paths. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:69: bodyReader := bytes.NewReader(body) or inline https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:74: if headers != nil { d https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:81: // workaround for https://code.google.com/p/go/issues/detail?id=4454 i thought that issue was triggered by the NoContent code only https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:159: s.assertBody(c, resp, badRequestResponse) it seems to me that all these bad-status tests could read well as a table-driven test, but YMMV.
Sign in to reply to this message.
LGTM, but a few suggestions below https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:32: ep += baseURL + "/" maybe this var should be versionPath? But... the concept of a base path is not to be lightly dismissed -- IIRC we had a bug in pyjuju that caused some irritation when we tried to use ec2-alikes whose api base path was not at the top level. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:39: func New(hostname, baseURL, token, tenantId string) *Nova { I'd rather Path than URL, I think. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:72: flavor.Links = n.links("/flavors/", flavor.Id) This method looks pretty redundant now. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:141: server.Links = n.links("/servers/", server.Id) Ditto. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:24: // verbatim real Nova responses I suspect we'll want multiple versions of some of these, but I don't think they're necessary now. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:134: body = strings.Replace(body, "$ENDPOINT$", endpoint(), 1) Not sure it's a big benefit to replace only once -- can we generally be sure there'll only be one reference to, eg $ENDPOINT$ in a response? https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:148: if resp.body != "" { Do we need this condition? https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:163: eresp := errorResponse Nice https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:175: sendError(err, w, r) return? https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:188: // true if handled, false if no errors found. Maybe invert the bool's meaning? Conventions are at war in my mind, so I'm not sure... https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:238: resp.Flavors = entities resp := struct { Flavors []nova.Entity }{entities} (I don't think you need to bother with the explicit json name when it's what would be picked anyway (it will be picked anyway, right?)) https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:272: case "DELETE": case "PUT", "DELETE": https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:296: resp.Flavors = flavors This can be compressed as above. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:321: urlVersion := "/" + n.baseURL + "/" This would be less surprising if the field were "basePath" instead. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:44: func (s *NovaHTTPSuite) assertJSON(c *C, resp *http.Response, expected interface{}) { Doesn't need to be a method. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:54: func (s *NovaHTTPSuite) assertBody(c *C, resp *http.Response, expected response) { ditto https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:287: } These quick error tests would be great for a table, I think, even if the more complex ones stand better alone.
Sign in to reply to this message.
*** Submitted: Second phase of nova testing service: HTTP API. Implemented common HTTP API handling as well as flavors API. This is the first step towards the rest of the HTTP API implementation. R=TheMue, rog, jameinel, fwereade CC= https://codereview.appspot.com/6924043 https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:28: // endpoint URL from the passed path. On 2012/12/12 15:02:33, rog wrote: > s/passed/given/ ? Done. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:32: ep += baseURL + "/" On 2012/12/12 15:18:44, fwereade wrote: > maybe this var should be versionPath? > > But... the concept of a base path is not to be lightly dismissed -- IIRC we had > a bug in pyjuju that caused some irritation when we tried to use ec2-alikes > whose api base path was not at the top level. Done. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:39: func New(hostname, baseURL, token, tenantId string) *Nova { On 2012/12/12 15:18:44, fwereade wrote: > I'd rather Path than URL, I think. Done. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:72: flavor.Links = n.links("/flavors/", flavor.Id) On 2012/12/12 15:18:44, fwereade wrote: > This method looks pretty redundant now. Done. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:141: server.Links = n.links("/servers/", server.Id) On 2012/12/12 15:18:44, fwereade wrote: > Ditto. Done. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:24: // verbatim real Nova responses On 2012/12/12 15:18:44, fwereade wrote: > I suspect we'll want multiple versions of some of these, but I don't think > they're necessary now. In a follow up, I'll remove all that we don't really need and return an error on all the others. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:30: This server could not verify that you are authorized to access the ` + On 2012/12/12 15:02:33, rog wrote: > if you're going to use + to concatenate parts of the string, it's probably worth > doing it for all of it. Not sure how to include the NLs inside - it will be too ugly to have `...`+"\r\n"+... at each EOL. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:134: body = strings.Replace(body, "$ENDPOINT$", endpoint(), 1) On 2012/12/12 15:18:44, fwereade wrote: > Not sure it's a big benefit to replace only once -- can we generally be sure > there'll only be one reference to, eg $ENDPOINT$ in a response? Fair point, I'll make them global. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:148: if resp.body != "" { On 2012/12/12 15:18:44, fwereade wrote: > Do we need this condition? Yes, because I need to know the actual Content-Length to set, after replacing the vars. I had a problem before not setting it correctly (too short, before replacing). And the whole point I need to set it is because of that issue. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:163: eresp := errorResponse On 2012/12/12 15:18:44, fwereade wrote: > Nice Yeah! https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:175: sendError(err, w, r) On 2012/12/12 15:18:44, fwereade wrote: > return? (facepalm) of course! https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:178: if len(data) == 0 { On 2012/12/12 15:02:33, rog wrote: > why not just always set content length, as above? > > you could even make a function so it's the same code used by both places: > > func writeResponseBody(w http.ResponseWriter, data []byte) { > w.Header().Set("Content-Length", strconv.Itoa(len(data))) > w.Write(data) > } Done. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:188: // true if handled, false if no errors found. On 2012/12/12 15:18:44, fwereade wrote: > Maybe invert the bool's meaning? Conventions are at war in my mind, so I'm not > sure... Done. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:226: if hasSlash { On 2012/12/12 15:02:33, rog wrote: > i don't think this can ever happen, because the handler path passed to the > ServeMux doesn't have a trailing /, so won't be triggered for sub-paths. Fair enough, I checked and it's true. I'll remove it, since I'm already checking for trailing slash in unauthorizedNotFound(). I was having trouble with 301's automatically when I have a handler for /path/ and pass /path, so that's why I use the latter now. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:238: resp.Flavors = entities On 2012/12/12 15:18:44, fwereade wrote: > resp := struct { > Flavors []nova.Entity > }{entities} > > (I don't think you need to bother with the explicit json name when it's what > would be picked anyway (it will be picked anyway, right?)) Only when unmarshaling the case does not matter. when marshaling it'll use the exact field names. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:272: case "DELETE": On 2012/12/12 15:18:44, fwereade wrote: > case "PUT", "DELETE": Done. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:296: resp.Flavors = flavors On 2012/12/12 15:18:44, fwereade wrote: > This can be compressed as above. See my comment above (if it's about the json tag). https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:321: urlVersion := "/" + n.baseURL + "/" On 2012/12/12 15:18:44, fwereade wrote: > This would be less surprising if the field were "basePath" instead. Done. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:44: func (s *NovaHTTPSuite) assertJSON(c *C, resp *http.Response, expected interface{}) { On 2012/12/12 15:18:44, fwereade wrote: > Doesn't need to be a method. Done. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:54: func (s *NovaHTTPSuite) assertBody(c *C, resp *http.Response, expected response) { On 2012/12/12 15:18:44, fwereade wrote: > ditto Done. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:69: bodyReader := bytes.NewReader(body) On 2012/12/12 15:02:33, rog wrote: > or inline Done. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:74: if headers != nil { On 2012/12/12 15:02:33, rog wrote: > d Done. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:81: // workaround for https://code.google.com/p/go/issues/detail?id=4454 On 2012/12/12 15:02:33, rog wrote: > i thought that issue was triggered by the NoContent code only Any time when len(body) == 0, hence the transfer-encoding fails to serialize it properly (or the client fails to parse it, whatever). https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:159: s.assertBody(c, resp, badRequestResponse) On 2012/12/12 15:02:33, rog wrote: > it seems to me that all these bad-status tests could read well as a table-driven > test, but YMMV. I was thinking of it, but was not sure whether it'll be a good idea to do all of them as table-based tests. Couldn't come up with a common expressive, yet easy-to-read way to define them. But after discussing it with William I'll do a portion of them in a table, leading to obvious simplification. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:287: } On 2012/12/12 15:18:44, fwereade wrote: > These quick error tests would be great for a table, I think, even if the more > complex ones stand better alone. As discussed, I can implement table-based approach for most of the trivial ones (request(method, url) -> response(code, body)).
Sign in to reply to this message.
a few after-the-fact comments, which you may wish to take account of in a future CL. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http.go:238: resp.Flavors = entities On 2012/12/12 15:18:44, fwereade wrote: > resp := struct { > Flavors []nova.Entity > }{entities} > > (I don't think you need to bother with the explicit json name when it's what > would be picked anyway (it will be picked anyway, right?)) nope. json names are marshalled as is. it's only unmarshalling that has some case-folding logic AFAIK https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:81: // workaround for https://code.google.com/p/go/issues/detail?id=4454 On 2012/12/12 17:32:24, dimitern wrote: > On 2012/12/12 15:02:33, rog wrote: > > i thought that issue was triggered by the NoContent code only > > Any time when len(body) == 0, hence the transfer-encoding fails to serialize it > properly (or the client fails to parse it, whatever). i'd be surprised if that was true. can you reproduce the bug without using StatusNoContent? (also, the bug was in server code, not client code, which this is) https://codereview.appspot.com/6924043/diff/9002/testservices/novaservice/ser... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/6924043/diff/9002/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:107: func makeFlavors(flavor ...nova.FlavorDetail) []nova.FlavorDetail { you could just return flavor here (no need for the append). alternatively, you could do: type flavorSlice []nova.FlavorDetail then instead of the makeFlavors call below, you'd use a more concise version: flavorSlice{{Id: "fl1"}} https://codereview.appspot.com/6924043/diff/9002/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:119: var simpleTests = []struct { much nicer, thanks. https://codereview.appspot.com/6924043/diff/9002/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:234: c.Logf("#%d. %s %s -> %d\n", i+1, t.method, t.url, t.expect.code) it's conventional to print the index (i) unchanged. we don't mind if counting starts at zero.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2012/12/12 18:12:13, rog wrote: > a few after-the-fact comments, which you may wish to take account of in a future > CL. > > https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... > File testservices/novaservice/service_http.go (right): > > https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... > testservices/novaservice/service_http.go:238: resp.Flavors = entities > On 2012/12/12 15:18:44, fwereade wrote: > > resp := struct { > > Flavors []nova.Entity > > }{entities} > > > > (I don't think you need to bother with the explicit json name when it's what > > would be picked anyway (it will be picked anyway, right?)) > > nope. json names are marshalled as is. it's only unmarshalling that has some > case-folding logic AFAIK > > https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... > File testservices/novaservice/service_http_test.go (right): > > https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser... > testservices/novaservice/service_http_test.go:81: // workaround for > https://code.google.com/p/go/issues/detail?id=4454 > On 2012/12/12 17:32:24, dimitern wrote: > > On 2012/12/12 15:02:33, rog wrote: > > > i thought that issue was triggered by the NoContent code only > > > > Any time when len(body) == 0, hence the transfer-encoding fails to serialize > it > > properly (or the client fails to parse it, whatever). > > i'd be surprised if that was true. can you reproduce the bug without using > StatusNoContent? > > (also, the bug was in server code, not client code, which this is) > > https://codereview.appspot.com/6924043/diff/9002/testservices/novaservice/ser... > File testservices/novaservice/service_http_test.go (right): > > https://codereview.appspot.com/6924043/diff/9002/testservices/novaservice/ser... > testservices/novaservice/service_http_test.go:107: func makeFlavors(flavor > ...nova.FlavorDetail) []nova.FlavorDetail { > you could just return flavor here (no need for the append). > > alternatively, you could do: > > type flavorSlice []nova.FlavorDetail > > then instead of the makeFlavors call below, you'd use a more concise version: > > flavorSlice{{Id: "fl1"}} > > https://codereview.appspot.com/6924043/diff/9002/testservices/novaservice/ser... > testservices/novaservice/service_http_test.go:119: var simpleTests = []struct { > much nicer, thanks. > > https://codereview.appspot.com/6924043/diff/9002/testservices/novaservice/ser... > testservices/novaservice/service_http_test.go:234: c.Logf("#%d. %s %s -> %d\n", > i+1, t.method, t.url, t.expect.code) > it's conventional to print the index (i) unchanged. > we don't mind if counting starts at zero. Thanks for the suggestions, I'll add the to the next CL along with the rest of the HTTP API implementation.
Sign in to reply to this message.
|