Skip to content

Commit

Permalink
When persisting a graph, use exceptions for invalid operations
Browse files Browse the repository at this point in the history 
Previously, we inserted exception information into a RDF model for future
(possible) use, and relied on the client to check it. Instead, we should handle
this exceptional case using exceptions, which may be aggregated by some
consumers (e.g. JcrPropertyStatementListener) into a single exception.
  • Loading branch information
@cbeer
cbeer committed Oct 9, 2014
1 parent e96995d commit 6a47770
Show file tree
Hide file tree
Showing 14 changed files with 69 additions and 194 deletions.
29 changes: 1 addition & 28 deletions fcrepo-http-api/src/main/java/org/fcrepo/http/api/ContentExposingResource.java
View file
Expand Up @@ -18,13 +18,10 @@
import com.google.common.base.Function;
import com.google.common.collect.Iterators;
import com.hp.hpl.jena.graph.Triple;
import com.hp.hpl.jena.query.Dataset;
import com.hp.hpl.jena.rdf.model.Literal;
import com.hp.hpl.jena.rdf.model.Model;
import com.hp.hpl.jena.rdf.model.Resource;
import com.hp.hpl.jena.rdf.model.Statement;
import com.hp.hpl.jena.rdf.model.StmtIterator;
import org.apache.commons.lang.StringUtils;
import org.apache.jena.riot.Lang;
import org.fcrepo.http.commons.api.rdf.HttpTripleUtil;
import org.fcrepo.http.commons.domain.Prefer;
Expand Down Expand Up @@ -59,7 +56,6 @@
import javax.jcr.Session;
import javax.servlet.http.HttpServletResponse;
import javax.ws.rs.BadRequestException;
import javax.ws.rs.ForbiddenException;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.core.CacheControl;
import javax.ws.rs.core.Context;
Expand Down Expand Up @@ -87,7 +83,6 @@
import static javax.ws.rs.core.Response.status;
import static org.apache.commons.lang.StringUtils.isBlank;
import static org.apache.jena.riot.RDFLanguages.contentTypeToLang;
import static org.fcrepo.kernel.rdf.GraphProperties.PROBLEMS_MODEL_NAME;
import static org.slf4j.LoggerFactory.getLogger;

