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

ensureDir shouldn't fail when a directory already exists, but mkdir throws a write permission error for it #1012

Closed
rotemdan opened this issue Aug 5, 2023 · 7 comments

Comments

@rotemdan
Copy link

rotemdan commented Aug 5, 2023

  • Operating System: Windows 10 x64
  • Node.js version: v18.17.0
  • fs-extra version: 11.1.1

Reproduction:

import { ensureDir } from "fs-extra"

await ensureDir("D:\\")

Result:

[Error: EPERM: operation not permitted, mkdir 'D:\'] {
  errno: -4048,
  code: 'EPERM',
  syscall: 'mkdir',
  path: 'D:\\'
}

I think that since the method is meant to 'ensure' a directory (which would include the root directory), it should ensure the drive (D: in this case) exists, and do nothing otherwise.

Workaround

Here is my workaround, which also tries to ensure the specified drive actually exists:

import fsExtra from "fs-extra"
import { existsSync } from "fs"
import path from "path"

export async function ensureDir(dirPath) {
	dirPath = path.normalize(dirPath)

	const parsedPath = path.parse(dirPath)

	if (parsedPath.root && !existsSync(parsedPath.root)) {
		throw new Error(`The root path '${parsedPath.root}' does not exist`)
	}

	if (parsedPath.dir != parsedPath.root) {
		return fsExtra.ensureDir(dirPath)
	}
}
@RyanZim
Copy link
Collaborator

RyanZim commented Aug 5, 2023

ensureDir is basically a wrapper around fs.mkdir() with the recursive option set:

module.exports.makeDir = async (dir, options) => {
checkPath(dir)
return fs.mkdir(dir, {
mode: getMode(options),
recursive: true
})
}

The Node.js docs state:

Calling fs.mkdir() when path is a directory that exists results in an error only when recursive is false.

So this seems to actually a bug in Node.js itself. Can you verify and file a Node issue? Also would be good to check if fs.mkdirSync() has the same bug.

@rotemdan
Copy link
Author

rotemdan commented Aug 5, 2023

The issue I was describing is that ensureDir was failing with a permission error when it was given a root path of a drive, like (D:\) :

The logic I was using in my workaround attempt is:

  • A root directory always exists if the drive exists.
  • If the user said ensureDir('D:\\'), and the drive D exists, it means that everything is fine! We don't need to care if the current user has permission to write in its root directory. Maybe the user wanted to just ensure the drive itself exists, and doesn't care about being about being able to write to its root directory?

I interpreted the idea of "ensuring a directory exists" to mean:

  • Ensure it's even possible that it exists (that the drive exists)
  • Create it if needed.

And in the case of a root directory, I don't see it being necessary to error due permissions issues (as they may actually be common).

Edit: We can extend this idea and say that for any directory, as long as it exists it's fine and we don't care if the user has permissions to write to it? (if that's an issue as well?)

@rotemdan
Copy link
Author

rotemdan commented Aug 5, 2023

Here's my modified workaround, which tries to first check if the path exists and is a directory, and will then return and ignore permission errors in that case:

export async function ensureDir(dirPath: string) {
	dirPath = path.normalize(dirPath)

	if (existsSync(dirPath)) {
		const dirStats = await stat(dirPath)

		if (!dirStats.isDirectory()) {
			throw new Error( `${dirPath} exists but is not a directory.`)
		}
	} else {
		return fsExtra.ensureDir(dirPath)
	}
}

@RyanZim
Copy link
Collaborator

RyanZim commented Aug 7, 2023

Yeah, I'm not sure what the logic for permission issues should be. But in any case, this seems to be something that should be discussed on the Node core level.

@rotemdan
Copy link
Author

rotemdan commented Aug 7, 2023

Here's the way I see it:

  • The goal of a method called mkdir is to make a new directory. It doesn't necessary imply what should happen in the directory already exists, or if there are permission errors in creating it, even in the case where it already exists.
  • The goal of a method called ensureDir is to ensure the directory exists. This means that if the directory already exists, it has done its job, and it shouldn't matter if the user has (or would have) permissions to that directory. If it doesn't exist, then obviously it's a problem if it can't be created.

By the nature of naming itself, the user may develop expectations on how the method should behave in certain situations.

If the goal of what is currently called ensureDir was just to be alias of mkdir({ recursive: true }), and nothing more, then it should have been called mkdirRecursive.

Arbitrarily declaring that ensureDir is just an alias of mkdir({ recursive: true }), and nothing more, is confusing for users, and creates false expectations.

@rotemdan rotemdan changed the title ensureDir fails with a permission error when given a root directory ensureDir shouldn't fail when a directory already exists, but mkdir throws a write permission error for it Aug 7, 2023
@RyanZim
Copy link
Collaborator

RyanZim commented Aug 7, 2023

ensureDir also has aliases mkdirp & mkdirs. It actually predates the recursive: true option; Node added this option, largely inspired by fs-extra. It was long after names were chosen that it became an alias.

@rotemdan
Copy link
Author

rotemdan commented Aug 7, 2023

I'm not sure what this means. You're basically just asserting that ensureDir is an arbitrary name, and it doesn't matter to you how it's interpreted.

I mean, if you insist that calling something ensureDir doesn't carry any sort of responsibility to "ensure" the directory exist, in the sense that it should always succeed as long as it exist, that's okay.

There's not much I can do. I guess.

I mean also given the random EPRM errors that Windows may produce, then, basically using this function may randomly fail, even when the directory exists, so that's okay too.

I've said what I could. You can close the issue if you want.

@RyanZim RyanZim closed this as completed Oct 24, 2023
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

No branches or pull requests

2 participants