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

Lessen leakage of JCR dependent code that extends beyond the kernel. #783

Merged
merged 3 commits into from Apr 27, 2015

Conversation

awoods
Copy link

@awoods awoods commented Apr 24, 2015

<dependency>
<groupId>org.modeshape</groupId>
<artifactId>modeshape-jcr</artifactId>
<scope>provided</scope>
Copy link
Contributor

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...)?

Copy link
Author

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.

Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Contributor

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 **
@awoods
Copy link
Author

awoods commented Apr 24, 2015

As noted in the second commit,
** Please do not commit/merge this PR without first squashing these commits **

return e.getPath().substring(0, e.getPath().lastIndexOf("/"));
public static String getPath(final Event e) {
try {
if (e.getType() == PROPERTY_ADDED ||
Copy link
Contributor

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().

Copy link
Author

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.

Copy link
Contributor

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?

@ajs6f ajs6f self-assigned this Apr 27, 2015
ajs6f added a commit that referenced this pull request Apr 27, 2015
Lessen leakage of JCR dependent code that extends beyond the kernel.

Resolves https://jira.duraspace.org/browse/FCREPO-1487.
@ajs6f ajs6f merged commit b379030 into fcrepo:master Apr 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants