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

bourgeoa · 2019-08-05T14:44:52Z

recreate all for tests each step

bourgeoa · 2019-08-05T19:38:12Z

@RubenVerborgh
As you can see a full clean NSS master has the same ACL time outs. Not related from any modification I introduced except may be from my process :
clone NSS master on github --> my github --> git on my server --> PR to my github --> PR to NSS github

I can try to suppress the my github in between.

What shall I do with my PR : wait until master succeed tests or represent it so you can approve it ?

RubenVerborgh · 2019-08-05T19:43:43Z

OK, thx, will need to involve the NSS team then.

bourgeoa · 2019-08-06T09:30:56Z

This takes back PR #1280 after passing tests on clean master.

The fix works on NSS multiuser under docker and solid-ide
On solid-ide create/update file gives as expected $.unknown (until solid-file-client update)
It also allow slug with extension.

I join tests result on my server git and node with completely different errors (I suppose related to my server port usage)

API
"before all" hook:
Uncaught Error: listen EADDRINUSE :::5000
at Object._errnoException (util.js:1022:11)
at _exceptionWithHostPort (util.js:1044:20)
at Server.setupListenHandle [as _listen2] (net.js:1351:14)
at listenInCluster (net.js:1392:12)
at Server.listen (net.js:1476:7)
at Promise (test/integration/capability-discovery-test.js:37:11)
at new Promise ()
at startServer (test/integration/capability-discovery-test.js:36:12)
at Context.before (test/integration/capability-discovery-test.js:43:7)

Header handler
WAC-Allow
a resource that is read/write/append/control for the user, nothing for the public
"before all" hook:
Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/volume1/homes/admin/github/bourgeoa/node-solid-server/test/integration/header-test.js)
HTTP COPY API
"before all" hook:
Uncaught Error: listen EADDRINUSE :::8443
at Object._errnoException (util.js:1022:11)
at _exceptionWithHostPort (util.js:1044:20)
at Server.setupListenHandle [as _listen2] (net.js:1351:14)
at listenInCluster (net.js:1392:12)
at Server.listen (net.js:1476:7)
at Context. (test/integration/http-copy-test.js:22:26)

=============================== Coverage summary ===============================
Statements : 79.43% ( 2499/3146 )
Branches : 69.02% ( 791/1146 )
Functions : 76.06% ( 483/635 )
Lines : 79.7% ( 2454/3079 )

npm ERR! code ELIFECYCLE
npm ERR! errno 3
npm ERR! [email protected] nyc: cross-env NODE_TLS_REJECT_UNAUTHORIZED=0 nyc --reporter=text-summary mocha
npm ERR! Exit status 3
npm ERR!
npm ERR! Failed at the [email protected] nyc script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

bourgeoa · 2019-08-10T13:09:28Z

I have redone tests on my server for last NSS4.4.2 and NSSv5.1.1, v5.1.4, v5.1.6 and my PR.
To adapt to my server i had to :

Has my server is using port 5000, I changed in test port 5000 to 3456 as it used to be in NSSv4.4.2
stop my docker instance of solid on 8443 to free the port 8443
The results are :
only NSSv4.4.2 passes all tests
all tested NSSv5 version including my PR had only one error left and the same for all NSSv5 version tested. (exemple of test results for PR fix issues #1120 #1256 from NSS5.1.6 ressource-mapper #1282)

805 passing (1m)
22 pending
1 failing

Header handler
WAC-Allow
a resource that is read/write/append/control for the user, nothing for the public
"before all" hook:
Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/volume1/homes/admin/github/awwright/node-solid-server/test/integration/header-test.js)

As a conclusion there is no regression with PR #1282 and it appears to be a long awaited solution to solve problems introduced by NSSv5.1.6 with the new resource-mapper.js.

The actual NSSv5.1.6 version has stopped a lot of developers in their work.

bourgeoa · 2019-08-10T13:10:02Z

I'd like to add a comment on a process that could be introduced to upgarde versions :
create a upgrade community user panel

ask for validation on personal server on a tagged version proposal
after validation push to dev.inrupt.net or an other dedicated server to prereleases and ask for validation to a group of developers/users
push the new version to solid.community. That could be the official release

If preferred step 1. and 2. couId be in only one step.The main purpose being a validation process on step 2.
It is just a proposal and I don't know where to propose it.

michielbdejong · 2019-08-14T13:08:59Z

@bourgeoa as discussed on the phone, I'll have a look on Monday!

bourgeoa · 2019-08-15T22:37:54Z

@michielbdejong
I pushed the last change but I have this conflicting file package-lock.json and I don't know how to resolve the conflicts. I hope you can push the PR to a new NSS version.

I tested all the changes on my server for NSSv5.1.6 and the new NSSv5.1.7 (multiuser and docker ) and everything is OK no new test error.
The server in NSSv5.1.7 seems to works OK and solid-ide with the old solid-file-client works as expected. The new databrowser is a nice improvement.

