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

BNodes are BNodes and should be treated as such #748

Closed
wants to merge 21 commits into from

Conversation

Projects
None yet
2 participants
@ajs6f
Copy link
Contributor

commented Mar 13, 2015

DO NOT MERGE YET

@ajs6f ajs6f force-pushed the BNodesAreBNodes branch 4 times, most recently from bb4129c to 86dca3a Mar 13, 2015


/**
* @param idTranslator
* @param context

This comment has been minimized.

Copy link
@awoods

awoods Mar 18, 2015

Member

Please change context to model
...and add meaningful descriptions.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Mar 18, 2015

Author Contributor

"Meaning descriptions"?


private boolean isSkolem(final RDFNode n) {
return n.isURIResource() &&
(n.asResource().getURI().indexOf('?') == -1) &&

This comment has been minimized.

Copy link
@awoods

awoods Mar 18, 2015

Member

Please add comments detailing the "why" of these criteria.

(n.asResource().getURI().indexOf('?') == -1) &&
idTranslator.inDomain(n.asResource()) &&
!idTranslator.asString(n.asResource()).contains("/fcr:") &&
idTranslator.convert(n.asResource()).hasType("fedora:Skolem");

This comment has been minimized.

Copy link
@awoods

awoods Mar 18, 2015

Member

Please use FedoraJcrTypes.FEDORA_SKOLEMNODE instead of the String literal.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Mar 18, 2015

Author Contributor

Good catch.

if (!hashAndSkolemCreator.exists(getSession(), skolemPath)) {
LOGGER.debug("Creating skolem node at: {}", skolemPath);
try {
hashAndSkolemCreator.findOrCreate(getSession(), skolemPath).getNode().addMixin("fedora:Skolem");

This comment has been minimized.

Copy link
@awoods

awoods Mar 18, 2015

Member

Please use FedoraJcrTypes.FEDORA_SKOLEMNODE instead of the String literal.

LOGGER.debug("under path: {}", path);
if (!skolemAndHashCreator.exists(session, path)) {
LOGGER.debug("Creating skolem node at: {}", path);
skolemAndHashCreator.findOrCreate(session, path).getNode().addMixin("fedora:Skolem");

This comment has been minimized.

Copy link
@awoods

awoods Mar 18, 2015

Member

Please use FedoraJcrTypes.FEDORA_SKOLEMNODE instead of the String literal.


@Before
public void setUp() {
when(mockSkolemFedoraResource.hasType("fedora:Skolem")).thenReturn(true);

This comment has been minimized.

Copy link
@awoods

awoods Mar 18, 2015

Member

Please use FedoraJcrTypes.FEDORA_SKOLEMNODE instead of the String literal.

@Before
public void setUp() {
when(mockSkolemFedoraResource.hasType("fedora:Skolem")).thenReturn(true);
when(mockNonSkolemFedoraResource.hasType("fedora:Skolem")).thenReturn(false);

This comment has been minimized.

Copy link
@awoods

awoods Mar 18, 2015

Member

Please use FedoraJcrTypes.FEDORA_SKOLEMNODE instead of the String literal.

when(mockIdTranslator.inDomain(any(Resource.class))).thenReturn(true);
when(mockIdTranslator.asString(any(Resource.class))).thenReturn("non-fcr-URI");
when(mockIdTranslator.convert(any(Resource.class))).thenReturn(mockSkolemFedoraResource);
when(mockSkolemFedoraResource.hasType("fedora:Skolem")).thenReturn(false);

This comment has been minimized.

Copy link
@awoods

awoods Mar 18, 2015

Member

Please use FedoraJcrTypes.FEDORA_SKOLEMNODE instead of the String literal.

@@ -578,7 +584,8 @@ public boolean apply(final javax.jcr.Property property) {

final javax.jcr.Node skolemizedNode = session.getNodeByIdentifier(values[0].getString());

assertTrue(skolemizedNode.getPath().contains("/.well-known/genid/"));
assertTrue(Lists.transform(asList(skolemizedNode.getMixinNodeTypes()), toStringFunction()).contains(
"fedora:Skolem"));

This comment has been minimized.

Copy link
@awoods

awoods Mar 18, 2015

Member

Please use FedoraJcrTypes.FEDORA_SKOLEMNODE instead of the String literal.


private final RDFVisitor nodeSkolemizer;

public static final Resource SKOLEM_TYPE = createResource(REPOSITORY_NAMESPACE + "skolem");

This comment has been minimized.

Copy link
@awoods

awoods Mar 18, 2015

Member

Remove unused constant SKOLEM_TYPE.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Mar 18, 2015

Author Contributor

I think you're looking at a previous commit. That's not there anymore.


A. Soroka
The University of Virginia Library

On Mar 18, 2015, at 4:19 PM, Andrew Woods notifications@github.com wrote:

In fcrepo-kernel-impl/src/main/java/org/fcrepo/kernel/impl/rdf/Skolemizer.java:

+/**

  • * Skolemization is abstractly a function from RDF nodes to RDF nodes, but here we implement it, purely for
  • * convenience of operation, as a function from triples to triples. {@link #nodeSkolemizer} represents the
  • * node-to-node function. This class will make two blank nodes with the same identifier into the same skolem resource
  • * because it uses only one prefix for its lifetime and because the mapping from a given blank node to a skolem
  • * resource depends only on that prefix and on a MurmurHash3 of the identifier of the blank node in question. An
  • * instance of this class should be used only with a contextual topic of one single resource and only for one
  • * document's scope of RDF about that resource.
  • * @author ajs6f
  • */
    +public class Skolemizer implements Function<Statement, Statement>, Supplier<Set> {
    +
  • private final RDFVisitor nodeSkolemizer;
  • public static final Resource SKOLEM_TYPE = createResource(REPOSITORY_NAMESPACE + "skolem");

Remove unused constant SKOLEM_TYPE.


Reply to this email directly or view it on GitHub.

@@ -192,7 +193,7 @@ public Triple apply(final Statement input) {
}
servletResponse.addHeader("Vary", "Accept, Range, Accept-Encoding, Accept-Language");

return Response.ok(rdfStream).build();
return ok(rdfStream.withThisContext(rdfStream.transform(new Deskolemizer(idTranslator, null)))).build();

This comment has been minimized.

Copy link
@awoods

awoods Mar 19, 2015

Member

Does the logic here, at the fcrepo-http-api level, imply that blank nodes are not completely handled a level deeper within the kernel... for clients that interact with Fedora objects directly as opposed through HTTP?

This comment has been minimized.

Copy link
@ajs6f

ajs6f Mar 19, 2015

Author Contributor

Hm. Very good point. Let me see if I can move this deeper, out of HTTP. I think it very likely that I can.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Mar 28, 2015

Author Contributor

I'm coming around to see this differently-- I now don't think this stuff is out of place in the HTTP layer. This is on the reasoning that bnodes are scoped entirely to the document in which they appear, and there is no notion of document in our code below the HTTP layer… I think I could make this look better in the HTTP layer, but I really do think now that it belongs here.
I think that clients interacting at a deeper layer need to handle deskolemization themselves, because only those clients themselves will know what the appropriate scope is in which to deskolemize.
If this is to be handled in the kernel, what is the notion of scope in play? What is a "document" in the kernel? We operate only over streams of triples in the kernel…

This comment has been minimized.

Copy link
@awoods

awoods Mar 28, 2015

Member

Thanks for the further thought, @ajs6f. Let's go with your assumption until we have a pressing need to do otherwise.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Mar 28, 2015

Author Contributor

In that case, I will rebase and maybe tighten up how this works in the HTTP layer and we should be able to move forward.

@@ -295,7 +296,9 @@ public RdfStream apply(final FedoraResource child) {
}


return rdfStream;
return rdfStream.withThisContext(rdfStream.transform(new Deskolemizer(idTranslator, resource == null ? null

This comment has been minimized.

Copy link
@awoods

awoods Mar 19, 2015

Member

ditto from above

@ajs6f ajs6f force-pushed the BNodesAreBNodes branch 2 times, most recently from 32341d5 to b702d42 Apr 6, 2015

@ajs6f ajs6f force-pushed the BNodesAreBNodes branch from b702d42 to 44d2343 Apr 13, 2015

@@ -141,6 +146,10 @@

protected abstract String externalPath();

protected Deskolemizer deskolemizer() {
return deskolemizer == null ? deskolemizer = new Deskolemizer(translator(), null) : deskolemizer;

This comment has been minimized.

Copy link
@awoods

awoods Apr 14, 2015

Member

Please unpack this single statement into its intelligible, multi-line parts.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Apr 14, 2015

Author Contributor

You should make such style considerations clear and get them into Checkstyle rules, if you intend to enforce them.

This comment has been minimized.

Copy link
@awoods

awoods Apr 14, 2015

Member

That would be great if we could encode subjective measures of "readability" into checkstyles. I try not to encroach on other's style preferences, but when a style impedes personal readability, I raise the flag.
Thanks for the niggly update.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Apr 14, 2015

Author Contributor

If you are objecting to the ternary operator, then that should be encoded in Checkstyle. If, on the other hand, your qualm is genuinely subjective, it's not a valid criticism.

}

return idTranslator;
return idTranslator == null ? idTranslator =

This comment has been minimized.

Copy link
@awoods

awoods Apr 14, 2015

Member

For the reasons above (and the fact that no logic is being changed here), please revert this update.

*
* @author ajs6f
*/
public class Deskolemizer implements Function<Triple, Triple> {

This comment has been minimized.

Copy link
@awoods

awoods Apr 14, 2015

Member

Shouldn't Deskolemizer implement Deskolemizer?
...which would strongly argue for a different name of the implementing class, e.g. DeskolemizerImpl.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Apr 14, 2015

Author Contributor

Yes, this is an oversight. As far as the name goes, I don't know how much difference it makes. The only reason that there is a Deskolemizer interface is because the workings of this process tangle up into the kernel API, not because we have any reasonable expectation of more than one implementation of deskolemization. There is no clean way to introduce blank nodes, and this code is crap, but it's as good as I think we can expect to do. If you think it clearer, I can certainly change the name here to DeskolemizerImpl.

This comment has been minimized.

Copy link
@awoods

awoods Apr 14, 2015

Member

I would either suggest:

  1. Change the implementation name to DeskolemizerImpl and have it implement Deskolemizer, or
  2. Simply remove the Deskolemizer interface.
*
* @author ajs6f
*/
public interface Deskolemizer extends Function<Triple, Triple> {

This comment has been minimized.

Copy link
@awoods

awoods Apr 14, 2015

Member

...alternatively, this interface could be removed, as it is not used.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Apr 14, 2015

Author Contributor

Yes, I think it's probably cleaner to just remove it, since the type being passed around in the kernel is now Service<Container>.

}
private final Skolemizer skolemizer;

private final Service<Container> skolemAndHashCreator;

This comment has been minimized.

Copy link
@awoods

awoods Apr 14, 2015

Member

Given its type and use of skolemAndHashCreator in this class, I would find it more clear if the variable were named containerService.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Apr 14, 2015

Author Contributor

I disagree. containerService is just a restatement of the type of the param. skolemAndHashCreator actually explains how the param is used. I'd like to hear from someone else.

* to identifiers rooted at this URI.
*/
public Skolemizer(final Resource topic) {
final String prefix = topic + "/" + randomUUID().toString().replace('-', '/');

This comment has been minimized.

Copy link
@awoods

awoods Apr 14, 2015

Member

I have not probed more deeply yet, but I am concerned that Resource.toString() here will create a String that is not compatible with JCR path creation performed in JcrPropertyStatementListener.addedStatement().
https://github.com/apache/jena/blob/master/jena-core/src/main/java/com/hp/hpl/jena/rdf/model/Resource.java#L95-L104

Hopefully this is a non-issue.

@@ -141,6 +146,10 @@

protected abstract String externalPath();

protected Deskolemizer deskolemizer() {

This comment has been minimized.

Copy link
@awoods

awoods Apr 14, 2015

Member

It does not appear that this method is used outside of this class. Is there a reason for not making it private?

This comment has been minimized.

Copy link
@ajs6f

ajs6f Apr 14, 2015

Author Contributor

Sure, why not?

existingAncestorPath.deleteCharAt(existingAncestorPath.length() - 1);
}
break;
String potentialPath = path.startsWith("/") ? path : "/" + path;

This comment has been minimized.

Copy link
@awoods

awoods Apr 14, 2015

Member

I am not sure I see the relevance of this update in this PR... but I guess it is an improvement

@awoods

This comment has been minimized.

Copy link
Member

commented Apr 14, 2015

Resolved with: ceacc98

@awoods awoods closed this Apr 14, 2015

@osmandin osmandin deleted the BNodesAreBNodes branch Aug 28, 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.