Clear does not support filters or limit

[smyrman]

According to the README, a DELETE request on the resource list view has this effect:

Delete all items from the collection matching the context and/or filters.

However, when I tried locally, it deletes everything and ignores the filter and/or limit.

[smyrman]

I tried with the rest-layer-mongo storage backend btw.

[smyrman]

It should also be noted that sub-resources that link to a parent object, are not deleted when deleting the parent object. E.g. in case of the #129 schema,

$ bat DELETE :2015/api/as

will not delete any of the "bs".

[rs]

There is no support for cascading deletion if this is what you are referring to.

The delete with filter bug definitely need to be fixed.

[Dragomir-Ivanov]

@smyrman For the cascading deletion case, this is stated on the Rest-Layer documentation site as:
□ "Cascading deletes on sub resources". However I hope this is still on the radar.

[rs]

Yes, it is not yet implemented.

[smyrman]

It looks like at least part of the problem is here (maybe all of it):

rest-layer/rest/routing.go

Line 201 in 0df3129

case "GET":

As you can see, there is a case for GET and HEAD, but no case for DELETE. The DELETE case should handle limit, page, offset and filter, but ignore the projection (fields). In order to reuse as much code as possible, one solution could be to handle the field selectioon bit first, and then use a fall-through:

	case "GET":
		if fields := r.Params.Get("fields"); fields != "" {
		p, err := query.ParseProjection(fields)
		if err == nil {
			err = p.Validate(rsc.Validator())
		}
		if err != nil {
			return nil, &Error{422, fmt.Sprintf("Invalid `fields` parameter: %s", err), nil}
		}
		q.Projection = p
		fallthrough
	case "DELETE":
		limit := -1
		if l, found, err := getUintParam(r.Params, "limit"); found {
			if err != nil {
				return nil, &Error{422, err.Error(), nil}
			}
			limit = l
		} else if l := rsc.Conf().PaginationDefaultLimit; l > 0 {
			limit = l
		}
		// ...

Of-course, splitting out in some re-usable functions (or methods) with some built-in error handling could work equally well.

[smyrman]

And because I never remember how a fallthrough works, here is a reminder:

• https://play.golang.org/p/h0V-ApTbZa

[Dragomir-Ivanov]

Of-course, splitting out in some re-usable functions (or methods) with some built-in error handling could work equally well.

I think reusable functions will be better, given that we will need to put HEAD, in the switch above.

[smyrman]

right, probabably a more readable option anyway 👍

[smyrman]

I have started on this a while back. The fix itself is really fast, but there are a few breaking tests that have taken some time to update. Mostly just updating them to test the external (HTTP) interface instead of the internal route.

[smyrman]

@rs, on the linked issues in the -mem/-mongo repos, I think we should let Clear handle Window (limit/offset) equally to how it's handled in Find.

If there is a use-case for allowing Clear to delete all resources (matching a query) we could let the parsing in rest-layer ignore the resource default limit during parsing. Alternatively, we could allow limit=-1 to be set by the user DELETE foo/1?filter={foo:"baz"}&limit=-1.

Any preferences?

As a FYI, this is my current test output for DELETE:

% go test -run TestDeleteList ./rest                                        :(
--- FAIL: TestDeleteList (0.00s)
    --- FAIL: TestDeleteList/limit=2 (0.00s)
    	method_test.go:61: Expected HTTP header["X-Total"][0] to equal "2", got "5"
    	method_delete_test.go:46: Expected resource 'foo' to contain 3 items, got 0
    --- FAIL: TestDeleteList/filter={foo:"odd"},limit=2,skip=1 (0.00s)
    	method_test.go:61: Expected HTTP header["X-Total"][0] to equal "2", got "3"
    	method_delete_test.go:46: Expected resource 'foo' to contain 3 items, got 2
    --- FAIL: TestDeleteList/limit=2,skip=1 (0.00s)
    	method_test.go:61: Expected HTTP header["X-Total"][0] to equal "2", got "5"
    	method_delete_test.go:46: Expected resource 'foo' to contain 3 items, got 0
    --- FAIL: TestDeleteList/filter={foo:"odd"},limit=2 (0.00s)
    	method_test.go:61: Expected HTTP header["X-Total"][0] to equal "2", got "3"
    	method_delete_test.go:46: Expected resource 'foo' to contain 3 items, got 2
FAIL
FAIL	github.com/rs/rest-layer/rest	0.020s

I will post an early PR for my work so far for the test code.

[rs]

To make sure I understand what you are proposing, a DELETE on a resource list would only delete up to the default limit number of items? Do you feel like it's what most would expect?

[smyrman]

To start with the obvious, I think users should be able to expect the following:

1. DELETE /foo?sort=-created&limit=1 to delete the last inserted (newest) item.
2. DELETE /foo?sort=created&limit=2 to delete the first two inserted (oldest) items.
3. A programatic call rsc.Delete(ctx, query.Query{Sort: query.Sort{{Name:"created", Reversed: false}}, Window:query.Window{Offset:1,Limit:1}} to delete the second oldest item.

To make sure I understand what you are proposing, a DELETE on a resource list would only delete up to the default limit number of items? Do you feel like it's what most would expect?

I am not sure what most users would expect in the case of a default limit for list DELETE (clear), which is also why I want your input on this. There are also some other concerns besides user expectations, such as which is the safest (least destructive) assumption.

I actually see three options now, where I now like B the best:
A: Ignore the resource default limit for list DELETE queries; delete all matching items. This is perhaps one of the more intuitive options, but also the one that would lead to the largest accidental data loss if the user expected the default limit to apply, misspelled limit or filter or otherwise did a mistake.
B: require limit to always be set for list DELETE queries, allow limit=-1 to be set to indicate no limit. This is the most explicit option, and probably also the safest option. Requires some additional implementation changes, as the current route resolver does not differ between list and item access when parsing the URL Query.
C: respect the default resource limit for list DELETE queries, allow limit=-1 to be set to indicate no limit. Also among the safer assumptions, as less data would be lost (by default) on request typos/errors.

To show the consequence of each variant in an example, this is what option A, B and C would mean for requests against a resource foo with a default limit of 25:

1. DELETE /foo or DELETE /foo?filer={foo:"baz"} (spelling error for "filter")
   A: deletes all items in foo
   B: returns a 422 error message due to no limit parameter
   C: deletes 25 fist listed items by default ordering
2. DELETE /foo?sort=-created&skip=10
   A: deletes all except the latest 10 items in foo
   B: returns a 422 error message due to no limit parameter
   C: deletes item 11-35
3. DELETE /foo?sort=-created&page=2
   A: returns a 422 error message due to no limit when page is specified
   B: returns a 422 error message due to no limit parameter
   C: deletes item 26-50
4. DELETE /foo?filter={foo:"baz"}
   A: deleted all items where field foo equals "baz" in foo
   B: returns a 422 error message due to no limit parameter
   C: deleted the first 25 items by default ordering where field foo equals "baz" in foo
5. DELETE /foo?filter={foo:"baz"}&limit=-1
   A: returns a 422 error message due to limit < 0
   B: deleted all items where field foo equals "baz" in foo
   C: deleted all items where field foo equals "baz" in foo

[rs]

If not A, B would be my prefered as it is explicit. With B, in dev you could assume A is in place as your dev env might not have enough element to show actual B’s behavior.

But I would assume A is the right behavior in the first place.

[Dragomir-Ivanov]

I also prefer A, as it is most expected behavior, inline with how actual DB cli tools work.

[smyrman]

also prefer A, as it is most expected behavior, inline with how actual DB cli tools work.

Well the DB CLI also do not appply the default limit on a Find reqiest.

Anyway I can start with A for this issue, and maybe we can make it more sequre later, e.g. through resource config, if necesarry.

[Dragomir-Ivanov]

@smyrman I think that we don't need to care about security/correctness at this level. After all developer of the end product will need to make tests of his rest-layer API usage.

[smyrman]

@smyrman I think that we don't need to care about security/correctness at this level.

@Dragomir-Ivanov, security from shooting yourself in the foot, is never a bad thing to have, but we can discuss that in a separate issue.

It's probably more along the lines of adding a soft-delete or maybe actions with undo abilities, rather than anyhing related to parsing of filters & limit (I.e. nothing directly related to this issue).

I may link the issue if I create one, but I don't have any concrete suggestions atm.