fix 1120 1256 from NSS5.1.6 ressource-mapper by bourgeoa · Pull Request #1280 · nodeSolidServer/node-solid-server

bourgeoa · 2019-08-02T14:06:08Z

@RubenVerborgh @jaxoncreed
fix issues #1120 #1256 from NSS5.1.6 ressource-mapper (should replace PR #1260 )

ressource-mapper function mapUrlToFile :
( it seems that for some reason using searchIndex = true in function definition mapUrlToFile causes problem. searchIndex is not updated when calling directly mapUrlToFile - but updated if called through an other function like ldp.get or get.js )
-- createIfNotExists = true : replace isIndex by isFolder (I suppose not needed use of searchIndex = true)
-- createIfNotExists = false : reorganization of tests around isFolder and isIndex in lieu of searchIndex and isIndex
get.js modified along PR Resource-mapper now considers content type when inserting index.html #1260 (use case not clear to me)
ldp.js modified to avoid double extension when using slug with extension

Tested on a pod (created with docker) and solid-Ide.

There is a remaining problem : Pod connexion to root needs login (?? something with allow.js)

RubenVerborgh

Thanks, the main idea of the fix looks good (distinguishing between isFolder and searchIndex).
Still needs unit tests though on the ResourceMapper side.

RubenVerborgh · 2019-08-02T16:22:12Z

 /*
 * A ResourceMapper maintains the mapping between HTTP URLs and server filenames,
- * following the principles of the “sweet spot” discussed in
+ * following the principles of the “sweet spot� discussed in


encoding issue

RubenVerborgh · 2019-08-02T16:24:02Z

      throw new Error('Disallowed /.. segment in URL')
    }
-    let isIndex = searchIndex && filePath.endsWith('/')
+    let isFolder = filePath.endsWith('/')


Is this actually an assumption we can make? Or should it be passed in explicitly as a parameter?

RubenVerborgh · 2019-08-02T16:26:19Z

+        if (match === undefined) {throw new HTTPError(404, `Resource not found: ${pathname}`)}
+
+      // Check if the index file exists
+      } else if (isIndex && files.includes(this._indexFilename)) {


isIndex should be searchIndex here

RubenVerborgh · 2019-08-02T16:27:05Z

    }
-    let isIndex = searchIndex && filePath.endsWith('/')
+    let isFolder = filePath.endsWith('/')
+    let isIndex = searchIndex && isFolder


I think this variable can be dropped (as per my comment below).

Not being a coder. I need help.

L127 OK searchIndex and drop of isIndex in L95

L94 do you want to replace isFolder by filePath.endsWith('/') everywhere it's used or to use an other variable name or to pass a parameter (this test was already used)
(The code allready used the endsWith('/') I would like to avoid at this stage to add a new parameter and push a test on all mapUrlToFile calls)

I suppose I have to modify the tests that errored N°5 and 6 in travis due to my modifications. It may relates to explicitely set searchIndex value to true.

How can I send my modifications : as a new PR or something else (I have never done it)
Sorry for my deeply need of help

Not being a coder.

This is good stuff though 👍

L94, okay, leave as is.

How can I send my modifications

You just push another commit to the branch you have created. So just keep pushing to your own master branch, and they will end up here. Don't hesitate to ask if you're uncertain, happy to explain!

ressource-mapper tests
I have a problem with that test :

itMapsUrl(mapper, 'a URL ending with a slash to a folder when index is skipped',

{ url: 'http://localhost/space/', contentType: 'application/octet-stream', createIfNotExists: true, searchIndex: false }, { path: `${rootPath}space/`, contentType: 'application/octet-stream' })

Is it a use case ?
It creates a folder with contentType.
In all tests cases it is the only case where a folder is created. I missed this behaviour.
If yes I need to slightly modify my code and I did not found where it is used (PUT, PATCH, POST)

Can you confirm.

Is tthere no code calling it with createIfNotExists: true and searchIndex: false? Because this code might be used to create a folder.

bourgeoa · 2019-08-03T20:10:09Z

fix finally Ok and tested on pod with docker and in using solid-ide.

The tests cases where a bit complex to elaborate. I tried to make them clean enough to understand. I reintroduced isIndex to make it clearer. I can suppress it if you prefer as it is only used once

all ressource-mapper tests passed.
And finally podroot opens normally on podroot/index.html as expected
Do I have to do something for the other integration tests that failed ?

nota : I checked with notepad++ and I confirm that there is no code calling createIfNotExists : true and searchIndex: false

RubenVerborgh

Minor comments; tests are still failing though, seems like test changes were not incorporated.

RubenVerborgh · 2019-08-03T21:01:20Z

+      // create a new folder
+      if (!searchIndex && isFolder) {
+  	  // else create a new file
+      } else {


if statement with empty body is to be avoided

I shall replace with

if ((!searchIndex && isFolder) === false) {

What shall I do with the tests ? Are the errors related to the fix ?

Or just if (searchIndex !! !isFolder).

Tests are indeed related; some behavior is changed, so should be updated. I think the behavior is correct though.

I suppose you meant if (searchIndex || !isFolder) which do not cover searchIndex: false and isFolder: false which would allow to create a file without contentType control or read without check it exists.

I think there are missing tests that should cover the case :

itMapsUrl(mapper, "a URL with a bogus extension that doesn't match the content type", { url: 'http://localhost/space/foo.ttl', contentType: 'text/html', createIfNotExists: true, searchIndex: false }, { path: `${rootPath}space/foo.ttl$.html`, contentType: 'text/html' })

and cases without createIfNotExists.

I suppose you meant if (searchIndex || !isFolder)

No, I meant that if ((!searchIndex && isFolder) === false) { is written more simply as if (searchIndex || !isFolder).

Case looks good.

bourgeoa · 2019-08-04T13:41:17Z

requested changes made.
fails tests relates to ACL timeout, all other tests passed on node 8.

RubenVerborgh · 2019-08-04T19:56:08Z

But are the timeouts causes by the changes, or where they present before?

bourgeoa · 2019-08-05T14:24:40Z

I close the pull request to begin a new one to check travis at every step
I think I bugged something

bourgeoa added 3 commits August 2, 2019 14:47

fix 1120 1256 with NSS5.1.6 ressource-mapper

d2f1d85

edit ressource-mapper cleanup

bddb196

edit get.js cleanup

be1d446

RubenVerborgh suggested changes Aug 2, 2019

View reviewed changes

bourgeoa added 2 commits August 3, 2019 20:05

update to ressource-mapper

5cef7dc

typing error replace isfolder by isFolder

7e3153d

bourgeoa marked this pull request as ready for review August 3, 2019 20:54

RubenVerborgh suggested changes Aug 3, 2019

View reviewed changes

minor editing

60cc593

bourgeoa added 2 commits August 5, 2019 15:43

return to v5.1.6 to test travis

2e39d61

return to master to test travis

5515d75

bourgeoa closed this Aug 5, 2019

bourgeoa mentioned this pull request Aug 6, 2019

fix issues #1120 #1256 from NSS5.1.6 ressource-mapper #1282

Closed

Uh oh!

Conversation

bourgeoa commented Aug 2, 2019

Uh oh!

RubenVerborgh 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!

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!

bourgeoa commented Aug 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RubenVerborgh 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!

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!

Uh oh!

bourgeoa commented Aug 4, 2019

Uh oh!

RubenVerborgh commented Aug 4, 2019

Uh oh!

bourgeoa commented Aug 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bourgeoa commented Aug 3, 2019 •

edited

Loading