Upgrade to rdflib 0.15.0, update PATCH tests by dmitrizagidulin · Pull Request #503 · nodeSolidServer/node-solid-server

dmitrizagidulin · 2017-05-17T18:37:26Z

Addresses issue #491.
Fixes PATCH- and Glob-related failed tests when upgrading to rdflib 0.15.0

dmitrizagidulin · 2017-05-17T20:57:34Z

cc @timbl / @dan-f for review.

dan-f

I fail to see how this "fixes Glob-related failed tests". I found several "glob-related tests", but none of them changed in this PR.

The first glob-related tests are actually skipped. I tested, on master, un-skipping that describe block, and all of the glob tests failed. This is before the rdflib upgrade.

The second "glob test" doesn't test globbing. It simply tests for a 200 response code and content-type text/turtle.

dan-f · 2017-05-18T13:57:56Z

        undefined,
        globGraph,
-        null,
+        filename,


So filename here gets bound to something like 'example.com/thing/*', which isn't an actual URI.

An asterisk is a valid URI character. But whether that's the preferred semantics, as far as the base uri when using file patterns, is a tough call. We'll need to discuss it with @timbl etc.

My point was not regarding the asterisk. It was regarding the lack of a URI scheme.

Ohh I see. Thanks! will fix

Fixed the base serialization uri to be the request uri.

dan-f · 2017-05-18T13:58:29Z

        globGraph,
-        null,
+        filename,
        'text/turtle')


What if the client doesn't want text/turtle? I hoped there's content negotiation, based on our spec.

From what I understand, glob functionality is only spec'd/implemented for Turtle.

From what I understand, the notion of "globbing all the graphs in an LDP container into a single graph" is agnostic to any particular serialization format.

We can open an issue to add support for multiple content types for globbing, for solid-server. This PR is just intended to upgrade the server to the latest rdflib version.

I opened issue #504 to add support for content negotiation with globbing.

dan-f · 2017-05-18T14:05:43Z

-            '@prefix profile: <http://ogp.me/ns/profile#>.\n' +
-            '\n' +
-            '   <#> profile:first_name "Timothy"; a schema:Person .\n')
+            '@prefix : </prefixSparql.ttl#>.\n@prefix schema: <http://schema.org/>.\n@prefix pro: <http://ogp.me/ns/profile#>.\n\n: a schema:Person; pro:first_name "Timothy".\n\n')


A bit confused about this change given the one above. Let's be consistent with formatting of data. I think the previous change is much more readable.

No prob, will change it to match.

Changed the expected value formatting to be multi-line. Thanks!

dmitrizagidulin · 2017-05-19T15:17:38Z

I fail to see how this "fixes Glob-related failed tests". I found several "glob-related tests", but none of them changed in this PR.

I just meant - this PR passes the failing tests that are introduced by upgrading to latest rdflib.

The first glob-related tests are actually skipped. I tested, on master, un-skipping that describe block, and all of the glob tests failed. This is before the rdflib upgrade.

Yeah, that's weird, I'm not sure what the story was with those. I added an issue, #505, to un-skip and implement those tests.

The second "glob test" doesn't test globbing. It simply tests for a 200 response code and content-type text/turtle.

Yeah, also odd. I expanded that test to actually check for result contents.

dan-f · 2017-05-19T16:41:46Z

        undefined,
        globGraph,
-        null,
+        requestUri,


This change is still untested.

melvincarvalho · 2017-05-28T11:21:22Z

A gentle bump on this.

Hope it's OK, that I added the blocker label, as this fix will enable me to log in to node solid server, and therefore, work more effectively.

dmitrizagidulin · 2017-05-31T19:00:28Z

 @prefix dc: <http://purl.org/dc/elements/1.1/> .
 @prefix ex: <http://example.org/stuff/1.0/> .

+</sampleContainer/example*> a :glob.


Added to address @dan-f's comment; tests the requestUri being used as a serialization base for globs. (per @timbl's instructions)

Uh... I would expect this triple to be dynamically added by the glob code, not statically added to the sampleContainer/example1.ttl graph.

