Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 8 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d2f1d85
fix 1120 1256 with NSS5.1.6 ressource-mapper
bourgeoa Aug 2, 2019
bddb196
edit ressource-mapper cleanup
bourgeoa Aug 2, 2019
be1d446
edit get.js cleanup
bourgeoa Aug 2, 2019
5cef7dc
update to ressource-mapper
bourgeoa Aug 3, 2019
7e3153d
typing error replace isfolder by isFolder
bourgeoa Aug 3, 2019
60cc593
minor editing
bourgeoa Aug 4, 2019
2e39d61
return to v5.1.6 to test travis
bourgeoa Aug 5, 2019
5515d75
return to master to test travis
bourgeoa Aug 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/handlers/get.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'))
} catch (err) {
// use globHandler if magic is detected
if (err.status === 404 && glob.hasMagic(path)) {
Expand Down
2 changes: 1 addition & 1 deletion lib/ldp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ class LDP {
requestUrl = requestUrl.replace(/\/*$/, '/')

const { path: containerFilePath } = await this.resourceMapper.mapUrlToFile({ url: requestUrl })
let fileName = slug + extension
let fileName = slug.endsWith(extension) ? slug : slug + extension
if (await promisify(fs.exists)(utilPath.join(containerFilePath, fileName))) {
fileName = `${uuid.v1()}-${fileName}`
}
Expand Down
56 changes: 27 additions & 29 deletions lib/resource-mapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
* https://www.w3.org/DesignIssues/HTTPFilenameMapping.html
*
* This class implements this mapping in a single place
Expand Down Expand Up @@ -91,22 +91,28 @@ class ResourceMapper {
if (filePath.indexOf('/..') >= 0) {
throw new Error('Disallowed /.. segment in URL')
}
let isIndex = searchIndex && filePath.endsWith('/')
let isFolder = filePath.endsWith('/')
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

const?

let isIndex = searchIndex && isFolder
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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


// Create the path for a new file
let path
if (createIfNotExists) {
path = filePath
// Append index filename if needed
if (isIndex) {
if (contentType !== this._indexContentType) {
throw new Error(`Index file needs to have ${this._indexContentType} as content type`)
}
path += this._indexFilename
}
// If the extension is not correct for the content type, append the correct extension
if (searchIndex && this._getContentTypeByExtension(path) !== contentType) {
path += `$${contentType in extensions ? `.${extensions[contentType][0]}` : '.unknown'}`
// create a new folder
if (!searchIndex && isFolder) {
// else create a new file
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

if statement with empty body is to be avoided

Copy link
Member Author

Choose a reason for hiding this comment

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

I shall replace with

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just if (searchIndex !! !isFolder).

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// Append index filename if needed
if (isIndex) {
RubenVerborgh marked this conversation as resolved.
Show resolved Hide resolved
if (contentType !== this._indexContentType) {
throw new Error(`Index file needs to have ${this._indexContentType} as content type`)
}
path += this._indexFilename
}
// If the extension is not correct for the content type, append the correct extension
if (this._getContentTypeByExtension(path) !== contentType) {
path += `$${contentType in extensions ? `.${extensions[contentType][0]}` : '.unknown'}`
}
}
// Determine the path of an existing file
} else {
Expand All @@ -116,23 +122,15 @@ class ResourceMapper {

// Find a file with the same name (minus the dollar extension)
let match = ''
if (searchIndex) {
const files = await this._readdir(folder)
// Search for files with the same name (disregarding a dollar extension)
if (!isIndex) {
match = files.find(f => this._removeDollarExtension(f) === filename)
// Check if the index file exists
} else if (files.includes(this._indexFilename)) {
match = this._indexFilename
}
}
// Error if no match was found (unless URL ends with '/', then fall back to the folder)
if (match === undefined) {
if (isIndex) {
match = ''
} else {
throw new HTTPError(404, `Resource not found: ${pathname}`)
}
const files = await this._readdir(folder)
// Search for files with the same name (disregarding a dollar extension)
if (!isFolder) {
match = files.find(f => this._removeDollarExtension(f) === filename)
if (match === undefined) {throw new HTTPError(404, `Resource not found: ${pathname}`)}

// Check if the index file exists
} else if (searchIndex && files.includes(this._indexFilename)) {
match = this._indexFilename
}
path = `${folder}${match}`
contentType = this._getContentTypeByExtension(match)
Expand Down