for information I filled an issue # 94 on solid-UI on a nasty bug that allow to delete .acl from /public (and you cannot recreate it) and worse from /(root) (you loose access to your pod).

bourgeoa · 2019-08-18T21:19:28Z

I pushed the solid/node-solid-server/package-lock.json and it solved the conflict.
All test passes

michielbdejong · 2019-08-19T14:34:39Z

Great, I'll have a look, maybe I can find time tonight, otherwise I'll do it tomorrow.

michielbdejong · 2019-08-19T18:41:33Z

@bourgeoa I had a look as promised, and must say this whole situation feels way too scary to touch without refactoring the resource-mapper to split out its different functions:

store content-type in $. extension on disk when storing, and retrieve it from there serving
deal with serving index.html if present and text/html is requested for a container
choosing whether the on-disk folder structure should include the host or not
setting a default Content-Type if missing
there are two separate and seemingly different code paths for mapping a URL to a file, one which creates resources if they don't exist, and one that doesn't

What further complicates this is that there seem to be three competing PRs open:

I wouldn't know which one to pick, or where to start to even review your PR with confidence, sorry! The last time we touched the resource mapper it took us several weeks to recover from the fallout so I want to be conservative here.

As I said, please start testing your code against https://github.com/inrupt/pod-server which is the rewrite of NSS in TypeScript, and which already has a much higher compliance level on https://github.com/solid/test-suite. Sorry I couldn't be of more help to you - I did try!

RubenVerborgh · 2019-08-19T18:44:00Z

lib/resource-mapper.js

