Skip to content

Commit

Permalink
Return 501 upon POST/PUT for LDPR
Browse files Browse the repository at this point in the history 
Resolves: https://jira.duraspace.org/browse/FCREPO-1396
  • Loading branch information
@osmandin
osmandin authored and Andrew Woods committed Apr 17, 2015
1 parent 2412c1c commit c2bd525
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 13 deletions.
32 changes: 30 additions & 2 deletions fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraLdp.java
View file
Expand Up @@ -21,6 +21,8 @@
import static javax.ws.rs.core.MediaType.APPLICATION_XML;
import static javax.ws.rs.core.MediaType.TEXT_HTML;
import static javax.ws.rs.core.MediaType.TEXT_PLAIN;
import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
import static javax.ws.rs.core.Response.Status.NOT_IMPLEMENTED;
import static javax.ws.rs.core.Response.created;
import static javax.ws.rs.core.Response.noContent;
import static javax.ws.rs.core.Response.ok;
Expand All @@ -37,6 +39,7 @@
import static org.fcrepo.http.commons.domain.RDFMediaType.TURTLE_X;
import static org.fcrepo.kernel.FedoraJcrTypes.FEDORA_BINARY;
import static org.fcrepo.kernel.FedoraJcrTypes.FEDORA_CONTAINER;
import static org.fcrepo.kernel.RdfLexicon.LDP_NAMESPACE;
import static org.fcrepo.kernel.impl.services.TransactionServiceImpl.getCurrentTransactionId;
import static org.slf4j.LoggerFactory.getLogger;

Expand Down Expand Up @@ -66,9 +69,11 @@
import javax.ws.rs.PathParam;
import javax.ws.rs.Produces;
import javax.ws.rs.QueryParam;
import javax.ws.rs.ServerErrorException;
import javax.ws.rs.core.Link;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
import javax.ws.rs.core.UriBuilderException;

import org.fcrepo.http.commons.domain.ContentLocation;
import org.fcrepo.http.commons.domain.PATCH;
Expand Down Expand Up @@ -236,9 +241,12 @@ public Response createOrReplaceObjectRdf(
@ContentLocation final InputStream requestBodyStream,
@QueryParam("checksum") final String checksum,
@HeaderParam("Content-Disposition") final ContentDisposition contentDisposition,
@HeaderParam("If-Match") final String ifMatch)
@HeaderParam("If-Match") final String ifMatch,
@HeaderParam("Link") final String link)
throws InvalidChecksumException, MalformedRdfException {

checkLinkForLdpResourceCreation(link);

final FedoraResource resource;
final Response.ResponseBuilder response;

Expand Down Expand Up @@ -398,9 +406,12 @@ public Response createObject(@QueryParam("checksum") final String checksum,
@HeaderParam("Content-Disposition") final ContentDisposition contentDisposition,
@HeaderParam("Content-Type") final MediaType requestContentType,
@HeaderParam("Slug") final String slug,
@ContentLocation final InputStream requestBodyStream)
@ContentLocation final InputStream requestBodyStream,
@HeaderParam("Link") final String link)
throws InvalidChecksumException, IOException, MalformedRdfException, AccessDeniedException {

checkLinkForLdpResourceCreation(link);

if (!(resource() instanceof Container)) {
throw new ClientErrorException("Object cannot have child nodes", CONFLICT);
}
Expand Down Expand Up @@ -608,4 +619,21 @@ private String mintNewPid(final String slug) {
return pid;
}

private void checkLinkForLdpResourceCreation(final String link) {
if (link != null) {
try {
final Link linq = Link.valueOf(link);
if ("type".equals(linq.getRel()) && (LDP_NAMESPACE + "Resource").equals(linq.getUri().toString())) {
LOGGER.info("Unimplemented LDPR creation requested with header link: {}", link);
throw new ServerErrorException("LDPR creation not implemented", NOT_IMPLEMENTED);
}
} catch (RuntimeException e) {
if (e instanceof IllegalArgumentException | e instanceof UriBuilderException) {
throw new ClientErrorException("Invalid link specified: " + link, BAD_REQUEST);
}
throw e;
}
}
}

}
39 changes: 28 additions & 11 deletions fcrepo-http-api/src/test/java/org/fcrepo/http/api/FedoraLdpTest.java
View file
Expand Up @@ -71,6 +71,7 @@
import javax.ws.rs.BadRequestException;
import javax.ws.rs.ClientErrorException;
import javax.ws.rs.core.HttpHeaders;
import javax.ws.rs.ServerErrorException;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Request;
import javax.ws.rs.core.Response;
Expand Down Expand Up @@ -600,11 +601,17 @@ public void testPutNewObject() throws Exception {
when(mockNodeService.exists(mockSession, "/some/path")).thenReturn(false);
when(mockContainerService.findOrCreate(mockSession, "/some/path")).thenReturn(mockContainer);

final Response actual = testObj.createOrReplaceObjectRdf(null, null, null, null, null);
final Response actual = testObj.createOrReplaceObjectRdf(null, null, null, null, null, null);

assertEquals(CREATED.getStatusCode(), actual.getStatus());
}

@Test(expected = ServerErrorException.class)
public void testPutNewObjectLdpr() throws Exception {
testObj.createOrReplaceObjectRdf(null, null, null, null, null,
"<http://www.w3.org/ns/ldp#Resource>; rel=\"type\"");
}

@Test
public void testPutNewObjectWithRdf() throws Exception {

Expand All @@ -615,7 +622,7 @@ public void testPutNewObjectWithRdf() throws Exception {
when(mockContainerService.findOrCreate(mockSession, "/some/path")).thenReturn(mockContainer);

final Response actual = testObj.createOrReplaceObjectRdf(NTRIPLES_TYPE,
toInputStream("_:a <info:x> _:c ."), null, null, null);
toInputStream("_:a <info:x> _:c ."), null, null, null, null);

assertEquals(CREATED.getStatusCode(), actual.getStatus());
verify(mockContainer).replaceProperties(eq(idTranslator), any(Model.class), any(RdfStream.class),
Expand All @@ -631,7 +638,7 @@ public void testPutNewBinary() throws Exception {
when(mockBinaryService.findOrCreate(mockSession, "/some/path")).thenReturn(mockBinary);

final Response actual = testObj.createOrReplaceObjectRdf(TEXT_PLAIN_TYPE,
toInputStream("xyz"), null, null, null);
toInputStream("xyz"), null, null, null, null);

assertEquals(CREATED.getStatusCode(), actual.getStatus());
}
Expand All @@ -648,7 +655,7 @@ public void testPutReplaceRdfObject() throws Exception {
when(mockContainerService.findOrCreate(mockSession, "/some/path")).thenReturn(mockObject);

final Response actual = testObj.createOrReplaceObjectRdf(NTRIPLES_TYPE,
toInputStream("_:a <info:x> _:c ."), null, null, null);
toInputStream("_:a <info:x> _:c ."), null, null, null, null);

assertEquals(NO_CONTENT.getStatusCode(), actual.getStatus());
verify(mockObject).replaceProperties(eq(idTranslator), any(Model.class), any(RdfStream.class),
Expand All @@ -667,7 +674,7 @@ public void testPutWithStrictIfMatchHandling() throws Exception {
when(mockContainerService.findOrCreate(mockSession, "/some/path")).thenReturn(mockObject);

testObj.createOrReplaceObjectRdf(NTRIPLES_TYPE,
toInputStream("_:a <info:x> _:c ."), null, null, null);
toInputStream("_:a <info:x> _:c ."), null, null, null, null);

}

Expand Down Expand Up @@ -719,7 +726,7 @@ public void testCreateNewObject() throws Exception {

when(mockContainerService.findOrCreate(mockSession, "/b")).thenReturn(mockContainer);

final Response actual = testObj.createObject(null, null, null, "b", null);
final Response actual = testObj.createObject(null, null, null, "b", null, null);

assertEquals(CREATED.getStatusCode(), actual.getStatus());
}
Expand All @@ -732,7 +739,7 @@ public void testCreateNewObjectWithSparql() throws Exception {
when(mockContainerService.findOrCreate(mockSession, "/b")).thenReturn(mockContainer);

final Response actual = testObj.createObject(null, null,
MediaType.valueOf(contentTypeSPARQLUpdate), "b", toInputStream("x"));
MediaType.valueOf(contentTypeSPARQLUpdate), "b", toInputStream("x"), null);

assertEquals(CREATED.getStatusCode(), actual.getStatus());
verify(mockContainer).updateProperties(eq(idTranslator), eq("x"), any(RdfStream.class), any(Service.class));
Expand All @@ -746,7 +753,7 @@ public void testCreateNewObjectWithRdf() throws Exception {
when(mockContainerService.findOrCreate(mockSession, "/b")).thenReturn(mockContainer);

final Response actual = testObj.createObject(null, null, NTRIPLES_TYPE, "b",
toInputStream("_:a <info:b> _:c ."));
toInputStream("_:a <info:b> _:c ."), null);

assertEquals(CREATED.getStatusCode(), actual.getStatus());
verify(mockContainer).replaceProperties(eq(idTranslator), any(Model.class), any(RdfStream.class),
Expand All @@ -763,7 +770,7 @@ public void testCreateNewBinary() throws Exception {

try (final InputStream content = toInputStream("x")) {
final Response actual = testObj.createObject(null, null, APPLICATION_OCTET_STREAM_TYPE, "b",
content);
content, null);

assertEquals(CREATED.getStatusCode(), actual.getStatus());
verify(mockBinary).setContent(content, APPLICATION_OCTET_STREAM, null, "", null);
Expand All @@ -780,7 +787,7 @@ public void testCreateNewBinaryWithContentTypeWithParams() throws Exception {
try (final InputStream content = toInputStream("x")) {
final MediaType requestContentType = MediaType.valueOf("some/mime-type; with=some; param=s");
final Response actual = testObj.createObject(null, null, requestContentType, "b",
content);
content, null);

assertEquals(CREATED.getStatusCode(), actual.getStatus());
verify(mockBinary).setContent(content, requestContentType.toString(), null, "", null);
Expand All @@ -792,8 +799,18 @@ public void testPostToBinary() throws Exception {
final FedoraBinary mockObject = (FedoraBinary)setResource(FedoraBinary.class);
doReturn(mockObject).when(testObj).resource();

testObj.createObject(null, null, null, null, null);
testObj.createObject(null, null, null, null, null, null);

}

@Test(expected = ServerErrorException.class)
public void testLDPRNotImplemented() throws Exception {
testObj.createObject(null, null, null, null, null, "<http://www.w3.org/ns/ldp#Resource>; rel=\"type\"");
}

@Test(expected = ClientErrorException.class)
public void testLDPRNotImplementedInvalidLink() throws Exception {
testObj.createObject(null, null, null, null, null, "Link: <http://www.w3.org/ns/ldp#Resource;rel=type");
}

@Test
Expand Down

5 comments on commit c2bd525

@cbeer
Copy link
Contributor

@cbeer cbeer commented on c2bd525 Apr 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um. Is 501 actually the right status code? Wouldn't 405 Method Not Allowed be much better? Here's the description of 501:

10.5.2 501 Not Implemented

The server does not support the functionality required to fulfill the request. This is the appropriate response when the server does not recognize the request method and is not capable of supporting it for any resource.

@escowles
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbeer I think 501 is the right response code. 405 would imply that we don't support POST or PUT, which isn't true. 501 is a better signal that we haven't implemented non-container LDP RDFSources.

@awoods
Copy link

@awoods awoods commented on c2bd525 Apr 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"501 Not Implemented" seems like a pretty good fit.

10.5.2 501 Not Implemented

The server does not support the functionality required to fulfill the request. This is the appropriate response when the server does not recognize the request method and is not capable of supporting it for any resource.

10.4.6 405 Method Not Allowed
The method specified in the Request-Line is not allowed for the resource identified by the Request-URI. The response MUST include an Allow header containing a list of valid methods for the requested resource.

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.

@ajs6f
Copy link
Contributor

@ajs6f ajs6f commented on c2bd525 Apr 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against 501 (although I wish that LDP had actually given us some guidance here). I do want to point out that given our intention to implement this LDP interaction model, whatever decision we make here has a very short life in front of it.

@azaroth42
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My 2c: 501 seems okay to me -- the second sentence seems like an example when it can be used, rather than the only situation that it can be used. Also agree it would have been nicer if LDP was more explicit about the error code.

Please sign in to comment.