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

SPARQLConnector.update should set ContentType charset #2095

Closed
robcast opened this issue Aug 22, 2022 · 6 comments · Fixed by #2112
Closed

SPARQLConnector.update should set ContentType charset #2095

robcast opened this issue Aug 22, 2022 · 6 comments · Fixed by #2112
Labels
bug Something isn't working good first issue Good for newcomers networking Related to networking. SPARQL store Related to a store.

Comments

@robcast
Copy link
Contributor

robcast commented Aug 22, 2022

I use rdflib to connect to a Blazegraph triplestore using the SPARQLUpdateStore.

When I add data containing non-ASCII characters using addN() it results in garbled data in the triplestore (UTF-8 interpreted as ISO-8859).

The problem seems to be that SPARQLConnector.update() does not set "charset=UTF-8" in the Content-Type header:

"Content-Type": "application/sparql-update",

but it does use UTF-8 in data=query.encode() in line 183.

The problem goes away if I patch the Content-Type to be "application/sparql-update; charset=UTF-8".

I can provide a PR with this one-line change if you like.

@aucampia aucampia added bug Something isn't working SPARQL store Related to a store. networking Related to networking. labels Aug 22, 2022
@aucampia
Copy link
Member

aucampia commented Aug 22, 2022

@robcast thanks for raising the issue, a PR will be welcome, but it should include tests. Whether or not you create a PR we will likely include a fix for this in the next release.

@aucampia aucampia added the good first issue Good for newcomers label Aug 22, 2022
@robcast
Copy link
Contributor Author

robcast commented Aug 22, 2022

I can try to add tests if it's not too hard :-) What kind of tests do you need?

The issue is at the system boundary between rdflib and the triplestore which makes it more complicated to test. Would https://github.com/RDFLib/rdflib/blob/master/test/test_store/test_store_sparqlupdatestore_mock.py be a place for that?

@aucampia
Copy link
Member

aucampia commented Aug 22, 2022

For the SPARQLConnector/SPARQLUpdatestore test should use some from of mocking, given that the expectation is that something happens on the network layer the best option is to use our http mock as is being done here.

There is already some http mock based tests in test/test_store/test_store_sparqlupdatestore_mock.py but they are a bit dated, though it would be the right file for tests.

@aucampia
Copy link
Member

Just a heads up, as there is already a PR waiting for review on test/test_store/test_store_sparqlupdatestore_mock.py (i.e. #2089) any other change to that file will only be merged after that PR.

Reviews are welcome though.

@aucampia
Copy link
Member

Just a heads up, as there is already a PR waiting for review on test/test_store/test_store_sparqlupdatestore_mock.py (i.e. #2089) any other change to that file will only be merged after that PR.

Reviews are welcome though.

#2089 has been merged thanks to the review from @gjhiggins - so it is now open for changes.

@robcast
Copy link
Contributor Author

robcast commented Sep 9, 2022

I added a PR with a test in test_store_sparqlupdatestore_mock.py.

The test is just a copy of the existing test asserting a different part in the request header. It is not minimal and the assertion could be integrated into the existing test.

aucampia pushed a commit that referenced this issue Sep 15, 2022
Add encoding "charset=UTF-8" to Content-Type header in `SPARQLConnector.update()` request.

Fixes #2095
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers networking Related to networking. SPARQL store Related to a store.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants