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

support headers in SPARQLConnector, allow urlencoding and take care of long uri's in commit #1230

Closed
wants to merge 1 commit into from

Conversation

nilutz
Copy link

@nilutz nilutz commented Jan 19, 2021

This is just something that came up while working with rdflib and the blazegraph db.

Currently any header that is set in the init is overwritten in the code. Simple checks whether there are headers in the self.kwargs prevents them from being overwritten with the default ones. In addition rdflib sets the postAsEncoded argument but never uses it anywhere. I modified the SPARQLConnector so that updates can be POSTed via urlencoded uri's. A drawback is that the commit method must take care of too long uris and split the commit into several pieces.

Maybe these changes are redundant in sense that the maintainers choose to do updates differently(this seemed fixed #315 and talked about here #1175), but maybe its worth a discussion, again - if not just ignore this PR.

…f long uri's in commit

Currently any header that is set in the init is overwritten in the code. In addition rdflib can use the postAsEncoded to sent an update via urlencoding.

This means that rdflib has to take care of long urls in when using commit()
@white-gecko
Copy link
Member

According to the SPARQL 1.1 Protocol update request are only specified to be url-encoded in the post body (application/x-www-form-urlencoded) or directly in the post body. There is no such case, that the query is encoded in the url for a post request.
In case of query requests there is the case that for get requests the query is put into the url.

query: https://www.w3.org/TR/sparql11-protocol/#query-operation
update: https://www.w3.org/TR/sparql11-protocol/#update-operation

@white-gecko
Copy link
Member

Can this be closed?

@FlorianLudwig
Copy link
Contributor

@nilutz Could you explain your use-case?

I agree with @white-gecko that this should not be merged as-is.

I would argue that postAsEncoded should be removed all-togther.

@nilutz
Copy link
Author

nilutz commented Feb 21, 2021

Hi,
I don't have an opinion on this. I wasn't aware that this is not part of the SPARQL 1.1 Protocol. I used rdflib with blazegraph and found some inconsistencies and this was part of my fix. I figured that rdflib was missing some pieces, but it turns out that it is "intended" as is, due to the SPARQL protocol guideline. I am not sure whether people use rdflib with blazegraph, but the current implementation is lacking some things. I would close this PR and think about how to make the SPARQLConnector generic enough so other maintainers can add plugins for the "common" graph databases. Especially since these issues keep popping up: #1231 , #1251, #955, #919 That being said that is not my decision to make ;) .
Thanks for looking into this! Keep up the good work.

@nilutz nilutz closed this Feb 21, 2021
@white-gecko
Copy link
Member

blazegraph states to support the "SPARQL 1.1 family of specifications" (https://github.com/blazegraph/database/wiki/About_Blazegraph) so I think, it should also stick to the standards.

I think, we are willing to adjust the RDFlib implementation to improve the standard support on our side and we are very happy about pull-requests that bring us further with this regard. If you find a fix to support blazegraph within the standards, chances are good that the pr can be accepted.

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.

3 participants