From fa34ba6209dfbc098e52b2518e7d26ba4f85a38c Mon Sep 17 00:00:00 2001 From: Peter Grant Date: Mon, 7 Mar 2016 16:41:10 -0800 Subject: [PATCH 1/5] Fix failing unit test after SendHandler refactor --- client/client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/client_test.go b/client/client_test.go index eeaafe6..14898b7 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -118,7 +118,7 @@ func testAPI() *jshapi.API { return object, nil }) - api := jshapi.New("", true) + api := jshapi.New("") api.Add(resource) return api From 6163f060140e2399544264128cb53b0c511260fc Mon Sep 17 00:00:00 2001 From: Peter Grant Date: Mon, 7 Mar 2016 16:49:26 -0800 Subject: [PATCH 2/5] Differentiate between a single object and a list with a single object --- document.go | 49 +++++++++++++++++++++++++++++++++++++++++++++--- document_test.go | 44 +++++++++++++++++++++++++++++++++++++++---- list.go | 5 +---- 3 files changed, 87 insertions(+), 11 deletions(-) diff --git a/document.go b/document.go index 642a451..b5bdef0 100644 --- a/document.go +++ b/document.go @@ -1,6 +1,7 @@ package jsh import ( + "encoding/json" "fmt" "net/http" "strings" @@ -13,6 +14,7 @@ of each attribute: http://jsonapi.org/format/#document-structure */ type Document struct { Data List `json:"data"` + Object *Object `json:"-"` Errors ErrorList `json:"errors,omitempty"` Links *Link `json:"links,omitempty"` Included []*Object `json:"included,omitempty"` @@ -50,7 +52,7 @@ func Build(payload Sendable) *Document { object, isObject := payload.(*Object) if isObject { - document.Data = List{object} + document.Object = object document.Status = object.Status } @@ -89,7 +91,10 @@ func (d *Document) Validate(r *http.Request, response bool) *Error { return nil } - if !d.HasErrors() && d.Data == nil { + if d.Object != nil && d.Data != nil { + return ISE("Both `Data` and `Object` cannot be set for a JSON response") + } + if !d.HasErrors() && d.Object == nil && d.Data == nil { return ISE("Both `errors` and `data` cannot be blank for a JSON response") } if d.HasErrors() && d.Data != nil { @@ -104,6 +109,11 @@ func (d *Document) Validate(r *http.Request, response bool) *Error { return nil } + if d.Object != nil { + if err := d.Object.Validate(r, response); err != nil { + return err + } + } err := d.Data.Validate(r, response) if err != nil { return err @@ -124,6 +134,10 @@ func (d *Document) AddObject(object *Object) *Error { return ISE("Cannot add data to a document already possessing errors") } + if d.Object != nil { + return ISE("Cannot add data to a non-collection") + } + if d.Status == 0 { d.Status = object.Status } @@ -166,12 +180,15 @@ First is just a convenience function that returns the first data object from the array */ func (d *Document) First() *Object { + if d.Object != nil { + return d.Object + } return d.Data[0] } // HasData will return true if the JSON document's Data field is set func (d *Document) HasData() bool { - return d.Data != nil && len(d.Data) > 0 + return d.Object != nil || (d.Data != nil && len(d.Data) > 0) } // HasErrors will return true if the Errors attribute is not nil. @@ -186,3 +203,29 @@ func (d *Document) Error() string { } return errStr } + +/* +MarshalJSON handles custom serialization required because the "data" element of a document +might be a collection of objects or a single object. +*/ +func (d *Document) MarshalJSON() ([]byte, error) { + // subtype that overrides data with a single object + type MarshalObject struct { + Document + Data *Object `json:"data,omitempty"` + } + + if d == nil { + return nil, nil + } + if d.Object != nil { + return json.Marshal(MarshalObject{ + Document: *d, + Data: d.Object, + }) + } + + // avoid stack overflow by using this subtype for marshaling + type MarshalList Document + return json.Marshal(MarshalList(*d)) +} diff --git a/document_test.go b/document_test.go index a84a288..7013467 100644 --- a/document_test.go +++ b/document_test.go @@ -4,6 +4,8 @@ import ( "net/http" "testing" + "encoding/json" + . "github.com/smartystreets/goconvey/convey" ) @@ -72,7 +74,7 @@ func TestDocument(t *testing.T) { } testObjectForInclusion := &Object{ - ID: "1", + ID: "1", Type: "Included", } @@ -81,7 +83,8 @@ func TestDocument(t *testing.T) { Convey("should accept an object", func() { doc := Build(testObject) - So(doc.Data, ShouldResemble, List{testObject}) + So(doc.Data, ShouldBeNil) + So(doc.Object, ShouldResemble, testObject) So(doc.Status, ShouldEqual, http.StatusAccepted) }) @@ -102,13 +105,13 @@ func TestDocument(t *testing.T) { validationErrors := doc.Validate(req, true) So(validationErrors, ShouldBeNil) - So(doc.Data, ShouldResemble, List{testObject}) + So(doc.Data, ShouldBeNil) + So(doc.Object, ShouldResemble, testObject) So(doc.Included, ShouldNotBeEmpty) So(doc.Included[0], ShouldResemble, testObjectForInclusion) So(doc.Status, ShouldEqual, http.StatusAccepted) }) - Convey("should accept a list", func() { list := List{testObject} doc := Build(list) @@ -126,6 +129,39 @@ func TestDocument(t *testing.T) { }) }) + Convey("->MarshalJSON()", func() { + testObject := &Object{ + ID: "1", + Type: "Test", + Status: http.StatusAccepted, + } + + Convey("should marshal a list with a single element as an array", func() { + list := List{testObject} + doc := Build(list) + j, err := json.Marshal(doc) + So(err, ShouldBeNil) + m := map[string]json.RawMessage{} + err = json.Unmarshal(j, &m) + So(err, ShouldBeNil) + data := string(m["data"]) + So(data, ShouldStartWith, "[") + So(data, ShouldEndWith, "]") + }) + + Convey("should marshal a single object as an object", func() { + doc := Build(testObject) + j, err := json.Marshal(doc) + So(err, ShouldBeNil) + m := map[string]json.RawMessage{} + err = json.Unmarshal(j, &m) + So(err, ShouldBeNil) + data := string(m["data"]) + So(data, ShouldStartWith, "{") + So(data, ShouldEndWith, "}") + }) + + }) }) } diff --git a/list.go b/list.go index 3d72bea..846b87e 100644 --- a/list.go +++ b/list.go @@ -52,8 +52,7 @@ func (list *List) UnmarshalJSON(rawData []byte) error { } /* -MarshalJSON returns a top level object for the "data" attribute if a single object. In -all other cases returns a JSON encoded list for "data". We use a pointer receiver here +MarshalJSON returns a JSON encoded list for "data". We use a pointer receiver here so we are able to distinguish between nil (don't serialize) and empty (serialize as []). */ func (list *List) MarshalJSON() ([]byte, error) { @@ -70,8 +69,6 @@ func (list *List) MarshalJSON() ([]byte, error) { switch { case count == 0: return []byte("[]"), nil - case count == 1: - return json.Marshal(marshalList[0]) default: return json.Marshal(marshalList) } From 92355c86a2340ed5954b80a25d0fee469c607b41 Mon Sep 17 00:00:00 2001 From: Derek Dowling Date: Tue, 8 Mar 2016 10:31:12 -0800 Subject: [PATCH 3/5] Document Mode Settings These changes are an attempt to address issues caused by what I am deeming various JSONAPI document "modes". So far, there are three types: 1. ObjectMode - for single object request/responses 2. ListMode - for list request/respones 3. ErrorMode - for receiving and sending error responses These modes come out of necessity due to the nature and complexity of the effects each mode have on the document and specification expectations. By clearly labeling each, it becomes easy to define, identify, test, and handle each case accordingly while also leaving the code base in a readable and maintainable state. --- Godeps/Godeps.json | 4 +- .../derekdowling/jsh-api/Godeps/Godeps.json | 4 +- .../github.com/derekdowling/jsh-api/README.md | 38 ++-- .../github.com/derekdowling/jsh-api/api.go | 51 ++++-- .../github.com/derekdowling/jsh-api/logger.go | 46 ----- .../derekdowling/jsh-api/resource.go | 30 ++-- .../github.com/derekdowling/jsh-api/sender.go | 33 ++++ client/action.go | 2 +- client/client.go | 12 +- client/client_test.go | 6 +- client/delete.go | 2 +- client/get.go | 4 +- client/patch.go | 2 +- client/post.go | 2 +- document.go | 164 ++++++++++++------ document_test.go | 131 +++++++++++--- parser.go | 31 ++-- 17 files changed, 366 insertions(+), 196 deletions(-) delete mode 100644 Godeps/_workspace/src/github.com/derekdowling/jsh-api/logger.go create mode 100644 Godeps/_workspace/src/github.com/derekdowling/jsh-api/sender.go diff --git a/Godeps/Godeps.json b/Godeps/Godeps.json index 68a6e35..24ba3d0 100644 --- a/Godeps/Godeps.json +++ b/Godeps/Godeps.json @@ -21,8 +21,8 @@ }, { "ImportPath": "github.com/derekdowling/jsh-api", - "Comment": "0.4.5-1-gb9fb2c1", - "Rev": "b9fb2c1082cd87ec29c1f4e569fb2f30376faec8" + "Comment": "0.4.6-4-ge612f9e", + "Rev": "e612f9e3c326b88bc555438a69a48fafb530a258" }, { "ImportPath": "github.com/jtolds/gls", diff --git a/Godeps/_workspace/src/github.com/derekdowling/jsh-api/Godeps/Godeps.json b/Godeps/_workspace/src/github.com/derekdowling/jsh-api/Godeps/Godeps.json index 3f017f4..f2b4c97 100644 --- a/Godeps/_workspace/src/github.com/derekdowling/jsh-api/Godeps/Godeps.json +++ b/Godeps/_workspace/src/github.com/derekdowling/jsh-api/Godeps/Godeps.json @@ -12,8 +12,8 @@ }, { "ImportPath": "github.com/derekdowling/go-json-spec-handler", - "Comment": "0.8.2", - "Rev": "2bfcfaf9dd924d0b97ffac16f2b6a18a82f50a03" + "Comment": "0.8.3", + "Rev": "a077109d98f6d6223555c633af4b04f9b7a3e3ec" }, { "ImportPath": "github.com/derekdowling/go-stdlogger", diff --git a/Godeps/_workspace/src/github.com/derekdowling/jsh-api/README.md b/Godeps/_workspace/src/github.com/derekdowling/jsh-api/README.md index 4780762..2ee925e 100644 --- a/Godeps/_workspace/src/github.com/derekdowling/jsh-api/README.md +++ b/Godeps/_workspace/src/github.com/derekdowling/jsh-api/README.md @@ -9,32 +9,42 @@ A [JSON API](http://jsonapi.org) specification micro-service builder created on ## Setup +The easiest way to get started is like so: + ```go import github.com/derekdowling/jsh-api -// if you want a custom logger -jshapi.Logger = yourLogger - -// create the base api -api := jshapi.New("") - -// implement jshapi/store.CRUD interface +// implement jshapi/store.CRUD interface and add resource specific middleware via Goji userStorage := &UserStorage{} resource := jshapi.NewCRUDResource("user", userStorage) +resource.UseC(yourUserMiddleware) -// resource specific middleware via Goji -resource.Use(yourUserMiddleware) - -// add resources to the API +// setup a logger, your shiny new API, and give it a resource +logger := log.New(os.Stderr, ": ", log.LstdFlags) +api := jshapi.Default("", true, logger) api.Add(resource) -// add top level API middleware -api.Use(yourTopLevelAPIMiddleware) - // launch your api http.ListenAndServe("localhost:8000", api) ``` +For a completely custom setup: + +```go +import github.com/derekdowling/jsh-api + +// manually setup your API +api := jshapi.New("") + +// add a custom send handler +jshapi.SendHandler = customHandler + +// add top level Goji Middleware +api.UseC(yourTopLevelAPIMiddleware) + +http.ListenAndServe("localhost:8000", api) +``` + ## Feature Overview ### Fast CRUD Implementation diff --git a/Godeps/_workspace/src/github.com/derekdowling/jsh-api/api.go b/Godeps/_workspace/src/github.com/derekdowling/jsh-api/api.go index 0e113d4..34444a9 100644 --- a/Godeps/_workspace/src/github.com/derekdowling/jsh-api/api.go +++ b/Godeps/_workspace/src/github.com/derekdowling/jsh-api/api.go @@ -2,12 +2,15 @@ package jshapi import ( "fmt" + "log" + "os" "path" "strings" "goji.io" "goji.io/pat" + "github.com/derekdowling/go-stdlogger" "github.com/derekdowling/goji2-logger" ) @@ -19,31 +22,53 @@ type API struct { Debug bool } -// New initializes a new top level API Resource Handler. The most basic implementation -// is: -// -// // optionally, set your own logger -// jshapi.Logger = yourLogger -// -// // create a new API -// api := jshapi.New("", nil) -func New(prefix string, debug bool) *API { +/* +SendHandler allows the customization of how API responses are sent and logged. This +is used by all jshapi.Resource objects. +*/ +var SendHandler = DefaultSender(log.New(os.Stderr, "jshapi: ", log.LstdFlags)) + +/* +New initializes a new top level API Resource without doing any additional setup. +*/ +func New(prefix string) *API { // ensure that our top level prefix is "/" prefixed if !strings.HasPrefix(prefix, "/") { prefix = fmt.Sprintf("/%s", prefix) } - // create our new logger - api := &API{ + // create our new API + return &API{ Mux: goji.NewMux(), prefix: prefix, Resources: map[string]*Resource{}, - Debug: debug, } +} + +/* +Default builds a new top-level API with a few out of the box additions to get people +started without needing to add a lot of extra functionality. + +The most basic implementation is: + + // create a logger, the std log package works, as do most other loggers + // std.Logger interface defined here: + // https://github.com/derekdowling/go-stdlogger/blob/master/logger.go + logger := log.New(os.Stderr, "jshapi: ", log.LstdFlags) + + // create the API. Specify a http://yourapi// if required + api := jshapi.Default("", false, logger) + api.Add(yourResource) + +*/ +func Default(prefix string, debug bool, logger std.Logger) *API { + + api := New(prefix) + SendHandler = DefaultSender(logger) // register logger middleware - gojilogger := gojilogger.New(Logger, debug) + gojilogger := gojilogger.New(logger, debug) api.UseC(gojilogger.Middleware) return api diff --git a/Godeps/_workspace/src/github.com/derekdowling/jsh-api/logger.go b/Godeps/_workspace/src/github.com/derekdowling/jsh-api/logger.go deleted file mode 100644 index 1f64331..0000000 --- a/Godeps/_workspace/src/github.com/derekdowling/jsh-api/logger.go +++ /dev/null @@ -1,46 +0,0 @@ -package jshapi - -import ( - "log" - "net/http" - "os" - - "golang.org/x/net/context" - - "github.com/derekdowling/go-json-spec-handler" - "github.com/derekdowling/go-stdlogger" -) - -// Logger can be overridden with your own logger to utilize any custom features -// it might have. Interface defined here: https://github.com/derekdowling/go-stdlogger/blob/master/logger.go -var Logger std.Logger = log.New(os.Stderr, "jshapi: ", log.LstdFlags) - -// SendAndLog is a jsh wrapper function that first prepares a jsh.Sendable response, -// and then handles logging 5XX errors that it encounters in the process. -func SendAndLog(ctx context.Context, w http.ResponseWriter, r *http.Request, sendable jsh.Sendable) { - - intentionalErr, isType := sendable.(jsh.ErrorType) - if isType { - // determine error status before taking any additional actions - var status int - - list, isList := intentionalErr.(jsh.ErrorList) - if isList { - status = list[0].Status - } - - err, isErr := intentionalErr.(*jsh.Error) - if isErr { - status = err.Status - } - - if status >= 500 { - Logger.Printf("Returning ISE: %s\n", intentionalErr.Error()) - } - } - - sendErr := jsh.Send(w, r, sendable) - if sendErr != nil && sendErr.Status >= 500 { - Logger.Printf("Error sending response: %s\n", sendErr.Error()) - } -} diff --git a/Godeps/_workspace/src/github.com/derekdowling/jsh-api/resource.go b/Godeps/_workspace/src/github.com/derekdowling/jsh-api/resource.go index 0537178..27e93f6 100644 --- a/Godeps/_workspace/src/github.com/derekdowling/jsh-api/resource.go +++ b/Godeps/_workspace/src/github.com/derekdowling/jsh-api/resource.go @@ -252,17 +252,17 @@ func (res *Resource) Action(actionName string, storage store.Get) { func (res *Resource) postHandler(ctx context.Context, w http.ResponseWriter, r *http.Request, storage store.Save) { parsedObject, parseErr := jsh.ParseObject(r) if parseErr != nil && reflect.ValueOf(parseErr).IsNil() == false { - SendAndLog(ctx, w, r, parseErr) + SendHandler(ctx, w, r, parseErr) return } object, err := storage(ctx, parsedObject) if err != nil && reflect.ValueOf(err).IsNil() == false { - SendAndLog(ctx, w, r, err) + SendHandler(ctx, w, r, err) return } - SendAndLog(ctx, w, r, object) + SendHandler(ctx, w, r, object) } // GET /resources/:id @@ -271,22 +271,22 @@ func (res *Resource) getHandler(ctx context.Context, w http.ResponseWriter, r *h object, err := storage(ctx, id) if err != nil && reflect.ValueOf(err).IsNil() == false { - SendAndLog(ctx, w, r, err) + SendHandler(ctx, w, r, err) return } - SendAndLog(ctx, w, r, object) + SendHandler(ctx, w, r, object) } // GET /resources func (res *Resource) listHandler(ctx context.Context, w http.ResponseWriter, r *http.Request, storage store.List) { list, err := storage(ctx) if err != nil && reflect.ValueOf(err).IsNil() == false { - SendAndLog(ctx, w, r, err) + SendHandler(ctx, w, r, err) return } - SendAndLog(ctx, w, r, list) + SendHandler(ctx, w, r, list) } // DELETE /resources/:id @@ -295,7 +295,7 @@ func (res *Resource) deleteHandler(ctx context.Context, w http.ResponseWriter, r err := storage(ctx, id) if err != nil && reflect.ValueOf(err).IsNil() == false { - SendAndLog(ctx, w, r, err) + SendHandler(ctx, w, r, err) return } @@ -306,17 +306,17 @@ func (res *Resource) deleteHandler(ctx context.Context, w http.ResponseWriter, r func (res *Resource) patchHandler(ctx context.Context, w http.ResponseWriter, r *http.Request, storage store.Update) { parsedObject, parseErr := jsh.ParseObject(r) if parseErr != nil && reflect.ValueOf(parseErr).IsNil() == false { - SendAndLog(ctx, w, r, parseErr) + SendHandler(ctx, w, r, parseErr) return } object, err := storage(ctx, parsedObject) if err != nil && reflect.ValueOf(err).IsNil() == false { - SendAndLog(ctx, w, r, err) + SendHandler(ctx, w, r, err) return } - SendAndLog(ctx, w, r, object) + SendHandler(ctx, w, r, object) } // GET /resources/:id/(relationships/)s @@ -325,11 +325,11 @@ func (res *Resource) toManyHandler(ctx context.Context, w http.ResponseWriter, r list, err := storage(ctx, id) if err != nil && reflect.ValueOf(err).IsNil() == false { - SendAndLog(ctx, w, r, err) + SendHandler(ctx, w, r, err) return } - SendAndLog(ctx, w, r, list) + SendHandler(ctx, w, r, list) } // All HTTP Methods for /resources/:id/ @@ -338,11 +338,11 @@ func (res *Resource) actionHandler(ctx context.Context, w http.ResponseWriter, r response, err := storage(ctx, id) if err != nil && reflect.ValueOf(err).IsNil() == false { - SendAndLog(ctx, w, r, err) + SendHandler(ctx, w, r, err) return } - SendAndLog(ctx, w, r, response) + SendHandler(ctx, w, r, response) } // addRoute adds the new method and route to a route Tree for debugging and diff --git a/Godeps/_workspace/src/github.com/derekdowling/jsh-api/sender.go b/Godeps/_workspace/src/github.com/derekdowling/jsh-api/sender.go new file mode 100644 index 0000000..3c1e743 --- /dev/null +++ b/Godeps/_workspace/src/github.com/derekdowling/jsh-api/sender.go @@ -0,0 +1,33 @@ +package jshapi + +import ( + "net/http" + + "github.com/derekdowling/go-json-spec-handler" + "github.com/derekdowling/go-stdlogger" + "golang.org/x/net/context" +) + +/* +Sender is a function type definition that allows consumers to customize how they +send and log API responses. +*/ +type Sender func(context.Context, http.ResponseWriter, *http.Request, jsh.Sendable) + +/* +DefaultSender is the default sender that will log 5XX errors that it encounters +in the process of sending a response. +*/ +func DefaultSender(logger std.Logger) Sender { + return func(ctx context.Context, w http.ResponseWriter, r *http.Request, sendable jsh.Sendable) { + sendableError, isType := sendable.(jsh.ErrorType) + if isType && sendableError.StatusCode() >= 500 { + logger.Printf("Returning ISE: %s\n", sendableError.Error()) + } + + sendError := jsh.Send(w, r, sendable) + if sendError != nil && sendError.Status >= 500 { + logger.Printf("Error sending response: %s\n", sendError.Error()) + } + } +} diff --git a/client/action.go b/client/action.go index 15dc943..2ec7984 100644 --- a/client/action.go +++ b/client/action.go @@ -16,7 +16,7 @@ func Action(baseURL string, resourceType string, id string, action string) (*jsh return nil, nil, err } - return Do(request) + return Do(request, jsh.ObjectMode) } /* diff --git a/client/client.go b/client/client.go index a45434b..f790e01 100644 --- a/client/client.go +++ b/client/client.go @@ -17,8 +17,8 @@ import ( Document validates the HTTP response and attempts to parse a JSON API compatible Document from the response body before closing it. */ -func Document(response *http.Response) (*jsh.Document, *jsh.Error) { - document, err := buildParser(response).Document(response.Body) +func Document(response *http.Response, mode jsh.DocumentMode) (*jsh.Document, *jsh.Error) { + document, err := buildParser(response).Document(response.Body, mode) if err != nil { return nil, err } @@ -110,7 +110,7 @@ Useful in conjunction with any of the method Request builders or for times when you want to send a request to a custom endpoint, but would still like a JSONAPI response. */ -func Do(request *http.Request) (*jsh.Document, *http.Response, error) { +func Do(request *http.Request, mode jsh.DocumentMode) (*jsh.Document, *http.Response, error) { client := &http.Client{} response, clientErr := client.Do(request) @@ -121,7 +121,7 @@ func Do(request *http.Request) (*jsh.Document, *http.Response, error) { ) } - doc, parseErr := ParseResponse(response) + doc, parseErr := ParseResponse(response, mode) if parseErr != nil { return nil, response, fmt.Errorf("Error parsing response: %s", parseErr.Error()) } @@ -133,7 +133,7 @@ func Do(request *http.Request) (*jsh.Document, *http.Response, error) { ParseResponse handles parsing an HTTP response into a JSON Document if possible. */ -func ParseResponse(response *http.Response) (*jsh.Document, error) { +func ParseResponse(response *http.Response, mode jsh.DocumentMode) (*jsh.Document, error) { skipCodes := []int{ http.StatusNoContent, @@ -146,7 +146,7 @@ func ParseResponse(response *http.Response) (*jsh.Document, error) { } } - document, err := Document(response) + document, err := Document(response, mode) if err != nil { return nil, err } diff --git a/client/client_test.go b/client/client_test.go index 14898b7..64d9c65 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -55,7 +55,7 @@ func TestParseResponse(t *testing.T) { } Convey("404 response parsing should not return a 406 error", func() { - doc, err := ParseResponse(response) + doc, err := ParseResponse(response, jsh.ObjectMode) So(doc, ShouldBeNil) So(err, ShouldBeNil) }) @@ -75,7 +75,7 @@ func TestResponseParsing(t *testing.T) { So(err, ShouldBeNil) Convey("should parse successfully", func() { - doc, err := Document(response) + doc, err := Document(response, jsh.ObjectMode) So(err, ShouldBeNil) So(doc.HasData(), ShouldBeTrue) @@ -94,7 +94,7 @@ func TestResponseParsing(t *testing.T) { So(err, ShouldBeNil) Convey("should parse successfully", func() { - doc, err := Document(response) + doc, err := Document(response, jsh.ListMode) So(err, ShouldBeNil) So(doc.HasData(), ShouldBeTrue) diff --git a/client/delete.go b/client/delete.go index 1898161..47d9106 100644 --- a/client/delete.go +++ b/client/delete.go @@ -19,7 +19,7 @@ func Delete(urlStr string, resourceType string, id string) (*http.Response, erro return nil, err } - _, response, err := Do(request) + _, response, err := Do(request, jsh.ObjectMode) if err != nil { return nil, err } diff --git a/client/get.go b/client/get.go index dcfceae..8339319 100644 --- a/client/get.go +++ b/client/get.go @@ -15,7 +15,7 @@ func Fetch(baseURL string, resourceType string, id string) (*jsh.Document, *http return nil, nil, err } - return Do(request) + return Do(request, jsh.ObjectMode) } /* @@ -44,7 +44,7 @@ func List(baseURL string, resourceType string) (*jsh.Document, *http.Response, e return nil, nil, err } - return Do(request) + return Do(request, jsh.ListMode) } /* diff --git a/client/patch.go b/client/patch.go index c9c8049..e6d3824 100644 --- a/client/patch.go +++ b/client/patch.go @@ -22,7 +22,7 @@ func Patch(baseURL string, object *jsh.Object) (*jsh.Document, *http.Response, e return nil, nil, err } - return Do(request) + return Do(request, jsh.ObjectMode) } // PatchRequest returns a fully formatted request with JSON body for performing diff --git a/client/post.go b/client/post.go index 965ac8f..cb26827 100644 --- a/client/post.go +++ b/client/post.go @@ -19,7 +19,7 @@ func Post(baseURL string, object *jsh.Object) (*jsh.Document, *http.Response, er return nil, nil, err } - return Do(request) + return Do(request, jsh.ObjectMode) } // PostRequest returns a fully formatted request with JSON body for performing diff --git a/document.go b/document.go index b5bdef0..d93d2db 100644 --- a/document.go +++ b/document.go @@ -7,14 +7,27 @@ import ( "strings" ) +// DocumentMode allows different specification settings to be enforced +// based on the specified mode. +type DocumentMode int + +const ( + // ObjectMode enforces fetch request/response specifications + ObjectMode DocumentMode = iota + // ListMode enforces listing request/response specifications + ListMode + // ErrorMode enforces error response specifications + ErrorMode +) + /* Document represents a top level JSON formatted Document. Refer to the JSON API Specification for a full descriptor of each attribute: http://jsonapi.org/format/#document-structure */ type Document struct { - Data List `json:"data"` - Object *Object `json:"-"` + Data List `json:"data"` + // Object *Object `json:"-"` Errors ErrorList `json:"errors,omitempty"` Links *Link `json:"links,omitempty"` Included []*Object `json:"included,omitempty"` @@ -24,10 +37,14 @@ type Document struct { } `json:"jsonapi"` // Status is an HTTP Status Code Status int `json:"-"` + // DataMode to enforce for the document + Mode DocumentMode `json:"-"` // empty is used to signify that the response shouldn't contain a json payload // in the case that we only want to return an HTTP Status Code in order to bypass // validation steps. - empty bool + empty bool + // validated confirms whether or not the document as a whole is validated and + // in a safe-to-send state. validated bool } @@ -52,96 +69,111 @@ func Build(payload Sendable) *Document { object, isObject := payload.(*Object) if isObject { - document.Object = object + document.Data = List{object} document.Status = object.Status + document.Mode = ObjectMode } list, isList := payload.(List) if isList { document.Data = list document.Status = http.StatusOK + document.Mode = ListMode } err, isError := payload.(*Error) if isError { document.Errors = ErrorList{err} document.Status = err.Status + document.Mode = ErrorMode } errorList, isErrorList := payload.(ErrorList) if isErrorList { document.Errors = errorList document.Status = errorList[0].Status + document.Mode = ErrorMode } return document } /* -Validate checks JSON Spec for the top level JSON document +Validate performs document level checks against the JSONAPI specification. It is +assumed that if this call returns without an error, your document is valid and +can be sent as a request or response. */ -func (d *Document) Validate(r *http.Request, response bool) *Error { +func (d *Document) Validate(r *http.Request, isResponse bool) *Error { - if d.Status < 100 || d.Status > 600 { + // if sending a response, we must have a valid HTTP status at the very least + // to send + if isResponse && d.Status < 100 || d.Status > 600 { return ISE("Response HTTP Status is outside of valid range") } - // if empty is set, skip all validations below + // There are certain cases such as HTTP 204 that send without a payload, + // this is the short circuit to make sure we don't false alarm on those cases if d.empty { return nil } - if d.Object != nil && d.Data != nil { - return ISE("Both `Data` and `Object` cannot be set for a JSON response") + // if we have errors, and they have been added in a way that does not trigger + // error mode, set it now so we perform the proper validations. + if d.HasErrors() && d.Mode != ErrorMode { + d.Mode = ErrorMode } - if !d.HasErrors() && d.Object == nil && d.Data == nil { - return ISE("Both `errors` and `data` cannot be blank for a JSON response") - } - if d.HasErrors() && d.Data != nil { - return ISE("Both `errors` and `data` cannot be set for a JSON response") + + switch d.Mode { + case ErrorMode: + if d.HasData() { + return ISE("Attempting to respond with 'data' in an error response") + } + case ObjectMode: + if d.HasData() && len(d.Data) > 1 { + return ISE("Cannot set more than one data object in 'ObjectMode'") + } + case ListMode: + if !d.HasErrors() && d.Data == nil { + return ISE("Data cannot be nil in 'ListMode', use empty array") + } } + if !d.HasData() && d.Included != nil { return ISE("'included' should only be set for a response if 'data' is as well") } - // if fields have already been validated, skip this part - if d.validated { - return nil - } - - if d.Object != nil { - if err := d.Object.Validate(r, response); err != nil { - return err - } - } - err := d.Data.Validate(r, response) + err := d.Data.Validate(r, isResponse) if err != nil { return err } - err = d.Errors.Validate(r, response) + err = d.Errors.Validate(r, isResponse) if err != nil { return err } + d.validated = true + return nil } // AddObject adds another object to the JSON Document after validating it. func (d *Document) AddObject(object *Object) *Error { - if d.HasErrors() { + if d.Mode == ErrorMode { return ISE("Cannot add data to a document already possessing errors") } - if d.Object != nil { - return ISE("Cannot add data to a non-collection") + if d.Mode == ObjectMode && len(d.Data) == 1 { + return ISE("Single 'data' object response is expected, you are attempting to add more than one element to be returned") } + // if not yet set, add the associated HTTP status with the object if d.Status == 0 { d.Status = object.Status } + // finally, actually add the object to data List if d.Data == nil { d.Data = List{object} } else { @@ -151,7 +183,10 @@ func (d *Document) AddObject(object *Object) *Error { return nil } -// AddError adds an error to the JSON Object by transfering it's Error objects. +/* +AddError adds an error to the Document. It will also set the document Mode to +"ErrorMode" if not done so already. +*/ func (d *Document) AddError(newErr *Error) *Error { if d.HasData() { @@ -159,7 +194,7 @@ func (d *Document) AddError(newErr *Error) *Error { } if newErr.Status == 0 { - return SpecificationError("Status code must be set for an error") + return ISE("Status code must be set for an error") } if d.Status == 0 { @@ -172,23 +207,26 @@ func (d *Document) AddError(newErr *Error) *Error { d.Errors = append(d.Errors, newErr) } + // set document to error mode + d.Mode = ErrorMode + return nil } /* -First is just a convenience function that returns the first data object from the -array +First will return the first object from the document data if possible. */ func (d *Document) First() *Object { - if d.Object != nil { - return d.Object + if !d.HasData() { + return nil } + return d.Data[0] } // HasData will return true if the JSON document's Data field is set func (d *Document) HasData() bool { - return d.Object != nil || (d.Data != nil && len(d.Data) > 0) + return d.Data != nil && len(d.Data) > 0 } // HasErrors will return true if the Errors attribute is not nil. @@ -205,27 +243,47 @@ func (d *Document) Error() string { } /* -MarshalJSON handles custom serialization required because the "data" element of a document -might be a collection of objects or a single object. +MarshalJSON handles the custom serialization case caused by case where the "data" +element of a document might be either a single resource object, or a collection of +them. */ func (d *Document) MarshalJSON() ([]byte, error) { - // subtype that overrides data with a single object - type MarshalObject struct { - Document - Data *Object `json:"data,omitempty"` + type MarshalDoc Document + doc := MarshalDoc(*d) + + // get raw data so we can perform top level operations + rawData, err := json.Marshal(doc) + if err != nil { + return nil, err } - if d == nil { - return nil, nil + // map back to top level JSON map for ease of use, SHITTY, I KNOW. + jDoc := map[string]interface{}{} + err = json.Unmarshal(rawData, &jDoc) + if err != nil { + return nil, ISE("Error marshaling document") } - if d.Object != nil { - return json.Marshal(MarshalObject{ - Document: *d, - Data: d.Object, - }) + + switch d.Mode { + case ErrorMode: + delete(jDoc, "data") + break + + // here we're expected to return an object as the data value rather than an array + // since it was a fetch request. Achieve this by replacing the single value array + // with the object itself + case ObjectMode: + if len(d.Data) == 0 { + jDoc["data"] = nil + } + + objects, success := jDoc["data"].([]interface{}) + if !success { + return nil, ISE("Unable to cast data to []interface{}") + } + jDoc["data"] = objects[0] + break } - // avoid stack overflow by using this subtype for marshaling - type MarshalList Document - return json.Marshal(MarshalList(*d)) + return json.Marshal(jDoc) } diff --git a/document_test.go b/document_test.go index 7013467..4692380 100644 --- a/document_test.go +++ b/document_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" + "github.com/davecgh/go-spew/spew" . "github.com/smartystreets/goconvey/convey" ) @@ -36,12 +37,23 @@ func TestDocument(t *testing.T) { }) Convey("->AddObject()", func() { + obj, err := NewObject("1", "user", nil) So(err, ShouldBeNil) - err = doc.AddObject(obj) - So(err, ShouldBeNil) - So(len(doc.Data), ShouldEqual, 1) + Convey("should successfully add an object", func() { + err := doc.AddObject(obj) + So(err, ShouldBeNil) + So(len(doc.Data), ShouldEqual, 1) + }) + + Convey("should prevent multiple data objects in ObjectMode", func() { + err := doc.AddObject(obj) + So(err, ShouldBeNil) + + err = doc.AddObject(obj) + So(err, ShouldNotBeNil) + }) }) Convey("->AddError()", func() { @@ -65,6 +77,14 @@ func TestDocument(t *testing.T) { }) }) + Convey("->First()", func() { + + Convey("should not explode for nil data", func() { + data := doc.First() + So(data, ShouldBeNil) + }) + }) + Convey("->Build()", func() { testObject := &Object{ @@ -83,8 +103,9 @@ func TestDocument(t *testing.T) { Convey("should accept an object", func() { doc := Build(testObject) - So(doc.Data, ShouldBeNil) - So(doc.Object, ShouldResemble, testObject) + // So(doc.Data, ShouldBeNil) + // So(doc.Object, ShouldResemble, testObject) + So(doc.Data, ShouldResemble, List{testObject}) So(doc.Status, ShouldEqual, http.StatusAccepted) }) @@ -105,8 +126,9 @@ func TestDocument(t *testing.T) { validationErrors := doc.Validate(req, true) So(validationErrors, ShouldBeNil) - So(doc.Data, ShouldBeNil) - So(doc.Object, ShouldResemble, testObject) + // So(doc.Data, ShouldBeNil) + // So(doc.Object, ShouldResemble, testObject) + So(doc.Data, ShouldResemble, List{testObject}) So(doc.Included, ShouldNotBeEmpty) So(doc.Included[0], ShouldResemble, testObjectForInclusion) So(doc.Status, ShouldEqual, http.StatusAccepted) @@ -130,38 +152,99 @@ func TestDocument(t *testing.T) { }) Convey("->MarshalJSON()", func() { + testObject := &Object{ ID: "1", Type: "Test", Status: http.StatusAccepted, } - Convey("should marshal a list with a single element as an array", func() { - list := List{testObject} - doc := Build(list) + Convey("should not include data for error response", func() { + doc.Data = nil + doc.AddError(ISE("Test Error")) j, err := json.Marshal(doc) So(err, ShouldBeNil) + m := map[string]json.RawMessage{} err = json.Unmarshal(j, &m) So(err, ShouldBeNil) - data := string(m["data"]) - So(data, ShouldStartWith, "[") - So(data, ShouldEndWith, "]") + spew.Dump(m) + + _, exists := m["data"] + So(exists, ShouldBeFalse) + + errors := string(m["errors"]) + So(errors, ShouldStartWith, "[") + So(errors, ShouldEndWith, "]") }) - Convey("should marshal a single object as an object", func() { - doc := Build(testObject) - j, err := json.Marshal(doc) - So(err, ShouldBeNil) - m := map[string]json.RawMessage{} - err = json.Unmarshal(j, &m) - So(err, ShouldBeNil) - data := string(m["data"]) - So(data, ShouldStartWith, "{") - So(data, ShouldEndWith, "}") + Convey("ListMode", func() { + + Convey("should marshal a list with a single element as an array", func() { + spew.Dump("listTest") + list := List{testObject} + doc := Build(list) + + spew.Dump(doc) + + j, err := json.Marshal(doc) + So(err, ShouldBeNil) + + m := map[string]json.RawMessage{} + err = json.Unmarshal(j, &m) + So(err, ShouldBeNil) + + data := string(m["data"]) + So(data, ShouldStartWith, "[") + So(data, ShouldEndWith, "]") + }) }) + Convey("ObjectMode", func() { + + Convey("should marshal a single object as an object", func() { + doc := Build(testObject) + j, err := json.Marshal(doc) + So(err, ShouldBeNil) + + m := map[string]json.RawMessage{} + err = json.Unmarshal(j, &m) + So(err, ShouldBeNil) + + data := string(m["data"]) + So(data, ShouldStartWith, "{") + So(data, ShouldEndWith, "}") + }) + + Convey("null case", func() { + doc := New() + + Convey("should marshal nil to null", func() { + doc.Data = nil + j, err := json.Marshal(doc) + So(err, ShouldBeNil) + + m := map[string]json.RawMessage{} + err = json.Unmarshal(j, &m) + So(err, ShouldBeNil) + + data := string(m["data"]) + So(data, ShouldEqual, "null") + }) + + Convey("should marshal an empty list to null", func() { + j, err := json.Marshal(doc) + So(err, ShouldBeNil) + + m := map[string]json.RawMessage{} + err = json.Unmarshal(j, &m) + So(err, ShouldBeNil) + + data := string(m["data"]) + So(data, ShouldEqual, "null") + }) + }) + }) }) }) - } diff --git a/parser.go b/parser.go index c91d9d9..f4875f6 100644 --- a/parser.go +++ b/parser.go @@ -43,7 +43,7 @@ as part of your full flow. } */ func ParseObject(r *http.Request) (*Object, *Error) { - document, err := ParseJSON(r) + document, err := ParseDoc(r, ObjectMode) if err != nil { return nil, err } @@ -65,7 +65,7 @@ ParseList validates the HTTP request and returns a resulting list of objects parsed from the request Body. Use just like ParseObject. */ func ParseList(r *http.Request) (List, *Error) { - document, err := ParseJSON(r) + document, err := ParseDoc(r, ListMode) if err != nil { return nil, err } @@ -73,13 +73,16 @@ func ParseList(r *http.Request) (List, *Error) { return document.Data, nil } -// ParseJSON is a convenience function that returns a top level jsh.JSON document -func ParseJSON(r *http.Request) (*Document, *Error) { - return NewParser(r).Document(r.Body) +/* +ParseDoc parses and returns a top level jsh.Document. In most cases, using +"ParseList" or "ParseObject" is preferable. +*/ +func ParseDoc(r *http.Request, mode DocumentMode) (*Document, *Error) { + return NewParser(r).Document(r.Body, mode) } -// Parser is an abstraction layer to support parsing JSON payload from many types -// of sources in order to allow other packages to use this parser +// Parser is an abstraction layer that helps to support parsing JSON payload from +// many types of sources, and allows other libraries to leverage this if desired. type Parser struct { Method string Headers http.Header @@ -94,10 +97,10 @@ func NewParser(request *http.Request) *Parser { } /* -Document returns a single JSON data object from the parser. In the process it will also validate -any data objects against the JSON API. +Document returns a single JSON data object from the parser. In the process it will +also validate any data objects against the JSON API. */ -func (p *Parser) Document(payload io.ReadCloser) (*Document, *Error) { +func (p *Parser) Document(payload io.ReadCloser, mode DocumentMode) (*Document, *Error) { defer closeReader(payload) err := validateHeaders(p.Headers) @@ -105,7 +108,11 @@ func (p *Parser) Document(payload io.ReadCloser) (*Document, *Error) { return nil, err } - document := &Document{Data: List{}} + document := &Document{ + Data: List{}, + Mode: mode, + } + decodeErr := json.NewDecoder(payload).Decode(document) if decodeErr != nil { return nil, ISE(fmt.Sprintf("Error parsing JSON Document: %s", decodeErr.Error())) @@ -114,7 +121,7 @@ func (p *Parser) Document(payload io.ReadCloser) (*Document, *Error) { // If the document has data, validate against specification if document.HasData() { for _, object := range document.Data { - + // TODO: currently this doesn't really do any user input // validation since it is validating against the jsh // "Object" type. Figure out how to options pass the From 8f5ab8bc1d396704028b065c2417edb6745c192a Mon Sep 17 00:00:00 2001 From: Derek Dowling Date: Tue, 8 Mar 2016 12:01:37 -0800 Subject: [PATCH 4/5] Adding additional test coverage, fixing broken tests --- document.go | 7 ++- document_test.go | 155 +++++++++++++++++++++++++++++------------------ list.go | 23 ------- list_test.go | 21 +++++++ 4 files changed, 121 insertions(+), 85 deletions(-) diff --git a/document.go b/document.go index d93d2db..b1eab76 100644 --- a/document.go +++ b/document.go @@ -190,11 +190,11 @@ AddError adds an error to the Document. It will also set the document Mode to func (d *Document) AddError(newErr *Error) *Error { if d.HasData() { - return ISE("Cannot add an error to a document already possessing data") + return ISE("Attempting to set an error, when the document has prepared response data") } if newErr.Status == 0 { - return ISE("Status code must be set for an error") + return ISE("No HTTP Status code provided for error, cannot add to document") } if d.Status == 0 { @@ -273,8 +273,9 @@ func (d *Document) MarshalJSON() ([]byte, error) { // since it was a fetch request. Achieve this by replacing the single value array // with the object itself case ObjectMode: - if len(d.Data) == 0 { + if d.Data == nil || len(d.Data) == 0 { jDoc["data"] = nil + break } objects, success := jDoc["data"].([]interface{}) diff --git a/document_test.go b/document_test.go index 4692380..86c7fdf 100644 --- a/document_test.go +++ b/document_test.go @@ -6,7 +6,6 @@ import ( "encoding/json" - "github.com/davecgh/go-spew/spew" . "github.com/smartystreets/goconvey/convey" ) @@ -63,6 +62,7 @@ func TestDocument(t *testing.T) { err := doc.AddError(testError) So(err, ShouldBeNil) So(len(doc.Errors), ShouldEqual, 1) + So(doc.Mode, ShouldEqual, ErrorMode) }) Convey("should error if validation fails while adding an error", func() { @@ -93,26 +93,52 @@ func TestDocument(t *testing.T) { Status: http.StatusAccepted, } - testObjectForInclusion := &Object{ - ID: "1", - Type: "Included", - } - - req := &http.Request{Method: "GET"} - Convey("should accept an object", func() { doc := Build(testObject) - // So(doc.Data, ShouldBeNil) - // So(doc.Object, ShouldResemble, testObject) So(doc.Data, ShouldResemble, List{testObject}) So(doc.Status, ShouldEqual, http.StatusAccepted) + So(doc.Mode, ShouldEqual, ObjectMode) + }) + + Convey("should accept a list", func() { + list := List{testObject} + doc := Build(list) + + So(doc.Data, ShouldResemble, list) + So(doc.Status, ShouldEqual, http.StatusOK) + So(doc.Mode, ShouldEqual, ListMode) + }) + + Convey("should accept an error", func() { + err := &Error{Status: http.StatusInternalServerError} + doc := Build(err) + + So(doc.Errors, ShouldNotBeEmpty) + So(doc.Status, ShouldEqual, err.Status) + So(doc.Mode, ShouldEqual, ErrorMode) }) + }) + + Convey("->Validate()", func() { + + testObject := &Object{ + ID: "1", + Type: "Test", + Status: http.StatusAccepted, + } + + testObjectForInclusion := &Object{ + ID: "1", + Type: "Included", + } + + req := &http.Request{Method: "GET"} Convey("should not accept an included object without objects in data", func() { doc := New() doc.Included = append(doc.Included, testObjectForInclusion) - doc.Status = 200 + doc.Status = http.StatusOK validationErrors := doc.Validate(req, true) @@ -126,30 +152,21 @@ func TestDocument(t *testing.T) { validationErrors := doc.Validate(req, true) So(validationErrors, ShouldBeNil) - // So(doc.Data, ShouldBeNil) - // So(doc.Object, ShouldResemble, testObject) So(doc.Data, ShouldResemble, List{testObject}) So(doc.Included, ShouldNotBeEmpty) So(doc.Included[0], ShouldResemble, testObjectForInclusion) So(doc.Status, ShouldEqual, http.StatusAccepted) }) - Convey("should accept a list", func() { - list := List{testObject} - doc := Build(list) + }) + }) +} - So(doc.Data, ShouldResemble, list) - So(doc.Status, ShouldEqual, http.StatusOK) - }) +func TestDocumentMarshaling(t *testing.T) { - Convey("should accept an error", func() { - err := &Error{Status: 500} - doc := Build(err) + Convey("Document Marshal Tests", t, func() { - So(doc.Errors, ShouldNotBeEmpty) - So(doc.Status, ShouldEqual, err.Status) - }) - }) + doc := New() Convey("->MarshalJSON()", func() { @@ -159,56 +176,54 @@ func TestDocument(t *testing.T) { Status: http.StatusAccepted, } - Convey("should not include data for error response", func() { - doc.Data = nil - doc.AddError(ISE("Test Error")) - j, err := json.Marshal(doc) - So(err, ShouldBeNil) - - m := map[string]json.RawMessage{} - err = json.Unmarshal(j, &m) - So(err, ShouldBeNil) - spew.Dump(m) - - _, exists := m["data"] - So(exists, ShouldBeFalse) - - errors := string(m["errors"]) - So(errors, ShouldStartWith, "[") - So(errors, ShouldEndWith, "]") - }) - Convey("ListMode", func() { Convey("should marshal a list with a single element as an array", func() { - spew.Dump("listTest") list := List{testObject} doc := Build(list) - spew.Dump(doc) - - j, err := json.Marshal(doc) + rawJSON, err := json.Marshal(doc) So(err, ShouldBeNil) m := map[string]json.RawMessage{} - err = json.Unmarshal(j, &m) + err = json.Unmarshal(rawJSON, &m) So(err, ShouldBeNil) data := string(m["data"]) So(data, ShouldStartWith, "[") So(data, ShouldEndWith, "]") }) + + Convey("should marshal an empty list", func() { + list := List{} + doc := Build(list) + + rawJSON, err := json.Marshal(doc) + So(err, ShouldBeNil) + + m := map[string]json.RawMessage{} + err = json.Unmarshal(rawJSON, &m) + So(err, ShouldBeNil) + + data := string(m["data"]) + So(data, ShouldEqual, "[]") + }) }) Convey("ObjectMode", func() { + doc := New() + doc.Mode = ObjectMode + Convey("should marshal a single object as an object", func() { - doc := Build(testObject) - j, err := json.Marshal(doc) + addErr := doc.AddObject(testObject) + So(addErr, ShouldBeNil) + + rawJSON, err := json.Marshal(doc) So(err, ShouldBeNil) m := map[string]json.RawMessage{} - err = json.Unmarshal(j, &m) + err = json.Unmarshal(rawJSON, &m) So(err, ShouldBeNil) data := string(m["data"]) @@ -216,16 +231,15 @@ func TestDocument(t *testing.T) { So(data, ShouldEndWith, "}") }) - Convey("null case", func() { - doc := New() + Convey("null cases", func() { Convey("should marshal nil to null", func() { doc.Data = nil - j, err := json.Marshal(doc) + rawJSON, err := json.Marshal(doc) So(err, ShouldBeNil) m := map[string]json.RawMessage{} - err = json.Unmarshal(j, &m) + err = json.Unmarshal(rawJSON, &m) So(err, ShouldBeNil) data := string(m["data"]) @@ -233,11 +247,12 @@ func TestDocument(t *testing.T) { }) Convey("should marshal an empty list to null", func() { - j, err := json.Marshal(doc) + doc.Data = List{} + rawJSON, err := json.Marshal(doc) So(err, ShouldBeNil) m := map[string]json.RawMessage{} - err = json.Unmarshal(j, &m) + err = json.Unmarshal(rawJSON, &m) So(err, ShouldBeNil) data := string(m["data"]) @@ -245,6 +260,28 @@ func TestDocument(t *testing.T) { }) }) }) + + Convey("ErrorMode", func() { + + Convey("should not include 'data' field for error response", func() { + doc.AddError(ISE("Test Error")) + + rawJSON, err := json.Marshal(doc) + So(err, ShouldBeNil) + + jMap := map[string]json.RawMessage{} + err = json.Unmarshal(rawJSON, &jMap) + So(err, ShouldBeNil) + + _, exists := jMap["data"] + So(exists, ShouldBeFalse) + + errors := string(jMap["errors"]) + So(errors, ShouldNotBeEmpty) + So(errors, ShouldStartWith, "[") + So(errors, ShouldEndWith, "]") + }) + }) }) }) } diff --git a/list.go b/list.go index 846b87e..713a1b8 100644 --- a/list.go +++ b/list.go @@ -50,26 +50,3 @@ func (list *List) UnmarshalJSON(rawData []byte) error { return nil } - -/* -MarshalJSON returns a JSON encoded list for "data". We use a pointer receiver here -so we are able to distinguish between nil (don't serialize) and empty (serialize as []). -*/ -func (list *List) MarshalJSON() ([]byte, error) { - // avoid stack overflow by using this subtype for marshaling - type MarshalList List - - if list == nil { - return nil, nil - } - - marshalList := MarshalList(*list) - count := len(marshalList) - - switch { - case count == 0: - return []byte("[]"), nil - default: - return json.Marshal(marshalList) - } -} diff --git a/list_test.go b/list_test.go index c1fe07e..feddadf 100644 --- a/list_test.go +++ b/list_test.go @@ -105,6 +105,27 @@ func TestList(t *testing.T) { So(err, ShouldBeNil) So(l, ShouldNotBeEmpty) }) + + Convey("should handle an empty array", func() { + jObj := `{"data": []}` + + l := List{} + err := l.UnmarshalJSON([]byte(jObj)) + So(err, ShouldBeNil) + So(l, ShouldNotBeNil) + }) + }) + + Convey("->MarshalJSON()", func() { + + Convey("should preserve an empty list", func() { + list := List{} + + jData, err := json.Marshal(list) + So(err, ShouldBeNil) + + So(string(jData), ShouldEqual, "[]") + }) }) }) } From c3ff84e773f431e218a86b3741d0ac87dd3a9b67 Mon Sep 17 00:00:00 2001 From: Derek Dowling Date: Tue, 8 Mar 2016 12:19:35 -0800 Subject: [PATCH 5/5] Implementing more elegant document marshaling logic --- document.go | 63 ++++++++++++++++++++++++++++------------------------- 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/document.go b/document.go index b1eab76..19ede25 100644 --- a/document.go +++ b/document.go @@ -248,43 +248,46 @@ element of a document might be either a single resource object, or a collection them. */ func (d *Document) MarshalJSON() ([]byte, error) { + // we use the MarshalDoc type to avoid recursively calling this function below + // when we marshal type MarshalDoc Document doc := MarshalDoc(*d) - // get raw data so we can perform top level operations - rawData, err := json.Marshal(doc) - if err != nil { - return nil, err - } - - // map back to top level JSON map for ease of use, SHITTY, I KNOW. - jDoc := map[string]interface{}{} - err = json.Unmarshal(rawData, &jDoc) - if err != nil { - return nil, ISE("Error marshaling document") - } - switch d.Mode { - case ErrorMode: - delete(jDoc, "data") - break - - // here we're expected to return an object as the data value rather than an array - // since it was a fetch request. Achieve this by replacing the single value array - // with the object itself case ObjectMode: - if d.Data == nil || len(d.Data) == 0 { - jDoc["data"] = nil - break + var data *Object + if len(d.Data) > 0 { + data = d.Data[0] } - objects, success := jDoc["data"].([]interface{}) - if !success { - return nil, ISE("Unable to cast data to []interface{}") + // subtype that overrides regular data List with a single Object for + // fetch style request/responses + type MarshalObject struct { + MarshalDoc + Data *Object `json:"data"` } - jDoc["data"] = objects[0] - break - } - return json.Marshal(jDoc) + return json.Marshal(MarshalObject{ + MarshalDoc: doc, + Data: data, + }) + + case ErrorMode: + // subtype that omits data as expected for error responses. We cannot simply + // use json:"-" for the data attribute otherwise it will not override the + // default struct tag of it the composed MarshalDoc struct. + type MarshalError struct { + MarshalDoc + Data *Object `json:"data,omitempty"` + } + + return json.Marshal(MarshalError{ + MarshalDoc: doc, + }) + + case ListMode: + return json.Marshal(doc) + default: + return nil, ISE(fmt.Sprintf("Unexpected DocumentMode value when marshaling: %d", d.Mode)) + } }