move modeshape-specific code out of the kernel-api #882

acoburn · 2015-08-12T14:27:32Z

Related to https://jira.duraspace.org/browse/FCREPO-1694

ajs6f · 2015-08-12T14:29:22Z

This is good, but we need to go even further and get the JCR stuff out of the API code. But this is a good start.

acoburn · 2015-08-12T14:30:02Z

We totally need to go further, this is just a start...

ajs6f · 2015-08-12T14:32:34Z

I think I will see if I can send a PR to this going a bit further. Maybe we can ping-pong it a bit. If not I will just merge this by itself.

awoods · 2015-09-10T02:00:20Z

...el-modeshape/src/main/java/org/fcrepo/kernel/modeshape/rdf/converters/PropertyConverter.java

@@ -94,7 +94,10 @@ public static String getPropertyNameFromPredicate(final Node node,
                                                      final Resource predicate,
                                                      final Map<String, String> namespaceMapping)
            throws RepositoryException {
-        final NamespaceRegistry namespaceRegistry = getNamespaceRegistry.apply(node);
+
+        final NamespaceRegistry namespaceRegistry = uncheck((final Node n) ->


Was there are reason for not just using NamespaceTools.getNamespaceRegistry(n)?
https://github.com/fcrepo4/fcrepo4/pull/882/files#diff-a456c1fc2649a798904fe05c3ae2e5fdR46

acoburn · 2015-11-06T14:28:58Z

This addresses the subtask: https://jira.duraspace.org/browse/FCREPO-1824

Now, the only remaining trace of Modeshape is in the o.f.k.a.services.RepositoryService class. The methods backupRepository and restoreRepository each return a org.modeshape.jcr.api.Problems object. I would like feedback on what to do with that.

awoods · 2015-11-10T21:26:48Z

fcrepo-auth-common/src/main/java/org/fcrepo/auth/common/FedoraUserSecurityContext.java

@@ -15,7 +15,7 @@
 */
 package org.fcrepo.auth.common;

-import static org.fcrepo.kernel.api.FedoraJcrTypes.JCR_CONTENT;
+import static org.modeshape.jcr.api.JcrConstants.JCR_CONTENT;


The initial intent of this work was to remove dependencies from other modules on the "fcrepo-kernel-modeshape" module. This particular refactoring seems to be moving in the opposite direction.

I disagree. The core issue is this: isolate all modeshape-related code in the fcrepo-kernel-modeshape module. And in order to achieve this, there are two primary issues to resolve.

First, this means removing all modeshape-specific code from fcrepo-kernel-api where it most certainly does not belong. That is the primary goal of this particular PR. And IMO it is the right place to begin.

The second part of the issue relates to removing specific dependencies on fcrepo-kernel-modeshape. The central issue here relates to how RdfStream::getTriples is used (See related issue at https://jira.duraspace.org/browse/FCREPO-1312). That is, a class that calls RdfStream::getTriples must pass in a class from the org.fcrepo.kernel.modeshape.rdf.impl package. See, for example here: http://git.io/v8DUq, where the fcrepo-http-api module is completely tied to the fcrepo-kernel-modeshape module. That sort of interdependence will need to change; but it should be a separate effort (separate from this PR).

In this particular case: fcrepo-auth-common is already entirely bound to modeshape, so using a JCR-related constant from org.modeshape.jcr.api instead of org.fcrepo.kernel.api makes perfect sense to me.

I agree with @acoburn on this, and I ask him to please create a ticket for that refactoring to get dependence on org.fcrepo.kernel.modeshape.rdf.impl out of the kernel. That was a gross oversight for which I think I can mostly be blamed. @cbeer wrote that, but I convinced him that it was a good idea. It was a good idea, but not good enough. We need to completely rethink the RDF generation subsystem in light of these things:

The new Java 8 Streams API

Our desire to refactor JCR into invisibility.

Our need to produce a specification that can be reimplemented in a practical way.

Let me further suggest. along the lines of @acoburn 's argument, that this module actually be renamed fcrepo-auth-common-modeshape.

The argument here makes sense. Can you also describe how this current PR moves us in the direction of the "parent" ticket:
https://jira.duraspace.org/browse/FCREPO-1694 : "Ensure that no non-deployable module compile-depends on fcrepo-kernel-modeshape"
...or maybe the intent/title of the above ticket is simply misleading.

It is confused. I had forgotten about the relationship between authN and MODE. We can alter the title to be more precise and therefore accurate. Did I miss anything other than authN?

find . -name 'pom.xml' -exec grep -H "fcrepo-kernel-modeshape" "{}" \;
results in:

./fcrepo-kernel-modeshape/pom.xml: <artifactId>fcrepo-kernel-modeshape</artifactId> ./fcrepo-connector-file/pom.xml: <artifactId>fcrepo-kernel-modeshape</artifactId> ./fcrepo-auth-common/pom.xml: <artifactId>fcrepo-kernel-modeshape</artifactId> ./fcrepo-http-api/pom.xml: <artifactId>fcrepo-kernel-modeshape</artifactId> ./fcrepo-http-api/pom.xml: <artifactId>fcrepo-kernel-modeshape</artifactId> ./pom.xml: <module>fcrepo-kernel-modeshape</module> ./fcrepo-webapp/pom.xml: <artifactId>fcrepo-kernel-modeshape</artifactId> ./fcrepo-http-commons/pom.xml: <artifactId>fcrepo-kernel-modeshape</artifactId> ./fcrepo-boms/fcrepo4-bom/pom.xml: <artifactId>fcrepo-kernel-modeshape</artifactId> ./fcrepo-jms/pom.xml: <artifactId>fcrepo-kernel-modeshape</artifactId>

fcrepo-jms has a test-dependency, which doesn't matter. It would be expected that fcrepo-webapp depends on the modeshape impl. Both fcrepo-connector-file and fcrepo-auth-common have a special relationship to modeshape and should remain as they are. The only concerning dependencies are fcrepo-http-api and fcrepo-http-commons.

Also, I believe that fcrepo-audit depends on fcrepo-kernel-modeshape (in the same way fcrepo-http-api does).

Is that a test dependency?

awoods · 2015-11-10T22:52:18Z

Regarding: #882 (comment)
We should probably change the backup/restore feature to transact in RDF instead of JCR/XML... that would likely resolve the ModeShape dependency in the code and for clients.

awoods · 2015-11-11T02:02:15Z

Resolved with:
f04635c and
42e6fe4

ajs6f · 2015-11-11T13:52:47Z

That is repo-wide backup/restore?

awoods · 2015-11-11T14:13:56Z

yes

ajs6f · 2015-11-11T14:15:20Z

We'll need more than RDF, right? The bitstreams need to come with. I hate to keep banging the same drum, but maybe we should look at @cwilper 's Semantic Packages again.

awoods · 2015-11-11T14:23:27Z

I think both options need to be available:

RDF and binaries
RDF only

I suspect the community would be more attracted to BagIt. How do you view the comparison between BagIt and SCP?

ajs6f · 2015-11-11T14:26:38Z

I view BagIt as a second-class choice. There's no instant translation of RDF into BagIt and the client libraries are awful. That having been said, Semantic Packages were never implemented, so there would be extra work there. It might be better to think of another choice-- RDF with links to binaries, instead of trying to package everything together.

escowles · 2015-11-11T14:31:24Z

For better or worse, BagIt is what everybody uses (that I've heard detail what they use, anyway). I think the mapping should be pretty easy: the RDF would be an extra tag file, the child nodes and binaries would be the payload, and the binary descriptions would be converted to sidecar RDF files.

ajs6f · 2015-11-11T14:34:16Z

Right, the idea of sidecar RDF files isn't very attractive, and I don't see that pushing the RDF into a tag file actually works all that well. But to be honest, I'm not going to do this work and I don't think that the "whole repo" backup is all that important anyway, so whatever gets the box checked most quickly is fine with me.

acoburn · 2015-11-11T14:38:22Z

Just to be clear, the question of BagIt vs. SCP vs. something else is actually orthogonal to the question of modeshape dependencies in the kernel-api. The problem with the org.fcrepo.kernel.api.services.RepositoryService class is that the backup and restore methods return a org.modeshape.jcr.api.Problems object. This object is effectively the same as a Collection<Throwable>, and by writing a small shim in the RepositoryServiceImpl class, the modeshape-specific class could be removed from the kernel-api. Does that sound reasonable?

ajs6f · 2015-11-11T14:39:50Z

Yes, this was a tangent. +1 to shimming over Problems.

DiegoPino · 2015-11-11T14:45:38Z

Sorry for crushing in here. But I have been doing some research related to this stuff for islandora. I found this people, doing https://researchobject.github.io/specifications/bundle/ looks more semantic ready than adapting bagIT

Their approach is very aligned to LDP and have even some tools. More over, they have very similar approaches on many things to what you are doing.

http://www.researchobject.org/specifications/

Just a contribution (maybe just adding noise to the conversation),but i really don't die for BagIT nor for a BagIT semantic monster.

awoods · 2015-11-11T14:51:49Z

@acoburn 👍 for shimmy-shimmy

ajs6f · 2015-11-11T14:53:19Z

@awoods Do we have a separate issue available to track discussion about the backup/restore serialization?

awoods · 2015-11-11T15:01:14Z

@ajs6f, yes: #946
Also, @DiegoPino, thanks for you link to RO-Bundle. It appears to be BagIt-meets-RDF. I obviously have not gone too deeply into it, but one concern would be around any potential restrictions in namespaces, predicates, and mandatory properties or structure that F4 may have a hard time supporting.
In any case, let's continue the discussion on this issue: #946

ajs6f · 2015-11-11T15:02:54Z

@DiegoPino Please put a link to RO-Bundle in #946.

DiegoPino · 2015-11-11T15:19:42Z

@ajs6f and @awoods: done in #946. Thanks

awoods reviewed Sep 10, 2015
View reviewed changes

acoburn and others added 5 commits November 6, 2015 07:32

move modeshape-specific code out of the kernel-api

0b9f431

Further squeezing of MODE out of the kernel API

e45e2cc

simplify implementation based on code comments

98c5d0f

remove fcrepo-jms dependency on fcrepo-kernel-modeshape

d5f1eca

move JCR/Modeshape out of FedoraType interface

3d2c5c1

awoods reviewed Nov 10, 2015
View reviewed changes

This was referenced Nov 10, 2015

rename FedoraTypes class name based on upstream changes fcrepo4-labs/fcrepo4-oaiprovider#28

Merged

package changes based on upstream changes fcrepo4-labs/fcrepo4-swordserver#14

Merged

awoods closed this Nov 11, 2015

acoburn deleted the fcrepo-1694 branch November 11, 2015 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move modeshape-specific code out of the kernel-api #882

move modeshape-specific code out of the kernel-api #882

acoburn commented Aug 12, 2015

ajs6f commented Aug 12, 2015

acoburn commented Aug 12, 2015

ajs6f commented Aug 12, 2015

awoods Sep 10, 2015

acoburn commented Nov 6, 2015

awoods Nov 10, 2015

acoburn Nov 10, 2015

ajs6f Nov 10, 2015

awoods Nov 10, 2015

ajs6f Nov 10, 2015

awoods Nov 10, 2015

acoburn Nov 10, 2015

acoburn Nov 10, 2015

ajs6f Nov 11, 2015

awoods commented Nov 10, 2015

awoods commented Nov 11, 2015

ajs6f commented Nov 11, 2015

awoods commented Nov 11, 2015

ajs6f commented Nov 11, 2015

awoods commented Nov 11, 2015

ajs6f commented Nov 11, 2015

escowles commented Nov 11, 2015

ajs6f commented Nov 11, 2015

acoburn commented Nov 11, 2015

ajs6f commented Nov 11, 2015

DiegoPino commented Nov 11, 2015

awoods commented Nov 11, 2015

ajs6f commented Nov 11, 2015

awoods commented Nov 11, 2015

ajs6f commented Nov 11, 2015

DiegoPino commented Nov 11, 2015

move modeshape-specific code out of the kernel-api #882

move modeshape-specific code out of the kernel-api #882

Conversation

acoburn commented Aug 12, 2015

ajs6f commented Aug 12, 2015

acoburn commented Aug 12, 2015

ajs6f commented Aug 12, 2015

Choose a reason for hiding this comment

acoburn commented Nov 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awoods commented Nov 10, 2015

awoods commented Nov 11, 2015

ajs6f commented Nov 11, 2015

awoods commented Nov 11, 2015

ajs6f commented Nov 11, 2015

awoods commented Nov 11, 2015

ajs6f commented Nov 11, 2015

escowles commented Nov 11, 2015

ajs6f commented Nov 11, 2015

acoburn commented Nov 11, 2015

ajs6f commented Nov 11, 2015

DiegoPino commented Nov 11, 2015

awoods commented Nov 11, 2015

ajs6f commented Nov 11, 2015

awoods commented Nov 11, 2015

ajs6f commented Nov 11, 2015

DiegoPino commented Nov 11, 2015