+      // create a new file
+      if (!(!searchIndex && isFolder)) {
+        // Append index filename if needed
+        if (searchIndex && isFolder) {


The sequence of those two if conditions is weird; we can do better

We already had a discussion about that in PR #1280 that begun this PR #1282
I initially proposed :

// create a new container if (!searchIndex && isFolder) { // else create a new resource } else { if (searchIndex && isFolder) {

for me it was clearer but you tell me you did not like void if statement.
It was for me the clearest way. The logic being explicit.
Would you prefer this :

// create a new container if (!searchIndex && isFolder) { return { path } // (do I need the {} around path) // else create a new resource } if (searchIndex && isFolder) {

Yeah I might have not expressed myself clearly.

What I mean is that

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

is equivalent to

if (searchIndex !! !isFolder) { if (isFolder) {

An other proposal will be : (symbolic code)

// create a fileIndex
if (searchIndex && isFolder) -- > path += this.fileIndex and check contentType or throw error
// if create a resource check extension
if (!isFolder) --> check extension against contentType as actually

as before (create a folder will be implicit), then

return { path, contentType : contentType || this._defaultContentType }

Also fine with me. Just not the repeated conditions.

RubenVerborgh · 2019-08-19T18:46:58Z

without refactoring the resource-mapper to split out its different functions:

Those listed are actually already split out, with the exception of the second one (because that is content negotiation and does not belong in the ResourceMapper, which is just an implementation of https://www.w3.org/DesignIssues/HTTPFilenameMapping.html).

So I'm fine with the ResourceMapper changes and functionality (minus the comment I placed). It's well tested and under control.

Make application/octet-stream the default Content-Type #1258 by @RubenVerborgh

It's not competing, it's complementary.

RubenVerborgh · 2019-08-19T18:47:54Z

lib/resource-mapper.js


-    // Create the path for a new file
+    // Create the path for a new ressource


bourgeoa · 2019-08-19T21:57:18Z

@RubenVerborgh @michielbdejong

If you check the travis test only the version under node 10 fails.

hope you like better the if sequence that has disappeared.
Tests made locally on NSSv5.1.7 with no difference to previous ones.
on my server under docker NSSv5.1.7 with the patch runs normally and solid-ide with solid-file-client too.
PR # 1260 from @jaxoncreed was taken in consideration as from the beginning in my PR.

jaxoncreed

In general, I think this looks okay. Given the number of pull requests that have been made on this issue, I think it should be merged in.

jaxoncreed · 2019-08-26T20:13:50Z

lib/handlers/get.js

@@ -45,7 +45,7 @@ async function handler (req, res, next) {

  let ret
  try {
-    ret = await ldp.get(options)
+    ret = await ldp.get(options, req.accepts('html'))


This might be better handled as

req.accepts(['text/turtle', 'application/ld+json', 'html']) === 'html'

so that we can handle requests where rdf has a higher weighted q value.

Shall I push your proposal (might be better ?) or do we keep it like it is ?
__
Just tried your proposal locally and npm run test, it fails on :

formats
turtle
should return turtle when listing container with an index page:
Error: expected "content-type" matching /text/html/, got "text/turtle"

So I propose to leave it like it is for node-solid-server EOL

jaxoncreed · 2019-10-21T20:47:29Z

Closing because fixed by a different pr

bourgeoa added 2 commits August 5, 2019 16:40

allow slug with extension

58e1a47

delete allow slug with extension

05153ae

RubenVerborgh assigned michielbdejong and jaxoncreed Aug 5, 2019

bourgeoa added 2 commits August 6, 2019 10:49

fix to ressource-mapper and slug with extension

9abcbb9

correction of add /lib/get.js by mistake

2f0ce48

bourgeoa mentioned this pull request Aug 10, 2019

header.test.js : WAC-Allow third test is broken #1285

Closed

bourgeoa added 2 commits August 15, 2019 23:41

do not add extention to .acl or .meta files

80f7aea

do not add extention to .acl or .meta files

4b8c079

resolve package-lock.json

1c5038c

RubenVerborgh reviewed Aug 19, 2019

View reviewed changes

simplify double if in resource-mapper.js

85ee73b

jaxoncreed mentioned this pull request Aug 26, 2019

Resource-mapper now considers content type when inserting index.html #1260

Closed

jaxoncreed reviewed Aug 26, 2019

View reviewed changes

This was referenced Sep 3, 2019

/profile/card should not use searchIndex in resource-mapper.mapUrlTofile #1297

Closed

.acl resource created with POST are created as .acl.ttl #1298

Closed

jaxoncreed mentioned this pull request Sep 27, 2019

GET on ANY container with an index.html returns HTML, not Turtle #1264

Closed

This was referenced Sep 27, 2019

When accessing the the top of a POD as data the server issues status 500 #1261

Closed

Unexpected behavior when client omits Content-Type on resource creation #1256

Closed

jaxoncreed closed this Oct 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bourgeoa commented Aug 5, 2019

bourgeoa commented Aug 5, 2019 •

edited

Loading

RubenVerborgh commented Aug 5, 2019

bourgeoa commented Aug 6, 2019

bourgeoa commented Aug 10, 2019

bourgeoa commented Aug 10, 2019

michielbdejong commented Aug 14, 2019

bourgeoa commented Aug 15, 2019

bourgeoa commented Aug 18, 2019

michielbdejong commented Aug 19, 2019

michielbdejong commented Aug 19, 2019

RubenVerborgh Aug 19, 2019

bourgeoa Aug 19, 2019 •

edited

Loading

RubenVerborgh Aug 19, 2019

bourgeoa Aug 19, 2019

RubenVerborgh Aug 19, 2019

RubenVerborgh commented Aug 19, 2019 •

edited

Loading

RubenVerborgh Aug 19, 2019

bourgeoa Aug 19, 2019

bourgeoa commented Aug 19, 2019 •

edited

Loading

jaxoncreed left a comment

jaxoncreed Aug 26, 2019

bourgeoa Aug 26, 2019 •

edited

Loading

jaxoncreed commented Oct 21, 2019


		// Create the path for a new file
		// Create the path for a new ressource

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

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

Conversation

bourgeoa commented Aug 5, 2019

bourgeoa commented Aug 5, 2019 • edited Loading

RubenVerborgh commented Aug 5, 2019

bourgeoa commented Aug 6, 2019

=============================== Coverage summary =============================== Statements : 79.43% ( 2499/3146 ) Branches : 69.02% ( 791/1146 ) Functions : 76.06% ( 483/635 ) Lines : 79.7% ( 2454/3079 )

bourgeoa commented Aug 10, 2019

bourgeoa commented Aug 10, 2019

michielbdejong commented Aug 14, 2019

bourgeoa commented Aug 15, 2019

bourgeoa commented Aug 18, 2019

michielbdejong commented Aug 19, 2019

michielbdejong commented Aug 19, 2019

RubenVerborgh Aug 19, 2019

Choose a reason for hiding this comment

bourgeoa Aug 19, 2019 • edited Loading

Choose a reason for hiding this comment

RubenVerborgh Aug 19, 2019

Choose a reason for hiding this comment

bourgeoa Aug 19, 2019

Choose a reason for hiding this comment

RubenVerborgh Aug 19, 2019

Choose a reason for hiding this comment

RubenVerborgh commented Aug 19, 2019 • edited Loading

RubenVerborgh Aug 19, 2019

Choose a reason for hiding this comment

bourgeoa Aug 19, 2019

Choose a reason for hiding this comment

bourgeoa commented Aug 19, 2019 • edited Loading

jaxoncreed left a comment

Choose a reason for hiding this comment

jaxoncreed Aug 26, 2019

Choose a reason for hiding this comment

bourgeoa Aug 26, 2019 • edited Loading

Choose a reason for hiding this comment

jaxoncreed commented Oct 21, 2019

bourgeoa commented Aug 5, 2019 •

edited

Loading

=============================== Coverage summary ===============================
Statements : 79.43% ( 2499/3146 )
Branches : 69.02% ( 791/1146 )
Functions : 76.06% ( 483/635 )
Lines : 79.7% ( 2454/3079 )

bourgeoa Aug 19, 2019 •

edited

Loading

RubenVerborgh commented Aug 19, 2019 •

edited

Loading

bourgeoa commented Aug 19, 2019 •

edited

Loading

bourgeoa Aug 26, 2019 •

edited

Loading