Swift test service double implemented.

Swift test service double implemented.

Implements a minimal number of OS Swift API, needed for juju testing.

https://code.launchpad.net/~dimitern/goose/swift-testing-service/+merge/136362

(do not edit description out of merge proposal)

[dimitern, 2012-11-27 10:41:39]

Please take a look.

[jameinel, 2012-11-27 11:03:07]

I think this is reasonable to land as a first draft of the SWIFT services, and
we can just iterate from here.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
File testservices/swiftservice/service.go (right):

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:44: return fmt.Errorf("container already
exists %q", name)
If at the HTTP request level it is not an error to AddContainer something that
already exists, I probably wouldn't make it an error at this level.
Instead, we could just return "(created bool)".

Note that we'll want doc comments for all of these APIs eventually.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:96: path := strings.Replace(r.URL.Path,
s.baseURL, "", -1)
I don't think we want Replace(-1) here. As if the base URL was something like
"foo/" that would replace all occurances. So "foo/foo/foo/" would end up as just
"".
Probably what we really want is just to look if the prefix matches and then
remove it. More like:
if r.URL.Path[:len(s.baseURL)] == s.baseURL { path =
r.URL.Path[len(s.baseURL):]}
And probably an error if it doesn't match?

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:98: path = strings.TrimRight(path, "/")
We can probably just unconditionally TrimRight here.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:106: }
I think that eventually we'll want to have knowledge of multiple users and
tokens, etc. But this is reasonable as a first step.
We'll probably want some generic code for checking Auth-Tokens (residing in
identityservice?)

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:111: return
If err != nil then we say Accepted? Shouldn't that be "if err == nil" ?

Maybe I'm missing the api of AddContainer (it is acceptable to try to PUT a
container that already exists.)
We'll want to make sure to get test coverage of that.

This is the sort of thing where you sort of want TDD, because it is easy to
forget that you handled this case, and then the test case doesn't get written.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:141: }
Ultimately, we probably want something like 'gorest' to do the work of finding
what methods need to be connected. But honestly, this doesn't look too bad.
I would probably split it up more as:
if len(parts) == 1 { DoContainerAPI(params) }
elif len(parts) == 2 {DoObjectAPI(params) }
else { w.Write(ERROR) }
And then the container stuff of GET/DELETE/PUT are all nicely clustered, and the
Object stuff is also nicely clustered, and you don't repeat the 'len(parts)'
checks everywhere.

[dfc, 2012-11-27 11:13:33]

Very nice.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
File testservices/swiftservice/service.go (right):

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:51: _, err := s.GetObject(container, name)
This can be expressed more compactly

