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

Store::SPARQL reads and writes to Virtuoso (>=6.1.7) #113

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

doriantaylor
Copy link
Contributor

We're probably going to have to chat about this one:

  1. I had to add some ugly state maintenance Test::RDF::Trine::Store because I couldn't guarantee an empty store to test context-less semantics. The tests seem to work on the ones that I tried, but I don't have all the backends.
  2. Virtuoso doesn't respect q= values in the Accept: header, which made response handling complicated. At the moment it's hard-coded to JSON because V does some stupid, stupid behaviour otherwise.
  3. The net effect is that robust, product-independent HTTP response handling will be very, very complicated. I stubbed out a provision for handling different products, but I have more pressing things to do than fill that all out.

Otherwise, it works.

@kasei
Copy link
Owner

kasei commented Aug 5, 2014

I'm not sure what's going on with some parts of the patch here. It seems they're coming from my repo, but didn't rebase correctly...? I've tried to merge it with my HEAD in the new kasei:merge-dorian-sparql-auth branch. I'm also going to add some comments to specific parts of the pull request asking some questions...

@@ -247,6 +247,8 @@ sub end_element {
push( @{ $results{ $addr } }, $vb );
}
} elsif ($tag eq 'bnode') {
# guess what! virtuoso uses funny bnodes
$string =~ s!nodeID://!!;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd much prefer we didn't hard-code this, and instead found some way to modularize the code so that the SPARQL store could apply this fix only when it thinks necessary (or have it be applied to all results, but only when called from the SPARQL store or when explicitly requested). I'll try to think about what sort of API would work well for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that bnodes come through the sparql-results+xml format as nodeID://… The net effect is that without that line, the SPARQL results XML parser crashes when it tries to create a malformed bnode.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, understood. I'd just like to fix this by updating the API instead of hard-coding the fix into the XML parser.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure how to do this, other than making some kind of callback you can pass into the constructor (ugly), or otherwise a Virtuoso-specific subclass (worse).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the ::Iterator::SAXHandler constructor could already take optional configuration arguments, it just wasn't documented. I've updated the code to allow passing in a custom handler to generate the bnode IDs. Have a look at commit eea4393.

@doriantaylor
Copy link
Contributor Author

I didn't notice this:

I'm not sure what's going on with some parts of the patch here. It seems they're coming from my repo, but didn't rebase correctly...?

I'm not sure. This is branched off 7e9559d. I had to do major, major surgery to my repo to tease apart disparate patches. It's possible I (or it) screwed something up. It seems like other PRs didn't have any issues like that, however.

So how about this:

  • Move JSON code into JSONHandler
  • Build up product-specific dispatching infrastructure with Virtuoso as reference
  • Create a new branch for multi-valued get_statements, but I'm leaving that block there.

@kasei
Copy link
Owner

kasei commented Aug 6, 2014

Do you have an example Virtuoso results file (with the "nodeID://" stuff included) that I can use in testing?

@kasei
Copy link
Owner

kasei commented Aug 6, 2014

Oh, never mind. Realized I could get one from DBPedia.

@kasei
Copy link
Owner

kasei commented Aug 6, 2014

With the new blank node ID generation stuff (in eea4393), I'm happy with your proposed path forward. Could you re-base with eea4393 before re-requesting a pull? Thanks!

@doriantaylor
Copy link
Contributor Author

Sure, I have to sync up anyway.

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.

2 participants