/**
Expand Down Expand Up @@ -499,9 +494,7 @@ protected void replaceResourceWithStream(final FedoraResource resource,
}

protected void patchResourcewithSparql(final FedoraResource resource, final String requestBody) {
final Dataset properties = resource.updatePropertiesDataset(translator(), requestBody);

handleProblems(resource, properties);
resource.updatePropertiesDataset(translator(), requestBody);
}

/**
Expand All @@ -514,26 +507,6 @@ private static URI checksumURI( final String checksum ) {
return null;
}

private void handleProblems(final FedoraResource resource, final Dataset properties) {

final Model problems = properties.getNamedModel(PROBLEMS_MODEL_NAME);

if (!problems.isEmpty()) {
LOGGER.info(
"Found these problems updating the properties for {}: {}", resource, problems);
final StringBuilder error = new StringBuilder();
final StmtIterator sit = problems.listStatements();
while (sit.hasNext()) {
final String message = getMessage(sit.next());
if (StringUtils.isNotEmpty(message) && error.indexOf(message) < 0) {
error.append(message + " \n");
}
}

throw new ForbiddenException(error.length() > 0 ? error.toString() : problems.toString());
}
}

/*
* Return the statement's predicate and its literal value if there's any
* @param stmt
Expand Down
15 changes: 1 addition & 14 deletions ...o-http-api/src/main/java/org/fcrepo/http/api/repository/FedoraRepositoriesProperties.java
View file
Expand Up @@ -16,11 +16,9 @@
package org.fcrepo.http.api.repository;

import static javax.ws.rs.core.Response.status;
import static javax.ws.rs.core.Response.Status.FORBIDDEN;
import static org.apache.http.HttpStatus.SC_BAD_REQUEST;
import static org.apache.http.HttpStatus.SC_NO_CONTENT;
import static org.apache.jena.riot.WebContent.contentTypeSPARQLUpdate;
import static org.fcrepo.kernel.rdf.GraphProperties.PROBLEMS_MODEL_NAME;
import static org.slf4j.LoggerFactory.getLogger;

import java.io.IOException;
Expand All @@ -45,8 +43,6 @@
import org.springframework.context.annotation.Scope;

import com.codahale.metrics.annotation.Timed;
import com.hp.hpl.jena.query.Dataset;
import com.hp.hpl.jena.rdf.model.Model;

/**
* Utility endpoint for running SPARQL Update queries on any object in the
Expand Down Expand Up @@ -85,17 +81,8 @@ public Response updateSparql(final InputStream requestBodyStream)
final UriAwareIdentifierConverter translator
= new UriAwareIdentifierConverter(session, UriBuilder.fromResource(FedoraLdp.class));

final Dataset dataset =
result.updatePropertiesDataset(translator, IOUtils
result.updatePropertiesDataset(translator, IOUtils
.toString(requestBodyStream));
if (dataset.containsNamedModel(PROBLEMS_MODEL_NAME)) {
final Model problems = dataset.getNamedModel(PROBLEMS_MODEL_NAME);

LOGGER.info("Found these problems updating the properties for {}: {}",
"/", problems.toString());
return status(FORBIDDEN).entity(problems.toString())
.build();
}

try {
session.save();
Expand Down
7 changes: 0 additions & 7 deletions fcrepo-http-api/src/test/java/org/fcrepo/http/api/FedoraBatchTest.java
View file
Expand Up @@ -24,7 +24,6 @@
import static org.fcrepo.http.commons.test.util.TestHelpers.getUriInfoImpl;
import static org.fcrepo.http.commons.test.util.TestHelpers.mockDatastream;
import static org.fcrepo.http.commons.test.util.TestHelpers.mockSession;
import static org.fcrepo.kernel.rdf.GraphProperties.PROBLEMS_MODEL_NAME;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.mockito.Matchers.any;
Expand Down Expand Up @@ -62,7 +61,6 @@
import javax.ws.rs.core.UriBuilder;
import javax.ws.rs.core.UriInfo;

import com.hp.hpl.jena.query.Dataset;
import com.hp.hpl.jena.rdf.model.Model;
import org.apache.commons.io.IOUtils;
import org.fcrepo.http.commons.api.rdf.UriAwareIdentifierConverter;
Expand Down Expand Up @@ -175,11 +173,6 @@ public void testBatchSparqlUpdate() throws Exception {
when(mockNodes.exists(mockSession, path + "/.")).thenReturn(true);
doReturn(mockObject).when(testObj).getResourceFromPath(path + "/.");

final Dataset mockDataset = mock(Dataset.class);
final Model mockProblems = mock(Model.class);
when(mockDataset.getNamedModel(PROBLEMS_MODEL_NAME)).thenReturn(mockProblems);
when(mockProblems.isEmpty()).thenReturn(true);
when(mockObject.updatePropertiesDataset(any(IdentifierConverter.class), eq("xyz"))).thenReturn(mockDataset);
final MultiPart multipart = new MultiPart();

final StreamDataBodyPart part = new StreamDataBodyPart(".",
Expand Down
18 changes: 0 additions & 18 deletions fcrepo-http-api/src/test/java/org/fcrepo/http/api/FedoraLdpTest.java
View file
Expand Up @@ -21,7 +21,6 @@
import com.google.common.collect.Lists;
import com.hp.hpl.jena.graph.NodeFactory;
import com.hp.hpl.jena.graph.Triple;
import com.hp.hpl.jena.query.Dataset;
import com.hp.hpl.jena.rdf.model.Model;
import com.hp.hpl.jena.rdf.model.RDFNode;
import com.hp.hpl.jena.rdf.model.Resource;
Expand Down Expand Up @@ -73,7 +72,6 @@
import static org.fcrepo.http.commons.test.util.TestHelpers.mockSession;
import static org.fcrepo.kernel.RdfLexicon.INBOUND_REFERENCES;
import static org.fcrepo.kernel.RdfLexicon.LDP_NAMESPACE;
import static org.fcrepo.kernel.rdf.GraphProperties.PROBLEMS_MODEL_NAME;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
Expand Down Expand Up @@ -596,11 +594,6 @@ public void testPatchObject() throws Exception {

final FedoraResource mockObject = setResource(FedoraObject.class);

final Dataset dataset = mock(Dataset.class);
final Model mockModel = mock(Model.class);
when(dataset.getNamedModel(PROBLEMS_MODEL_NAME)).thenReturn(mockModel);
when(mockModel.isEmpty()).thenReturn(true);
when(mockObject.updatePropertiesDataset(identifierConverter, "xyz")).thenReturn(dataset);
testObj.updateSparql(toInputStream("xyz"));
verify(mockVersionService).nodeUpdated(mockObject.getNode());
}
Expand All @@ -612,11 +605,6 @@ public void testPatchBinaryDescription() throws Exception {
doReturn(mockObject).when(testObj).resource();
when(mockObject.getContentNode()).thenReturn(mockBinaryNode);

final Dataset dataset = mock(Dataset.class);
final Model mockModel = mock(Model.class);
when(dataset.getNamedModel(PROBLEMS_MODEL_NAME)).thenReturn(mockModel);
when(mockModel.isEmpty()).thenReturn(true);
when(mockObject.updatePropertiesDataset(identifierConverter, "xyz")).thenReturn(dataset);
testObj.updateSparql(toInputStream("xyz"));
verify(mockVersionService).nodeUpdated(mockObject.getNode());
verify(mockVersionService).nodeUpdated(mockObject.getContentNode());
Expand Down Expand Up @@ -660,12 +648,6 @@ public void testCreateNewObjectWithSparql() throws Exception {

when(mockObjectService.findOrCreateObject(mockSession, "/b")).thenReturn(mockObject);

final Dataset dataset = mock(Dataset.class);
final Model mockModel = mock(Model.class);
when(dataset.getNamedModel(PROBLEMS_MODEL_NAME)).thenReturn(mockModel);
when(mockModel.isEmpty()).thenReturn(true);
when(mockObject.updatePropertiesDataset(identifierConverter, "x")).thenReturn(dataset);

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

Expand Down
3 changes: 0 additions & 3 deletions ...tp-api/src/test/java/org/fcrepo/http/api/repository/FedoraRepositoriesPropertiesTest.java
View file
Expand Up @@ -74,9 +74,6 @@ public void testSparqlUpdate() throws RepositoryException, IOException {
final InputStream mockStream =
new ByteArrayInputStream("my-sparql-statement".getBytes());
when(mockNodes.getObject(mockSession, "/")).thenReturn(mockObject);
when(
mockObject.updatePropertiesDataset(any(IdentifierConverter.class),
any(String.class))).thenReturn(mockDataset);
testObj.updateSparql(mockStream);

verify(mockObject).updatePropertiesDataset(any(IdentifierConverter.class),
Expand Down
2 changes: 1 addition & 1 deletion fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/FedoraLdpIT.java
View file
Expand Up @@ -1216,7 +1216,7 @@ public void testLinkToNonExistent() throws Exception {
("INSERT { <> <http://fedora.info/definitions/v4/rels-ext#isMemberOfCollection> " +
"<" + serverAddress + "non-existant> } WHERE {}").getBytes()));
patch.setEntity(e);
assertEquals(BAD_REQUEST.getStatusCode(), getStatus(patch));
assertEquals(CONFLICT.getStatusCode(), getStatus(patch));
}

@Test
Expand Down
4 changes: 2 additions & 2 deletions .../src/main/java/org/fcrepo/http/commons/exceptionhandlers/MalformedRdfExceptionMapper.java
View file
Expand Up @@ -21,7 +21,7 @@
import javax.ws.rs.ext.ExceptionMapper;
import javax.ws.rs.ext.Provider;

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;

/**
Expand All @@ -33,6 +33,6 @@ public class MalformedRdfExceptionMapper implements ExceptionMapper<MalformedRdf

@Override
public Response toResponse(final MalformedRdfException e) {
return status(BAD_REQUEST).entity(e.getMessage()).build();
return status(CONFLICT).entity(e.getMessage()).build();
}
}
26 changes: 10 additions & 16 deletions fcrepo-kernel-impl/src/main/java/org/fcrepo/kernel/impl/FedoraResourceImpl.java
View file
Expand Up @@ -18,11 +18,9 @@
import static com.google.common.base.Throwables.propagate;
import static com.google.common.collect.ImmutableSet.copyOf;
import static com.google.common.collect.Lists.newArrayList;
import static com.hp.hpl.jena.rdf.model.ModelFactory.createDefaultModel;
import static com.hp.hpl.jena.update.UpdateAction.execute;
import static com.hp.hpl.jena.update.UpdateFactory.create;
import static org.apache.commons.codec.digest.DigestUtils.shaHex;
import static org.fcrepo.kernel.rdf.GraphProperties.PROBLEMS_MODEL_NAME;
import static org.fcrepo.kernel.rdf.GraphProperties.URI_SYMBOL;
import static org.fcrepo.kernel.services.functions.JcrPropertyFunctions.isFrozen;
import static org.fcrepo.kernel.services.functions.JcrPropertyFunctions.property2values;
Expand Down Expand Up @@ -238,15 +236,21 @@ public boolean hasType(final String type) {
* (org.fcrepo.kernel.identifiers.IdentifierConverter, java.lang.String)
*/
@Override
public Dataset updatePropertiesDataset(final IdentifierConverter<Resource,Node> subjects,
public void updatePropertiesDataset(final IdentifierConverter<Resource,Node> subjects,
final String sparqlUpdateStatement) {
final Dataset dataset = getPropertiesDataset(subjects);

final JcrPropertyStatementListener listener =
new JcrPropertyStatementListener(subjects, getSession());

dataset.getDefaultModel().register(listener);

final UpdateRequest request =
create(sparqlUpdateStatement, dataset.getContext().getAsString(
URI_SYMBOL));
create(sparqlUpdateStatement, subjects.reverse().convert(getNode()).toString());
dataset.getDefaultModel().setNsPrefixes(request.getPrefixMapping());
execute(request, dataset);
return dataset;

listener.assertNoExceptions();
}

/* (non-Javadoc)
Expand All @@ -268,18 +272,8 @@ public Dataset getPropertiesDataset(final IdentifierConverter<Resource,Node> gra

final Dataset dataset = DatasetFactory.create(propertiesStream.asModel());

final Model problemsModel = createDefaultModel();

final JcrPropertyStatementListener listener =
JcrPropertyStatementListener.getListener(graphSubjects, getSession(), problemsModel);

dataset.getDefaultModel().register(listener);

dataset.addNamedModel(PROBLEMS_MODEL_NAME, problemsModel);

dataset.getContext().set(URI_SYMBOL, graphSubjects.reverse().convert(getNode()));


return dataset;
}

Expand Down

0 comments on commit 6a47770

Please sign in to comment.