Skip to content

API Client and Lock Service#1236

Merged
technoweenie merged 21 commits intogit-lfs:masterfrom
ttaylorr:api-client
May 24, 2016
Merged

API Client and Lock Service#1236
technoweenie merged 21 commits intogit-lfs:masterfrom
ttaylorr:api-client

Conversation

@ttaylorr
Copy link
Contributor

@ttaylorr ttaylorr commented May 21, 2016

This PR introduces the Git LFS API client, as well as the LockService, for interacting with the new LFS file locking API.

API Client

The API client is broken down into several key components, all of which are described here:

Request Schemas

A request schema provides a definition or template type from which to build an actual request. Right now, the definition of this type is as follows:

// RequestSchema provides a schema from which to generate sendable requests.
type RequestSchema struct {
    // Method is the method that should be used when making a particular API
    // call.
    Method string
    // Path is the relative path that this API call should be made against.
    Path string
    // Query is the query parameters used in the request URI.
    Query map[string]string
    // Body is the body of the request.
    Body interface{}
    // Into is an optional field used to represent the data structure into
    // which a response should be serialized.
    Into interface{}
}

Request/Response Lifecycle

The lifecycle type is responsible for a few things:

  1. Building an executable request based on the given schema.
  2. Executing that request against an API server (and serialize the result, if applicable)
  3. Cleaning up after that request has finished.

Right now, things are a little net/http specific, and the only Lifecycle implementation is the *api.HttpLifecycle type. That type knows how to do all of the things described above in an HTTP-specific context. Internally, it uses the *http.Client type to execute the requests.

In the future, I would like to abstract away the HTTP-specific stuff, and open the API up to an SSH, or even WebSocket implementation.

Services

Services are responsible for implementing API methods by providing code to generate *api.RequestSchemas, which can be passed to the *api.Client in use to be sent through the lifecycle subsystem, and eventually, a response is returned.

Lock Service

This PR includes the LockService type, which implements all of the APIs type definitions, as well as the three methods specified in the File Locking API proposal (Lock, Unlock, and Search).

An example use of the LockService would be as follows.

client := api.NewClient(...)
locks, schema := client.Locks.Search(&api.LockSearchRequest{
        // ...
})

resp, err := client.Do(schema)
if err != nil {
        panic(err.Error())
}

for _, l := range locks.Locks {
        // ...
}

fmt.Fprint("API call succeeded with status %d\n", resp.StatusCode())

schema package

We also introduced a way to test that our implementation of the API as defined in the docs/ directory is correct, by using JSON schema.

This is a nice boundary to pick, since we have tests for the Service methods, and tests for the API client code, so the actual API implementation is tested separately.

To test a new API type, simply add a *_schema.json file to the api/schema/ directory, and you're good to go:

func TestLockResponseSchema(t *testing.T) {
        schema.Validate(t, schema.LockResponse, &api.LockResponse{
                // ...
        })
}

or, alternatively:

func TestInvalidLockResponseSchema(t *testing.T) {
        schema.Refute(t, schema.LockResponse, &api.LockResponse{
                // ...
        })
}

Thanks in advance to anyone who reads through this PR, I appreciate you taking the time to look through this rather lengthy diff. 😄

Special thanks to @technoweenie and @sinbad for listening to my design proposals, as well as providing invaluable suggestions and advice along the way. 👋

Enjoy!

@ttaylorr
Copy link
Contributor Author

Hmm... looks like there was some weirdness going on with one of gojsonschema's dependencies not being properly vendored. @technoweenie: any thoughts?

@technoweenie
Copy link
Contributor

Argh, we have to rewrite all the imports.

@technoweenie
Copy link
Contributor

Try this:

