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
Lessen leakage of JCR dependent code that extends beyond the kernel. #783
Conversation
<dependency> | ||
<groupId>org.modeshape</groupId> | ||
<artifactId>modeshape-jcr</artifactId> | ||
<scope>provided</scope> |
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'm a little confused about this scope-- is the thought that the container will provide this artifact (which clearly ain't the case...)?
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 issue is that a few existing pom.xml files depend on modeshape-jcr
and only some of them actually define that dependency. Scope provided
ensures that the given dependency does not propagate transitively. However, thanks to you raising this flag, I see that this PR only works because fcrepo-http-commons/pom.xml
is still such a perpetrator of transitively leaking the modeshape-jcr
dependency to the other modules... otherwise, the dependency is never actually provided
by the container (or fcrepo-webapp), which causes build failure.
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.
Right, right. In theory, we want the only transitive dependency to be "pure" JCR API, not impl APIs, right?
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.
Right... and ideally, most of the Fedora modules would not even depend on "pure" JCR. I would like to see as few modules as possible care anything about JCR.
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.
Good point. That is a higher and better ideal. We should factor as much JCR as we can down through the modules to kernel-impl
.
** Please do not commit/merge this PR without first squashing these commits **
As noted in the second commit, |
return e.getPath().substring(0, e.getPath().lastIndexOf("/")); | ||
public static String getPath(final Event e) { | ||
try { | ||
if (e.getType() == PROPERTY_ADDED || |
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 would be better for this test to use a constant collection of PROPERTY_ADDED
, PROPERTY_CHANGED
, PROPERTY_REMOVED
and Collection.contains()
.
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 am going to suggest not including non-related clean-up/refactoring to this PR.
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.
Okay. Maybe write a TODO in here?
Lessen leakage of JCR dependent code that extends beyond the kernel. Resolves https://jira.duraspace.org/browse/FCREPO-1487.
Resolves: https://jira.duraspace.org/browse/FCREPO-1487