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

Soft delete feature for comment [NO MERGE] #525

Closed
wants to merge 3 commits into from
Closed

Conversation

cbeer
Copy link
Contributor

@cbeer cbeer commented Oct 13, 2014

No description provided.

@@ -195,6 +199,39 @@ public Response moveObject(@HeaderParam("Destination") final String destinationU
}
}

@POST
Copy link
Contributor Author

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
Copy link
Contributor Author

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)

@ajs6f @escowles ?

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.)

Copy link
Contributor

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?

Copy link

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?
Copy link
Contributor Author

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 ?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

@cbeer cbeer closed this Oct 16, 2014
@cbeer cbeer deleted the soft-delete branch October 17, 2014 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants