Skip to content

Commit

Permalink
Add FedoraContent#deleteContent to support LDP-NR deletions
Browse files Browse the repository at this point in the history
  • Loading branch information
@cbeer
cbeer committed Sep 3, 2014
1 parent b99b567 commit a2b9358
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 4 deletions.
29 changes: 28 additions & 1 deletion fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraContent.java
View file
Expand Up @@ -28,13 +28,15 @@
import javax.jcr.RepositoryException;
import javax.jcr.Session;
import javax.servlet.http.HttpServletResponse;
import javax.ws.rs.DELETE;
import javax.ws.rs.GET;
import javax.ws.rs.HeaderParam;
import javax.ws.rs.OPTIONS;
import javax.ws.rs.PUT;
import javax.ws.rs.Path;
import javax.ws.rs.PathParam;
import javax.ws.rs.QueryParam;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.core.Context;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.PathSegment;
Expand Down Expand Up @@ -104,7 +106,7 @@ public Response options(@PathParam("path") final List<PathSegment> pathList,


private void addOptionsHttpHeaders(final HttpServletResponse servletResponse) {
servletResponse.addHeader("Allow", "HEAD,GET,PUT,OPTIONS");
servletResponse.addHeader("Allow", "HEAD,GET,PUT,DELETE,OPTIONS");
}

/**
Expand Down Expand Up @@ -206,4 +208,29 @@ public Response getContent(@PathParam("path") final List<PathSegment> pathList,
}
}

/**
* Delete the binary content and the parent object
*/
@DELETE
@Timed
public Response deleteContent(@PathParam("path") final List<PathSegment> pathList,

This comment has been minimized.

Sign in to view

Copy link
@cbeer

cbeer Sep 3, 2014

Author Contributor

This method really bothers me looking at it as a REST API, but makes us conform to LDP. LDP wants us, when we delete a binary resource, to also delete the parent resource.

The only alternative I came up with (and have @azaroth42 puzzling over) is inverting our fcr:content paradigm. As is, if you create a binary resource in fcrepo4, you create two (REST) resources, e.g. /a/b/c and /a/b/c/fcr:content.

Instead, we could invert this and when you create the binary resource you create:

  • /a/b/c, which if you retrieve it you get the bytestream, and
  • /a/b/c/fcr:metadata (or whatever) which describes the bytestream

This would make the REST relationship a little nicer, but would be a ton of work.

@ajs6f @barmintor @escowles ?

This comment has been minimized.

Sign in to view

Copy link
@barmintor

barmintor via email Sep 4, 2014

Contributor

This comment has been minimized.

Sign in to view

Copy link
@barmintor

barmintor via email Sep 4, 2014

Contributor

This comment has been minimized.

Sign in to view

Copy link
@cbeer

cbeer Sep 4, 2014

Author Contributor

@barmintor , you have it exactly right. LDP wants us to respond to DELETE .../fcr:content and clean up the description of the bytestream at the same time. Having a child REST resource delete its parent gives me pause.

This comment has been minimized.

Sign in to view

Copy link
@ajs6f

ajs6f Sep 4, 2014

Contributor

Wow. LDP seems... quite wrong. Quite surprising. We're sure we have the right interpretation of LDP? Where are you seeing this?

This comment has been minimized.

Sign in to view

Copy link
@escowles

escowles Sep 4, 2014

Contributor

@cbeer which makes more sense with the content/metadata reversed: if you delete the resource (content) then also removing the child metadata node makes sense. IMHO, it seems like typically you would delete both the content and the datastream node at the same time. If you just want to replace the content, then it seems like you would just PUT to update it instead of deleting and replacing.

This comment has been minimized.

Sign in to view

Copy link
@cbeer

cbeer Sep 4, 2014

Author Contributor

I think LDP (and our interpretation) is correct (for their use cases). Our API (and JCR) just invert the relationship between the binary and the description. For LDP, the binary is actually primary resource, and it happens to have a description somewhere (hence, my proposal to invert our API).

That said, @azaroth42 made the point yesterday that there may be value in independent binary and description.

This comment has been minimized.

Sign in to view

Copy link
@ajs6f

ajs6f Sep 4, 2014

Contributor

Independent binary and description is certainly the classic Fedora model, and I would not like to lose the ability to distinguish and choose datastreams by properties other than name. Here's another question: can we have a datastream with no binary content? A "null-value"? Is that too crazy?

This comment has been minimized.

Sign in to view

Copy link
@barmintor

barmintor via email Sep 4, 2014

Contributor

This comment has been minimized.

Sign in to view

Copy link
@cbeer

cbeer Sep 4, 2014

Author Contributor

@ajs6f In fcrepo4, we can't. In LDP, we actually should be distinguishing between containers, RDF resources, and non-RDF resources. Right now, in fcrepo4, every RDF resource is also a container.

This comment has been minimized.

Sign in to view

Copy link
@barmintor

barmintor via email Sep 4, 2014

Contributor

This comment has been minimized.

Sign in to view

Copy link
@azaroth42

azaroth42 via email Sep 4, 2014

This comment has been minimized.

Sign in to view

Copy link
@azaroth42

azaroth42 via email Sep 4, 2014

This comment has been minimized.

Sign in to view

Copy link
@ajs6f

ajs6f Sep 4, 2014

Contributor

@barmintor : I think they do, but the semantics change-- e.g. it's no longer the fixity of data in management, it's the fixity of the data that was in management. That's @azaroth42 's "Preserving other metadata about the asset that was attached to the node." Whether that is more confusing than useful is another question.

This comment has been minimized.

Sign in to view

Copy link
@ajs6f

ajs6f Sep 4, 2014

Contributor

Well, making the change for compliance... just how big a deal would it be? It seems like mechanical work, but there'd be a lot of it.

This comment has been minimized.

Sign in to view

Copy link
@barmintor

barmintor via email Sep 4, 2014

Contributor

This comment has been minimized.

Sign in to view

Copy link
@ajs6f

ajs6f Sep 4, 2014

Contributor

@cbeer : Those classes are disjoint in LDP? I didn't see that in the spec, but maybe I missed it?

Right now we have:

Object a Container
Object a RDF Resource
Datastream a RDF Resource
Content a non-RDF Resource

So the question is about Object, and whether it can legitimately be both a Container and a RDF Resource? Or are you saying that we are also doing Datastream a Container and maybe we shouldn't be?

This comment has been minimized.

Sign in to view

Copy link
@cbeer

cbeer Sep 4, 2014

Author Contributor

LDP wants us to have:

RDF resource
RDF resource as a container
Non-RDF resource


(not sure the diagram actually helps, but..)

We've mapped those three things onto two concepts:

Fedora Object (RDF resource as a container)
Fedora Datastream (Non-RDF resource, with an RDF resource description to describe it)

And lost the "plain old RDF resource; no membership/containment semantics at all" case.

In LDP land, we should have a type of Fedora Object that doesn't have children (e.g. you can't POST to the object, or use PUT to create sub-members of it (possibly.. LDP doesn't really have anything to say about PUT)).

Here's the section of the spec that w'ere currently failing by not distinguishing those three cases:

5.2.3.4 LDP servers that successfully create a resource from a RDF representation in the request entity body must honor the client's requested interaction model(s). If any requested interaction model cannot be honored, the server must fail the request.

If the request header specifies a LDPR interaction model, then the server must handle subsequent requests to the newly created resource's URI as if it is a LDPR (even if the content contains an rdf:type triple indicating a type of LDPC).
If the request header specifies a LDPC interaction model, then the server must handle subsequent requests to the newly created resource's URI as if it is a LDPC.
This specification does not constrain the server's behavior in other cases.
Clients use the same syntax, that is HTTP Link headers, to specify the desired interaction model when creating a resource as servers use to advertise it on responses.

Note: A consequence of this is that LDPCs can be used to create LDPCs, if the server supports doing so.

This comment has been minimized.

Sign in to view

Copy link
@ajs6f

ajs6f Sep 4, 2014

Contributor

I don't quite follow this. Containers are RDF resources, right? So why do we need a "plain old RDF resource"? In what way are we not implementing LDP by not implementing this type? Just because it exists, it doesn't seem to me that it follows that we must provide an example of it...

This comment has been minimized.

Sign in to view

Copy link
@ajs6f

ajs6f Sep 4, 2014

Contributor

@cbeer: I still don't get it. I understand that we must distinguish these things, at least in our minds. I don't understand why it follows that we must provide examples of each. In LDP, Containers and RDF Sources are clearly not disjoint. And there is nothing that I found in LDP like an operation that must create an RDF Resource that is not a Container...

@ajs6f In fcrepo4, we can't. In LDP, we actually should be distinguishing between containers, RDF resources, and
non-RDF resources. Right now, in fcrepo4, every RDF resource is also a container.

This comment has been minimized.

Sign in to view

Copy link
@cbeer

cbeer Sep 4, 2014

Author Contributor

@ajs6f All containers are RDF sources. Some RDF sources are not containers. The blurb from the spec about interaction models suggests it's required that the server support (or outright reject) requests from the client to create non-container resources.

I think there may be benefits to supporting that distinction, especially with Fedora objects for which the container semantics are not immediately clear (what does it mean for an object that isn't an aggregation to contain other resources, anyway?)

This comment has been minimized.

Sign in to view

Copy link
@ajs6f

ajs6f Sep 4, 2014

Contributor

@cbeer : Okay, now I'm starting to get where you're coming from. Can you give me a pointer into the spec? I need to understand what it means to support the creation of non-container resources (for us, datastreams). Does it mean that you have to support them any- and everywhere?

As far as the semantics of container-objects, I understood us to be leaving them intentionally undefined to allow repo instances to use them as locally interesting...
@Context final Request request,
@Context final HttpServletResponse servletResponse) throws RepositoryException {

try {
final String path = toPath(pathList);

final Datastream ds = datastreamService.getDatastream(session, path);
evaluateRequestPreconditions(request, servletResponse, ds, session);

nodeService.deleteObject(session, path);
session.save();
return noContent().build();
} catch (final WebApplicationException ex) {
return (Response)ex.getResponse();
} finally {
session.logout();
}
}

}
25 changes: 22 additions & 3 deletions fcrepo-http-api/src/test/java/org/fcrepo/http/api/FedoraContentTest.java
View file
Expand Up @@ -22,12 +22,12 @@
import static org.fcrepo.http.commons.test.util.TestHelpers.mockDatastream;
import static org.fcrepo.http.commons.test.util.TestHelpers.mockSession;
import static org.fcrepo.http.commons.test.util.TestHelpers.setField;
import static org.fcrepo.kernel.RdfLexicon.NON_RDF_SOURCE;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertNotNull;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyString;
import static org.mockito.Matchers.eq;
import static org.mockito.Matchers.isA;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
Expand All @@ -40,7 +40,6 @@
import java.net.URISyntaxException;
import java.text.ParseException;
import java.util.Date;
import java.util.List;

import javax.jcr.Node;
import javax.jcr.RepositoryException;
Expand Down Expand Up @@ -215,4 +214,24 @@ public void testGetContent() throws RepositoryException, IOException {
assertEquals("asdf", actualContent);
}

@Test
public void testDeleteContent() throws RepositoryException {
final String pid = "FedoraDatastreamsTest1";
final String dsId = "testDS";
final String path = "/" + pid + "/" + dsId;
final String dsContent = "asdf";

final Datastream mockDs = mockDatastream(pid, dsId, dsContent);
when(mockDatastreams.getDatastream(isA(Session.class), isA(String.class))).thenReturn(mockDs);
when(mockDs.getEtagValue()).thenReturn("");
final Request mockRequest = mock(Request.class);

final Response actual = testObj.deleteContent(createPathList(pid + "/" + dsId), mockRequest, mockResponse);

assertNotNull(actual);
assertEquals(NO_CONTENT.getStatusCode(), actual.getStatus());
verify(mockNodeService).deleteObject(mockSession, path);
verify(mockSession).save();
}

}
14 changes: 14 additions & 0 deletions fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/FedoraContentIT.java
View file
Expand Up @@ -37,6 +37,7 @@
import org.apache.commons.io.IOUtils;
import org.apache.http.Header;
import org.apache.http.HttpResponse;
import org.apache.http.client.methods.HttpDelete;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.methods.HttpHead;
import org.apache.http.client.methods.HttpOptions;
Expand Down Expand Up @@ -429,9 +430,22 @@ public void testOptions() throws Exception {
assertTrue("Should allow GET", methods.contains(HttpGet.METHOD_NAME));
assertTrue("Should allow PUT", methods.contains(HttpPut.METHOD_NAME));
assertTrue("Should allow OPTIONS", methods.contains(HttpOptions.METHOD_NAME));
assertTrue("Should allow DELETE", methods.contains(HttpDelete.METHOD_NAME));

}

@Test
public void testDeleteDatastream() throws Exception {
final String pid = getRandomUniquePid();
createObject(pid);

createDatastream(pid, "ds1", "marbles for everyone");
final String location = serverAddress + pid + "/ds1/fcr:content";
assertEquals(204, getStatus(new HttpDelete(location)));
assertEquals("Object wasn't really deleted!", 404, getStatus(new HttpGet(location)));
}


private static List<String> headerValues( final HttpResponse response, final String headerName ) {
final List<String> values = new ArrayList<String>();
for ( final Header header : response.getHeaders(headerName) ) {
Expand Down

0 comments on commit a2b9358

Please sign in to comment.