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
move modeshape-specific code out of the kernel-api #882
Closed
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
0b9f431
move modeshape-specific code out of the kernel-api
acoburn e45e2cc
Further squeezing of MODE out of the kernel API
ajs6f 98c5d0f
simplify implementation based on code comments
acoburn d5f1eca
remove fcrepo-jms dependency on fcrepo-kernel-modeshape
acoburn 3d2c5c1
move JCR/Modeshape out of FedoraType interface
acoburn File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 howRdfStream::getTriples
is used (See related issue at https://jira.duraspace.org/browse/FCREPO-1312). That is, a class that callsRdfStream::getTriples
must pass in a class from theorg.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 oforg.fcrepo.kernel.api
makes perfect sense to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:Let me further suggest. along the lines of @acoburn 's argument, that this module actually be renamed
fcrepo-auth-common-modeshape
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find . -name 'pom.xml' -exec grep -H "fcrepo-kernel-modeshape" "{}" \;
results in:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fcrepo-jms
has a test-dependency, which doesn't matter. It would be expected thatfcrepo-webapp
depends on the modeshape impl. Bothfcrepo-connector-file
andfcrepo-auth-common
have a special relationship to modeshape and should remain as they are. The only concerning dependencies arefcrepo-http-api
andfcrepo-http-commons
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I believe that fcrepo-audit depends on fcrepo-kernel-modeshape (in the same way fcrepo-http-api does).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that a test dependency?