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.

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

Issue 1604: Fix 'appendRequestPath: false' scenarios for Restify.plugins.serverStatic #1860

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jmbeltran07
Copy link

@jmbeltran07 jmbeltran07 commented Oct 26, 2020

Pre-Submission Checklist

  • Opened an issue discussing these changes before opening the PR
  • Ran the linter and tests via make prepush
  • Included comprehensive and convincing tests for changes

Issues

Closes:

Restify.plugins.serverStatic option "appendRequestPath" is not working properly.
According to documentation, when appendRequestPath = false, the file we are requesting should be served directly from directory, this wasn't the case. for example:

var restify = require('restify');

const server = restify.createServer({
    name: 'myapp',
    version: '1.0.0'
  });

server.get('/endpoint/*', restify.plugins.serveStatic({
    directory: __dirname, // dirname is /Users/keithlee96/testFolder/test-api
    default: 'index.html',
    appendRequestPath: false
}));

server.listen(9090, function(){
    console.log('%s listening at %s', server.name, server.url);
});

if we make a call to /endpoint it will attempt to serve __dirname/endpoint/index.html, instead of __dirname/index.html (because we are setting appendRequestPath: false
another example is that if we call /endpoint/myCoolResource.json it will also attempt to serve __dirname/endpoint/myCoolResource.json instead of __dirname/myCoolResource.json

Changes

With this PR when the user is setting appendRequestPath = false, the server will serve the requested resource specified in the endpoint (or the default static resource) which is located directly on directory.
Now the behavior adequate with what's described on http://restify.com/docs/plugins-api/#servestatic

Also we are modifying the function testNoAppendPath used by the tests which were passing as false positives so it catches these errors in the future. Also, we are adding more tests to increase the coverage of Restify.plugins.serverStatic function.

@jmbeltran07 jmbeltran07 changed the title fix appendRequestPath = false scenarios Issue 1604: Fix appendRequestPath = false scenarios for Restify.plugins.serverStatic Oct 26, 2020
@jmbeltran07 jmbeltran07 changed the title Issue 1604: Fix appendRequestPath = false scenarios for Restify.plugins.serverStatic Issue 1604: Fix 'appendRequestPath: false' scenarios for Restify.plugins.serverStatic Oct 26, 2020
Copy link
Member

@retrohacker retrohacker left a comment

Choose a reason for hiding this comment

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

Thanks for this thoughtful contribution 🙌

);
var fileName = decodeURIComponent(path.basename(req.path()));
// test for requested file, if it doesn't exist we send the directory as path since appendRequestPath = false
if (!fs.existsSync(path.join(opts.directory, fileName))) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: mind breaking these nested function invocations up into multiple lines w/ variables indicating what each path is supposed to represent?

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Author

@jmbeltran07 jmbeltran07 Oct 28, 2020

Choose a reason for hiding this comment

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

Sure! let me work on it and I'll update the PR! great catch & suggestion!

Choose a reason for hiding this comment

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

Hi! I noticed the Issue #1604 is still open and has gotten some activity recently from people saying the issue is still unfixed. Are there any plans to submit this PR, or would you prefer if someone else creates a clone of this PR and submits that? - though that seems like unnecessary duplicate effort

@rajatkumar
Copy link
Member

rajatkumar commented Oct 28, 2020

Just for completeness, we also happen to have another flavor of serving static files with restify which has it's API syntax (and functionality) similar to that of expressjs.

We had seen a lot of issues with restify.plugins.serveStatic and hence at some point, we built restify.plugins.serveStaticFiles.

Here is a small example of how to use it, feel free to play around with it: https://codesandbox.io/s/gallant-raman-9x9jk?file=/app.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants