Handle errors with sparql update queries that reference non-existent …

…resources - Fix empty query error handling Resolves: https://www.pivotaltracker.com/story/show/71903492
fcrepo · Jun 2, 2014 · 42020c4 · 42020c4
1 parent c822f0e
commit 42020c4
Show file tree

Hide file tree

Showing 3 changed files with 106 additions and 26 deletions.
diff --git a/fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraNodes.java b/fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraNodes.java
@@ -33,6 +33,7 @@
 import static javax.ws.rs.core.Response.Status.FORBIDDEN;
 import static javax.ws.rs.core.Response.Status.OK;
 import static org.apache.commons.lang.ArrayUtils.contains;
+import static org.apache.commons.lang.StringUtils.isBlank;
 import static org.apache.http.HttpStatus.SC_BAD_GATEWAY;
 import static org.apache.http.HttpStatus.SC_BAD_REQUEST;
 import static org.apache.http.HttpStatus.SC_CONFLICT;
@@ -360,40 +361,47 @@ public Response updateSparql(@PathParam("path")
         final String path = toPath(pathList);
         LOGGER.debug("Attempting to update path: {}", path);
 
-        try {
-
-            if (requestBodyStream != null) {
+        if (null == requestBodyStream) {
+            return status(SC_BAD_REQUEST).entity("SPARQL-UPDATE requests must have content!").build();
+        }
 
-                final FedoraResource resource =
-                        nodeService.getObject(session, path);
+        try {
+            final String requestBody = IOUtils.toString(requestBodyStream);
+            if (isBlank(requestBody)) {
+                return status(SC_BAD_REQUEST).entity("SPARQL-UPDATE requests must have content!").build();
+            }
 
-                evaluateRequestPreconditions(request, resource);
+            final FedoraResource resource =
+                    nodeService.getObject(session, path);
 
-                final Dataset properties = resource.updatePropertiesDataset(new HttpIdentifierTranslator(
-                        session, FedoraNodes.class, uriInfo), IOUtils
-                        .toString(requestBodyStream));
+            evaluateRequestPreconditions(request, resource);
 
+            final Dataset properties = resource.updatePropertiesDataset(new HttpIdentifierTranslator(
+                    session, FedoraNodes.class, uriInfo), requestBody);
 
-                final Model problems = properties.getNamedModel(PROBLEMS_MODEL_NAME);
-                if (!problems.isEmpty()) {
-                    LOGGER.info(
-                                   "Found these problems updating the properties for {}: {}",
-                                   path, problems);
-                    return status(FORBIDDEN).entity(problems.toString())
-                            .build();
+            final Model problems = properties.getNamedModel(PROBLEMS_MODEL_NAME);
+            if (!problems.isEmpty()) {
+                LOGGER.info(
+                        "Found these problems updating the properties for {}: {}",
+                        path, problems);
+                return status(FORBIDDEN).entity(problems.toString())
+                        .build();
+            }
 
-                }
+            session.save();
+            versionService.nodeUpdated(resource.getNode());
 
-                session.save();
-                versionService.nodeUpdated(resource.getNode());
+            addCacheControlHeaders(servletResponse, resource);
 
-                addCacheControlHeaders(servletResponse, resource);
+            return noContent().build();
 
-                return noContent().build();
+        } catch ( RuntimeException ex ) {
+            final Throwable cause = ex.getCause();
+            if ( cause != null && cause instanceof PathNotFoundException ) {
+                // the sparql update referred to a repository resource that doesn't exist
+                return status(SC_BAD_REQUEST).entity(cause.getMessage()).build();
             }
-            return status(SC_BAD_REQUEST).entity(
-                    "SPARQL-UPDATE requests must have content!").build();
-
+            throw ex;
         } finally {
             session.logout();
         }

diff --git a/fcrepo-http-api/src/test/java/org/fcrepo/http/api/FedoraNodesTest.java b/fcrepo-http-api/src/test/java/org/fcrepo/http/api/FedoraNodesTest.java
@@ -17,6 +17,7 @@
 
 import static com.hp.hpl.jena.graph.NodeFactory.createAnon;
 import static javax.jcr.PropertyType.PATH;
+import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
 import static javax.ws.rs.core.Response.Status.CONFLICT;
 import static javax.ws.rs.core.Response.Status.CREATED;
 import static javax.ws.rs.core.Response.Status.NO_CONTENT;
@@ -54,6 +55,7 @@
 
 import javax.jcr.ItemExistsException;
 import javax.jcr.Node;
+import javax.jcr.PathNotFoundException;
 import javax.jcr.RepositoryException;
 import javax.jcr.Session;
 import javax.jcr.Value;
@@ -422,6 +424,51 @@ public void testSparqlUpdate() throws RepositoryException, IOException {
         verify(mockSession).logout();
     }
 
+    @Test
+    public void testSparqlUpdateNull() throws IOException, RepositoryException {
+        final String pid = "test-pid";
+
+        final Response result = testObj.updateSparql(createPathList(pid),
+                                                     getUriInfoImpl(),
+                                                     null,
+                                                     mockRequest,
+                                                     mockResponse);
+        assertNotNull(result);
+        assertEquals(BAD_REQUEST.getStatusCode(), result.getStatus());
+    }
+
+    @Test
+    public void testSparqlUpdateEmpty() throws IOException, RepositoryException {
+        final String pid = "test-pid";
+        final InputStream mockStream = new ByteArrayInputStream("".getBytes());
+
+        final Response result = testObj.updateSparql(createPathList(pid),
+                                                     getUriInfoImpl(),
+                                                     mockStream,
+                                                     mockRequest,
+                                                     mockResponse);
+        assertNotNull(result);
+        assertEquals(BAD_REQUEST.getStatusCode(), result.getStatus());
+    }
+
+    @Test
+    public void testSparqlUpdateError() throws IOException, RepositoryException {
+        final String pid = "test-pid";
+        final String path = "/" + pid;
+        final InputStream mockStream = new ByteArrayInputStream("my-sparql-statement".getBytes());
+        final Exception ex = new RuntimeException(new PathNotFoundException("expected"));
+        when(mockNodes.getObject(mockSession, path)).thenThrow(ex);
+
+        final Response result = testObj.updateSparql(createPathList(pid),
+                                                     getUriInfoImpl(),
+                                                     mockStream,
+                                                     mockRequest,
+                                                     mockResponse);
+        assertNotNull(result);
+        assertEquals(BAD_REQUEST.getStatusCode(), result.getStatus());
+        assertEquals("expected", result.getEntity().toString());
+    }
+
     @Test
     public void testReplaceRdf() throws Exception {
         final String pid = "FedoraObjectsRdfTest1";

diff --git a/fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/FedoraNodesIT.java b/fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/FedoraNodesIT.java
@@ -32,6 +32,7 @@
 import static org.apache.jena.riot.WebContent.contentTypeNTriples;
 import static java.util.regex.Pattern.DOTALL;
 import static java.util.regex.Pattern.compile;
+import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
 import static javax.ws.rs.core.Response.Status.CREATED;
 import static javax.ws.rs.core.Response.Status.NOT_FOUND;
 import static javax.ws.rs.core.Response.Status.NOT_MODIFIED;
@@ -720,6 +721,29 @@ public void testGetObjectGraphByUUID() throws Exception {
 
     }
 
+    @Test
+    public void testLinkToNonExistent() throws Exception {
+        final HttpResponse createResponse = createObject("");
+        final String subjectURI = createResponse.getFirstHeader("Location").getValue();
+        final HttpPatch patch = new HttpPatch(subjectURI);
+        patch.addHeader("Content-Type", "application/sparql-update");
+        final BasicHttpEntity e = new BasicHttpEntity();
+        e.setContent(new ByteArrayInputStream(
+            ("INSERT { <> <http://fedora.info/definitions/v4/rels-ext#isMemberOfCollection> " +
+                 "<" + serverAddress + "non-existant> } WHERE {}").getBytes()));
+        patch.setEntity(e);
+        assertEquals(BAD_REQUEST.getStatusCode(), getStatus(patch));
+    }
+
+    @Test
+    public void testEmtpyPatch() throws Exception {
+        final HttpResponse createResponse = createObject("");
+        final String subjectURI = createResponse.getFirstHeader("Location").getValue();
+        final HttpPatch patch = new HttpPatch(subjectURI);
+        patch.addHeader("Content-Type", "application/sparql-update");
+        assertEquals(BAD_REQUEST.getStatusCode(), getStatus(patch));
+    }
+
     @Test
     public void testUpdateObjectGraph() throws Exception {
         final HttpResponse createResponse = createObject("");
@@ -1477,6 +1501,7 @@ public void testFederatedDatastream() throws IOException {
         patch.addHeader("Content-Type", "application/sparql-update");
         final BasicHttpEntity e = new BasicHttpEntity();
         e.setContent(new ByteArrayInputStream(sparql.getBytes()));
+        patch.setEntity(e);
         assertEquals("Couldn't link to external datastream!", 204, getStatus(patch));
     }
 
@@ -1572,7 +1597,6 @@ public String apply(final Header h) {
     }
 
     @Test
-    @Ignore("Works in real life")
     public void testLinkedDeletion() throws Exception {
         createObject("linked-from");
         createObject("linked-to");
@@ -1584,10 +1608,11 @@ public void testLinkedDeletion() throws Exception {
         patch.addHeader("Content-Type", "application/sparql-update");
         final BasicHttpEntity e = new BasicHttpEntity();
         e.setContent(new ByteArrayInputStream(sparql.getBytes()));
+        patch.setEntity(e);
         assertEquals("Couldn't link resources!", 204, getStatus(patch));
 
         final HttpDelete delete = new HttpDelete(serverAddress + "linked-to");
-        assertEquals("Deleting linked-to should error!", 412, getStatus(delete));
+        assertEquals("Error deleting linked-to!", 204, getStatus(delete));
 
         final HttpGet get = new HttpGet(serverAddress + "linked-from");
         assertEquals("Linked to should still exist!", 200, getStatus(get));