fs.rename doesn't work as documented

[TanninOne]

Version: 10.7.0
Platform: Windows 10 x64
Subsystem: fs

The documentation for fs.rename is not entirely correct or at least incomplete.
It is documented to

Asynchronously rename file at oldPath to the pathname provided as newPath. In the case that newPath already exists, it will be overwritten.

At least on Windows it will also work on directories but it will not overwrite a destination directory but instead throw a EPERM error indicating a permission problem.
If oldPath is a directory and newPath is a file, the file will be replaced, if oldPath is a file and newPath is a directory, the exception is thrown.

Now I'm going to say it's probably a good thing that it's not replace an entire directory but the EPERM error message is misleading and it would probably be better to be precise in the documentation:

Asynchronously rename file or directory at oldPath to the pathname provided as newPath. In the case that newPath is a file and already exists, it will be overwritten, if there is a directory at newPath an error will be raised instead.

[benjamingr]

@TanninOne I think this is worth fixing - if you would like - a docs PR would be appreciated.

If you are not sure how to write one let us know and we'll guide you though. (I promise it's not too hard or scary!)

If you do not let us know and I'll add a label inviting people to contributing it - but I figured it'd be nice to give you the first shot since you noticed the problem and know how to solve it :)

[Shivang44]

I think this is worth investigating a little more to see in what ways the documentation and the code should be updated.

On Linux and Windows:

1a) renaming fileA to fileB succeeds regardless of if fileB exists or not.
1b) renaming directoryA to directoryB, if directoryB doesn't exist, succeeds.

I believe both 1a and 1b are "correct" and expected behaviors. However, there's difference between Linux and Windows when renaming directories:

On Linux:

2a) renaming directoryA to directoryB, if directoryB exists and is empty, succeeds.
2b) renaming directoryA to directoryB, if directoryB exists and is NOT empty, throws a ENOTEMPTY error.

I like these behaviors, they are safe and also provide good error messages so you can debug the issue.

However, on Windows:

3a) Renaming directoryA to directoryB, regardless of if directoryB is empty, throws an EPERM error.

This behavior is unexpected and also misleading, as OP said.

So, in summary, I think we should match the behavior on both platforms when renaming directories. That desired behavior, is open to discussion of course, since it's not documented. It think would make sense to adopt the linux behavior on windows since it's both safe and is more descriptive of the actual problem. So on windows,

• Renaming dirA to dirB, if dirB is empty, should succeed, like on linux, rather than throw a EPERM error as it does right now.
• Renaming dirA to dirB, if dirB is not empty, should fail, like on linux, but with a ENOTEMPTY error rather than a EPERM error it throws right now.

We could then also fix the docs to mention the behavior of renaming directories? I can start working on a PR for this right now.

Please let me know if anybody disagrees or has any feedback on my test/results/discussion!

[benjamingr]

It think would make sense to adopt the linux behavior on windows since it's both safe and is more descriptive of the actual problem. So on windows,

I assume it works this way because of platform I/O limitations and behaviour rather than a deliberate choice by Node.js - feel free to open a libuv issue. In the meantime until it gets addressed in libuv we should probably fix the documentation to not be incorrect.

[BridgeAR]

It would be best to actually have the identical behavior on the long term. In general it would be great if our FS module would behave identical in all cases. However, that is not possible at least on a short term perspective. Therefore having the doc change right now is definitely a good thing.

[Shivang44]

I am currently working on a PR in libuv that changes the behavior on windows to match Linux. However I'm not sure if Nodejs always uses the latest version of libuv or how the libuv dependency versioning works. In the meantime, I agree with everyone saying we should update the documentation (looks like we have a PR in the works for that too!).

[Shivang44]

Hey everyone, I have made the relevant PR in libuv: libuv/libuv#1941

With these new libuv changes, renaming is consistent between Unix and Windows (from my testing):

• Renaming dirA to dirB, if dirB is empty, now succeeds on both Unix and Windows
• Renaming dirA to dirB, if dirB is not empty, now fails with a ENOTEMPTY error on both Unix and Windows.

I'm sure there are some problems with my code (I am very new to C/C++) so I'm sure there will be many revisions of the PR before it is accepted.

[gentios]

Hi everyone as a newcomer I would like to update the documentation and contribute to this good first issue

[gabrielchung]

Hi, I am new to open source development. May I know where I contribute my test case? Thanks.

const fs = require('fs');

const src_dir_path = './1';
const src_path  = './1/1.txt';
const dest_path = './2';

console.log('cleaning - START');
if (fs.existsSync(src_path)) { fs.unlinkSync(src_path); }
if (fs.existsSync(dest_path)) {fs.unlinkSync(dest_path); }
console.log('cleaning - END');

console.log('preparing - START');
fs.mkdirSync(src_dir_path);
fs.writeFileSync(src_path);
fs.mkdirSync(dest_path);
console.log('preparing - END')

console.log('src_path exists - ' + (fs.existsSync(src_path) ? 'TRUE' : 'FALSE'));
console.log('dest_path exists - ' + (fs.existsSync(dest_path) ? 'TRUE' : 'FALSE'));

fs.rename(src_path, dest_path, function(err){
    if (err) {
        console.log('err: ' + err);
    } else {
        console.log('rename - ok');
    }
});

[shwetajoshi601]

Hello,

I am new to open source. I would like to update the documentation and contribute. Could someone guide me on how I can do this?

[andyalcantara]

@TanninOne, @benjamingr do you mind if I make this PR?

[TanninOne]

I don't mind, thank you!

[AdityaSrivast]

@Trott I would like to work on this issue. Can you please guide me?

[Trott]

@Trott I would like to work on this issue. Can you please guide me?

High level guidance would be: Follow the instructions at https://www.nodetodo.org/getting-started/ (but ignore the last bullet point which is about how to find an issue to work on). Then, find the relevant markdown-formatted text to be updated in doc/api/fs.md, and update it. Hope that helps!

[Trott]

(That will do a lot of compiling that you might not really need to do for this specific change, but it will set you up for any code changes that you might want to do later. I'd recommend doing it. And if you run into problems, reporting the issues here will help us improve things.)

[ghost]

Seems to have been addressed by e7ca398. Is this good to close?

[sarthak0906]

It does seem good for the issue, and this issue can be closed.

[anantakrishna]

Seems to have been addressed by e7ca398. Is this good to close?

I don't think it properly addresses the mismatch described above in #21957 (comment). Another PR #22014 was supposed to address it more appropriately, but it was closed abruptly.

I'm new here, maybe I misunderstand something…