Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent removing of (Required,Default) fields in HTTP PUT. Fixes #174 #206

Merged
merged 2 commits into from
Nov 30, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Prevent removing of (Required,Default) fields in HTTP PUT. Fixes #174
  • Loading branch information
Dragomir-Ivanov committed Nov 30, 2018
commit f56d27da447fd568b64788a3ac6c2f08a3a9ab92
34 changes: 34 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,14 @@ The REST Layer framework is composed of several sub-packages:
- [Data Integrity & Concurrency Control](#data-integrity-and-concurrency-control)
- [HTTP Headers Support](#http-headers-support)
- [Prefer](#prefer)
- [HTTP Methods](#http-methods)
- [HEAD](#head)
- [GET](#get)
- [POST](#post)
- [PUT](#put)
- [PATCH](#patch)
- [DELETE](#delete)
- [OPTIONS](#options)
- [Data Validation](#data-validation)
- [Nullable Values](#nullable-values)
- [Extensible Data Validation](#extensible-data-validation)
Expand Down Expand Up @@ -1179,6 +1187,32 @@ Prefer: return=minimal
HTTP/1.1 204 No Content
```

## HTTP Methods

Following HTTP Methods are supported currently.

### HEAD
The same as [GET](#get), except that it doesn't return any body.

### GET
Used to query a resource with its sub/embedded resources.

### POST
Used to create new resource document, where new `ID` is generated from the server.

### PUT
Used to create/update single resource document given its `ID`.\
Dragomir-Ivanov marked this conversation as resolved.
Show resolved Hide resolved
Be aware when dealing with resource fields with `Default` set. Initial creation for such resources will set particular field to its default value if omitted, however on subsequent `PUT` calls this field will be deleted if omitted. If persistent `Default` field is needed use `{Required: true}` with it.

### PATCH
Used to update/patch single resource document given its `ID`.

Dragomir-Ivanov marked this conversation as resolved.
Show resolved Hide resolved
### DELETE
Used to delete single resource document given its `ID`, or via [Query](#quering).

### OPTIONS
Used to tell the client, which HTTP Methods are supported on a given resource.

## Data Validation

Data validation is provided out-of-the-box. Your configuration includes a schema definition for every resource managed by the API. Data sent to the API to be inserted/updated will be validated against the schema, and a resource will only be updated if validation passes. See [Field Definition](#field-definition) section to know more about how to configure your validators.
Expand Down
307 changes: 287 additions & 20 deletions rest/method_item_put_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,27 @@ import (
"github.com/rs/rest-layer/schema/query"
)

func checkPayload(name string, id interface{}, payload map[string]interface{}) requestCheckerFunc {
return func(t *testing.T, vars *requestTestVars) {
var item *resource.Item

s := vars.Storers[name]
q := query.Query{Predicate: query.Predicate{&query.Equal{Field: "id", Value: id}}, Window: &query.Window{Limit: 1}}
if items, err := s.Find(context.Background(), &q); err != nil {
t.Errorf("s.Find failed: %s", err)
return
} else if len(items.Items) != 1 {
t.Errorf("item with ID %v not found", id)
return
} else {
item = items.Items[0]
}
if !reflect.DeepEqual(payload, item.Payload) {
t.Errorf("Unexpected stored payload for item %v:\nexpect: %#v\ngot: %#v", id, payload, item.Payload)
}
}
}

func TestPutItem(t *testing.T) {
now := time.Now()
yesterday := now.Add(-24 * time.Hour)
Expand Down Expand Up @@ -50,26 +71,6 @@ func TestPutItem(t *testing.T) {
Storers: map[string]resource.Storer{"foo": s1, "foo.sub": s2},
}
}
checkPayload := func(name string, id interface{}, payload map[string]interface{}) requestCheckerFunc {
return func(t *testing.T, vars *requestTestVars) {
var item *resource.Item

s := vars.Storers[name]
q := query.Query{Predicate: query.Predicate{&query.Equal{Field: "id", Value: id}}, Window: &query.Window{Limit: 1}}
if items, err := s.Find(context.Background(), &q); err != nil {
t.Errorf("s.Find failed: %s", err)
return
} else if len(items.Items) != 1 {
t.Errorf("item with ID %v not found", id)
return
} else {
item = items.Items[0]
}
if !reflect.DeepEqual(payload, item.Payload) {
t.Errorf("Unexpected stored payload for item %v:\nexpect: %#v\ngot: %#v", id, payload, item.Payload)
}
}
}

tests := map[string]requestTest{
`NoStorage`: {
Expand Down Expand Up @@ -315,3 +316,269 @@ func TestPutItem(t *testing.T) {
t.Run(n, tc.Test)
}
}

func TestPutItemDefault(t *testing.T) {
now := time.Now()

sharedInit := func() *requestTestVars {
s1 := mem.NewHandler()
s1.Insert(context.Background(), []*resource.Item{
{ID: "1", ETag: "a", Updated: now, Payload: map[string]interface{}{"id": "1", "foo": "odd"}},
{ID: "2", ETag: "b", Updated: now, Payload: map[string]interface{}{"id": "2", "foo": "odd", "bar": "value"}},
})
idx := resource.NewIndex()
idx.Bind("foo", schema.Schema{
Fields: schema.Fields{
"id": {Sortable: true, Filterable: true},
"foo": {Filterable: true},
"bar": {Filterable: true, Default: "default"},
},
}, s1, resource.DefaultConf)
return &requestTestVars{
Index: idx,
Storers: map[string]resource.Storer{"foo": s1},
}
}

tests := map[string]requestTest{
`pathID:not-found,body:valid,default:missing`: {
Init: sharedInit,
NewRequest: func() (*http.Request, error) {
body := bytes.NewReader([]byte(`{"foo": "baz"}`))
return http.NewRequest("PUT", "/foo/66", body)
},
ResponseCode: http.StatusCreated,
ResponseBody: `{"id": "66", "foo": "baz", "bar": "default"}`,
ExtraTest: checkPayload("foo", "66", map[string]interface{}{"id": "66", "foo": "baz", "bar": "default"}),
},
`pathID:not-found,body:valid,default:set`: {
Init: sharedInit,
NewRequest: func() (*http.Request, error) {
body := bytes.NewReader([]byte(`{"foo": "baz", "bar": "value"}`))
return http.NewRequest("PUT", "/foo/66", body)
},
ResponseCode: http.StatusCreated,
ResponseBody: `{"id": "66", "foo": "baz", "bar": "value"}`,
ExtraTest: checkPayload("foo", "66", map[string]interface{}{"id": "66", "foo": "baz", "bar": "value"}),
},
`pathID:found,body:valid,default:missing`: {
Init: sharedInit,
NewRequest: func() (*http.Request, error) {
body := bytes.NewReader([]byte(`{"foo": "baz"}`))
return http.NewRequest("PUT", "/foo/1", body)
},
ResponseCode: http.StatusOK,
ResponseBody: `{"id": "1", "foo": "baz"}`,
smyrman marked this conversation as resolved.
Show resolved Hide resolved
ExtraTest: checkPayload("foo", "1", map[string]interface{}{"id": "1", "foo": "baz"}),
},
`pathID:found,body:valid,default:set`: {
Init: sharedInit,
NewRequest: func() (*http.Request, error) {
body := bytes.NewReader([]byte(`{"foo": "baz", "bar": "value"}`))
return http.NewRequest("PUT", "/foo/1", body)
},
ResponseCode: http.StatusOK,
ResponseBody: `{"id": "1", "foo": "baz", "bar": "value"}`,
ExtraTest: checkPayload("foo", "1", map[string]interface{}{"id": "1", "foo": "baz", "bar": "value"}),
},
`pathID:found,body:valid,default:delete`: {
Init: sharedInit,
NewRequest: func() (*http.Request, error) {
body := bytes.NewReader([]byte(`{"foo": "baz"}`))
return http.NewRequest("PUT", "/foo/2", body)
},
ResponseCode: http.StatusOK,
ResponseBody: `{"id": "2", "foo": "baz"}`,
Dragomir-Ivanov marked this conversation as resolved.
Show resolved Hide resolved
ExtraTest: checkPayload("foo", "2", map[string]interface{}{"id": "2", "foo": "baz"}),
},
}

for n, tc := range tests {
tc := tc // capture range variable
t.Run(n, tc.Test)
}
}

func TestPutItemRequired(t *testing.T) {
now := time.Now()

sharedInit := func() *requestTestVars {
s1 := mem.NewHandler()
s1.Insert(context.Background(), []*resource.Item{
{ID: "1", ETag: "a", Updated: now, Payload: map[string]interface{}{"id": "1", "foo": "odd"}},
{ID: "2", ETag: "b", Updated: now, Payload: map[string]interface{}{"id": "2", "foo": "odd", "bar": "original"}},
})
idx := resource.NewIndex()
idx.Bind("foo", schema.Schema{
Fields: schema.Fields{
"id": {Sortable: true, Filterable: true},
"foo": {Filterable: true},
"bar": {Filterable: true, Required: true},
},
}, s1, resource.DefaultConf)
return &requestTestVars{
Index: idx,
Storers: map[string]resource.Storer{"foo": s1},
}
}

tests := map[string]requestTest{
`pathID:not-found,body:valid,required:missing`: {
Init: sharedInit,
NewRequest: func() (*http.Request, error) {
body := bytes.NewReader([]byte(`{"foo": "baz"}`))
return http.NewRequest("PUT", "/foo/66", body)
},
ResponseCode: http.StatusUnprocessableEntity,
ResponseBody: `{
"code": 422,
"message": "Document contains error(s)",
"issues": {
"bar": ["required"]
}
}`,
},
`pathID:not-found,body:valid,required:set`: {
Init: sharedInit,
NewRequest: func() (*http.Request, error) {
body := bytes.NewReader([]byte(`{"foo": "baz", "bar": "value"}`))
return http.NewRequest("PUT", "/foo/1", body)
},
ResponseCode: http.StatusOK,
ResponseBody: `{"id": "1", "foo": "baz", "bar": "value"}`,
ExtraTest: checkPayload("foo", "1", map[string]interface{}{"id": "1", "foo": "baz", "bar": "value"}),
},
`pathID:found,body:valid,required:missing`: {
Init: sharedInit,
NewRequest: func() (*http.Request, error) {
body := bytes.NewReader([]byte(`{"foo": "baz"}`))
return http.NewRequest("PUT", "/foo/1", body)
},
ResponseCode: http.StatusUnprocessableEntity,
ResponseBody: `{
"code": 422,
"message": "Document contains error(s)",
"issues": {
"bar": ["required"]
}
}`,
},
`pathID:found,body:valid,required:change`: {
Init: sharedInit,
NewRequest: func() (*http.Request, error) {
body := bytes.NewReader([]byte(`{"foo": "baz", "bar": "value1"}`))
return http.NewRequest("PUT", "/foo/2", body)
},
ResponseCode: http.StatusOK,
ResponseBody: `{"id": "2", "foo": "baz", "bar": "value1"}`,
ExtraTest: checkPayload("foo", "2", map[string]interface{}{"id": "2", "foo": "baz", "bar": "value1"}),
},
`pathID:found,body:valid,required:delete`: {
Init: sharedInit,
NewRequest: func() (*http.Request, error) {
body := bytes.NewReader([]byte(`{"foo": "baz"}`))
return http.NewRequest("PUT", "/foo/2", body)
},
ResponseCode: http.StatusUnprocessableEntity,
ResponseBody: `{
"code": 422,
"message": "Document contains error(s)",
"issues": {
"bar": ["required"]
}
}`,
},
}

for n, tc := range tests {
tc := tc // capture range variable
t.Run(n, tc.Test)
}
}

func TestPutItemRequiredDefault(t *testing.T) {
now := time.Now()

sharedInit := func() *requestTestVars {
s1 := mem.NewHandler()
s1.Insert(context.Background(), []*resource.Item{
{ID: "1", ETag: "a", Updated: now, Payload: map[string]interface{}{"id": "1", "foo": "odd"}},
{ID: "2", ETag: "b", Updated: now, Payload: map[string]interface{}{"id": "2", "foo": "odd", "bar": "original"}},
})
idx := resource.NewIndex()
idx.Bind("foo", schema.Schema{
Fields: schema.Fields{
"id": {Sortable: true, Filterable: true},
"foo": {Filterable: true},
"bar": {Filterable: true, Required: true, Default: "default"},
},
}, s1, resource.DefaultConf)
return &requestTestVars{
Index: idx,
Storers: map[string]resource.Storer{"foo": s1},
}
}

tests := map[string]requestTest{
`pathID:not-found,body:valid,required-default:missing`: {
Init: sharedInit,
NewRequest: func() (*http.Request, error) {
body := bytes.NewReader([]byte(`{"foo": "baz"}`))
return http.NewRequest("PUT", "/foo/66", body)
},
ResponseCode: http.StatusCreated,
ResponseBody: `{"id": "66", "foo": "baz", "bar": "default"}`,
ExtraTest: checkPayload("foo", "66", map[string]interface{}{"id": "66", "foo": "baz", "bar": "default"}),
},
`pathID:not-found,body:valid,required-default:set`: {
Init: sharedInit,
NewRequest: func() (*http.Request, error) {
body := bytes.NewReader([]byte(`{"foo": "baz", "bar": "value"}`))
return http.NewRequest("PUT", "/foo/1", body)
},
ResponseCode: http.StatusOK,
ResponseBody: `{"id": "1", "foo": "baz", "bar": "value"}`,
ExtraTest: checkPayload("foo", "1", map[string]interface{}{"id": "1", "foo": "baz", "bar": "value"}),
},
`pathID:found,body:valid,required-default:missing`: {
Init: sharedInit,
NewRequest: func() (*http.Request, error) {
body := bytes.NewReader([]byte(`{"foo": "baz"}`))
return http.NewRequest("PUT", "/foo/1", body)
},
ResponseCode: http.StatusUnprocessableEntity,
ResponseBody: `{
"code": 422,
"message": "Document contains error(s)",
"issues": {
"bar": ["required"]
smyrman marked this conversation as resolved.
Show resolved Hide resolved
}
}`,
},
`pathID:found,body:valid,required-default:change`: {
Init: sharedInit,
NewRequest: func() (*http.Request, error) {
body := bytes.NewReader([]byte(`{"foo": "baz", "bar": "value"}`))
return http.NewRequest("PUT", "/foo/2", body)
},
ResponseCode: http.StatusOK,
ResponseBody: `{"id": "2", "foo": "baz", "bar": "value"}`,
ExtraTest: checkPayload("foo", "2", map[string]interface{}{"id": "2", "foo": "baz", "bar": "value"}),
},
`pathID:found,body:valid,required-default:delete`: {
Init: sharedInit,
NewRequest: func() (*http.Request, error) {
body := bytes.NewReader([]byte(`{"foo": "baz"}`))
return http.NewRequest("PUT", "/foo/2", body)
},
ResponseCode: http.StatusOK,
ResponseBody: `{"id": "2", "foo": "baz", "bar": "default"}`,
smyrman marked this conversation as resolved.
Show resolved Hide resolved
ExtraTest: checkPayload("foo", "2", map[string]interface{}{"id": "2", "foo": "baz", "bar": "default"}),
},
}

for n, tc := range tests {
tc := tc // capture range variable
t.Run(n, tc.Test)
}
}
4 changes: 3 additions & 1 deletion schema/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ func (s Schema) Prepare(ctx context.Context, payload map[string]interface{}, ori
// previous value as the client would have no way to resubmit the stored value.
if def.Hidden && !def.ReadOnly {
changes[field] = oValue
} else if def.Required && def.Default != nil {
changes[field] = def.Default
Dragomir-Ivanov marked this conversation as resolved.
Show resolved Hide resolved
} else {
changes[field] = Tombstone
}
Expand Down Expand Up @@ -227,7 +229,7 @@ func (s Schema) validate(changes map[string]interface{}, base map[string]interfa
}
// Check required fields.
if def.Required {
if value, found := changes[field]; !found || value == nil {
if value, found := changes[field]; !found || value == nil || value == Tombstone {
smyrman marked this conversation as resolved.
Show resolved Hide resolved
if found {
// If explicitly set to null, raise the required error.
addFieldError(errs, field, "required")
Expand Down