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.

Sign up for GitHub

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

Jump to bottom

Applied code changes to fresh clone. #596

Closed
wants to merge 2 commits into from
Closed

Applied code changes to fresh clone. #596

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
615d47a
Applied code changes to fresh clone.
nikhiltri Oct 28, 2014
dab6b91
Changes per code review comments
nikhiltri Oct 29, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 83 additions & 21 deletions fcrepo-http-api/src/main/java/org/fcrepo/http/api/responses/StreamingBaseHtmlProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,18 @@
*/
package org.fcrepo.http.api.responses;

import static com.google.common.collect.ImmutableMap.builder;
import static com.hp.hpl.jena.graph.Node.ANY;
import static javax.ws.rs.core.MediaType.APPLICATION_XHTML_XML;
import static javax.ws.rs.core.MediaType.APPLICATION_XHTML_XML_TYPE;
import static javax.ws.rs.core.MediaType.TEXT_HTML;
import static javax.ws.rs.core.MediaType.TEXT_HTML_TYPE;
import static org.apache.commons.lang.StringUtils.isBlank;
import static org.fcrepo.http.commons.responses.RdfSerializationUtils.getAllValuesForPredicate;
import static org.fcrepo.http.commons.responses.RdfSerializationUtils.getFirstValueForPredicate;
import static org.fcrepo.http.commons.responses.RdfSerializationUtils.mixinTypesPredicate;
import static org.fcrepo.http.commons.responses.RdfSerializationUtils.primaryTypePredicate;
import static com.google.common.collect.ImmutableList.copyOf;
import static com.google.common.collect.ImmutableMap.builder;
import static org.slf4j.LoggerFactory.getLogger;

import java.io.IOException;
Expand All @@ -33,13 +37,17 @@
import java.lang.annotation.Annotation;
import java.lang.reflect.Type;
import java.net.URL;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Properties;

import javax.annotation.PostConstruct;
import javax.jcr.RepositoryException;
import javax.jcr.Session;
import javax.jcr.nodetype.NodeType;
import javax.jcr.nodetype.NodeTypeIterator;
import javax.ws.rs.Produces;
import javax.ws.rs.WebApplicationException;
Expand All @@ -49,8 +57,10 @@
import javax.ws.rs.ext.MessageBodyWriter;
import javax.ws.rs.ext.Provider;

import com.google.common.base.Predicate;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.hp.hpl.jena.graph.Node;
import com.hp.hpl.jena.rdf.model.Model;
import org.apache.velocity.Template;
Expand Down Expand Up @@ -109,6 +119,27 @@ public class StreamingBaseHtmlProvider implements MessageBodyWriter<RdfStream> {
private static final Logger LOGGER =
getLogger(StreamingBaseHtmlProvider.class);

protected final Predicate<NodeType> acceptWhenTemplateExists = new Predicate<NodeType>() {
Copy link

Choose a reason for hiding this comment

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

Change from protected to private.

@Override
public boolean apply(final NodeType nodeType) {
final String nodeTypeName = nodeType.getName();
if (isBlank(nodeTypeName)) {
return false;
}
return velocity.resourceExists(getTemplateLocation(nodeTypeName));
}
};

protected final Predicate<String> acceptWhenTemplateMapContainsKey = new Predicate<String>() {
Copy link

Choose a reason for hiding this comment

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

Change from protected to private.

@Override
public boolean apply(final String key) {
if (isBlank(key)) {
return false;
}
return templatesMap.containsKey(key);
}
};

@PostConstruct
void init() throws IOException {

Expand All @@ -127,29 +158,28 @@ void init() throws IOException {
final ImmutableMap.Builder<String, Template> templatesMapBuilder = builder();
final Session session = sessionFactory.getInternalSession();
try {
// we search all of the possible node primary types
// we search all of the possible node primary types and mixins
for (final NodeTypeIterator primaryNodeTypes =
session.getWorkspace().getNodeTypeManager()
.getPrimaryNodeTypes(); primaryNodeTypes.hasNext();) {
final NodeType primaryNodeType =
primaryNodeTypes.nextNodeType();
final String primaryNodeTypeName =
primaryNodeTypes.nextNodeType().getName();
// for each node primary type, we try to find a template
final String templateLocation =
templatesLocation + "/" +
primaryNodeTypeName.replace(':', '-') +
templateFilenameExtension;
if (velocity.resourceExists(templateLocation)) {
final Template template =
velocity.getTemplate(templateLocation);
template.setName(templateLocation);
LOGGER.debug("Found template: {}", templateLocation);
templatesMapBuilder.put(primaryNodeTypeName, template);
LOGGER.debug("which we will use for nodes with primary type: {}",
primaryNodeTypeName);
primaryNodeType.getName();

// Create a list of the primary type and all its parents
final List<NodeType> nodeTypesList = new ArrayList<NodeType>();
Copy link

Choose a reason for hiding this comment

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

Change from:
final List<NodeType> nodeTypesList = new ArrayList<NodeType>();
to:
final List<NodeType> nodeTypesList = new ArrayList<>();

nodeTypesList.add(primaryNodeType);
nodeTypesList.addAll(Arrays.asList(primaryNodeType.getSupertypes()));

// Find a template that matches the primary type or one of its parents
final NodeType templateMatch = Iterables.find(nodeTypesList, acceptWhenTemplateExists, null);
if (templateMatch != null) {
addTemplate(primaryNodeTypeName, templateMatch.getName(), templatesMapBuilder);
} else {
// No HTML representation available for that kind of node
LOGGER.debug("Didn't find template for nodes with primary type: {} in location: {}",
primaryNodeTypeName, templateLocation);
LOGGER.debug("Didn't find template for nodes with primary type or its parents: {} in location: {}",
primaryNodeTypeName, templatesLocation);
}
}

Expand All @@ -158,9 +188,7 @@ void init() throws IOException {

for (final String key : otherTemplates) {
final Template template =
velocity.getTemplate(templatesLocation + "/" +
key.replace(':', '-') +
templateFilenameExtension);
velocity.getTemplate(getTemplateLocation(key));
templatesMapBuilder.put(key, template);
}

Expand Down Expand Up @@ -235,6 +263,23 @@ private Template getTemplate(final Model rdf, final Node subject,
}
}

if (template == null) {
LOGGER.trace("Attempting to discover the mixin types of the node for the resource in question...");
final Iterator<String> mixinTypes =
getAllValuesForPredicate(rdf, subject,
mixinTypesPredicate);

LOGGER.debug("Found mixins: {}", mixinTypes);
if (mixinTypes != null) {
final ImmutableList<String> copy = copyOf(mixinTypes);
final String mixin = Iterables.find(copy, acceptWhenTemplateMapContainsKey, null);
if (mixin != null) {
LOGGER.debug("Matched mixin type: {}", mixin);
Copy link

Choose a reason for hiding this comment

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

When I run the test with a debugger (or simply look for the debug statement here "Matched mixin type"), this block is not being executed... which is one of the main reasons for this PR.

template = templatesMap.get(mixin);
}
}
}

if (template == null) {
LOGGER.trace("Attempting to discover the primary type of the node for the resource in question...");
final String nodeType =
Expand Down Expand Up @@ -271,4 +316,21 @@ public long getSize(final RdfStream t, final Class<?> type,
// we don't know in advance how large the result might be
return -1;
}

private void addTemplate(final String primaryNodeTypeName, final String templateNodeTypeName,
final ImmutableMap.Builder<String, Template> templatesMapBuilder) {
final String templateLocation = getTemplateLocation(templateNodeTypeName);
final Template template =
velocity.getTemplate(templateLocation);
template.setName(templateLocation);
LOGGER.debug("Found template: {}", templateLocation);
templatesMapBuilder.put(primaryNodeTypeName, template);
LOGGER.debug("which we will use for nodes with primary type: {}",
primaryNodeTypeName);
}

private String getTemplateLocation(final String nodeTypeName) {
return templatesLocation + "/" +
nodeTypeName.replace(':', '-') + templateFilenameExtension;
}
}
35 changes: 34 additions & 1 deletion ...o-http-api/src/test/java/org/fcrepo/http/api/responses/StreamingBaseHtmlProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ public class StreamingBaseHtmlProviderTest {
private final StreamingBaseHtmlProvider testProvider = new StreamingBaseHtmlProvider();

private final RdfStream testData = new RdfStream();
private final RdfStream testData2 = new RdfStream();

@Mock
private Session mockSession;
Expand All @@ -93,13 +94,18 @@ public void setup() throws RepositoryException {
testData.session(mockSession);
testData.topic(createURI("test:subject"));
testData.concat(
new Triple(createURI("test:subject"),
new Triple(createURI("test:subject"),
createURI("test:predicate"),
createLiteral("test:object")));
testData.concat(
new Triple(createURI("test:subject"), primaryTypePredicate,
createLiteral("nt:file")));

testData2.session(mockSession);
testData2.topic(createURI("test:subject2"));
testData2.concat(
new Triple(createURI("test:subject2"), primaryTypePredicate,
createLiteral("childOf:ntFile")));
final UriInfo info = Mockito.mock(UriInfo.class);
setField(testProvider, "uriInfo", info);
}
Expand Down Expand Up @@ -180,4 +186,31 @@ public Object answer(final InvocationOnMock invocation) {
assertTrue("Got no output from serialization!", results.length > 0);

}

@SuppressWarnings({"unchecked", "rawtypes"})
@Test
public void testWriteToWithParentTemplate() throws WebApplicationException,
Copy link

Choose a reason for hiding this comment

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

Ideally, this test would exercise the block in StreamingBaseHtmlProvider.getTemplate():

           final String mixin = Iterables.find(copy, acceptWhenTemplateMapContainsKey, null);
           if (mixin != null) {
               LOGGER.debug("Matched mixin type: {}", mixin);
              template = templatesMap.get(mixin);
           }

However, the test as written results in mixin being null and the if block above being skipped.

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 changed the predicate of testData2 to mixinTypesPredicate (line 108 above) so the mixin is found here.

IllegalArgumentException, IOException {
final Template mockTemplate = mock(Template.class);
final ByteArrayOutputStream outStream = new ByteArrayOutputStream();

doAnswer(new Answer<Object>() {

@Override
public Object answer(final InvocationOnMock invocation) {
outStream.write("abcdefighijk".getBytes(), 0, 10);
return "I am pretending to merge a template for you.";
}
}).when(mockTemplate).merge(isA(Context.class), isA(Writer.class));

setField(testProvider, "templatesMap",
of("childOf:ntFile", mockTemplate,
"grandchildOf:ntFile", mockTemplate));
testProvider.writeTo(testData2, RdfStream.class, mock(Type.class),
new Annotation[] {}, MediaType
.valueOf("text/html"),
(MultivaluedMap) new MultivaluedHashMap<>(), outStream);
final byte[] results = outStream.toByteArray();
assertTrue("Got no output from serialization!", results.length > 0);
}
}
17 changes: 17 additions & 0 deletions fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/FedoraHtmlIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,13 @@
*/
package org.fcrepo.integration.http.api;

import static org.apache.commons.lang.StringUtils.contains;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

import org.apache.http.HttpResponse;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.util.EntityUtils;
import org.junit.Test;

/**
Expand Down Expand Up @@ -60,4 +64,17 @@ public void testGetDatastreamNode() throws Exception {
method.addHeader("Accept", "text/html");
assertEquals(200, getStatus(method));
}

@Test
public void testGetTemplate() throws Exception {
final String pid = getRandomUniquePid();
createObject(pid);
addMixin(pid, "http://fedora.info/definitions/v4/rest-api#resource");

final HttpGet method = new HttpGet(serverAddress + pid);
method.addHeader("Accept", "text/html");
final HttpResponse response = execute(method);
final String html = EntityUtils.toString(response.getEntity());
assertTrue(contains(html, "class=\"nt_folder\""));
}
}
41 changes: 39 additions & 2 deletions ...o-http-commons/src/main/java/org/fcrepo/http/commons/responses/RdfSerializationUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,25 @@
*/
package org.fcrepo.http.commons.responses;

import static com.google.common.collect.ImmutableList.copyOf;
import static com.google.common.collect.Iterables.transform;
import static com.hp.hpl.jena.graph.NodeFactory.createURI;
import static com.hp.hpl.jena.rdf.model.ResourceFactory.createProperty;
import static com.hp.hpl.jena.rdf.model.ResourceFactory.createResource;
import static org.fcrepo.kernel.RdfLexicon.JCR_NAMESPACE;
import static org.fcrepo.kernel.impl.rdf.JcrRdfTools.getRDFNamespaceForJcrNamespace;
import static org.slf4j.LoggerFactory.getLogger;

import java.util.Iterator;

import com.google.common.base.Function;
import com.google.common.collect.ImmutableList;
import com.hp.hpl.jena.graph.Node;
import com.hp.hpl.jena.rdf.model.Model;
import com.hp.hpl.jena.rdf.model.NodeIterator;
import com.hp.hpl.jena.rdf.model.RDFNode;
import org.slf4j.Logger;

import com.hp.hpl.jena.graph.Node;

/**
* Utilities to help with serializing a graph to an HTTP resource
*
Expand All @@ -50,6 +56,18 @@ private RdfSerializationUtils() {
createURI(getRDFNamespaceForJcrNamespace(JCR_NAMESPACE) +
"primaryType");

/**
* The RDF predicate that will indicate the mixin types.
*/
public static Node mixinTypesPredicate =
createURI(getRDFNamespaceForJcrNamespace(JCR_NAMESPACE) +
"mixinTypes");

public static final Function<RDFNode, String> stringConverter = new Function<RDFNode, String>() {
Copy link

Choose a reason for hiding this comment

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

This function should be private.

public String apply(final RDFNode statement) {
return statement.asLiteral().getLexicalForm();
}
};

/**
* Get the very first value for a predicate as a string, or null if the
Expand All @@ -72,4 +90,23 @@ public static String getFirstValueForPredicate(final Model rdf,
return null;
}

/**
* Get all the values for a predicate as a string array, or null if the
* predicate is not used
*
* @param rdf
* @param subject
* @param predicate
* @return first value for the given predicate or null if not found
Copy link

Choose a reason for hiding this comment

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

Documentation should read:
@return all values ...

*/
public static Iterator<String> getAllValuesForPredicate(final Model rdf,
final Node subject, final Node predicate) {
final NodeIterator statements =
Copy link

Choose a reason for hiding this comment

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

As a point of readability/clarity, this NodeIterator is actually an iterator of "objects" not "statements".
It may be more clear to name the variable accordingly.

rdf.listObjectsOfProperty(createResource(subject.getURI()),
createProperty(predicate.getURI()));

final ImmutableList<RDFNode> copy = copyOf(statements);
return transform(copy, stringConverter).iterator();
}

}
20 changes: 19 additions & 1 deletion ...tp-commons/src/test/java/org/fcrepo/http/commons/responses/RdfSerializationUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,18 @@
*/
package org.fcrepo.http.commons.responses;

import static com.google.common.collect.ImmutableList.copyOf;
import static com.google.common.collect.ImmutableList.of;
import static com.hp.hpl.jena.graph.NodeFactory.createURI;
import static com.hp.hpl.jena.rdf.model.ModelFactory.createDefaultModel;
import static com.hp.hpl.jena.rdf.model.ResourceFactory.createProperty;
import static com.hp.hpl.jena.rdf.model.ResourceFactory.createResource;
import static com.hp.hpl.jena.rdf.model.ResourceFactory.createTypedLiteral;
import static org.fcrepo.http.commons.responses.RdfSerializationUtils.getFirstValueForPredicate;
import static org.fcrepo.http.commons.responses.RdfSerializationUtils.getAllValuesForPredicate;
import static org.junit.Assert.assertEquals;

import java.util.Iterator;
import java.util.ArrayList;
import java.util.List;

Expand Down Expand Up @@ -52,7 +56,12 @@ public void setup() {
testData.add(createResource("test:subject"),
createProperty("test:predicate"),
createTypedLiteral("test:object"));

testData.add(createResource("test:subject"),
createProperty("test:anotherPredicate"),
createTypedLiteral("test:object1"));
testData.add(createResource("test:subject"),
createProperty("test:anotherPredicate"),
createTypedLiteral("test:object2"));
final List<PathSegment> segments = new ArrayList<>();
segment = Mockito.mock(PathSegment.class);
segments.add(segment);
Expand All @@ -66,4 +75,13 @@ public void testGetFirstValueForPredicate() {
assertEquals("Didn't find correct value for predicate!", "test:object", foundValue);
}

@Test
public void testGetAllValuesForPredicate() {
final Iterator<String> foundValues =
getAllValuesForPredicate(testData, createURI("test:subject"),
createURI("test:anotherPredicate"));
assertEquals("Didn't find correct values for predicate!", copyOf(foundValues),
of("test:object1", "test:object2"));
}

}