diff --git a/vendor/_nuts/github.com/xeipuuv/gojsonreference/reference.go b/vendor/_nuts/github.com/xeipuuv/gojsonreference/reference.go
index d4d2eca..3713a81 100644
--- a/vendor/_nuts/github.com/xeipuuv/gojsonreference/reference.go
+++ b/vendor/_nuts/github.com/xeipuuv/gojsonreference/reference.go
@@ -27,7 +27,7 @@ package gojsonreference

 import (
        "errors"
-       "github.com/xeipuuv/gojsonpointer"
+       "github.com/github/git-lfs/vendor/_nuts/github.com/xeipuuv/gojsonpointer"
        "net/url"
        "path/filepath"
        "runtime"
diff --git a/vendor/_nuts/github.com/xeipuuv/gojsonschema/jsonLoader.go b/vendor/_nuts/github.com/xeipuuv/gojsonschema/jsonLoader.go
index 0b40574..53b3733 100644
--- a/vendor/_nuts/github.com/xeipuuv/gojsonschema/jsonLoader.go
+++ b/vendor/_nuts/github.com/xeipuuv/gojsonschema/jsonLoader.go
@@ -38,7 +38,7 @@ import (
        "runtime"
        "strings"

-       "github.com/xeipuuv/gojsonreference"
+       "github.com/github/git-lfs/vendor/_nuts/github.com/xeipuuv/gojsonreference"
 )

 var osFS = osFileSystem(os.Open)
diff --git a/vendor/_nuts/github.com/xeipuuv/gojsonschema/schema.go b/vendor/_nuts/github.com/xeipuuv/gojsonschema/schema.go
index 577c786..c4cb9f4 100644
--- a/vendor/_nuts/github.com/xeipuuv/gojsonschema/schema.go
+++ b/vendor/_nuts/github.com/xeipuuv/gojsonschema/schema.go
@@ -32,7 +32,7 @@ import (
        "reflect"
        "regexp"

-       "github.com/xeipuuv/gojsonreference"
+       "github.com/github/git-lfs/vendor/_nuts/github.com/xeipuuv/gojsonreference"
 )

 var (
diff --git a/vendor/_nuts/github.com/xeipuuv/gojsonschema/schemaPool.go b/vendor/_nuts/github.com/xeipuuv/gojsonschema/schemaPool.go
index 70ae731..ab14242 100644
--- a/vendor/_nuts/github.com/xeipuuv/gojsonschema/schemaPool.go
+++ b/vendor/_nuts/github.com/xeipuuv/gojsonschema/schemaPool.go
@@ -30,7 +30,7 @@ import (
        "errors"
        "net/http"

-       "github.com/xeipuuv/gojsonreference"
+       "github.com/github/git-lfs/vendor/_nuts/github.com/xeipuuv/gojsonreference"
 )

 type schemaPoolDocument struct {
diff --git a/vendor/_nuts/github.com/xeipuuv/gojsonschema/subSchema.go b/vendor/_nuts/github.com/xeipuuv/gojsonschema/subSchema.go
index b249b7e..a2e72a5 100644
--- a/vendor/_nuts/github.com/xeipuuv/gojsonschema/subSchema.go
+++ b/vendor/_nuts/github.com/xeipuuv/gojsonschema/subSchema.go
@@ -31,7 +31,7 @@ import (
        "regexp"
        "strings"

-       "github.com/xeipuuv/gojsonreference"
+       "github.com/github/git-lfs/vendor/_nuts/github.com/xeipuuv/gojsonreference"
 )

 const (

@ttaylorr
Copy link
Contributor Author

@technoweenie thanks! Applied your commit, but it looks like we may still be missing some of github.com/stretchr/testify's dependencies. We may want to look into a vendoring solution that doesn't force us to vendor all subdeps of a given package. Did you end up messing around with your own scripts this weekend?

@sinbad
Copy link
Contributor

sinbad commented May 23, 2016

On deps, I'd say this is a good time to move to go's 1.6's standard vendor/ dir and switch to either GoDep or Glide, both of which understand this approach. Then we can drop the GO15VENDOREXPERIMENT=0 requirement when people are running the latest go. It means requiring go 1.5+ though.

As for vendoring all sub-deps, this is a given if you expect the project to build straight after clone. The alternative is to not commit any of the vendored packages and require a post-clone command instead (e.g. glide install). The downside of that is you have to remember to run it whenever the dependencies change in a breaking way or scratch your head for a couple of minutes before you remember. I don't see any benefit of a halfway house where direct deps are committed but subdeps aren't.

Personally I prefer committing all deps because then the only time you have to worry about your package management is when you personally change the dependencies. But either works.

api/lifecycle.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought part of the idea of abstracting this was to make the interface non-specific to HTTP, but you're including http.Request in the interface here. Response conversely is non-protocol specific, which is more what I expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, but I had mentioned to @technoweenie that I thought this was a good intermediate step between the old HTTP-specific code and the new API client stuff. Eventually, I'd like to find the correct abstraction between the *http.Request and api.Request, but until we know what the SSH API is going to look like, this is difficult.

If it's okay with you, I think we should leave this and the Method field on RequestSchema as is until we can see what an SSH api would need specifically.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. Maybe a TODO comment though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you go 😄

@technoweenie
Copy link
Contributor

We may want to look into a vendoring solution that doesn't force us to vendor all subdeps of a given package.

Yup, until then, I'd just update the imports here.

Did you end up messing around with your own scripts this weekend?

Nah, I think I'll try glide instead.

@ttaylorr
Copy link
Contributor Author

Hey @sinbad, thanks for the feedback 👍 I went ahead and moved all of our dependencies into vendor/ using Glide over in #1243. Grabbing lunch in a few, but will address your comments shortly.

@ttaylorr
Copy link
Contributor Author

OK, pushed a commit which uses the *httputil.HttpClient, and migrates those tests to run using a testify.Suite so we can Setup() and TearDown() the creds check bypass.

ttaylorr added 17 commits May 24, 2016 09:27
- rename client to v1, v2_client to client
The initial thought here is to introduce a MethodTestCase type that
encapsulates the behavior of testing a single method in a particular given
service.

To do so, a httptest.Server is created and the schema is turned into a request
which is fired at that server. Thet MethodTestCase, of course, knows how to
respond to different requests, and the behavior of those responses is tested.

What I dislike is that we have to write three things which are mostly the same
to test any endpoint in any case on the API:
  1) a request type (Go struct)
  2) an expected response type (Go type)
  3) the actual response (a mutltline Go string, which is really just JSON)

This seems redundant, so I may explore other options for implementing this sort
of thing in the future.
One of the things I'd mentioned that I disliked about the initial
implementation of MethodTestCase was that it required you to type out three
things (see previous commit). Of particular importance are items (2) and (3),
the expected and actual response, one a Go type, the other a JSON string.

This commit reduces that typing by 1/2. Since all we were doing was typing the
same data in two different formats, the only thing that we were really testing
was the encoding/json package. That is redundant, and a bad boundary to include
during testing. Therefore, instead of having to type out the JSON-encoded
string, this commit delegates that responsibility into the MethodTestCase's
server, and just uses a `json.Encoder` to do the work.

Sweet!
MethodTestCase had lots of problems, most caused by a good degree of ambiguity
in what we were actually testing.

AssertSchema(), on the other hand, simplifies this problem greatly by correctly
identifying the responsibilities of an *api.RequestSchema. An
*api.RequestSchema is responsible for one thing, and one thing only: to provide
a template for requests to be made against the API, completely regardless of
protocol or implementation.

Most notably, these responsibilities do not include:
 - Preforming an HTTP request, or
 - reading the response of an http request

So MethodTestCase gets simplified to a simple assert.Equal(). We expect to get
some *api.RequestSchema out, and we expect it to have certain parameters. Now,
all of the API's Service tests are rather simple: create a schema, compare,
rinse and repeat.

The only thing left to do is test the schema of the request and response
payload. This is what I had tried to do with the original MethodTestCase
implementation, but the way the system is desinged, trying to accomplish this
involves integrating with too many unrelated parts. To address this, I plan to
introduce a JSON Schema implementation that compares generated request/reponse
payloads with their ideal types.
@ttaylorr ttaylorr force-pushed the api-client branch 3 times, most recently from d934947 to 3c3ad2b Compare May 24, 2016 15:49
@ttaylorr
Copy link
Contributor Author

Huh, not sure why Travis ran the last test against Go 1.3.1 (not in the .travis.yml), but that's what caused the failure. Must have been an intermediate Travis bug.

@technoweenie
Copy link
Contributor

Looks good 🤘

@technoweenie technoweenie merged commit ac1a109 into git-lfs:master May 24, 2016
@ttaylorr ttaylorr deleted the api-client branch May 24, 2016 20:19
@ttaylorr
Copy link
Contributor Author

Thanks @technoweenie, @sinbad! 👋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants