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

fix: reply all files and directries within the folders defined in root #276

Merged
merged 2 commits into from
Mar 28, 2022

Conversation

is2ei
Copy link
Contributor

@is2ei is2ei commented Mar 21, 2022

Fixes #203

Checklist

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

This feature is not documented or do I overlook it?

What is going to happen when the same file name is in both directories?

lib/dirList.js Outdated
* @param {ListOptions} options
* @param {string} route request route
*/
send: async function ({ reply, dir, options, route, prefix }) {
let entries
try {
entries = await dirList.list(dir, options)
if (Array.isArray(dir)) {
entries = await Promise.all(dir.map(async (d) => await dirList.list(d, options)))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
entries = await Promise.all(dir.map(async (d) => await dirList.list(d, options)))
entries = await Promise.all(dir.map((d) => dirList.list(d, options)))

Copy link
Contributor Author

@is2ei is2ei Mar 27, 2022

Choose a reason for hiding this comment

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

@Eomm
Thanks for your comment.
I found my change would produce an inconsistency.
I created some examples.

Let's say we have some directories like this:

$ tree test/static3/
test/static3/
├── dir1
│   ├── hello.html
│   └── inner
│       └── index.html
└── dir2
    ├── hello.html
    ├── index.html
    ├── inner
    └── world.html

4 directories, 5 files

Then, the test below will pass.

t.test('dir list adopts first found dir when the root is an array', t => {
  t.plan(2)

  const options = {
    root: [path.join(__dirname, '/static3/dir1'), path.join(__dirname, '/static3/dir2')],
    prefix: '/public',
    index: false,
    list: true
  }

  const route = '/public/'
  const content = { dirs: ['inner'], files: ['hello.html'] }

  helper.arrange(t, options, (url) => {
    t.test(route, t => {
      t.plan(3)
      simple.concat({
        method: 'GET',
        url: url + route
      }, (err, response, body) => {
        t.error(err)
        t.equal(response.statusCode, 200)
        t.equal(body.toString(), JSON.stringify(content))
      })
    })
  })
})

Also, the test below also passes.

t.test('reply all files and directories when root is an array', t => {
  t.plan(2)
  const options = {
    root: [path.join(__dirname, '/static3/dir1'), path.join(__dirname, '/static3/dir2')],
    prefix: '/public',
    list: {
      format: 'json',
      names: ['index.json']
    }
  }
  const route = '/public/index.json'
  const content = { dirs: ['inner', 'inner'], files: ['hello.html', 'hello.html', 'world.html'] }

  helper.arrange(t, options, (url) => {
    t.test(route, t => {
      t.plan(3)
      simple.concat({
        method: 'GET',
        url: url + route
      }, (err, response, body) => {
        t.error(err)
        t.equal(response.statusCode, 200)
        t.equal(body.toString(), JSON.stringify(content))
      })
    })
  })
})

The root options are the same, but the results are different. So I'd like to update my change.

@is2ei is2ei force-pushed the bugfix/203-accept-array-root branch from 75093e4 to eca3860 Compare March 27, 2022 05:01
@is2ei
Copy link
Contributor Author

is2ei commented Mar 27, 2022

@Eomm
Sorry for the late reply. I updated the change. Could you review it again?

index.js Outdated
@@ -213,7 +213,7 @@ async function fastifyStatic (fastify, opts) {
if (opts.list && dirList.handle(pathname, opts.list)) {
dirList.send({
reply,
dir: dirList.path(opts.root, pathname),
dir: Array.isArray(opts.root) ? opts.root[0] : dirList.path(opts.root, pathname),
Copy link
Member

Choose a reason for hiding this comment

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

This statement does not apply to the least astonishment principale.

The user would expect that the array should be processed, but it is not

I think we have two options:

  1. manage the array
  2. throw an error as multi-root is not supported

Copy link
Contributor Author

@is2ei is2ei Mar 28, 2022

Choose a reason for hiding this comment

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

@Eomm

Thanks.

I guess option No. 2 would be nice because the directory listing feature would be confusing if it supports multiple root. So, I'd like to add validation to the validateOptions function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Eomm

I updated it. Could you review it again?

@is2ei is2ei force-pushed the bugfix/203-accept-array-root branch from eca3860 to a02190c Compare March 28, 2022 09:02
@is2ei is2ei requested a review from Eomm March 28, 2022 09:09
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

README.md Outdated Show resolved Hide resolved
@Eomm Eomm merged commit c70c25d into fastify:master Mar 28, 2022
@is2ei is2ei deleted the bugfix/203-accept-array-root branch May 8, 2022 16:04
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.

list option cause fastify to crash if root is an array
3 participants