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.

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

Adding appropriate inbound triples based on LDP containment properties #842

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
3 participants
@escowles
Copy link
Contributor

commented Jul 16, 2015

  • adding IT to cover typical PCDM use case of proxies

Fixes https://jira.duraspace.org/browse/FCREPO-1497

escowles added some commits Jul 16, 2015

Adding appropriate inbound triples based on LDP containment propertie…
…s, adding IT to cover typical PCDM use case of proxies
final HttpGet getContainer = new HttpGet(serverAddress + pid1 + "/members");
getContainer.addHeader("Prefer", "return=representation;include=\"http://www.w3.org/ns/ldp#PreferMembership\"");
getContainer.addHeader("Accept", "application/n-triples");
final GraphStore containerGraph = getGraphStore(getContainer);

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

Use a CloseableGraphStore with try-with-resource here.

// retrieve the parent and verify the outbound triples exist
final HttpGet getParent = new HttpGet(serverAddress + pid1);
getParent.addHeader("Accept", "application/n-triples");
final GraphStore parentGraph = getGraphStore(getParent);

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

Use a CloseableGraphStore with try-with-resource here.

final String uuid = getRandomUniqueId();
final String pid1 = uuid + "/parent";
final String pid2 = uuid + "/child";
createObject(pid1);

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

Use createObjectAndClose() if you don't need to keep the response open.

final HttpGet getMember = new HttpGet(serverAddress + pid2);
getMember.addHeader("Prefer", "return=representation; include=\"" + INBOUND_REFERENCES.toString() + "\"");
getMember.addHeader("Accept", "application/n-triples");
final GraphStore memberGraph = getGraphStore(getMember);

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

Use a CloseableGraphStore with try-with-resource here.

