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
Soft delete feature for comment [NO MERGE] #525
Conversation
@@ -195,6 +199,39 @@ public Response moveObject(@HeaderParam("Destination") final String destinationU | |||
} | |||
} | |||
|
|||
@POST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beats me what the right REST API for deleted resources is. Here's an idea. Or do we need a "deleted resources" endpoint?
@ajs6f ?
* @return | ||
* @throws RepositoryException | ||
*/ | ||
@POST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any opinions about the appropriate REST API for soft-deleted objects? I'm hoping these are the only two operations we need, although I have a nagging feeling we'll need a way to retrieve the content of a soft-deleted object (although I'd like to say: too bad; if you need it, restore the object)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree: delete and restore seem like the only operations we need -- the 410 Gone from a GET (and HEAD?) should be enough to know if it exists. Having /fcr:restore and /fcr:delete seem reasonable enough to me. The other alternative that comes to mind would be to have some kind of delete/restore token with a POST request, but that's probably more complicated/confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I don't like this whole idea. I don't see why this is a repository- and not an application- concern. I can think of fifteen different states that it would be convenient for me if the repo could manage in special ways… are we going to impl all of them, and if not, why is "soft delete" special?
But if we're going to do it, I'm not sure but that I might actually prefer the token-with-a-request. What about token-with-DELETE for soft-delete? DELETE's semantics match pretty well:
…the server SHOULD NOT indicate success unless, at the time the response is given, it intends to delete the resource or move it to an inaccessible location.
Then POST or PUT with token for restore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had several discussions on a "soft delete" approach in the Thursday committers calls and collectively came to the decision of having "resource status" and ontology support.
https://www.pivotaltracker.com/story/show/54611050
This current shift is apparently being driven by LDP requirements. @cbeer, could you please detail what the requirements are exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajs6f I think the resource status flag that @awoods is very much an application concern and I don't understand why fcrepo4 needs special baked-in-support for it.
This patch is essentially about tombstoning for deleted objects, and if we're going to bother with that, we might as well satisfy the soft-delete use case (which I understand to mean, DELETE doesn't remove data). In fact, true tombstones (without soft-delete at all) would be more useful (IMO), and is all that's necessary to satisfy LDP:
5.2.3.11 LDP servers that allow member creation via POST should not re-use URIs.
6.1.2 LDP servers should not re-use URIs, regardless of the mechanism by which members are created (POST, PUT, etc.). Certain specific cases exist where a LDPC server might delete a resource and then later re-use the URI when it identifies the same resource, but only when consistent with Web architecture. While it is difficult to provide absolute implementation guarantees of non-reuse in all failure scenarios, re-using URIs creates ambiguities for clients that are best avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR? DELETE is a soft-delete, and they need some special knowledge to purge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, so post-this-PR, to purge something, you have to soft-delete it first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until someone offers something better than:
https://github.com/fcrepo4/fcrepo4/pull/525/files#diff-b38668d53e04dd723d9b7edc483e552cR204
(which I'd say is a prerequisite for merging; I don't actually want soft deletes, just tombstoning.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I'm really confused. It doesn't seem like any Fedora site wants soft deletes per se, they want workflow state. and LDP doesn't want soft deletes, it wants tombstoning. So I'm still not sure why we can't just do "resource state" and leave it at that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajs6f, agreed. Soft deletes via "resource state" and tombstoning on "curl -X DELETE" seems like a reasonable approach under the circumstances.
property.getValue().getBinary().getStream(), | ||
ImportUUIDBehavior.IMPORT_UUID_COLLISION_THROW); | ||
|
||
// TODO: assert inbound properties, maybe? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if re-asserting the inbound properties is an expected behavior or not. Maybe we should just provide the data somewhere else and make the client decide. @ajs6f ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reasserting the inbound props means editing resources that aren't the one against which the request was actually made, right? I don't like that. I don't like editing them when the original soft delete request was made, either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always remove the inbound properties (soft delete or no) to preserve referential integrity. Some day, we ought to make that behavior opt-in (Prefer: include="preserve-integrity-over-convenience"
).
+1 to not restoring properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, we're talking only about Reference
or WeakReference
properties?
No description provided.