Migrate to Jena 3.1.0 #1075

acoburn · 2016-07-25T01:09:49Z

Resolves: https://jira.duraspace.org/browse/FCREPO-1672

This PR is divided into three commits as follows (mostly to make review easier):

Change the package names to org.apache.jena and increment version in pom.xml
Update implementation and test code to support the new version
Rework the ldp testsuite (which depends on Jena 2.x) so that we can avoid dependency convergences.

ajs6f · 2016-07-25T01:43:30Z

Okay, obviously the vast majority of line changes are going to be com.hp.hpl => org.apache. Do you have any suggestions for substantive review? Are there places you remember making particular choices that should be reviewed? I don't think Jena's API actually changed all that much across the period in question, so nothing comes to my mind.

acoburn · 2016-07-25T02:00:56Z

@ajs6f this is why I have three commits here. I'd look particularly at the second one f99a317 -- that's where all of the impl code changes are.

acoburn · 2016-07-25T02:02:58Z

@ajs6f also, in the first commit, there is this change: 890c31f#diff-f8c7031221097a6120fd8c87ded12841L20

GraphStoreWrapper -> GraphStoreBasic (GraphStoreWrapper doesn't seem to exist any longer)

Otherwise, the first commit is just about renaming the packages.

ajs6f · 2016-07-25T16:09:22Z

GraphStore is now deprecated. I'm inclined to take this opportunity to replace it with either Dataset or DatasetGraph (preferably the former).

ajs6f · 2016-07-25T16:11:45Z

So after the third commit, the LDP suite runs against the actual fcrepo-webapp?

ajs6f · 2016-07-25T16:13:26Z

fcrepo-kernel-modeshape/src/main/java/org/fcrepo/kernel/modeshape/FedoraJcrConstants.java

@@ -46,6 +46,8 @@

    public static final String VERSIONABLE = "mix:versionable";

+    public static final String FIELD_DELIMITER = "\30^^\30";


Is this @cbeer 's unspeakable insignium?

acoburn · 2016-07-25T16:59:20Z

@ajs6f yes, the LDP suite runs against fcrepo-webapp

awoods · 2016-07-27T03:52:59Z

fcrepo-kernel-modeshape/src/main/java/org/fcrepo/kernel/modeshape/FedoraBinaryImpl.java

@@ -246,7 +248,7 @@ public URI getContentDigest() {
    public String getMimeType() {
        try {
            if (hasProperty(HAS_MIME_TYPE)) {
-                return getProperty(HAS_MIME_TYPE).getString();
+                return getProperty(HAS_MIME_TYPE).getString().replace(FIELD_DELIMITER + XSDstring.getURI(), "");


Instead of hardcoding the clean-up of the FIELD_DELIMITER suffixes throughout the codebase, it seems cleaner to simply create a small utility in https://github.com/fcrepo4-exts/fcrepo4-upgrade-utils to be executed as part of the 4.6.0 to 4.7.0 upgrade process.

So you are suggesting removing that construct altogether? What would you propose to replace that with?

To me, that seems like a separate issue.

@awoods to be clear, with RDF 1.1, all Literals have a datatype, so the string literals that previously had no datatype now will have a datatype, meaning that when they are persisted to Modeshape, they will always have the FIELD_DELIMITER + xsd:string suffix. Any pre Jena 3.x data will not have that suffix. For RDF output, that's not a problem, but for certain methods (such as getMimeType) you clearly don't want that value (in fact, Jersey will blow up if the mimetype contains that suffix). An alternative would be to use the ValueConverter class here, which is probably a good idea. But again, I think that refactoring goes beyond the simple migration to Jena 3.x of this PR.

I appreciate the explanation, @acoburn. It first pass, it looked like you were just cleaning out the FIELD_DELIMITER cruft from the past.
I would like to give this PR a closer look, but have limited time today. If someone in addition to @ajs6f gives it the thumbs-up, feel free to move it forward. Otherwise, I will aim to give it a closer look tonight.

@awoods I would like to address @ajs6f's comment about the use of GraphStore before this is merged. That will likely happen today.

Thank you for doing that, @acoburn . That GraphStore thing has been bugging me forever.

Resolves: https://jira.duraspace.org/browse/FCREPO-1672

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

* GraphStore to Dataset * createAnon to createBlankNode

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

acoburn · 2016-07-27T17:17:47Z

@ajs6f: I added another commit (e9292b0) replacing GraphStore with Dataset/DatasetGraph. I also replaced the (deprecated) use of createAnon with createBlankNode. I reordered the commits so that the LDP integration suite changes are still the final commit.

ajs6f · 2016-07-27T17:35:38Z

Looks good to me.

acoburn · 2016-07-27T17:38:10Z

@awoods ok, this PR is in your court now. I suspect that it will require additional PRs for the various downstream applications

awoods · 2016-08-24T22:49:20Z

...el-modeshape/src/test/java/org/fcrepo/integration/kernel/modeshape/FedoraResourceImplIT.java

@@ -752,6 +755,8 @@ public void testReplacePropertiesHashURIs() throws RepositoryException {
        final Model updatedModel3 = object.getTriples(subjects, PROPERTIES).collect(toModel());

        updatedModel3.remove(subject, dcCreator, hashResource);
+        updatedModel3.removeAll(hashResource, (Property)null, (RDFNode)null);


This line seemed peculiar, so I tested with it commented out.. with all tests passing.
It appears that the update in this line can be removed/reverted.

I am happy to remove it as a part of this PR, unless there is some reason for this update that I am missing.

Yes, go ahead and remove it

awoods · 2016-08-25T14:17:32Z

Resolved with:
46497f5
a9dd847
336424a
4bfdd25

acoburn mentioned this pull request Jul 25, 2016

Allow deletion of non-string literals via sparql-update #1076

Merged

ajs6f reviewed Jul 25, 2016
View reviewed changes

acoburn mentioned this pull request Jul 26, 2016

Add interfaces for replacement of the existing IdentifierConverter class #1055

Merged

awoods reviewed Jul 27, 2016
View reviewed changes

acoburn added 4 commits July 27, 2016 07:35

Migrate to Jena 3.1

136de74

Resolves: https://jira.duraspace.org/browse/FCREPO-1672

Update code to handle changes with Jena version

3104bf9

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

Stop using deprecated Jena APIs:

e9292b0

* GraphStore to Dataset * createAnon to createBlankNode

Update LDP integration suite

cccbcc1

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

awoods reviewed Aug 24, 2016
View reviewed changes

awoods closed this Aug 25, 2016

acoburn deleted the fcrepo-1672 branch August 25, 2016 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to Jena 3.1.0 #1075

Migrate to Jena 3.1.0 #1075

acoburn commented Jul 25, 2016

ajs6f commented Jul 25, 2016

acoburn commented Jul 25, 2016

acoburn commented Jul 25, 2016

ajs6f commented Jul 25, 2016

ajs6f commented Jul 25, 2016

ajs6f Jul 25, 2016 •

edited

acoburn Jul 25, 2016

acoburn commented Jul 25, 2016

awoods Jul 27, 2016

acoburn Jul 27, 2016

acoburn Jul 27, 2016

awoods Jul 27, 2016

acoburn Jul 27, 2016

ajs6f Jul 27, 2016

acoburn commented Jul 27, 2016

ajs6f commented Jul 27, 2016

acoburn commented Jul 27, 2016

awoods Aug 24, 2016

awoods Aug 24, 2016

acoburn Aug 24, 2016

awoods commented Aug 25, 2016

		@@ -46,6 +46,8 @@

		public static final String VERSIONABLE = "mix:versionable";

		public static final String FIELD_DELIMITER = "\30^^\30";

Migrate to Jena 3.1.0 #1075

Migrate to Jena 3.1.0 #1075

Conversation

acoburn commented Jul 25, 2016

ajs6f commented Jul 25, 2016

acoburn commented Jul 25, 2016

acoburn commented Jul 25, 2016

ajs6f commented Jul 25, 2016

ajs6f commented Jul 25, 2016

ajs6f Jul 25, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acoburn commented Jul 25, 2016

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

acoburn commented Jul 27, 2016

ajs6f commented Jul 27, 2016

acoburn commented Jul 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awoods commented Aug 25, 2016

ajs6f Jul 25, 2016 •

edited