Skip to content

Commit

Permalink
Returns full response after hooks modification
Browse files Browse the repository at this point in the history 
When `Prefer: return=minimal` HTTP request header is set, server will
try to return `204 No Content` response code. However since rest-layer's hooks
can modify the resource item server-side, rest-layer will return full body if that happens,
even if `return=minimal` requested.
If field projection is requested, only fields within projection
 are considered for the logic above.

Resolves rs#256
  • Loading branch information
@Dragomir-Ivanov
Dragomir-Ivanov committed Aug 22, 2019
1 parent 8908708 commit 109b5a2
Show file tree
Hide file tree
Showing 8 changed files with 106 additions and 15 deletions.
2 changes: 1 addition & 1 deletion resource/item.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func NewItem(payload map[string]interface{}) (*Item, error) {
if !found {
return nil, errors.New("Missing ID field")
}
etag, err := genEtag(payload)
etag, err := GenEtag(payload)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion resource/resource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ func recalcEtag(items []*Item) error {
if v == nil {
continue
}
etag, err := genEtag(v.Payload)
etag, err := GenEtag(v.Payload)
if err != nil {
return err
}
Expand Down
4 changes: 2 additions & 2 deletions resource/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import (
"fmt"
)

// Etag computes an etag based on content of the payload.
func genEtag(payload map[string]interface{}) (string, error) {
// Etag computes an etag based on containt of the payload.
func GenEtag(payload map[string]interface{}) (string, error) {
b, err := json.Marshal(payload)
if err != nil {
return "", err
Expand Down
4 changes: 2 additions & 2 deletions resource/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ import (
)

func TestGenEtag(t *testing.T) {
_, err := genEtag(map[string]interface{}{"id": 1, "field": func() {}})
_, err := GenEtag(map[string]interface{}{"id": 1, "field": func() {}})
assert.EqualError(t, err, "json: unsupported type: func()")
e, err := genEtag(map[string]interface{}{"id": 1, "field": "data"})
e, err := GenEtag(map[string]interface{}{"id": 1, "field": "data"})
assert.NoError(t, err)
assert.Equal(t, "77bbb326e8b529284d96557621ca5432", e)
}
7 changes: 1 addition & 6 deletions rest/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,7 @@ func (h *Handler) ServeHTTPC(ctx context.Context, w http.ResponseWriter, r *http
h.FallbackHandlerFunc(ctx, w, r)
return
}
if r.Method != "HEAD" && body != nil && (status == 200 || status == 201) && isNoContent(r) {
skipBody = true
if status == 200 {
status = 204
}
}

h.sendResponse(ctx, w, status, headers, body, skipBody)
}

Expand Down
33 changes: 32 additions & 1 deletion rest/method_item_patch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,21 @@ func itemPatch(ctx context.Context, r *http.Request, route *RouteMatch) (status
e = NewError(err)
return e.Code, nil, e
}

preHookEtag := item.ETag

This comment has been minimized.

Sign in to view

Copy link
@smyrman

smyrman Aug 23, 2019

If we end up going for this solution, we should try to make sure we only do this extra work (projection generation and E-tag extraction) if a minimal response is actually requested.

Either way this looks perfect for testing the feature out a bit more 😉
if len(q.Projection) > 0 {
projected, err := q.Projection.Eval(ctx, item.Payload, restResource{rsrc})
if err != nil {
e = NewError(err)
return e.Code, nil, e
}
preHookEtag, err = resource.GenEtag(projected)
if err != nil {
e = NewError(err)
return e.Code, nil, e
}
}

// Store the modified document by providing the original doc to instruct
// handler to ensure the stored document didn't change between in the
// interval. An ErrPreconditionFailed will be thrown in case of race
Expand All @@ -109,11 +124,27 @@ func itemPatch(ctx context.Context, r *http.Request, route *RouteMatch) (status
return e.Code, nil, e
}

postHookEtag := item.ETag
// Evaluate projection so response gets the same format as read requests.
item.Payload, err = q.Projection.Eval(ctx, item.Payload, restResource{rsrc})
if err != nil {
e = NewError(err)
return e.Code, nil, e
}
return 200, nil, item

if len(q.Projection) > 0 {
postHookEtag, err = resource.GenEtag(item.Payload)
if err != nil {
e = NewError(err)
return e.Code, nil, e
}
}

status = 200
if isNoContent(r) && preHookEtag == postHookEtag {
item = nil
status = 204
}

return status, nil, item
}
34 changes: 33 additions & 1 deletion rest/method_item_put.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,20 @@ func itemPut(ctx context.Context, r *http.Request, route *RouteMatch) (status in
e = NewError(err)
return e.Code, nil, e
}

preHookEtag := item.ETag
if len(q.Projection) > 0 {
projected, err := q.Projection.Eval(ctx, item.Payload, restResource{rsrc})
if err != nil {
e = NewError(err)
return e.Code, nil, e
}
preHookEtag, err = resource.GenEtag(projected)
if err != nil {
e = NewError(err)
return e.Code, nil, e
}
}
// If we have an original item, pass it to the handler so we make sure
// we are still replacing the same version of the object as handler is
// supposed check the original etag before storing when an original object
Expand All @@ -97,11 +111,29 @@ func itemPut(ctx context.Context, r *http.Request, route *RouteMatch) (status in
return e.Code, nil, e
}
}
// Evaluate projection so response gets the same format as read requests.

postHookEtag := item.ETag
item.Payload, err = q.Projection.Eval(ctx, item.Payload, restResource{rsrc})
if err != nil {
e = NewError(err)
return e.Code, nil, e
}

if len(q.Projection) > 0 {
postHookEtag, err = resource.GenEtag(item.Payload)
if err != nil {
e = NewError(err)
return e.Code, nil, e
}
}

if isNoContent(r) && preHookEtag == postHookEtag {
item = nil
if status == 200 {
status = 204
}
// 201 will be returned as is, but with empty body
}

return status, nil, item
}
35 changes: 34 additions & 1 deletion rest/method_post.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,28 @@ func listPost(ctx context.Context, r *http.Request, route *RouteMatch) (status i
e = NewError(err)
return e.Code, nil, e
}

preHookEtag := item.ETag
if len(q.Projection) > 0 {
projected, err := q.Projection.Eval(ctx, item.Payload, restResource{rsrc})
if err != nil {
e = NewError(err)
return e.Code, nil, e
}
preHookEtag, err = resource.GenEtag(projected)
if err != nil {
e = NewError(err)
return e.Code, nil, e
}
}

// TODO: add support for batch insert
if err = rsrc.Insert(ctx, []*resource.Item{item}); err != nil {
e = NewError(err)
return e.Code, nil, e
}

postHookEtag := item.ETag
// Evaluate projection so response gets the same format as read requests.
item.Payload, err = q.Projection.Eval(ctx, item.Payload, restResource{rsrc})
if err != nil {
Expand All @@ -57,5 +74,21 @@ func listPost(ctx context.Context, r *http.Request, route *RouteMatch) (status i
}
}
headers.Set("Content-Location", fmt.Sprintf("%s/%s", r.URL.Path, itemID))
return 201, headers, item

if len(q.Projection) > 0 {
postHookEtag, err = resource.GenEtag(item.Payload)
if err != nil {
e = NewError(err)
return e.Code, nil, e
}
}

status = 201
if isNoContent(r) && preHookEtag == postHookEtag {
item = nil
// Should we keep 201 here as well, like on PUT?

This comment has been minimized.

Sign in to view

Copy link
@smyrman

smyrman Aug 23, 2019

Yeah, we should probably always keep the 201.
status = 204
}

return status, headers, item
}

0 comments on commit 109b5a2

Please sign in to comment.