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

ensureFile misleading error message #696

Closed
TanninOne opened this issue Jun 7, 2019 · 9 comments
Closed

ensureFile misleading error message #696

TanninOne opened this issue Jun 7, 2019 · 9 comments
Labels
Milestone

Comments

@TanninOne
Copy link

  • Operating System: Windows 10
  • Node.js version: 10.11.0
  • fs-extra version: 4.0.3

Say you have a file /foo/bar.txt
and call ensureFile('/foo/bar.txt/narf.txt') the error message you get is ENOENT for the path /foo/bar.txt/narf.txt which is confusing, because the purpose of the call is to create the file, why is it failing with the file being missing?

The cause is that pathExists (or fs.exists) only tests for the existence of the filesystem object, it doesn't verify whether it's a directory, so in this case the function proceeds to try to create /foo/bar.txt/narf.txt even though bar.txt is not a directory.

I think instead the error message should be EEXIST with path /foo/bar.txt - signaling that there is a file "bar.txt" that can't be replaced with the required intermediate directory required.

I do realize this is a minor thing but I feel like good error messages are important to quickly identify issues.

@RyanZim
Copy link
Collaborator

RyanZim commented Jun 7, 2019

Agreed, ENONET is a confusing error.

@RyanZim RyanZim added the bug label Jun 7, 2019
@lukechilds
Copy link
Contributor

@RyanZim do you want a PR for this?

@jprichardson has asked me to help out with this project and I'd like to start on something simple to familiarise myself with the codebase a little.

@RyanZim
Copy link
Collaborator

RyanZim commented Jun 19, 2019

@lukechilds sure, go ahead!

@lukechilds
Copy link
Contributor

I think this is already fixed, just tried writing a test to target the behaviour described in this issue but it fails:

  1) fs-extra
       + createFileSync
         > when the file does exist
           should give clear error if node in directory tree is a file:
     AssertionError [ERR_ASSERTION]: Input A expected to strictly deep-equal input B:
+ expected - actual

  Comparison {
-   code: 'ENOTDIR'
+   code: 'ENOENT'
  }

I'm not getting ENOENT as described in the issue, but ENOTDIR which I think is pretty clear.

Also noticed in the original issue:

  • fs-extra version: 4.0.3

The current version is 8.0.1.

@TanninOne can you try upgrading to fs-extra@8.0.1 and let me know if the issue is resolved for you?

@TanninOne
Copy link
Author

No, I still get the error exactly as I described with fs-extra@8.0.1
I also don't see why it would, I can tell you exactly where in the code things go wrong:
In lib/ensure/file.js line 39 it calls "fs.existsSync(dir)" to determine if the parent exists, so if you had called createFileSync('foo/bar') it would test if "foo" exists, it does not test if foo is actually a directory.
Then the code goes on to call "writeFileSync('foo/bar', '')" which fails because foo is a file.

The same is true in the async code except it calls "pathExists" which uses fs.access to check for the existence of "foo" but doesn't verify it's a directory.
I can't rule out that different operating systems may produce different error codes because libuv isn't exactly great at translating windows error codes to posix codes (which is a pet-peeve of mine) but fs-extra tests for the existence of the parent directory anyway. You could just use fs.stat to check for existence and then also check the isDirectory flag. If it's false, you throw a ENOTDIR error. That is then consistent independent of the operating system.

@lukechilds
Copy link
Contributor

lukechilds commented Jun 20, 2019

@TanninOne thanks for the quick response.

I totally missed that you were on Windows. This test is passing for me on macOS and Linux:

https://travis-ci.com/lukechilds/node-fs-extra/builds/116252408

Is this test accurately describing your problem?

it('should give clear error if node in directory tree is a file', () => {
  const existingFile = path.join(TEST_DIR, Math.random() + 'ts-e', Math.random() + '.txt')
  fse.mkdirsSync(path.dirname(existingFile))
  fs.writeFileSync(existingFile)

  const file = path.join(existingFile, Math.random() + '.txt')
  try {
    fse.createFileSync(file)
    assert.fail()
  } catch (err) {
    assert.strictEqual(err.code, 'ENOTDIR')
  }
})

e.g The test should fail with the behaviour you're experiencing but pass once it's resolved? Or am I misunderstanding the issue?

I'll spin up a Windows VM and test in there.

@lukechilds
Copy link
Contributor

lukechilds commented Jun 20, 2019

Ok, had some issues getting a Windows VM working but tested a Windows build on AppVeyor and it fails. So does indeed seem to be a Windows only problem: https://ci.appveyor.com/project/lukechilds/node-fs-extra/builds/25413529/job/m231uubewxg78kf0

@TanninOne
Copy link
Author

Yes, the test reproduces the issue.

Thank you for looking into this!

@RyanZim RyanZim added this to the 9.0.0 milestone Jan 30, 2020
@RyanZim
Copy link
Collaborator

RyanZim commented Feb 1, 2020

Fixed in #744; will be released in 9.0.0.

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

Successfully merging a pull request may close this issue.

3 participants