realtymaps / promise-ftp

a promise-based ftp client for node.js
MIT License
81 stars 27 forks source link

rmdir recursive not working correctly #13

Open Eschon opened 8 years ago

Eschon commented 8 years ago

I have a directory that is nested like this:

/images
    /icons
       icon.png
       anotherIcon.png
       …

Now if I try to do

connection.rmdir('/images', true);

I get the following error:

{ Error: /htdocs/images/icons: Das Verzeichnis ist nicht leer
    at makeError (/Users/egon/src/iski.world/node_modules/ftp/lib/connection.js:1067:13)
    at Parser.<anonymous> (/Users/egon/src/iski.world/node_modules/ftp/lib/connection.js:113:25)
    at emitTwo (events.js:106:13)
    at Parser.emit (events.js:191:7)
    at Parser._write (/Users/egon/src/iski.world/node_modules/ftp/lib/parser.js:59:10)
    at doWrite (_stream_writable.js:307:12)
    at writeOrBuffer (_stream_writable.js:293:5)
    at Parser.Writable.write (_stream_writable.js:220:11)
    at Socket.ondata (/Users/egon/src/iski.world/node_modules/ftp/lib/connection.js:273:20)
    at emitOne (events.js:96:13) code: 550 }

I'm not sure if the error message comes from your library or some dependency so just in case the translation for "Das Verzeichnis ist nicht leer" is "The directory isn't empty".

On my js directory that has no subdirectories it works without problems.

zacronos commented 8 years ago

Huh. Well I'm certainly not generating any German-language errors. :-)

I'm guessing that text is from the FTP server, and the error is being generated by the ftp library based on the response from the server. Does it make sense that the FTP server would produce German-language errors?

I don't currently have an FTP server I can easily use for testing, but my guess off the top of my head is that it could be your server that doesn't support recursive delete, even though the ftp library is attempting it. The alternative is that the ftp library is failing to recognize the recursive flag.

Could you attempt a recursive rmdir via a CLI or GUI FTP client? Or alternatively, could you try using promise-ftp on other servers and see if you get the same result? Those tests would help determine whether this is a dependency bug or just a server configuration issue.

Eschon commented 8 years ago

Does it make sense that the FTP server would produce German-language errors?

Since it is either hosted in Germany or Austria, it would make sense.

it could be your server that doesn't support recursive delete

I don't think that's the case since it works on my js directory. Maybe it doesn't support it on directories with deeper nesting?

Could you attempt a recursive rmdir via a CLI or GUI FTP client?

I tried deleting a folder with Cyberduck and it works without problems but I'm not sure how it does the deleting. Maybe it just goes through all subdirectories recursively and that's why it works.

Or alternatively, could you try using promise-ftp on other servers and see if you get the same result?

I tried it on a different Server but I got the same Error. But I think that all FTP servers I have available are by the same host so this test probably doesn't help much.

zacronos commented 8 years ago

Ok, tell me if I have this right -- you can delete a directory with files in it, but not a directory with another directory in it?

Aha, reading my own documentation more closely (which is mostly copied from the ftp documentation), this almost makes sense: "If recursive, this call will delete the contents of the directory if it is not empty." I bet this only deletes the files in that directory, not at further levels of nesting, as you guessed.

Since this library is 95% just providing a promise-based API that passes through to the underlying ftp library, I'm sorry to say this isn't really a bug in this library, and I don't have the time to implement a true recursive rmdir. However, I'll mark this issue as a feature request, and if you or someone else wants to implement custom recursive rmdir logic, I'd be happy to accept the PR. (Or, you could post an issue to ftp and see if they will fix it on their end.)

For now, I'll just change the documentation here to make the situation clear.

Eschon commented 8 years ago

Ok, thanks. That makes sense.

Actually there is already an open issue on the node-ftp repository: https://github.com/mscdex/node-ftp/issues/159

Eschon commented 8 years ago

Ok I just tried to reproduce my issue with node-ftp but it works there, so it seems like the issue is on your side.

Here's my node-ftp code that I used for testing:

const Client = require('ftp');
const ftpCredentials = require('./ftpCredentials');

const c = new Client();

c.on('ready', () => {
  c.rmdir('test', true, (err) => {
    if (err) throw err;
    c.end();
  });
});

c.connect(ftpCredentials));
hpohlmeyer commented 6 years ago

Seems to work for me.

const PromiseFtp = require('promise-ftp');

function testRecursiveDelete(credentials) {
  try {
    const ftp = new PromiseFtp();

    // connect to the server
    await ftp.connect(credentials);

    // create folder with subdirectories
    await ftp.mkdir('/foo/bar/baz', true);

    // upload a file to the deepest subdirectory
    await ftp.put('/path/to/my/file.txt', '/foo/bar/baz/file.txt');

    // delete the main folder and its contents
    await ftp.rmdir('/foo', true);
  } catch (err) {
    console.error(err);
  }
}

Would be nice, if anyone could double-check that.
Than this could be closed and the documentation can be adjusted.