fcrepo / fcrepo-specification

Fedora API Specification
Apache License 2.0
17 stars 15 forks source link

Behaviour clarification for implementations that choose to accept DELETE requests? #111

Closed dannylamb closed 7 years ago

dannylamb commented 7 years ago

As someone who has been tasked with integrating a Fedora repository in a larger technology stack, the wide range of behaviour that could be exhibited when handling a DELETE request is a concern.

If a DELETE is issued against the root node of a subtree, the DELETE may be performed on the entire subtree. If AuthZ is taken into consideration, a policy could be enacted where the issuer of the DELETE request cannot delete one or more child nodes, and if not specified that could be problematic.

Is it worth stating "Return a 403 if there is an AuthZ policy that blocks deletion on any resource affected by the request."? Otherwise we could be encouraging some strange behaviour, like partially applying a DELETE to some but not all of the subtree.

dannylamb commented 7 years ago

@acoburn I understand where you're coming from as an implementor, and I'm not attempting to enshrine language that will make Trellis impossible. And for the record, I would like for Islandora to work with Trellis.

But the situations that I described are based off of the reference implementation. And yours are clearly driven by Trellis and it's asynchronous nature. It's plain to see how different they are.

I think it would be a benefit to those who are integrating with one or more Fedora implmentations to see at least some non-normative language around things like "don't expect to be working with trees" and "everything may be asynchronous". We don't have to put it in normative language, but I don't want those sorts of things to be in a README or buried in a wiki either.

dannylamb commented 7 years ago

@acoburn That is exactly what I'm trying to alleviate here. There are a lot of assumptions being made based on previous behaviour. I'm trying to make sure folks don't bring those assumptions with them when reading this doucment.

ajs6f commented 7 years ago

This is a great example of why "swappability" is not the right goal for the Fedora spec, and why we didn't start with that goal. As someone working with @dannylamb to build CLAW, I appreciate that writing against unspecified behavior here is not practical. As someone about to start work with @acoburn on Trellis, I appreciate that almost any specification here will block important use cases, which is exactly the opposite of the original purpose of the specification effort, which was to make Fedora useful to a larger audience with more diverse needs.

The right strategy here (as I've argued elsewhere) is to keep the Fedora specification silent and avoid cutting off sections of the community, and also to enable client communities (e.g. CLAW) to specify their own needs (e.g. specific DELETE behavior) as well. "Swappability" is not the same thing as interoperability and this issue shows that very well.

dannylamb commented 7 years ago

Yeah, so I think we've pretty quickly started talking past each other here. I certainly started things off on the wrong foot by dredging up interpretations of the reference implementation, so I'll take the blame for that.

But I sincerely think there needs to be some language clarifying that certain behaviours are not guaranteed if that is the case. Especially if those lack of guarantees are needed for an in progress implementation to thrive. And perhaps most importantly, that type of language could be used to stem the tide of proposed refinements on top of JCR that you both find yourselves constantly having to shut down.

ajs6f commented 7 years ago

Additional non-normative language that clarifies when the spec is intentionally remaining silent is a darn good idea. If that's what we're talking about here, then 👍 . Depending on the specifics, it might even include some of the reasoning, so that future readers can understand how the issues looked at this point.

zimeon commented 7 years ago

I agree with @acoburn's first comment that LDP and the current Fedora spec draft do not require recursive delete (LDP says only "member resources may be removed" and Fedora spec doesn't mention except in reference to versioning).

Given that support for DELETE is optional in LDP (and thus for Fedora at present) I'm happy to stay silent on behavior in the Fedora spec. There clearly needs to be a definition of behavior in some other specification of what the Community Fedora implementation does (currently underspecified; and probably an implementation like Derby that is intended to be swappable should thus also do).

I think @dannylamb's and @ajs6f's last points gets back to a specification style issue of minimal vs pointing out what is imported or implications.

ajs6f commented 7 years ago

@zimeon 👍 to staying silent, of course. I do think that the best course is for interested parties to specify additional needs (e.g. specific DELETE behavior, etc.) with reference to specific clients or middleware, e.g. what does CLAW need or what does Hydra need, as opposed to with respect to specific implementations e.g. what does the "community impl" do. That's both more open-ended and more useful to integrators. But that gets outside the scope of this ticket.

escowles commented 7 years ago

Can a client include a Prefer header in a DELETE request? Or is there some other way for a client to request, or an implementation to advertise, a specific kind of DELETE functionality?

ajs6f commented 7 years ago

I find nothing in RFC7240 that would prevent that. In a situation in which an impl cannot or will not apply a preference, is Preference-Applied-without-the-preference enough to support a reasonable interaction? "Empty" Preference-Applied plus a 4xx of some kind?

escowles commented 7 years ago

