fix 1120 1256 from NSS5.1.6 ressource-mapper #1280

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

lib/resource-mapper.js

@@ -7,7 +7,7 @@ const HTTPError = require('./http-error')

 /*
 * 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

lib/resource-mapper.js

    // Parse the URL and find the base file path
    const { pathname, hostname } = this._parseUrl(url)
    const filePath = this.resolveFilePath(hostname, decodeURIComponent(pathname))
    if (filePath.indexOf('/..') >= 0) {
      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

lib/resource-mapper.js

+        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

lib/resource-mapper.js

    // Parse the URL and find the base file path
    const { pathname, hostname } = this._parseUrl(url)
    const filePath = this.resolveFilePath(hostname, decodeURIComponent(pathname))
    if (filePath.indexOf('/..') >= 0) {
      throw new Error('Disallowed /.. segment in URL')
    }
-    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

lib/resource-mapper.js

+      // 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.

lib/resource-mapper.js

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix 1120 1256 from NSS5.1.6 ressource-mapper #1280

fix 1120 1256 from NSS5.1.6 ressource-mapper #1280

bourgeoa commented Aug 2, 2019

RubenVerborgh left a comment

RubenVerborgh Aug 2, 2019

RubenVerborgh Aug 2, 2019

RubenVerborgh Aug 2, 2019

RubenVerborgh Aug 2, 2019

RubenVerborgh Aug 2, 2019

bourgeoa Aug 2, 2019

RubenVerborgh Aug 2, 2019

bourgeoa Aug 2, 2019

RubenVerborgh Aug 2, 2019

bourgeoa commented Aug 3, 2019 •

edited

Loading

RubenVerborgh left a comment

RubenVerborgh Aug 3, 2019

bourgeoa Aug 3, 2019

RubenVerborgh Aug 3, 2019

bourgeoa Aug 4, 2019

RubenVerborgh Aug 4, 2019

bourgeoa commented Aug 4, 2019

RubenVerborgh commented Aug 4, 2019

bourgeoa commented Aug 5, 2019

fix 1120 1256 from NSS5.1.6 ressource-mapper #1280

fix 1120 1256 from NSS5.1.6 ressource-mapper #1280

Conversation

bourgeoa commented Aug 2, 2019

RubenVerborgh left a 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

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

bourgeoa commented Aug 3, 2019 • edited Loading

RubenVerborgh left a 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

Choose a reason for hiding this comment

bourgeoa commented Aug 4, 2019

RubenVerborgh commented Aug 4, 2019

bourgeoa commented Aug 5, 2019

bourgeoa commented Aug 3, 2019 •

edited

Loading