Bug 18713

Summary: Add support for named graphs and rfds:seeAlso to Koha::RDF
Product: Koha Reporter: David Cook <dcook>
Component: CatalogingAssignee: David Cook <dcook>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: brendan, josef.moravec, katrin.fischer, m.de.rooy, magnus, mirko
Version: masterKeywords: dependency
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10787
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18586
Whiteboard:
Change sponsored?: --- Patch complexity: ---
Bot Control: --- When did the bot last check this:
Who signed the patch off: Text to go in the release notes:
Bug Depends on: 18585, 18586    
Bug Blocks: 21359    
Attachments: Bug 18713 - Add support for named graphs and rfds:seeAlso to Koha::RDF
Bug 18713 - Add support for named graphs and rfds:seeAlso to Koha::RDF
Bug 18713 - Add support for named graphs and rfds:seeAlso to Koha::RDF
[SIGNED-OFF] Bug 18713 - Add support for named graphs and rfds:seeAlso to Koha::RDF
Bug 18713 - Add support for named graphs and rfds:seeAlso to Koha::RDF

Description David Cook 2017-06-01 06:28:11 UTC
When I download RDFXML via OAI-PMH, I delete existing named graphs and create new named graphs. This functionality isn't specific to OAI-PMH, so I'm putting it in Koha::RDF.

There is also a method for adding rdfs:seeAlso to a $subject and an $object. In my case, I'm linking a Koha record to an imported record. It's a generic relationship though, which will be useful in many other scenarios, so it's going in Koha::RDF too.
Comment 1 David Cook 2017-06-01 06:48:53 UTC
Created attachment 63894 [details] [review]
Bug 18713 - Add support for named graphs and rfds:seeAlso to Koha::RDF

This patch adds methods to Koha::RDF that creates or deletes a named
graph by removing/adding RDF quads from/to a triplestore.

This patch also can create a rdfs:seeAlso relationship between
two RDF entities.
Comment 2 David Cook 2017-06-01 07:44:24 UTC
Working on unit tests now but have found a bug in RDF::Trine::Store::SPARQL so will have to do a workaround tomorrow for that.
Comment 3 David Cook 2017-06-01 23:15:49 UTC
Actually the workaround might be more inefficient so I'd rather just patch RDF::Trine::Store::SPARQL...
Comment 4 David Cook 2017-06-02 01:48:12 UTC
(In reply to David Cook from comment #3)
> Actually the workaround might be more inefficient so I'd rather just patch
> RDF::Trine::Store::SPARQL...

Well actually... I thought of a different workaround which will be fine for now, and I've sent a pull request with a patch for RDF::Trine::Store::SPARQL (https://github.com/kasei/perlrdf/pull/150). The author of RDF::Trine has already said that he'd welcome a patch for the bug, so that's good. 

For the previous bug I found in RDF::Trine::Store::SPARQL, I sent a pull request 23 days ago, it was merged into master 14 days ago, and a new version of RDF::Trine was posted on CPAN 1 day ago (a dev release was on CPAN on May 19th so 14 days ago). So I imagine that by the time we're at Koha 17.11, the updated version will be on CPAN and be easy to package.
Comment 5 David Cook 2017-06-02 06:17:14 UTC Comment hidden (obsolete)
Comment 6 David Cook 2017-06-02 06:18:30 UTC
Created attachment 63909 [details] [review]
Bug 18713 - Add support for named graphs and rfds:seeAlso to Koha::RDF

This patch adds methods to Koha::RDF that creates or deletes a named
graph by removing/adding RDF quads from/to a triplestore.

This patch also can create a rdfs:seeAlso relationship between
two RDF entities.

This patch includes unit tests for both of these changes.

_TEST PLAN_
1) Run "prove t/Koha/RDF.t"
2) Note that all tests have run successfully.
Comment 7 Josef Moravec 2017-06-06 07:15:41 UTC
Created attachment 64020 [details] [review]
[SIGNED-OFF] Bug 18713 - Add support for named graphs and rfds:seeAlso to Koha::RDF

This patch adds methods to Koha::RDF that creates or deletes a named
graph by removing/adding RDF quads from/to a triplestore.

This patch also can create a rdfs:seeAlso relationship between
two RDF entities.

This patch includes unit tests for both of these changes.

_TEST PLAN_
1) Run "prove t/Koha/RDF.t"
2) Note that all tests have run successfully.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 8 Brendan Gallagher 2017-06-16 17:23:49 UTC
Hmm..  I am getting failed tests on this.  

 #   Failed test '2 statements in model (in default graph and a named graph)'
    #   at t/Koha/RDF.t line 65.
    #          got: '1'
    #     expected: '2'

    #   Failed test '3 statements in model (1 in default graph and 2 in separate named graphs'
    #   at t/Koha/RDF.t line 71.
    #          got: '1'
    #     expected: '3'

    #   Failed test '2 statements in model (1 in default graph and 1 in a named graph) after 1 of the named graphs was deleted'
    #   at t/Koha/RDF.t line 77.
    #          got: '1'
    #     expected: '2'
    # Looks like you failed 3 tests of 3.

