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

DELETE support and semantics #187

Closed
wants to merge 14 commits into from
Closed

DELETE support and semantics #187

wants to merge 14 commits into from

Conversation

[[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

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

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

@acoburn
Copy link
Member

acoburn commented Jun 29, 2020

I have considered the Expect header but decided against it.

The Expect header is used for 100-continue requests only. Its original use as a "must-understand" extension mechanism has been removed from RFC 7231

      Note: The Expect header field was added after the original
      publication of HTTP/1.1 [RFC2068] as both the means to request an
      interim 100 (Continue) response and the general mechanism for
      indicating must-understand extensions.  However, the extension
      mechanism has not been used by clients and the must-understand
      requirements have not been implemented by many servers, rendering
      the extension mechanism useless.  This specification has removed
      the extension mechanism in order to simplify the definition and
      processing of 100-continue. 

@csarven csarven requested a review from acoburn July 8, 2020 20:12
@csarven
Copy link
Member Author

csarven commented Jul 9, 2020

Closing this PR in favour of #188 (including the commits from here; feature/delete). Please continue with the reviews there.

@csarven csarven closed this Jul 9, 2020
@csarven csarven deleted the feature/delete branch December 4, 2020 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants