Skip to content

Commit

Permalink
Fix the issue of moving into itself
Browse files Browse the repository at this point in the history
  • Loading branch information
manidlou committed Jul 10, 2017
1 parent 85f531e commit 16dc81e
Show file tree
Hide file tree
Showing 3 changed files with 235 additions and 52 deletions.
192 changes: 192 additions & 0 deletions lib/move/__tests__/move-prevent-moving-into-itself.test.js
@@ -0,0 +1,192 @@
'use strict'

const assert = require('assert')
const os = require('os')
const path = require('path')
const fs = require(process.cwd())
const klawSync = require('klaw-sync')

/* global beforeEach, afterEach, describe, it */

const FILES = [
'file0.txt',
path.join('dir1', 'file1.txt'),
path.join('dir1', 'dir2', 'file2.txt'),
path.join('dir1', 'dir2', 'dir3', 'file3.txt')
]

const dat0 = 'file0'
const dat1 = 'file1'
const dat2 = 'file2'
const dat3 = 'file3'

describe('+ move() - prevent moving into itself', () => {
let TEST_DIR, src, dest

beforeEach(() => {
TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'move-prevent-moving-into-itself')
src = path.join(TEST_DIR, 'src')
fs.mkdirpSync(src)

fs.outputFileSync(path.join(src, FILES[0]), dat0)
fs.outputFileSync(path.join(src, FILES[1]), dat1)
fs.outputFileSync(path.join(src, FILES[2]), dat2)
fs.outputFileSync(path.join(src, FILES[3]), dat3)
})

afterEach(() => fs.removeSync(TEST_DIR))

describe('> when source is a file', () => {
it(`should move the file successfully even when dest parent is 'src/dest'`, done => {
const destFile = path.join(TEST_DIR, 'src', 'dest', 'destfile.txt')
return testSuccessFile(src, destFile, done)
})

it(`should move the file successfully when dest parent is 'src/src_dest'`, done => {
const destFile = path.join(TEST_DIR, 'src', 'src_dest', 'destfile.txt')
return testSuccessFile(src, destFile, done)
})

it(`should move the file successfully when dest parent is 'src/dest_src'`, done => {
const destFile = path.join(TEST_DIR, 'src', 'dest_src', 'destfile.txt')
return testSuccessFile(src, destFile, done)
})

it(`should move the file successfully when dest parent is 'src/dest/src'`, done => {
const destFile = path.join(TEST_DIR, 'src', 'dest', 'src', 'destfile.txt')
return testSuccessFile(src, destFile, done)
})

it(`should move the file successfully when dest parent is 'srcsrc/dest'`, done => {
const destFile = path.join(TEST_DIR, 'srcsrc', 'dest', 'destfile.txt')
return testSuccessFile(src, destFile, done)
})
})

describe('> when source is a directory', () => {
it(`should move the directory successfully when dest is 'src_dest'`, done => {
dest = path.join(TEST_DIR, 'src_dest')
return testSuccessDir(src, dest, done)
})

it(`should move the directory successfully when dest is 'src-dest'`, done => {
dest = path.join(TEST_DIR, 'src-dest')
return testSuccessDir(src, dest, done)
})

it(`should move the directory successfully when dest is 'dest_src'`, done => {
dest = path.join(TEST_DIR, 'dest_src')
return testSuccessDir(src, dest, done)
})

it(`should move the directory successfully when dest is 'src_dest/src'`, done => {
dest = path.join(TEST_DIR, 'src_dest', 'src')
return testSuccessDir(src, dest, done)
})

it(`should move the directory successfully when dest is 'src-dest/src'`, done => {
dest = path.join(TEST_DIR, 'src-dest', 'src')
return testSuccessDir(src, dest, done)
})

it(`should move the directory successfully when dest is 'dest_src/src'`, done => {
dest = path.join(TEST_DIR, 'dest_src', 'src')
return testSuccessDir(src, dest, done)
})

it(`should move the directory successfully when dest is 'src_src/dest'`, done => {
dest = path.join(TEST_DIR, 'src_src', 'dest')
return testSuccessDir(src, dest, done)
})

it(`should move the directory successfully when dest is 'src-src/dest'`, done => {
dest = path.join(TEST_DIR, 'src-src', 'dest')
return testSuccessDir(src, dest, done)
})

it(`should move the directory successfully when dest is 'srcsrc/dest'`, done => {
dest = path.join(TEST_DIR, 'srcsrc', 'dest')
return testSuccessDir(src, dest, done)
})

it(`should move the directory successfully when dest is 'dest/src'`, done => {
dest = path.join(TEST_DIR, 'dest', 'src')
return testSuccessDir(src, dest, done)
})

it('should move the directory successfully when dest is very nested that all its parents need to be created', done => {
dest = path.join(TEST_DIR, 'dest', 'src', 'foo', 'bar', 'baz', 'qux', 'quux', 'waldo',
'grault', 'garply', 'fred', 'plugh', 'thud', 'some', 'highly', 'deeply',
'badly', 'nasty', 'crazy', 'mad', 'nested', 'dest')
assert(!fs.existsSync(dest))
return testSuccessDir(src, dest, done)
})

it(`should return error when dest is 'src/dest'`, done => {
dest = path.join(TEST_DIR, 'src', 'dest')
return testError(src, dest, done)
})

it(`should return error when dest is 'src/src_dest'`, done => {
dest = path.join(TEST_DIR, 'src', 'src_dest')
return testError(src, dest, done)
})

it(`should return error when dest is 'src/dest_src'`, done => {
dest = path.join(TEST_DIR, 'src', 'dest_src')
return testError(src, dest, done)
})

it(`should return error when dest is 'src/dest/src'`, done => {
dest = path.join(TEST_DIR, 'src', 'dest', 'src')
return testError(src, dest, done)
})
})
})

function testSuccessFile (src, destFile, done) {
const srcFile = path.join(src, FILES[0])

fs.move(srcFile, destFile, err => {
assert.ifError(err)
const f0 = fs.readFileSync(destFile, 'utf8')
assert.strictEqual(f0, dat0, 'file contents matched')
assert(!fs.existsSync(srcFile))
return done()
})
}

function testSuccessDir (src, dest, done) {
const srclen = klawSync(src).length

assert(srclen > 2) // assert src has contents

fs.move(src, dest, err => {
assert.ifError(err)
const destlen = klawSync(dest).length

assert.strictEqual(destlen, srclen, 'src and dest length should be equal')

const f0 = fs.readFileSync(path.join(dest, FILES[0]), 'utf8')
const f1 = fs.readFileSync(path.join(dest, FILES[1]), 'utf8')
const f2 = fs.readFileSync(path.join(dest, FILES[2]), 'utf8')
const f3 = fs.readFileSync(path.join(dest, FILES[3]), 'utf8')

assert.strictEqual(f0, dat0, 'file contents matched')
assert.strictEqual(f1, dat1, 'file contents matched')
assert.strictEqual(f2, dat2, 'file contents matched')
assert.strictEqual(f3, dat3, 'file contents matched')
assert(!fs.existsSync(src))
return done()
})
}

function testError (src, dest, done) {
fs.move(src, dest, err => {
assert(err)
assert.strictEqual(err.message, `Cannot move '${src}' to a subdirectory of itself, '${dest}'.`)
assert(fs.existsSync(src))
assert(!fs.existsSync(dest))
return done()
})
}
17 changes: 2 additions & 15 deletions lib/move/__tests__/move.test.js
Expand Up @@ -173,19 +173,6 @@ describe('move', () => {
})
})

it('should not create directory structure if mkdirp is false', done => {
const src = `${TEST_DIR}/a-file`
const dest = `${TEST_DIR}/does/not/exist/a-file-dest`

// verify dest directory does not exist
assert(!fs.existsSync(path.dirname(dest)))

fse.move(src, dest, {mkdirp: false}, err => {
assert.strictEqual(err.code, 'ENOENT')
done()
})
})

it('should create directory structure by default', done => {
const src = `${TEST_DIR}/a-file`
const dest = `${TEST_DIR}/does/not/exist/a-file-dest`
Expand Down Expand Up @@ -331,7 +318,7 @@ describe('move', () => {
})
})

describe.skip('> when trying to move a folder into itself', () => {
describe('> when trying to move a folder into itself', () => {
it('should produce an error', done => {
const SRC_DIR = path.join(TEST_DIR, 'test')
const DEST_DIR = path.join(TEST_DIR, 'test', 'test')
Expand All @@ -342,7 +329,7 @@ describe('move', () => {

fse.move(SRC_DIR, DEST_DIR, err => {
assert(fs.existsSync(SRC_DIR))
assert(err)
assert.equal(err.message, `Cannot move '${SRC_DIR}' to a subdirectory of itself, '${DEST_DIR}'.`)
done()
})
})
Expand Down
78 changes: 41 additions & 37 deletions lib/move/index.js
Expand Up @@ -13,40 +13,35 @@ const path = require('path')
const remove = require('../remove').remove
const mkdirp = require('../mkdirs').mkdirs

function move (source, dest, options, callback) {
function move (src, dest, options, callback) {
if (typeof options === 'function') {
callback = options
options = {}
}

const shouldMkdirp = ('mkdirp' in options) ? options.mkdirp : true
const overwrite = options.overwrite || options.clobber || false

if (shouldMkdirp) {
mkdirs()
} else {
doRename()
}

function mkdirs () {
isSrcSubdir(src, dest, (err, itIs) => {
if (err) return callback(err)
if (itIs) return callback(new Error(`Cannot move '${src}' to a subdirectory of itself, '${dest}'.`))
mkdirp(path.dirname(dest), err => {
if (err) return callback(err)
doRename()
})
}
})

function doRename () {
if (path.resolve(source) === path.resolve(dest)) {
fs.access(source, callback)
if (path.resolve(src) === path.resolve(dest)) {
fs.access(src, callback)
} else if (overwrite) {
fs.rename(source, dest, err => {
fs.rename(src, dest, err => {
if (!err) return callback()

if (err.code === 'ENOTEMPTY' || err.code === 'EEXIST') {
remove(dest, err => {
if (err) return callback(err)
options.overwrite = false // just overwriteed it, no need to do it again
move(source, dest, options, callback)
move(src, dest, options, callback)
})
return
}
Expand All @@ -57,49 +52,44 @@ function move (source, dest, options, callback) {
remove(dest, err => {
if (err) return callback(err)
options.overwrite = false
move(source, dest, options, callback)
move(src, dest, options, callback)
})
}, 200)
return
}

if (err.code !== 'EXDEV') return callback(err)
moveAcrossDevice(source, dest, overwrite, callback)
moveAcrossDevice(src, dest, overwrite, callback)
})
} else {
fs.link(source, dest, err => {
fs.link(src, dest, err => {
if (err) {
if (err.code === 'EXDEV' || err.code === 'EISDIR' || err.code === 'EPERM' || err.code === 'ENOTSUP') {
moveAcrossDevice(source, dest, overwrite, callback)
return
return moveAcrossDevice(src, dest, overwrite, callback)
}
callback(err)
return
return callback(err)
}
fs.unlink(source, callback)
return fs.unlink(src, callback)
})
}
}
}

function moveAcrossDevice (source, dest, overwrite, callback) {
fs.stat(source, (err, stat) => {
if (err) {
callback(err)
return
}
function moveAcrossDevice (src, dest, overwrite, callback) {
fs.stat(src, (err, stat) => {
if (err) return callback(err)

if (stat.isDirectory()) {
moveDirAcrossDevice(source, dest, overwrite, callback)
moveDirAcrossDevice(src, dest, overwrite, callback)
} else {
moveFileAcrossDevice(source, dest, overwrite, callback)
moveFileAcrossDevice(src, dest, overwrite, callback)
}
})
}

function moveFileAcrossDevice (source, dest, overwrite, callback) {
function moveFileAcrossDevice (src, dest, overwrite, callback) {
const flags = overwrite ? 'w' : 'wx'
const ins = fs.createReadStream(source)
const ins = fs.createReadStream(src)
const outs = fs.createWriteStream(dest, { flags })

ins.on('error', err => {
Expand All @@ -113,7 +103,7 @@ function moveFileAcrossDevice (source, dest, overwrite, callback) {
fs.unlink(dest, () => {
// note: `err` here is from the input stream errror
if (err.code === 'EISDIR' || err.code === 'EPERM') {
moveDirAcrossDevice(source, dest, overwrite, callback)
moveDirAcrossDevice(src, dest, overwrite, callback)
} else {
callback(err)
}
Expand All @@ -131,11 +121,11 @@ function moveFileAcrossDevice (source, dest, overwrite, callback) {
ins.pipe(outs)

function onClose () {
fs.unlink(source, callback)
fs.unlink(src, callback)
}
}

function moveDirAcrossDevice (source, dest, overwrite, callback) {
function moveDirAcrossDevice (src, dest, overwrite, callback) {
const options = {
overwrite: false
}
Expand All @@ -150,13 +140,27 @@ function moveDirAcrossDevice (source, dest, overwrite, callback) {
}

function startNcp () {
ncp(source, dest, options, err => {
ncp(src, dest, options, err => {
if (err) return callback(err)
remove(source, callback)
remove(src, callback)
})
}
}

// return true if dest is a subdir of src, otherwise false.
// extract dest base dir and check if that is the same as src basename
function isSrcSubdir (src, dest, cb) {
fs.stat(src, (err, st) => {
if (err) return cb(err)
if (st.isDirectory()) {
return cb(null, src !== dest &&
dest.indexOf(src) > -1 &&
dest.split(path.dirname(src) + path.sep)[1].split(path.sep)[0] === path.basename(src))
}
return cb(null, false)
})
}

module.exports = {
move: u(move)
}

0 comments on commit 16dc81e

Please sign in to comment.