Is it me and I missed something?
Comment 9 David Cook 2017-06-18 23:41:19 UTC
(In reply to Brendan Gallagher from comment #8)
> Is it me and I missed something?

While working with RDF::Trine, I've noticed that there are a few bugs. I've been sending patches to the maintainer to fix them as I find them, but it seems that you've encountered a bug in an older version of RDF::Trine which has already been fixed long ago.

I'm operating off of the most recent versions of RDF::Trine. I'm guessing that you're on Debian Jessie which is on version 1.011 (https://packages.debian.org/jessie/librdf-trine-perl). It looks like there was a bug that was fixed in 1.013. After that, the tests pass. 

The current version of RDF::Trine is 1.017 and there is a dev version 1.017_1 which is in testing on http://cpantesters.org/ and will yield a 1.018 in the next few weeks I imagine. 

I've already talked to Mirko and he's happy to package the latest RDF::Trine in the Koha Debian repository. Perhaps I should get him to do that before QA continues on this patch?

If you want to double-check what I'm saying now, you can clone the git repo from https://github.com/kasei/perlrdf, then run something like 'perl -I ~/perlrdf/RDF-Trine/lib t/Koha/RDF.t' and then the tests should pass.
Comment 10 David Cook 2017-06-22 00:54:59 UTC
Hey Brendan, I just noticed that Debian Stretch is the new Debian stable. It looks like it has RDF::Trine 1.015, which should pass these tests.

Alternatively, Mirko has uploaded RDF::Trine 1.018 to Koha unstable (and Jonas Smedegaard has uploaded RDF::Trine 1.018 to Debian unstable):

"I have backported librdf-trine-perl 1.018-1 to the Koha unstable repository. Please use option 1: install the package from our repository and report problems if you encounter any." (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10662)
Comment 11 David Cook 2017-08-03 00:07:36 UTC
Mirko backported the latest version of RDF::Trine to Koha unstable I believe, so it can be installed from there. That should allow this patch to work correctly.
Comment 12 Katrin Fischer 2017-10-15 14:01:59 UTC
Assigning to David as he is the author of the patch.
Comment 13 Josef Moravec 2017-12-08 08:45:33 UTC
Created attachment 69633 [details] [review]
Bug 18713 - Add support for named graphs and rfds:seeAlso to Koha::RDF

This patch adds methods to Koha::RDF that creates or deletes a named
graph by removing/adding RDF quads from/to a triplestore.

This patch also can create a rdfs:seeAlso relationship between
two RDF entities.

This patch includes unit tests for both of these changes.

_TEST PLAN_
1) Run "prove t/Koha/RDF.t"
2) Note that all tests have run successfully.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Marcel de Rooy 2018-03-09 07:40:30 UTC
Test fails. Seems to be reported before. Versioning/packaging ? Needs further attention..