This isn't an official designation or anything, just a triple @timbl recommended to test the serialization base in the output.

I find it a really confusing example. Why not just add <#a> dc:title "foo" . to example1.ttl and then expect to see <example1.ttl#a> dc:title "foo" . in the glob?

I don't want to hold this up any longer

The example tests the serialization base being the glob request uri. The one you mention, <#a> dc:title "foo" . tests the parsing base (relative to example1.ttl). I'll add this line also.

Huh. Sounds like I am missing some context from your discussion. I think you should merge if you and @timbl are happy with the test.

FYI, this is not how you test glob. </sampleContainer/example*> a :glob. being in a document makes no sense to me. Globbing means doing a GET on /sampleContainer/example* and returning the contents from multiple documents that start with the name example.

@deiu would something like </sampleContainer/example*> :test "testing serialization base" . make more sense, instead?

I don't have a stake in this argument; was just adding the statement at @timbl's suggestion.

First, you don't have to keep this PR open on my account. Please merge if you think it's the right thing to do. I'm only pointing out that having a triple with the subject /example* in that document makes absolutely no sense. The * character must be removed by the server before it starts to glob resources together, it should never be treated as a regular resource (which is what the test currently does). Its presence there also shows that the test was written by someone doesn't really understand how globbing works.

@deiu I've replaced the <example*> a :glob statement with the example you & @dan-f suggested.

dmitrizagidulin added 2 commits May 17, 2017 14:35

Upgrade to rdflib 0.15.0, update PATCH tests

7d1eeba

Pass base uri to glob serialization

f357fab

dmitrizagidulin added the ready for review label May 17, 2017

dan-f suggested changes May 18, 2017

View reviewed changes

dmitrizagidulin added 2 commits May 19, 2017 11:10

Fix base serialization uri for glob requests

bff0c22

Add expected results for glob test

15a680e

dmitrizagidulin force-pushed the rdflib-15 branch 2 times, most recently from 6cd583b to ab255b3 Compare May 19, 2017 15:26

Fix formatting for PATCH tests

c3a7b9e

dmitrizagidulin force-pushed the rdflib-15 branch from ab255b3 to c3a7b9e Compare May 19, 2017 15:28

dan-f reviewed May 19, 2017

View reviewed changes

Comment thread lib/handlers/get.js

undefined,

globGraph,

null,

requestUri,

dan-f May 19, 2017

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

This change is still untested.

melvincarvalho added the blocker label May 28, 2017

HTTP glob get test - add statement to test serialize base

4f7d620

dmitrizagidulin commented May 31, 2017

View reviewed changes

Use compact Collection syntax (per tim's request)

2e4d52a

dmitrizagidulin force-pushed the rdflib-15 branch from b32a776 to 2e4d52a Compare May 31, 2017 20:22

dan-f approved these changes Jun 1, 2017

View reviewed changes

dmitrizagidulin added 2 commits June 1, 2017 13:25

Update glob test / expected value

8983394

Convert glob test to use rdflib matching

9afac6c

dmitrizagidulin merged commit cba687c into master Jun 2, 2017

RubenVerborgh added this to the 4.0.0 milestone Aug 10, 2017

dmitrizagidulin mentioned this pull request Aug 16, 2017

Upgrade to latest rdflib 0.13.0 #491

Closed

dmitrizagidulin deleted the rdflib-15 branch June 7, 2018 20:50

Uh oh!

Conversation

dmitrizagidulin commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitrizagidulin commented May 17, 2017

Uh oh!

dan-f left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dan-f May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dan-f May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitrizagidulin May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitrizagidulin commented May 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melvincarvalho commented May 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deiu Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deiu Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dmitrizagidulin commented May 17, 2017 •

edited

Loading

dan-f May 18, 2017 •

edited

Loading

dan-f May 18, 2017 •

edited

Loading

dmitrizagidulin May 19, 2017 •

edited

Loading

deiu Jun 1, 2017 •

edited

Loading

deiu Jun 1, 2017 •

edited

Loading