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.

Sign up for GitHub

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

Jump to bottom

DELETE support and semantics #187

Closed
wants to merge 14 commits into from
Closed

DELETE support and semantics #187

Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a30bb1e
Init delete operation in context of WAC
csarven Jun 29, 2020
96264b8
Reject delete root container or its ACL
csarven Jun 29, 2020
a5cc118
Add acl:Write requirement to delete a resource
csarven Jun 29, 2020
d28ec63
Add removal of containment triple on container
csarven Jun 29, 2020
bd435d3
Add acl:Control requirement to delete an ACL
csarven Jun 29, 2020
ad120fd
Add rules to delete a container
csarven Jun 29, 2020
342febe
Add option to omit DELETE in Allow
csarven Jun 29, 2020
36c0d8a
Init non-normative for delete's clean-up
csarven Jun 29, 2020
b46b54a
Add common 404/410 response to GET after DELETE
csarven Jun 29, 2020
918484a
Note reinstating resources and ACL change
csarven Jun 29, 2020
b7426e9
Add todo related to delete
csarven Jun 29, 2020
c11eab4
Specify delete container without recursion
csarven Jun 29, 2020
682d354
Minor: spacing
csarven Jun 30, 2020
8d41201
Move Deleting Resources under Reading and Writing Resources
csarven Jul 5, 2020
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
45 changes: 45 additions & 0 deletions main/resource-access.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,51 @@ A Solid server MUST reject `PUT`, `POST` and `PATCH` requests without the
`Content-Type` header with a status code of `400`.
[[Source](https://github.com/solid/specification/issues/70#issuecomment-535499628)]

#### Deleting resources #### {#delete}

Servers MUST support the HTTP `DELETE` method [[!RFC7231]].
[[Source](https://github.com/solid/specification/issues/39#issuecomment-538017667)]

When using Web Access Control ([[#wac]]):

Servers MUST NOT allow delete requests to a storage's root container and associated ACL resource and exclude the `DELETE` method in the HTTP response header `Allow` in response to safe method requests [[!RFC7231]].
[[Source](https://github.com/solid/specification/issues/37#issuecomment-627281466)]

To delete a resource, an `acl:agent` MUST have `acl:Write` privilege per the ACL inheritance algorithm on the resource and the containing container.
[[Source](https://github.com/solid/solid-spec/issues/195#issuecomment-559799154)]

When a contained resource is deleted, the server MUST also remove the corresponding containment triple, which has the effect of removing the deleted resource from the containing container.
[[Source](https://www.w3.org/TR/ldp#ldpc-del-contremovesconttriple)]

When a contained resource is deleted, the server MUST also delete the associated resources (see the [[#rm]] section).

To delete an ACL resource (see [[#ar-wac]]), an `acl:agent` MUST have `acl:Control` privileges per the ACL inheritance algorithm on the resource directly associated with it.
[[Source](https://github.com/solid/specification/issues/145)]
[[Source](https://github.com/solid/specification/issues/41)]

A container can only be deleted if it contains no resources ([[#resource-containment]]). When a `DELETE` method request is made to a container, the server MUST delete the contained resources except containers. To recursively delete a container, server MUST accept client's `DELETE` request including the HTTP `Prefer` header with `return="representation"; include="http://www.w3.org/ns/ldp#PreferContainment"` (see [[!LDP]]'s Preferences on the Prefer Request Header). Server MUST respond with the `409` status code and response body including containment triples about unaffected resources of the delete request for agents with `acl:Read` privilege per the ACL inheritance algorithm.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is confusing. First, this sentence states that a container can only be deleted if it contains no resources. Then it describes recursive delete, which contradicts the first statement.

As a general comment, recursive delete becomes very complicated in the case of complex ACLs, atomicity guarantees and the prospect of partial failure. If you plan to support recursive delete in the specification, I would suggest that there be a mechanism for reporting partial failures to clients (or, as would be my preference, don't support recursive delete at all)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the Prefer: return=representation part is strange. That hint (return=representation) is typically used to control the response of the HTTP operation, not the internal server behavior.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I assume the first sentence could still hold if you implement recursive delete via a depth-first search - i.e. the server starts deleting the leaf-nodes, and then only deletes child containers once they are empty, and keep popping back up the stack.
Partial failures do still make be nervous though - i.e. having some mechanism to report them back to the user, and making it very clear to client-app developers to not assume a recursive delete will just happen on the server (i.e. where they update their user interface to remove an entire folder structure). Instead they need to be aware that the delete operation might partially fail (and if it does fail for one resource, does it stop the recursive delete operation at that point, or does it continue deleting remaining leaf-nodes and remaining empty containers?). And so those developers need to know that they need to wait for a server response (or an async notification) telling them what actually happened on the server before updating their UI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean with the confusion. It was intended to cover the DELETE request to a container. Perhaps it is redundant in that the third sentence mentions recursion - with the assumption that includes the effective request URI (ie. the container itself), in addition to the contained resources. If we omit the first sentence, it is clear?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True re: Prefer: return=representation and typical use so far. I figured that Prefer header in context of DELETE would be sufficient. It still has the client preferring to see the resulting representation of the container. This is where the 409 also comes into play ie. if there is a partial fail, client can know (provided that they have Read).

There wasn't a particular way where a client can request its preference to delete a container recursively - at least in the issues that I've looked into. Happy to look at alternative approaches.

Copy link
Member

@acoburn acoburn Jun 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DELETE /foo/ would remove /foo/ if it has no contained resources. If /foo/ contains resources, DELETE /foo/ would remove them instead.

FWIW, I would be strongly 👎 on this.

Imagine the case where you have two clients writing to /foo/. The first checks /foo/ and finds that it is empty, it then issues DELETE /foo/ expecting that it has deleted /foo/, but in fact it has deleted /foo/bar and /foo/baz (which were created by the second client in the intervening time).

That scenario would become extremely difficult for clients to reason about.

Copy link
Member

@acoburn acoburn Jun 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would reusing LDP's Prefer pattern be completely wrong in context of DELETE?

The use of return=representation is incorrect. The use of Prefer is not incorrect. But it is also important to note that Prefer headers are "hints" to servers, not hard requirements. In fact, if a server cannot handle the Prefer request, it is supposed to just ignore the header:

   A server that does not recognize or is unable to comply with
   particular preference tokens in the Prefer header field of a request
   MUST ignore those tokens and continue processing instead of signaling
   an error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nods..

Proposal: postpone if and how recursive deletion can work to another PR. For now, the base requirement needs to be along these lines:

When a DELETE method request is made to a container, the server MUST delete the container if it contains no resources. If the container contains resources, the server MUST respond with the 409 status code and response body describing the error.

When we need to, we should add recursive delete in a way without interfering with those requirements.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to see a version with no MAY and no "prefer". This spec has to be tight and unambiguous. Any time the word MAY appeared is a red flag! And using anything like "prefer" to actually have hard must semantics sounded a bad idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in c11eab4

[[Source](https://github.com/solid/specification/issues/132#issuecomment-562314633)]
[[Source](https://github.com/solid/specification/issues/41#issuecomment-580212776)]
[[Source](https://github.com/solid/specification/issues/41#issuecomment-582154098)]

Server MAY omit the `DELETE` method in the HTTP response header `Allow` in response to safe method requests to a container, if `acl:agent` does not have the `acl:Write` privilege to a resource under its URI path.

[[Source](https://github.com/solid/specification/issues/41)]

<em>This section is non-normative.</em>

The server might perform additional actions, as described in the normative references like [[!RFC7231]]. For example, the server could remove membership triples referring to the deleted resource, perform additional cleanup tasks for resources it knows are no longer referenced or have not been accessed for some period of time, and so on.

Subsequent `GET` requests to the deleted resource usually results in a `404` or `410` status code, although HTTP allows others.
[[Source](https://github.com/solid/specification/issues/72)]
[[Source](https://github.com/solid/specification/issues/46)]

As deleted resources can be reinstated with the same URI, access controls on the reinstated resource can change per the ACL inheritance algorithm.
[[Source](https://github.com/solid/specification/issues/145#issuecomment-618918284)]

Issue:
Pertaining to events and loss of control mitigation: https://github.com/solid/specification/issues/41#issuecomment-534679278

### Required client-side implementation ### {#http-client}

A Solid client MUST be an HTTP/1.1 client [[!RFC7230]][[!RFC7231]].
Expand Down