# Subtest: Create and delete named graphs
    1..3
    not ok 1 - 2 statements in model (in default graph and a named graph)
    #   Failed test '2 statements in model (in default graph and a named graph)'
    #   at t/Koha/RDF.t line 65.
    #          got: '1'
    #     expected: '2'
    not ok 2 - 3 statements in model (1 in default graph and 2 in separate named graphs
    #   Failed test '3 statements in model (1 in default graph and 2 in separate named graphs'
    #   at t/Koha/RDF.t line 71.
    #          got: '1'
    #     expected: '3'
    not ok 3 - 2 statements in model (1 in default graph and 1 in a named graph) after 1 of the named graphs was deleted
    #   Failed test '2 statements in model (1 in default graph and 1 in a named graph) after 1 of the named graphs was deleted'
    #   at t/Koha/RDF.t line 77.
    #          got: '1'
    #     expected: '2'
    # Looks like you failed 3 tests of 3.
not ok 4 - Create and delete named graphs
#   Failed test 'Create and delete named graphs'
#   at t/Koha/RDF.t line 78.
Comment 15 David Cook 2018-03-12 00:29:25 UTC
(In reply to Marcel de Rooy from comment #14)
> Test fails. Seems to be reported before. Versioning/packaging ? Needs
> further attention..
> 

What operating system are you using, Marcel, and what version of RDF::Trine? 

Mirko has the required version in the Koha unstable package repo and the particular bug in question making the test fail should be resolved in Debian stable (Stretch) now.
Comment 16 Marcel de Rooy 2018-03-22 08:02:46 UTC
(In reply to David Cook from comment #15)
> (In reply to Marcel de Rooy from comment #14)
> > Test fails. Seems to be reported before. Versioning/packaging ? Needs
> > further attention..
> > 
> 
> What operating system are you using, Marcel, and what version of RDF::Trine? 
> 
> Mirko has the required version in the Koha unstable package repo and the
> particular bug in question making the test fail should be resolved in Debian
> stable (Stretch) now.

Debian Jessie. Do we support Koha on Stretch? No problems when switching to MariaDB?
I have not tested the unstable repo very recently, but last time I tried it did not work (yes unstable).
Comment 17 David Cook 2018-03-22 22:26:43 UTC
(In reply to Marcel de Rooy from comment #16)
>
> Debian Jessie. Do we support Koha on Stretch? No problems when switching to
> MariaDB?
> I have not tested the unstable repo very recently, but last time I tried it
> did not work (yes unstable).

No idea about Koha on Stretch. I heard something about it a while ago but I don't know. 

I'm not touching the database, so MariaDB shouldn't matter.

I suppose it's a bit of a catch-22... you need the dependency for it to pass QA but the dependency won't be added until the patch passes QA?

You could probably just download the package from http://debian.koha-community.org/koha/pool/main/libr/librdf-trine-perl/librdf-trine-perl_1.018-1~koha1_all.deb and try it just for the sake of QA.
Comment 18 Marcel de Rooy 2018-04-06 07:00:59 UTC
(In reply to David Cook from comment #17)
> I suppose it's a bit of a catch-22... you need the dependency for it to pass
> QA but the dependency won't be added until the patch passes QA?

No, that cant be true. I will reset the status.
If another QAer can test on Stretch, he may recommend to add the higher version to the Koha repo.
Comment 19 Mirko Tietgen 2018-04-06 07:18:48 UTC
Not sure if I get it right. Is the test failing because the dependency is not declared in the patch set? I think it should be, which should lead to the dependency being pulled from the unstable repo (in a dev environment).

Also, what version do we actually need?

Stretch has 1.015. Our repo has 1.018, AFAIR because that was the latest unstable available at the time of packaging and stretch was not yet released.
Which version do we actually require for it to work for us? Is 1.015 enough?
Comment 20 Mirko Tietgen 2018-04-06 07:21:10 UTC
(In reply to Marcel de Rooy from comment #18)

> No, that cant be true. I will reset the status.
> If another QAer can test on Stretch, he may recommend to add the higher
> version to the Koha repo.

Stretch is not required to test this. The dependency is in the unstable repository and works with Jessie. I suppose it is just not declared in the patch?
Comment 21 Mirko Tietgen 2018-04-06 07:27:40 UTC
From this[1] comment by David on bug 10662 I take we need 1.018 anyway and the version in Stretch is not enough for all enhancements.

It should pass the test for this patch, but we require a higher version for the harvesting client.

David, can you confirm that is correct?

[1] https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10662#c128
Comment 22 David Cook 2018-04-08 23:57:47 UTC
(In reply to Mirko Tietgen from comment #21)
> From this[1] comment by David on bug 10662 I take we need 1.018 anyway and
> the version in Stretch is not enough for all enhancements.
> 
> It should pass the test for this patch, but we require a higher version for
> the harvesting client.
> 
> David, can you confirm that is correct?
> 
> [1] https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10662#c128

It seems like the dependency isn't specified anywhere here. I think maybe I just did that for bug #10662?

For bug #18713, version 1.015 should be enough (now that I've reviewed the versions more thoroughly again).

However, for bug #10662, we'll need version 1.017 or 1.018, since that bug does bulk SPARQL operations and older versions like 1.015 don't work for that.
Comment 23 David Cook 2018-04-09 00:00:45 UTC
Or maybe I added the dependency to #18585 ...
Comment 24 Mirko Tietgen 2018-04-09 08:09:38 UTC
(In reply to David Cook from comment #23)
> Or maybe I added the dependency to #18585 ...

I'd say you either could set this bug to 'depends on' one of the bugs that have the dependency declared, or you also add it for this one.
Comment 25 David Cook 2018-04-09 23:56:02 UTC
(In reply to Mirko Tietgen from comment #24)
> (In reply to David Cook from comment #23)
> > Or maybe I added the dependency to #18585 ...
> 
> I'd say you either could set this bug to 'depends on' one of the bugs that
> have the dependency declared, or you also add it for this one.

Done :)

Guess I better update #18585 now then..
Comment 26 David Cook 2018-08-13 02:04:37 UTC
After gaining more experience with RDF and triplestores, I'm not really happy with this one anymore. 

This functionality should probably be moved into a Koha::RDF::Triplestore module, as it's very triplestore specific... or maybe Koha::RDF::Model... since you could use other non-triplestore models. 

But there's other things that can be done in Koha::RDF that don't need to relate to models/triplestores...
Comment 27 David Cook 2018-09-11 19:32:05 UTC
This one still seems to apply on master.