Skip to content

Commit e93caf6

Browse files
smyrmansmyrman
committed
Allow clear in rest package
Fixes issue #127 by allowing window (limit, skip, page) and predicate (filter) parameters to be parsed for DELETE requests. Updated URL parsing to give better error messages, including an issues section, similar to what's done for document errors. Updates all request method tests to use the public package interface over package internals. This should extend test coverage to include parsing of URL parameters and help prevent bugs like #127 from occurring again.
1 parent 6a60deb commit e93caf6

15 files changed

+1414
-1661
lines changed

‎internal/testutil/json_eq.go‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
package testutil
2+
3+
import (
4+
"encoding/json"
5+
"reflect"
6+
"testing"
7+
)
8+
9+
// JSONEq logs an error to t if expect and actual does not contain equivalent
10+
// JSON structures, or if either results in a JSON unmarshal error.
11+
// Returns true if no error was logged.
12+
func JSONEq(t testing.TB, expect, actual []byte) bool {
13+
t.Helper()
14+
var ei interface{}
15+
var ai interface{}
16+
17+
if err := json.Unmarshal(expect, &ei); err != nil {
18+
t.Errorf("failed to unmarshal JSON from expect:\ninput: %q\nerror: %s", expect, err.Error())
19+
return false
20+
}
21+
if err := json.Unmarshal(actual, &ai); err != nil {
22+
t.Errorf("failed to unmarshal JSON from actual:\ninput: %q\nerror: %s", actual, err.Error())
23+
return false
24+
}
25+
26+
if !reflect.DeepEqual(ei, ai) {
27+
// FIXME: a future version should probably log a JSON diff instead.
28+
t.Errorf("JSON not equal\nexpect: `%s`\nactual: `%s`", expect, actual)
29+
return false
30+
}
31+
return true
32+
}

‎rest/method_delete_test.go‎

Lines changed: 136 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -1,131 +1,162 @@
1-
package rest
1+
package rest_test
22

33
import (
4-
"bytes"
54
"context"
65
"net/http"
7-
"net/url"
86
"testing"
97

108
"github.com/rs/rest-layer/resource/testing/mem"
119
"github.com/rs/rest-layer/resource"
1210
"github.com/rs/rest-layer/schema"
1311
"github.com/rs/rest-layer/schema/query"
14-
"github.com/stretchr/testify/assert"
1512
)
1613

17-
func TestHandlerDeleteList(t *testing.T) {
18-
s := mem.NewHandler()
19-
s.Insert(context.TODO(), []*resource.Item{
20-
{ID: "1", Payload: map[string]interface{}{}},
21-
{ID: "2", Payload: map[string]interface{}{}},
22-
{ID: "3", Payload: map[string]interface{}{}},
23-
{ID: "4", Payload: map[string]interface{}{}},
24-
{ID: "5", Payload: map[string]interface{}{}},
25-
})
26-
index := resource.NewIndex()
27-
test := index.Bind("test", schema.Schema{}, s, resource.DefaultConf)
28-
r, _ := http.NewRequest("DELETE", "/test", bytes.NewBufferString("{}"))
29-
rm := &RouteMatch{
30-
ResourcePath: []*ResourcePathComponent{
31-
&ResourcePathComponent{
32-
Name: "test",
33-
Resource: test,
14+
func TestDeleteList(t *testing.T) {
15+
sharedInit := func() *requestTestVars {
16+
s := mem.NewHandler()
17+
s.Insert(context.Background(), []*resource.Item{
18+
{ID: "1", Payload: map[string]interface{}{"id": "1", "foo": "odd"}},
19+
{ID: "2", Payload: map[string]interface{}{"id": "2", "foo": "even"}},
20+
{ID: "3", Payload: map[string]interface{}{"id": "3", "foo": "odd"}},
21+
{ID: "4", Payload: map[string]interface{}{"id": "4", "foo": "even"}},
22+
{ID: "5", Payload: map[string]interface{}{"id": "5", "foo": "odd"}},
23+
})
24+
25+
idx := resource.NewIndex()
26+
idx.Bind("foo", schema.Schema{
27+
Fields: schema.Fields{
28+
"id": {Sortable: true, Filterable: true},
29+
"foo": {Filterable: true},
3430
},
35-
},
36-
}
37-
status, headers, body := listDelete(context.TODO(), r, rm)
38-
assert.Equal(t, http.StatusNoContent, status)
39-
assert.Equal(t, http.Header{"X-Total": []string{"5"}}, headers)
40-
assert.Nil(t, body)
31+
}, s, resource.Conf{AllowedModes: resource.ReadWrite, PaginationDefaultLimit: 2})
4132

42-
l, err := s.Find(context.TODO(), &query.Query{})
43-
assert.NoError(t, err)
44-
assert.Len(t, l.Items, 0)
45-
}
33+
return &requestTestVars{
34+
Index: idx,
35+
Storers: map[string]resource.Storer{"foo": s},
36+
}
37+
}
38+
checkFooIDs := func(ids ...interface{}) requestCheckerFunc {
39+
return func(t *testing.T, vars *requestTestVars) {
40+
s := vars.Storers["foo"]
41+
items, err := s.Find(context.Background(), &query.Query{Sort: query.Sort{{Name: "id", Reversed: false}}})
42+
if err != nil {
43+
t.Errorf("s.Find failed: %s", err)
44+
}
45+
if el, al := len(ids), len(items.Items); el != al {
46+
t.Errorf("Expected resource 'foo' to contain %d items, got %d", el, al)
47+
return
48+
}
49+
for i, eid := range ids {
50+
if aid := items.Items[i].ID; eid != aid {
51+
el := len(ids)
52+
t.Errorf("Expected item %d/%d to have ID %q, got ID %q", i+1, el, eid, aid)
53+
}
54+
}
55+
}
56+
}
4657

47-
func TestHandlerDeleteListFilter(t *testing.T) {
48-
s := mem.NewHandler()
49-
s.Insert(context.TODO(), []*resource.Item{
50-
{ID: "1", Payload: map[string]interface{}{"foo": "bar"}},
51-
{ID: "2", Payload: map[string]interface{}{"foo": "bar"}},
52-
{ID: "3", Payload: map[string]interface{}{"foo": "baz"}},
53-
{ID: "4", Payload: map[string]interface{}{"foo": "baz"}},
54-
{ID: "5", Payload: map[string]interface{}{}},
55-
})
56-
index := resource.NewIndex()
57-
test := index.Bind("test", schema.Schema{
58-
Fields: schema.Fields{"foo": {Filterable: true}},
59-
}, s, resource.DefaultConf)
60-
r, _ := http.NewRequest("DELETE", "/test", bytes.NewBufferString("{}"))
61-
rm := &RouteMatch{
62-
ResourcePath: []*ResourcePathComponent{
63-
&ResourcePathComponent{
64-
Name: "test",
65-
Resource: test,
58+
tests := map[string]requestTest{
59+
`clearAll`: {
60+
Init: sharedInit,
61+
NewRequest: func() (*http.Request, error) {
62+
return http.NewRequest("DELETE", "/foo", nil)
6663
},
64+
ResponseCode: http.StatusNoContent,
65+
ResponseBody: ``,
66+
ResponseHeader: http.Header{"X-Total": []string{"5"}},
67+
ExtraTest: checkFooIDs(),
6768
},
68-
Params: url.Values{
69-
"filter": []string{`{"foo": "bar"}`},
69+
`limit=2`: {
70+
Init: sharedInit,
71+
NewRequest: func() (*http.Request, error) {
72+
return http.NewRequest("DELETE", `/foo?limit=2`, nil)
73+
},
74+
ResponseCode: http.StatusNoContent,
75+
ResponseBody: ``,
76+
ResponseHeader: http.Header{"X-Total": []string{"2"}},
77+
ExtraTest: checkFooIDs("3", "4", "5"),
7078
},
71-
}
72-
status, headers, body := listDelete(context.TODO(), r, rm)
73-
assert.Equal(t, http.StatusNoContent, status)
74-
assert.Equal(t, http.Header{"X-Total": []string{"2"}}, headers)
75-
assert.Nil(t, body)
76-
77-
l, err := s.Find(context.TODO(), &query.Query{})
78-
assert.NoError(t, err)
79-
if assert.Len(t, l.Items, 3) {
80-
assert.Equal(t, "3", l.Items[0].ID)
81-
assert.Equal(t, "4", l.Items[1].ID)
82-
assert.Equal(t, "5", l.Items[2].ID)
83-
}
84-
}
85-
86-
func TestHandlerDeleteListInvalidFilter(t *testing.T) {
87-
index := resource.NewIndex()
88-
test := index.Bind("test", schema.Schema{}, nil, resource.DefaultConf)
89-
r, _ := http.NewRequest("DELETE", "/test", bytes.NewBufferString("{}"))
90-
rm := &RouteMatch{
91-
ResourcePath: []*ResourcePathComponent{
92-
&ResourcePathComponent{
93-
Name: "test",
94-
Resource: test,
79+
`limit=2,skip=1`: {
80+
Init: sharedInit,
81+
NewRequest: func() (*http.Request, error) {
82+
return http.NewRequest("DELETE", `/foo?limit=2&skip=1`, nil)
9583
},
84+
ResponseCode: http.StatusNoContent,
85+
ResponseBody: ``,
86+
ResponseHeader: http.Header{"X-Total": []string{"2"}},
87+
ExtraTest: checkFooIDs("1", "4", "5"),
9688
},
97-
Params: url.Values{
98-
"filter": []string{"invalid"},
89+
`filter=invalid`: {
90+
Init: sharedInit,
91+
NewRequest: func() (*http.Request, error) {
92+
return http.NewRequest("DELETE", `/foo?filter=invalid`, nil)
93+
},
94+
ResponseCode: http.StatusUnprocessableEntity,
95+
ResponseBody: `{
96+
"code": 422,
97+
"message": "URL parameters contain error(s)",
98+
"issues": {
99+
"filter": ["char 0: expected '{' got 'i'"]
100+
}}`,
101+
ExtraTest: checkFooIDs("1", "2", "3", "4", "5"),
99102
},
100-
}
101-
status, headers, body := listDelete(context.TODO(), r, rm)
102-
assert.Equal(t, 422, status)
103-
assert.Nil(t, headers)
104-
if assert.IsType(t, body, &Error{}) {
105-
err := body.(*Error)
106-
assert.Equal(t, 422, err.Code)
107-
assert.Equal(t, "Invalid `filter` parameter: char 0: expected '{' got 'i'", err.Message)
108-
}
109-
}
110-
111-
func TestHandlerDeleteListNoStorage(t *testing.T) {
112-
index := resource.NewIndex()
113-
test := index.Bind("test", schema.Schema{}, nil, resource.DefaultConf)
114-
r, _ := http.NewRequest("DELETE", "/test", bytes.NewBufferString("{}"))
115-
rm := &RouteMatch{
116-
ResourcePath: []*ResourcePathComponent{
117-
&ResourcePathComponent{
118-
Name: "test",
119-
Resource: test,
103+
`filter={foo:"even"}`: {
104+
Init: sharedInit,
105+
NewRequest: func() (*http.Request, error) {
106+
return http.NewRequest("DELETE", `/foo?filter={foo:"even"}`, nil)
107+
},
108+
ResponseCode: http.StatusNoContent,
109+
ResponseBody: ``,
110+
ResponseHeader: http.Header{"X-Total": []string{"2"}},
111+
ExtraTest: checkFooIDs("1", "3", "5"),
112+
},
113+
`filter={foo:"odd"}`: {
114+
Init: sharedInit,
115+
NewRequest: func() (*http.Request, error) {
116+
return http.NewRequest("DELETE", `/foo?filter={foo:"odd"}`, nil)
117+
},
118+
ResponseCode: http.StatusNoContent,
119+
ResponseBody: ``,
120+
ResponseHeader: http.Header{"X-Total": []string{"3"}},
121+
ExtraTest: checkFooIDs("2", "4"),
122+
},
123+
`filter={foo:"odd"},limit=2`: {
124+
Init: sharedInit,
125+
NewRequest: func() (*http.Request, error) {
126+
return http.NewRequest("DELETE", `/foo?filter={foo:"odd"}&limit=2`, nil)
120127
},
128+
ResponseCode: http.StatusNoContent,
129+
ResponseBody: ``,
130+
ResponseHeader: http.Header{"X-Total": []string{"2"}},
131+
ExtraTest: checkFooIDs("2", "4", "5"),
132+
},
133+
`filter={foo:"odd"},limit=2,skip=1`: {
134+
Init: sharedInit,
135+
NewRequest: func() (*http.Request, error) {
136+
return http.NewRequest("DELETE", `/foo?filter={foo:"odd"}&limit=2&skip=1`, nil)
137+
},
138+
ResponseCode: http.StatusNoContent,
139+
ResponseBody: ``,
140+
ResponseHeader: http.Header{"X-Total": []string{"2"}},
141+
ExtraTest: checkFooIDs("1", "2", "4"),
142+
},
143+
`NoStorage`: {
144+
// FIXME: For NoStorage, it's probably better to error early (during Bind).
145+
Init: func() *requestTestVars {
146+
index := resource.NewIndex()
147+
index.Bind("foo", schema.Schema{}, nil, resource.DefaultConf)
148+
return &requestTestVars{Index: index}
149+
},
150+
NewRequest: func() (*http.Request, error) {
151+
return http.NewRequest("DELETE", "/foo", nil)
152+
},
153+
ResponseCode: http.StatusNotImplemented,
154+
ResponseBody: `{"code": 501, "message": "No Storage Defined"}`,
121155
},
122156
}
123-
status, headers, body := listDelete(context.TODO(), r, rm)
124-
assert.Equal(t, http.StatusNotImplemented, status)
125-
assert.Nil(t, headers)
126-
if assert.IsType(t, body, &Error{}) {
127-
err := body.(*Error)
128-
assert.Equal(t, http.StatusNotImplemented, err.Code)
129-
assert.Equal(t, "No Storage Defined", err.Message)
157+
158+
for n, tc := range tests {
159+
tc := tc // capture range variable
160+
t.Run(n, tc.Test)
130161
}
131162
}

‎rest/method_get.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ package rest
22

33
import (
44
"context"
5-
"fmt"
5+
"errors"
66
"net/http"
77
"net/url"
88
"strconv"
@@ -56,7 +56,7 @@ func getUintParam(params url.Values, name string) (int, bool, error) {
5656
if v := params.Get(name); v != "" {
5757
i, err := strconv.ParseUint(v, 10, 32)
5858
if err != nil {
59-
return 0, true, &Error{422, fmt.Sprintf("Invalid `%s` parameter", name), nil}
59+
return 0, true, errors.New("must be positive integer")
6060
}
6161
return int(i), true, nil
6262
}

0 commit comments

Comments
 (0)