if _, err := ... ; err != nil { 

}

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:58: if ok := s.HasContainer(container); !ok
{
same

if err := s.AddContainer... ; err != nil {
   return err
}

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:78: _, err := s.GetObject(container, name)
same as above

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:82: s.containers[container][name] = nil
not sure if you need to nil out the reference here, the delete below will orphan
it.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:88: _, err := s.GetObject(container,
object)
same as above

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:92: return fmt.Sprintf("%s%s%s/%s",
s.hostname, s.baseURL, container, object), nil
Pedantic people would suggest using path.Join() here, but I think this is
reasonable.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:100: parts := strings.Split(path, "/")
You should use path.Split here

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:107: if len(parts) == 1 && r.Method ==
"PUT" {
Maybe a switch within a switch {

switch len(parts) {
case 1: 
    switch r.Method {
    case "PUT"

} 

and so forth

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/swift...
File testservices/swiftservice/swiftservice.go (right):

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/swift...
testservices/swiftservice/swiftservice.go:6: 
Comments ?

[gz, 2012-11-27 11:46:35]

Seems like a good start, a few cleanup suggestions for the service.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
File testservices/swiftservice/service.go (right):

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:100: parts := strings.Split(path, "/")
I would have container and object name explicitly here and check which is nil,
rather than leaving as parts.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:142: w.WriteHeader(http.StatusNotFound)
This throws away the err string and returns no body?

[dimitern, 2012-11-27 18:57:30]

Please take a look.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
File testservices/swiftservice/service.go (right):

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:44: return fmt.Errorf("container already
exists %q", name)
On 2012/11/27 11:03:07, john.meinel wrote:
> If at the HTTP request level it is not an error to AddContainer something that
> already exists, I probably wouldn't make it an error at this level.
> Instead, we could just return "(created bool)".
> 
> Note that we'll want doc comments for all of these APIs eventually.

Sure I'll add doc comments. Not sure about the error though - it seems to me
nice and consistent this way. Care to elaborate why not?

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:51: _, err := s.GetObject(container, name)
On 2012/11/27 11:13:33, dfc wrote:
> This can be expressed more compactly
> 
> if _, err := ... ; err != nil { 
> 
> }
Sure, I was not sure, but I'll do it, 10x

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:58: if ok := s.HasContainer(container); !ok
{
On 2012/11/27 11:13:33, dfc wrote:
> same
> 
> if err := s.AddContainer... ; err != nil {
>    return err
> }

Done.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:78: _, err := s.GetObject(container, name)
On 2012/11/27 11:13:33, dfc wrote:
> same as above

Done.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:82: s.containers[container][name] = nil
On 2012/11/27 11:13:33, dfc wrote:
> not sure if you need to nil out the reference here, the delete below will
orphan
> it.
Again, was not sure delete is enough, I'll change it, 10x.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:88: _, err := s.GetObject(container,
object)
On 2012/11/27 11:13:33, dfc wrote:
> same as above

Done.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:92: return fmt.Sprintf("%s%s%s/%s",
s.hostname, s.baseURL, container, object), nil
On 2012/11/27 11:13:33, dfc wrote:
> Pedantic people would suggest using path.Join() here, but I think this is
> reasonable.
Some strings have slashes, some don't so that's why I concatenate them like
this.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:96: path := strings.Replace(r.URL.Path,
s.baseURL, "", -1)
On 2012/11/27 11:03:07, john.meinel wrote:
> I don't think we want Replace(-1) here. As if the base URL was something like
> "foo/" that would replace all occurances. So "foo/foo/foo/" would end up as
just
> "".
> Probably what we really want is just to look if the prefix matches and then
> remove it. More like:
> if r.URL.Path[:len(s.baseURL)] == s.baseURL { path =
> r.URL.Path[len(s.baseURL):]}
> And probably an error if it doesn't match?
Right, makes sense

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:98: path = strings.TrimRight(path, "/")
On 2012/11/27 11:03:07, john.meinel wrote:
> We can probably just unconditionally TrimRight here.
Sure, just being precautions

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:100: parts := strings.Split(path, "/")
On 2012/11/27 11:13:33, dfc wrote:
> You should use path.Split here

Ok, good to know, I'll

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:100: parts := strings.Split(path, "/")
On 2012/11/27 11:46:35, gz wrote:
> I would have container and object name explicitly here and check which is nil,
> rather than leaving as parts.

I agree, I'll do it.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:106: }
On 2012/11/27 11:03:07, john.meinel wrote:
> I think that eventually we'll want to have knowledge of multiple users and
> tokens, etc. But this is reasonable as a first step.
> We'll probably want some generic code for checking Auth-Tokens (residing in
> identityservice?)
I prefer to to complicate the code at this point, linking it to identity. Once
we use it in tests, it might make sense to do that.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:107: if len(parts) == 1 && r.Method ==
"PUT" {
On 2012/11/27 11:13:33, dfc wrote:
> Maybe a switch within a switch {
> 
> switch len(parts) {
> case 1: 
>     switch r.Method {
>     case "PUT"
> 
> } 
> 
> and so forth
Was thinking of a switch, but wasn't sure how - 10x, I'll do it.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:111: return
On 2012/11/27 11:03:07, john.meinel wrote:
> If err != nil then we say Accepted? Shouldn't that be "if err == nil" ?
> 
> Maybe I'm missing the api of AddContainer (it is acceptable to try to PUT a
> container that already exists.)
> We'll want to make sure to get test coverage of that.
> 
> This is the sort of thing where you sort of want TDD, because it is easy to
> forget that you handled this case, and then the test case doesn't get written.

It is not an error to create an existing container - swift returns 202
(Accepted) instead of 201 (Created). There is an error in the if, I'll fix it.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:141: }
On 2012/11/27 11:03:07, john.meinel wrote:
> Ultimately, we probably want something like 'gorest' to do the work of finding
> what methods need to be connected. But honestly, this doesn't look too bad.
> I would probably split it up more as:
> if len(parts) == 1 { DoContainerAPI(params) }
> elif len(parts) == 2 {DoObjectAPI(params) }
> else { w.Write(ERROR) }
> And then the container stuff of GET/DELETE/PUT are all nicely clustered, and
the
> Object stuff is also nicely clustered, and you don't repeat the 'len(parts)'
> checks everywhere.
I was thinking of separating the obj/container handing code, but since we talked
about replacing it with gorest, I left everything in one nice small method, easy
to refactor. But since we decided not to use gorest, I suppose I can do that.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/servi...
testservices/swiftservice/service.go:142: w.WriteHeader(http.StatusNotFound)
On 2012/11/27 11:46:35, gz wrote:
> This throws away the err string and returns no body?

Fair enough, I'll add the error text in the body.

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/swift...
File testservices/swiftservice/swiftservice.go (right):

https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/swift...
testservices/swiftservice/swiftservice.go:6: 
On 2012/11/27 11:13:33, dfc wrote:
> Comments ?

Sure, coming up :)

[dimitern, 2012-11-27 19:01:19]

Please take a look.

[jameinel, 2012-11-28 10:19:33]

Overall LGTM.
I'd like to see the tests split up a little bit more, but that's probably the
only thing.

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/se...
File testservices/swiftservice/service_http.go (right):

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/se...
testservices/swiftservice/service_http.go:10: 
Did the response really have this crazy spacing? That is surprising.

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/se...
testservices/swiftservice/service_http.go:131: if token != s.token {
Did we test that if you pass an invalid token you get the same response as if
you don't pass any token at all? We might, but something we'll want to test.

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/se...
File testservices/swiftservice/service_http_test.go (right):

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/se...
testservices/swiftservice/service_http_test.go:54: 
Ultimately, I'd like to see this split out into stuff like:
TestPUTExistsAccepted
TestPUTMissingCreated
TestRemoveExistsOK
TestRemoveMissingError
etc.

It is ok to have it this way for now.

I realize there will be a small amount of extra setup and teardown in the tests.
However, changing one test will not affect the others, which is a huge win.

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/se...
testservices/swiftservice/service_http_test.go:101: 
Similarly here.

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/sw...
File testservices/swiftservice/swiftservice.go (right):

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/sw...
testservices/swiftservice/swiftservice.go:6: 
As mentioned by others, we'll want doc comments on the type and on the
individual functions.

[dimitern, 2012-11-28 20:19:58]

Submitting, with the changes suggested.

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/se...
File testservices/swiftservice/service_http.go (right):

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/se...
testservices/swiftservice/service_http.go:10: 
On 2012/11/28 10:19:33, jameinel wrote:
> Did the response really have this crazy spacing? That is surprising.

Yes, it's exactly how I sniffed it from the real canonistack session.

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/se...
testservices/swiftservice/service_http.go:131: if token != s.token {
On 2012/11/28 10:19:33, jameinel wrote:
> Did we test that if you pass an invalid token you get the same response as if
> you don't pass any token at all? We might, but something we'll want to test.

Sure, I'm adding that test.

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/se...
File testservices/swiftservice/service_http_test.go (right):

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/se...
testservices/swiftservice/service_http_test.go:54: 
On 2012/11/28 10:19:33, jameinel wrote:
> Ultimately, I'd like to see this split out into stuff like:
> TestPUTExistsAccepted
> TestPUTMissingCreated
> TestRemoveExistsOK
> TestRemoveMissingError
> etc.
> 
> It is ok to have it this way for now.
> 
> I realize there will be a small amount of extra setup and teardown in the
tests.
> However, changing one test will not affect the others, which is a huge win.

I was thinking of the same thing - it got kinda too much for just 2 test cases.

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/se...
testservices/swiftservice/service_http_test.go:101: 
On 2012/11/28 10:19:33, jameinel wrote:
> Similarly here.

Done.

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/sw...
File testservices/swiftservice/swiftservice.go (right):

https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/sw...
testservices/swiftservice/swiftservice.go:6: 
On 2012/11/28 10:19:33, jameinel wrote:
> As mentioned by others, we'll want doc comments on the type and on the
> individual functions.

Done.

[dimitern, 2012-11-28 20:20:44]

*** Submitted:

Swift test service double implemented.

Implements a minimal number of OS Swift API, needed for juju testing.

R=
CC=
https://codereview.appspot.com/6851112

[jameinel, 2012-11-29 06:31:40]

LGTM

I have some comments, but don't think it needs another review before landing.

https://codereview.appspot.com/6851112/diff/11001/testservices/swiftservice/s...
File testservices/swiftservice/service_http_test.go (right):

https://codereview.appspot.com/6851112/diff/11001/testservices/swiftservice/s...
testservices/swiftservice/service_http_test.go:38: func (s *SwiftHTTPSuite)
sendRequest(method, path string, body []byte) (*http.Response, error) {
All callers of this follow up with:
c.Assert(err, IsNil)
c.Assert(resp.ResponseCode, Equals, X)

So I would propose to move that code into this function, and simplify all of the
callers.

https://codereview.appspot.com/6851112/diff/11001/testservices/swiftservice/s...
testservices/swiftservice/service_http_test.go:50: if token != "" {
Where is 'token' being defined? I just realized I don't see its definition, and
it is both here and in the SetUpSuite.
If it is a global variable (which I'm not super excited about, but might be
necessary), then it *definitely* should not just be called "token".
The fact that we sometimes compare it to "" means that not only is it global
state, it is *mutable* global state, which is almost always incorrect.

https://codereview.appspot.com/6851112/diff/11001/testservices/swiftservice/s...
testservices/swiftservice/service_http_test.go:106: }
The 'removeContainer' here looks like cleanup. So if have a 'ensureNoContainer'
that removes it if it exists, but doesn't fail (removeContainer might already be
that), then we can do:

s.ensureNotContainer("test", c)
defer s.removeContainer("test", c)
s.sendRequest(c, "PUT", "test", nil, http.StatusCreated)

And that is the complete test. Though we might take it a step further, and
TearDownTest should be the thing that makes sure you don't leave any state in
the service.

Overall splitting these up looks good to me.