Simple RDF validation for ACL files by megoth · Pull Request #937 · nodeSolidServer/node-solid-server

megoth · 2018-11-15T11:40:31Z

This is PR based on the discussions at #882.

Simply explained, when putting ACL files it will parse the files to make the syntax is valid.

I also extracted a couple of utility-functions for integration-tests that I think make the tests a little bit easier to read.

Ryuno-Ki · 2018-11-15T19:45:34Z

    if (filePath.endsWith('/')) {
-      return callback(error(409,
-        'PUT not supported on containers, use POST instead'))
+      return callback(error(409, 'PUT not supported on containers, use POST instead'))


Wouldn't be a 405 - Method not allowed be more appropriate then?

Definitely 405.

Ryuno-Ki · 2018-11-15T19:46:49Z

  }
+
+  mimetypeIsRdf (mimeType) {
+    return RDF_MIME_TYPES.indexOf(mimeType) !== -1


Assuming RDF_MIME_TYPES is an Array you can use Array.prototype.includes I guess.

Better, make RDF_MIME_TYPES a Set.

Assuming RDF_MIME_TYPES is an Array you can use Array.prototype.includes I guess.

I actually did this initially when I wrote the code, but it failed for me, and I automatically reverted to Array.prototype.indexOf without much more research. My bad.

But yeah, will try making it a Set.

Ryuno-Ki · 2018-11-15T19:47:43Z

 describe('CORS Proxy', () => {
-  var ldp = ldnode({
+  var server = setupSupertestServer({
    root: path.join(__dirname, '../resources'),


Well, if you want to go cross-platform, path.join(__dirname, '..', 'resources') would make Windows users happy.

Well, if you want to go cross-platform, path.join(__dirname, '..', 'resources') would make Windows users happy.

Is it necessary though? It seems like path.join will normalize it nevertheless.

Hm, you made me look at the source code of path.join.

The call to win32.normalize does some … things. So it could be possible that it would normalise it on its own.

Ryuno-Ki · 2018-11-15T19:48:41Z

-var { checkDnsSettings } = require('../utils')
-
-var ldnode = require('../../index')
+var { checkDnsSettings, setupSupertestServer } = require('../utils')


Why a var instead of a const?

Consistency.

RubenVerborgh · 2018-11-15T20:05:08Z

    if (filePath.endsWith('/')) {
-      return callback(error(409,
-        'PUT not supported on containers, use POST instead'))
+      return callback(error(409, 'PUT not supported on containers, use POST instead'))


Definitely 405.

RubenVerborgh · 2018-11-15T20:05:50Z

  }
+
+  mimetypeIsRdf (mimeType) {
+    return RDF_MIME_TYPES.indexOf(mimeType) !== -1


Better, make RDF_MIME_TYPES a Set.

RubenVerborgh · 2018-11-15T20:06:03Z

-var { checkDnsSettings } = require('../utils')
-
-var ldnode = require('../../index')
+var { checkDnsSettings, setupSupertestServer } = require('../utils')


Consistency.

RubenVerborgh · 2018-11-15T20:07:21Z

+      try {
+        fs.writeFileSync(filePath, text)
+      } catch (err) {
+        return callback(error(500, err.message))


You might want to delete whatever was written do disk.

What if the file existed from before, and we need to recover that file?

If it exists from before, maybe I should do a backup of the content, and roll back if the write failed? Otherwise, I should delete whatever was written to disk?

RubenVerborgh · 2018-11-15T20:07:29Z

+  putText (host, resourcePath, text, callback) {
+    this._put(host, resourcePath, callback, (filePath) => {
+      try {
+        fs.writeFileSync(filePath, text)


I'm wondering about the difference of fs.writeFileSync and await util.promisify(fs.writeFile) in general. It seems that there's no difference in blocking IO, and it seems that it doesn't affect the main thread as filesystem operations are handled in worker threads.

It doesn't seem to be that much of a difference, so in this case I think I prefer fs.writeFileSync. But at some point we might want to have a convention on this, so that we stay consistent throughout the code.

megoth · 2018-11-15T22:40:55Z

@Ryuno-Ki @RubenVerborgh I've pushed a commit with some changes based on your feedback. I've left out a couple of them, which I've commented.

Ryuno-Ki · 2018-11-16T20:27:28Z

    if (filePath.endsWith('/')) {
-      return callback(error(409,
-        'PUT not supported on containers, use POST instead'))
+      return callback(error(405, 'PUT not supported on containers, use POST instead'))


Hm, I haven't checked, what error is doing, but from https://tools.ietf.org/html/rfc7231#section-6.5.5 you can read that

The origin server MUST generate an Allow header field in a 405 response containing a list of the target resource's currently supported methods.

So it would be Allow: PUT?

Maybe @RubenVerborgh or @kjetilk can answer this?

I haven't looked into that part of the code, but it sounds reasonable that we need to distinguish between containers and other resources. So far Allow seems to have been added fairly indiscriminatory, but I haven't seen all the code, so I don't know. But it is a different issue, isn't it?

Ryuno-Ki · 2018-11-16T20:30:44Z

+const server = setupSupertestServer({
+  live: true,
+  dataBrowserPath: 'default',
+  root: path.join(__dirname, '../resources'),


I'd have split it to path.join(__dirname, '..', 'resources'), but if it works everywhere …

Ryuno-Ki · 2018-11-16T20:33:25Z

I consider the 405 comment necessary to deal with, but I don't have any roles here ;-)

megoth · 2018-11-20T13:14:00Z

I consider the 405 comment necessary to deal with, but I don't have any roles here ;-)

But it has been fixed now, hasn't it? Or am I missing something?

megoth · 2018-11-24T15:56:04Z

I think I've handled all feedback that's been given on this. Anyone up for approving this PR? @kjetilk @RubenVerborgh (or maybe @rubensworks)?

rubensworks · 2018-11-26T08:35:58Z

Same rebasing issue here as in #953.

Also, should this be targeted to the release branch?

And make old tests pass

But no actual implementation in patch handler yet

And make old tests pass

But no actual implementation in patch handler yet

And make old tests pass

But no actual implementation in patch handler yet

And make old tests pass

But no actual implementation in patch handler yet

megoth · 2018-11-28T12:31:56Z

I've rebased this branch on release/v5.0.0 and think it's ready to be merged. Maybe @rubensworks, @RubenVerborgh or @kjetilk wants to give it a last review to verify that it looks good?

rubensworks · 2018-11-28T12:33:52Z

Ok, I'll have a look.

rubensworks

Looks good to me!

megoth requested review from RubenVerborgh and kjetilk November 15, 2018 11:40

ghost assigned megoth Nov 15, 2018

ghost added the in progress label Nov 15, 2018

Ryuno-Ki reviewed Nov 15, 2018

View reviewed changes

RubenVerborgh reviewed Nov 15, 2018

View reviewed changes

Ryuno-Ki reviewed Nov 16, 2018

View reviewed changes

megoth force-pushed the feature/simple-validation branch from 9d51f01 to d9039a4 Compare November 20, 2018 14:57

megoth requested a review from rubensworks November 24, 2018 15:56

megoth mentioned this pull request Nov 27, 2018

Update index.html based on whether meta-tag is set or not #953

Merged

megoth added 11 commits November 28, 2018 13:29

First iteration of simple validation

3b3ba58

Just a minor commit

ebfe3f3

Moving code around, to handle streams correctly

c541613

And make old tests pass

Adding test for Patch

ea9b40c

But no actual implementation in patch handler yet

Modified simple validation to only affect ACL files

18a7a04

Trying to make integrations tests a little easier to read

0e3392f

First iteration of simple validation

48da61c

Just a minor commit

f35e458

Moving code around, to handle streams correctly

6d845b1

And make old tests pass

Adding test for Patch

5b94a22

But no actual implementation in patch handler yet

Trying to make integrations tests a little easier to read

331dc0c

megoth added 12 commits November 28, 2018 13:29

Changes to code based on feedback

afbe557

A couple of fixes

29c115f

First iteration of simple validation

1be27f7

Just a minor commit

5a39e8e

Moving code around, to handle streams correctly

a0232e2

And make old tests pass

Adding test for Patch

af58d66

But no actual implementation in patch handler yet

First iteration of simple validation

8b0e8e7

Just a minor commit

fae913d

Moving code around, to handle streams correctly

9a936fc

And make old tests pass

Adding test for Patch

cd5dfdd

But no actual implementation in patch handler yet

Trying to make integrations tests a little easier to read

b5cfcf6

Rebased on v5.0.0

d939f7c

megoth force-pushed the feature/simple-validation branch from fa84a62 to d939f7c Compare November 28, 2018 12:29

megoth changed the base branch from master to release/v5.0.0 November 28, 2018 12:29

rubensworks approved these changes Nov 28, 2018

View reviewed changes

kjetilk merged commit 21a41b3 into release/v5.0.0 Nov 28, 2018

ghost removed the in progress label Nov 28, 2018

kjetilk deleted the feature/simple-validation branch November 28, 2018 21:37

megoth mentioned this pull request Feb 4, 2019

Validation of LD-files #882

Closed

Conversation

megoth commented Nov 15, 2018

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!

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!

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!

megoth Nov 15, 2018 • 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!

megoth commented Nov 15, 2018

Uh oh!

Ryuno-Ki Nov 16, 2018 • 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!

Ryuno-Ki commented Nov 16, 2018

Uh oh!

megoth commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

megoth commented Nov 24, 2018

Uh oh!

rubensworks commented Nov 26, 2018

Uh oh!

megoth commented Nov 28, 2018

Uh oh!

rubensworks commented Nov 28, 2018

Uh oh!

rubensworks left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

megoth Nov 15, 2018 •

edited

Loading

Ryuno-Ki Nov 16, 2018 •

edited

Loading

megoth commented Nov 20, 2018 •

edited

Loading