Add graph store HTTP protocol #27

cecton · 2017-08-02T08:55:35Z

Specifications at https://www.w3.org/TR/sparql11-http-rdf-update/#graph-management

asvetlov · 2017-08-02T09:02:50Z

aiosparql/client.py

@@ -178,6 +185,37 @@ def _pretty_print_query(self, query: str) -> str:
                    explanation=explanation)
            return await resp.json()

+    def _crud_request(self, method, graph=None, data=None):


What is data parameter for?

Looks it it's never used

HTTP body.

The graph store protocol is basically a crud endpoint where you post/delete/get raw data in any format you want (RDF XML, Turtle, etc...).

Just realized now that there is no parameter to set the format. Maybe I should start adding integration tests.

Oh I see. Yes integration test would be very useful at this point.

codecov · 2017-08-02T09:50:43Z

Codecov Report

Merging #27 into master will increase coverage by 0.15%.
The diff coverage is 97.72%.

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
+ Coverage   96.14%   96.29%   +0.15%     
==========================================
  Files           5        5              
  Lines         363      405      +42     
==========================================
+ Hits          349      390      +41     
- Misses         14       15       +1

Impacted Files	Coverage Δ
aiosparql/test_utils.py	`98.07% <100%> (+0.45%)`	⬆️
aiosparql/client.py	`96.42% <96.96%> (+0.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5182ae9...c9208e5. Read the comment docs.

cecton · 2017-08-02T14:13:56Z

aiosparql/client.py

+        async with self._crud_request("PATCH", graph=graph, data=data,
+                                      accept="application/json") as resp:
+            resp.raise_for_status()
+            return await resp.json()


I'm not sure about this code. According to the specs, the PATCH method is like an endpoint for SPARQL 1.1 update:

SPARQL 1.1 Update can be used as a patch document.

I suppose that without being able to really test it I should simply remove the code for now or raise NotImplementedError.

+1
If you are not sure about implementation -- just drop the method and wait for feature request with real use case.

cecton · 2017-08-02T14:15:15Z

aiosparql/client.py

+            resp.raise_for_status()
+
+    def head(self, *, format: str, graph: Optional[IRI] = None):
+        return self._crud_request("HEAD", graph=graph, accept=format)


I'm not sure about this code. According to the specs, the HEAD method does the exact same thing than GET but does not return content. So I suppose the format should be supplied by the client...

the HTTP HEAD method is identical to GET except that the server MUST NOT return a message-body in the response

Sorry, I don't follow

cecton · 2017-08-02T14:16:10Z

tests/integration/test_client.py

+            text = await res.text()
+            self.assertIn("@prefix", text)
+        async with await self.client.head(format="text/turtle") as res:
+            self.assertIn(res.status, (200, 501))


I love doing integration test with a server that does not implement everything...

cecton · 2017-08-02T14:16:32Z

tests/integration/test_client.py

+            self.assertIsInstance(res, dict)
+        except aiohttp.ClientResponseError as exc:
+            if exc.code != 501:
+                raise


PATCH is apparently not implemented either by Virtuoso.

cecton · 2017-08-07T10:34:22Z

@asvetlov ready for your review :) I will probably release after this one.

asvetlov

Please go ahead.
The only note: could you consider adding a fixture for running virtuoso container by pytest fixture later?
It simplifies local tests run I guess.

asvetlov · 2017-08-07T18:15:11Z

aiosparql/client.py

+            resp.raise_for_status()
+
+    def head(self, *, format: str, graph: Optional[IRI] = None):
+        return self._crud_request("HEAD", graph=graph, accept=format)


Sorry, I don't follow

asvetlov · 2017-08-07T18:16:40Z

aiosparql/client.py

+        async with self._crud_request("PATCH", graph=graph, data=data,
+                                      accept="application/json") as resp:
+            resp.raise_for_status()
+            return await resp.json()


+1
If you are not sure about implementation -- just drop the method and wait for feature request with real use case.

cecton · 2017-08-08T12:52:16Z

@asvetlov Yes I can spawn a container for virtuoso... would be simpler to use I suppose... but this doesn't look like it belongs to the test file itself. I guess I could make a "ci" directory with a start and stop script. Does that sound good/better/less good?

Fixes aio-libs#24

asvetlov · 2017-08-08T17:18:54Z

I forgot you use unittest style for this project.

It's up to you, you are the library author.
But I suggest considering switch to pytest everywhere (I see you've started already by adding test_client_context).

After that you could add code like used by aiopg: https://github.com/aio-libs/aiopg/blob/master/tests/conftest.py#L105-L194

It tests DB client against several Postgres versions by providing a fixture for container start and other fixture for making client connected to started DB server.

asvetlov · 2017-08-08T17:19:41Z

Maybe you'll find the trick useful for aiosparql too.

cecton · 2017-08-09T08:44:46Z

@asvetlov I think I will keep the idea for later for a new PR.

I don't mind much having py.test or unittest tests (even a mix of them) as long as the coverage is good and the code is well tested. It's never too late to change and besides this library is very very small because it's SPARQL: this lib only needs to respect the specifications of SPARQL and HTTP; the server it connects to doesn't really matter. I have worked with PostgreSQL too and I know for sure this is very very different. Overall, this library is some kind of helper to make HTTP requests (at least for aiosparql.client).

Now the syntax helpers are more complicated: in SPARQL every value has a scheme for the type and maybe a language if it's a literal, plus depending on the server, the type you specify may not be the type you will get in the database itself... escaping SPARQL is a hell and that's the main purpose of aiosparql.escape and generating nodes too aiosparql.syntax.

One thing is missing (somehow deliberately) in this library: parsing the SPARQL result and make internal objects. Most of the libraries I know does it but, to be honest, when you use JSON, I just don't find that very useful. So right now I keep an open door for it for later if someone or myself wants to implement something at some point. It's just not my priority.

cecton self-assigned this Aug 2, 2017

asvetlov reviewed Aug 2, 2017

View reviewed changes

cecton commented Aug 2, 2017

View reviewed changes

cecton mentioned this pull request Aug 4, 2017

Removes ClientSession from inheritance and implement graph store HTTP protocol #23

Closed

Moved unit tests to its own directory

bdc12f2

cecton force-pushed the graph-store-http-protocol branch from 61d9c87 to 9bbe7e7 Compare August 4, 2017 06:43

asvetlov approved these changes Aug 7, 2017

View reviewed changes

Added CRUD endpoint

c9208e5

Fixes aio-libs#24

cecton force-pushed the graph-store-http-protocol branch from 9bbe7e7 to c9208e5 Compare August 8, 2017 12:57

cecton merged commit 60196e9 into aio-libs:master Aug 9, 2017

cecton deleted the graph-store-http-protocol branch August 9, 2017 08:46

cecton mentioned this pull request Aug 9, 2017

Improve test environment #29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add graph store HTTP protocol #27

Add graph store HTTP protocol #27

cecton commented Aug 2, 2017 •

edited

Loading

asvetlov Aug 2, 2017

asvetlov Aug 2, 2017

cecton Aug 2, 2017

cecton Aug 2, 2017

codecov bot commented Aug 2, 2017 •

edited

Loading

cecton Aug 2, 2017

asvetlov Aug 7, 2017

cecton Aug 2, 2017

asvetlov Aug 7, 2017

cecton Aug 2, 2017

cecton Aug 2, 2017

cecton commented Aug 7, 2017

asvetlov left a comment

asvetlov Aug 7, 2017

asvetlov Aug 7, 2017

cecton commented Aug 8, 2017

asvetlov commented Aug 8, 2017

asvetlov commented Aug 8, 2017

cecton commented Aug 9, 2017 •

edited

Loading

Add graph store HTTP protocol #27

Add graph store HTTP protocol #27

Conversation

cecton commented Aug 2, 2017 • edited Loading

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

codecov bot commented Aug 2, 2017 • edited Loading

Codecov Report

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

cecton commented Aug 7, 2017

asvetlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Aug 8, 2017

asvetlov commented Aug 8, 2017

asvetlov commented Aug 8, 2017

cecton commented Aug 9, 2017 • edited Loading

cecton commented Aug 2, 2017 •

edited

Loading

codecov bot commented Aug 2, 2017 •

edited

Loading

cecton commented Aug 9, 2017 •

edited

Loading