.convert(input);
return create(subject(), memberRelation, membershipResource.asNode());
try {
final RDFNode membershipResource = new ValueConverter(resource().getNode().getSession(),

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

Why is the session in the transformation not the right one here?

This comment has been minimized.

Copy link
@awoods

awoods Jul 17, 2015

Member

I suspect the session() is not getting set in the first place. Is there danger in updating:
https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-kernel-impl/src/main/java/org/fcrepo/kernel/impl/rdf/impl/NodeRdfContext.java#L52
...to pass in to super() resource.getNode().getSession()?

This comment has been minimized.

Copy link
@escowles

escowles Jul 17, 2015

Author Contributor

That seems like a good suggestion to me -- I don't see the session getting set anywhere, and doing it in the NodeRdfContext would fix this awkward workaround, and might let us clean up similar workarounds elsewhere.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

I dimly, dimly remember this being the intent for that field; that it would be a convenient handle for the Session in context without constantly dealing with JCR's annoying RepositoryException addiction.

private void putReferencesIntoContext(final Iterator<Property> properties) throws RepositoryException {
while (properties.hasNext()) {
final Property p = properties.next();
concat(property2triple.apply(p));

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

This is going to result in many more object creations. It's really not great to concat one triple at a time if you can possibly handle them in bunches.


return Iterators.concat(Iterators.transform(properties, property2triple));
for ( final PropertyIterator it = p.getParent().getProperties(); it.hasNext(); ) {
final Property potentialProxy = it.nextProperty();

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

Why not leave this inside the for?

return Iterators.concat(Iterators.transform(properties, property2triple));

private void putProxyReferencesIntoContext(final Value v) throws RepositoryException {
if (v.getType() == PATH) {

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

Don't we have dedicated code for this kind of dereferencing?

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

Do we have the work that @cbeer did to set up that reference-property scheme documented somewhere? I have to admit I don't really understand it.

This comment has been minimized.

Copy link
@awoods

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

AWESOME SAUCE.

@@ -48,21 +57,36 @@ public ReferencesRdfContext(final FedoraResource resource,
throws RepositoryException {
super(resource, idTranslator);
property2triple = new PropertyToTriple(resource.getNode().getSession(), idTranslator);
concat(putStrongReferencePropertiesIntoContext());
concat(putWeakReferencePropertiesIntoContext());
session = resource.getNode().getSession();

This comment has been minimized.

Copy link
@awoods

awoods Jul 17, 2015

Member

The grandparent class of ReferencesRdfContext already has a Session data member:
https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-kernel/src/main/java/org/fcrepo/kernel/utils/iterators/RdfStream.java#L52

It is probably not set in this scenario either.
The comment above about passing in a Session at:
https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-kernel-impl/src/main/java/org/fcrepo/kernel/impl/rdf/impl/NodeRdfContext.java#L52
...could also resolve the issue of needing to produce a local session in this class.

private Iterator<Triple> putWeakReferencePropertiesIntoContext() throws RepositoryException {
final Iterator<Property> properties = resource().getNode().getWeakReferences();
private void putReferencesIntoContext(final Iterator<Property> properties) throws RepositoryException {
while (properties.hasNext()) {

This comment has been minimized.

Copy link
@awoods

awoods Jul 17, 2015

Member

This approach seems to lose the previous advantage of generating triples on-demand, since the update runs through the iterators during object creation.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

@awoods is dead right here. Stay lazy, stay happy.

This comment has been minimized.

Copy link
@escowles

escowles Jul 17, 2015

Author Contributor

I think the key thing here is to convert the logic to a Function or something similar. But there are several places where there could be nothing, or there could be several references, it might be easier if it was just calling concat() instead of returning a triple iterator. Is there something like Iterators.transform() that just calls a Callable instance?

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

Yes, you want to ensconce your actual computation in an abstraction that can remain on the heap-- a function. Are you saying that there are times when your computation, given a Property, may not return a triple? May just say, "Nothing to do here-- next Property, please!" or that it might return several triples?

This comment has been minimized.

Copy link
@escowles

escowles Jul 17, 2015

Author Contributor

Yes, basically it's iterating over the properties of the nodes that refer to it. For each of them, it might return nothing if the reference isn't from a proxy, or it might return several triples if that proxy is in several containers.

Now that I put it that way, it sounds like I need to break down the logic into a chain of filtering and transforming steps.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

Ok, no problem. Look at what is done in PropertiesRdfContext. The function mapped over an iteration of properties there goes from a property to an iterator of triples. (This is just flatmapping.) So if there are multiple triples from a property, no problem. If there is only one, no problem (iterator of one). And if there are none, no problem (empty iterator). This is a handy trick for this kind of situation-- rely on the fact that the "optional" monad is an example of a "collectionish" monad, or to put it another way, every collection type (including iterators here) has a clear choice of null object: the empty collection.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

Yes, chains of filtering, transforming, and concat'ing steps is the native idiom for this RDF context stuff. Think of the agglomeration of all these "context" classes as representing the transformation of JCR contents into RDF, not the process of running that transformation, just the transformation itself. Hopefully, @acoburn will get around to making it all readable by using Java 8 idioms. :)


return Iterators.concat(Iterators.transform(properties, property2triple));
for ( final PropertyIterator it = p.getParent().getProperties(); it.hasNext(); ) {

This comment has been minimized.

Copy link
@awoods

awoods Jul 17, 2015

Member

Minor note: some inline comments would be helpful.

}
}
private void putProxyReferencesIntoContext(final Node n) throws RepositoryException {
concat(new LdpContainerRdfContext(nodeConverter.convert(n), translator()));

This comment has been minimized.

Copy link
@awoods

awoods Jul 17, 2015

Member

Performing manual testing (see comment in ticket: https://jira.duraspace.org/browse/FCREPO-1497) I was not able to demonstrate the desired behavior.
That fact withstanding, I am vaguely concerned about potentially adding "LdpContainerRdfContexts" for all references coming off of incoming-reference nodes... but will check more deeply once I can see the desired behavior of this ticket.

@@ -203,8 +203,8 @@ public FedoraResourceTripleFunction(final String insertedContainerProperty,
return Iterators.transform(values, new Function<Value, Triple>() {
@Override
public Triple apply(final Value input) {
final RDFNode membershipResource = new ValueConverter(session(), translator())
.convert(input);
final RDFNode membershipResource = new ValueConverter(session(),

This comment has been minimized.

Copy link
@awoods

awoods Jul 20, 2015

Member

I do not see a change here. Please rollback this extraneous update.

private Function<Property, Iterator<Value>> potentialProxies = new Function<Property, Iterator<Value>>() {
@Override
public Iterator<Value> apply(final Property p) {
final Set<Value> values = new HashSet<Value>();

This comment has been minimized.

Copy link
@awoods

awoods Jul 20, 2015

Member

Minor: Change to:
final Set<Value> values = new HashSet<>();

public Iterator<Value> apply(final Property p) {
final Set<Value> values = new HashSet<Value>();
try {
for ( final PropertyIterator it = p.getParent().getProperties(); it.hasNext(); ) {

This comment has been minimized.

Copy link
@awoods

awoods Jul 20, 2015

Member

This for block seems to be duplicating the capabilities found in the PropertyValueIterator.
https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-kernel-impl/src/main/java/org/fcrepo/kernel/impl/rdf/impl/mappings/PropertyValueIterator.java

This whole apply method can probably be replaced by:

@Override
public Iterator<Value> apply(final Property p) {
   try {
      return Iterators.filter(new PropertyValueIterator(p.getParent().getProperties()), isReference);
   } catch (RepositoryException e) {
      throw new RepositoryRuntimeException(e);
   }
}
@awoods

This comment has been minimized.

Copy link
Member

commented Jul 20, 2015

Resolved with: 8845a22

@awoods awoods closed this Jul 20, 2015

@awoods awoods deleted the fcrepo-1497-indirect-container branch Jul 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.