@ajs6f I think returning a 400 with an empty Preference-Applied header sounds like a clear indication that the DELETE failed because the preference could not be complied with. A 405 Method Not Allowed would be more appropriate if DELETE wasn't supported at all.

ajs6f commented 7 years ago

@escowles If you want to be strict, 400 sounds about as good as it gets. But 422 (which comes from WebDAV) seems a bit more on point.

escowles commented 7 years ago

@ajs6f Yes, 422 looks like a good choice here, since it focuses on requests that can't be processed for semantic reasons, not syntactic ones.

ajs6f commented 7 years ago

@escowles So is this suggestion something like, "Impls gonna be impls, but if an impl receives a DELETE with a Preference that it cannot or will not honor, it MUST return a 422 with an empty Preference-Applied." and an accompanying small panoply of example preference choices in the ontology?

ajs6f commented 7 years ago

Or just a 422, no empty header?

escowles commented 7 years ago

That sounds good to me — and I think 422 is clear without the empty Preference-Applied header.

ajs6f commented 7 years ago

If we're willing to use 422, then Depth should certainly be acceptable.

ajs6f commented 7 years ago

Is depth (recursion) the only way that DELETE operations could differ? For example, an impl could reasonably choose to remove a related resource (say, a description) when a primary resource is deleted, but another might not.

barmintor commented 7 years ago

Depth not only seems like the right way to do this, I'm tempted to say we should make it a condition of DELETE. It gives the client a way to say what it wants, and the server the opportunity to say no if it can't.

@ajs6f in the particular case you describe, it would depend on how the LDP server modeled the description resource, right? The MODE impl represents it as an internal member, Cavendish wouldn't (but you can follow your nose there and DELETE that resource, too). It seems like a discoverable expectation.

ajs6f commented 7 years ago

@barmintor I don't see how you can discover it without possibly causing damage to the repo in question... it seems to me that Depth is good for what it covers, but that isn't everything.

barmintor commented 7 years ago

in the case that you wanted to delete a resource but only the resource and not its description or anything else and also you didn't want to retrieve any information about the resource before you deleted it, you would: DELETE Depth: 0

and either get a 4xx or not.

If you wanted to discover whether Depth: 1 or Depth: infinity would delete the description or not before issuing the DELETE, you could look at the URI of the describedby resource - internal members seem pretty well defined in that spec.

ajs6f commented 7 years ago

I don't understand Depth in that way at all-- are you suggesting that it can take on the semantic of containment or any other traceable relationship (e.g. "describedBy")?

barmintor commented 7 years ago

I'm suggesting that Depth is well-defined enough that you can tell whether the description indicated in describedby is an internal member or not by inspecting its URI, which would seem to tell you what Depth: 1 would do.

ajs6f commented 7 years ago

It would, but suppose that the impl deletes associated material (which is not kept as contained) as policy, and the client doesn't assume that. So the client uses an appropriately conservative Depth and still deletes more than it bargained for.

ajs6f commented 7 years ago

@acoburn so far we have the idea that on a per-request basis by means of a header (could be Depth, for example) clients can ask for a given policy vs. the policy is repo-wide. It seems at first glance to me that the former includes the latter, and a repo with a global policy just refuses all requests that don't ask for that policy?

barmintor commented 7 years ago

I think we are approaching the limits of implementation description that are useful in the implementation of middleware, here. I'd frankly be more comfortable saying:

  1. we require deletion of the description when a LDP-NR is deleted
  2. Depth is rejected 4xx on LDP-NR DELETE because LDP-NR don't have internal resources
  3. Depth is used for LDP-C DELETE (edit: with infinity the default as per RFC4918)

... than I would be saying that we're agnostic on what DELETE could mean if it's implemented, but the admin will know and your middleware will be coached by aligning some strategies internally with a policy that you keep in synch. If I take "deleting an object may or may not delete child resources" to the Hydra folks, we're going to be back in "let's just write the Hydra spec" territory, and I suspect that Islandora will feel the same way. The above also has the advantage of being an easily testable conformance (edit: for what it's worth, not even as a proposal; more than @dannylamb was raising).

barmintor commented 7 years ago

@dannylamb it seems like introducing the Depth header to clarify recursion combined with your original suggestion re: 403 would be a good proposal to write up?

dannylamb commented 7 years ago

I'll try and get to writing this up in the near term.

ajs6f commented 7 years ago

I have one request: we clarify whether Depth applies to basic containment or all containment.

dannylamb commented 7 years ago

@ajs6f I'm writing up a PR for this now. I'm going with basic containment.

ajs6f commented 7 years ago

@dannylamb Okay by me-- as long as we also have a clear understanding that the behavior of DELETE against non-Basic containers remains undefined.

dannylamb commented 7 years ago

Resolved via https://github.com/fcrepo/fcrepo-specification/pull/126