From 1ab72b7eafc5ba677e617d267b212015b178a161 Mon Sep 17 00:00:00 2001 From: Jeroen van Erp Date: Tue, 5 Jul 2016 12:21:51 +0200 Subject: [PATCH] Ensure that same name subdirs aren't omitted when doing SFTP (#253) * Updated version string in config * Fixed #252 with backwards compatible behaviour * Fixed logic and expanded test --- build.gradle | 5 +- .../java/net/schmizz/sshj/DefaultConfig.java | 2 +- .../schmizz/sshj/sftp/SFTPFileTransfer.java | 102 +++++--- .../sshj/sftp/SFTPClientTest.groovy | 244 ++++++++++++++++++ .../hierynomus/sshj/sftp/SFTPClientTest.java | 54 ---- 5 files changed, 314 insertions(+), 93 deletions(-) create mode 100644 src/test/groovy/com/hierynomus/sshj/sftp/SFTPClientTest.groovy delete mode 100644 src/test/java/com/hierynomus/sshj/sftp/SFTPClientTest.java diff --git a/build.gradle b/build.gradle index 16dd09af4..d3162d06f 100644 --- a/build.gradle +++ b/build.gradle @@ -1,10 +1,11 @@ plugins { id "java" + id "groovy" id "maven" id "idea" id "signing" id "osgi" - id "org.ajoberstar.release-opinion" version "1.4.0-rc.1" + id "org.ajoberstar.release-opinion" version "1.4.2" id "com.github.hierynomus.license" version "0.12.1" } @@ -72,6 +73,7 @@ dependencies { compile "net.vrallev.ecc:ecc-25519-java:1.0.1" testCompile "junit:junit:4.11" + testCompile 'org.spockframework:spock-core:1.0-groovy-2.4' testCompile "org.mockito:mockito-core:1.9.5" testCompile "org.apache.sshd:sshd-core:1.1.0" testRuntime "ch.qos.logback:logback-classic:1.1.2" @@ -204,4 +206,5 @@ uploadArchives { } } +tasks.compileGroovy.onlyIf { false } tasks.release.dependsOn 'build', 'uploadArchives' diff --git a/src/main/java/net/schmizz/sshj/DefaultConfig.java b/src/main/java/net/schmizz/sshj/DefaultConfig.java index 37ecd2ad2..8ff940d27 100644 --- a/src/main/java/net/schmizz/sshj/DefaultConfig.java +++ b/src/main/java/net/schmizz/sshj/DefaultConfig.java @@ -84,7 +84,7 @@ public class DefaultConfig private final Logger log = LoggerFactory.getLogger(getClass()); - private static final String VERSION = "SSHJ_0_14_0"; + private static final String VERSION = "SSHJ_0_16_0"; public DefaultConfig() { setVersion(VERSION); diff --git a/src/main/java/net/schmizz/sshj/sftp/SFTPFileTransfer.java b/src/main/java/net/schmizz/sshj/sftp/SFTPFileTransfer.java index 2c77987c7..ec8f0caff 100644 --- a/src/main/java/net/schmizz/sshj/sftp/SFTPFileTransfer.java +++ b/src/main/java/net/schmizz/sshj/sftp/SFTPFileTransfer.java @@ -29,6 +29,7 @@ import java.io.InputStream; import java.io.OutputStream; import java.util.EnumSet; +import java.util.List; public class SFTPFileTransfer extends AbstractFileTransfer @@ -67,7 +68,7 @@ public void download(String source, String dest) @Override public void upload(LocalSourceFile localFile, String remotePath) throws IOException { - new Uploader().upload(getTransferListener(), localFile, remotePath); + new Uploader(localFile, remotePath).upload(getTransferListener()); } @Override @@ -173,6 +174,31 @@ private void copyAttributes(final RemoteResourceInfo remote, final LocalDestFile private class Uploader { + private final LocalSourceFile source; + private final String remote; + + private Uploader(final LocalSourceFile source, final String remote) { + this.source = source; + this.remote = remote; + } + + private void upload(final TransferListener listener) throws IOException { + if (source.isDirectory()) { + makeDirIfNotExists(remote); // Ensure that the directory exists + uploadDir(listener.directory(source.getName()), source, remote); + setAttributes(source, remote); + } else if (source.isFile() && isDirectory(remote)) { + String adjustedRemote = engine.getPathHelper().adjustForParent(this.remote, source.getName()); + uploadFile(listener.file(source.getName(), source.getLength()), source, adjustedRemote); + setAttributes(source, adjustedRemote); + } else if (source.isFile()) { + uploadFile(listener.file(source.getName(), source.getLength()), source, remote); + setAttributes(source, remote); + } else { + throw new IOException(source + " is not a file or directory"); + } + } + private void upload(final TransferListener listener, final LocalSourceFile local, final String remote) @@ -182,20 +208,26 @@ private void upload(final TransferListener listener, adjustedPath = uploadDir(listener.directory(local.getName()), local, remote); } else if (local.isFile()) { adjustedPath = uploadFile(listener.file(local.getName(), local.getLength()), local, remote); - } else + } else { throw new IOException(local + " is not a file or directory"); - if (getPreserveAttributes()) - engine.setAttributes(adjustedPath, getAttributes(local)); + } + setAttributes(local, adjustedPath); + } + + private void setAttributes(LocalSourceFile local, String remotePath) throws IOException { + if (getPreserveAttributes()) { + engine.setAttributes(remotePath, getAttributes(local)); + } } private String uploadDir(final TransferListener listener, final LocalSourceFile local, final String remote) throws IOException { - final String adjusted = prepareDir(local, remote); + makeDirIfNotExists(remote); for (LocalSourceFile f : local.getChildren(getUploadFilter())) - upload(listener, f, adjusted); - return adjusted; + upload(listener, f, engine.getPathHelper().adjustForParent(remote, f.getName())); + return remote; } private String uploadFile(final StreamCopier.Listener listener, @@ -203,52 +235,50 @@ private String uploadFile(final StreamCopier.Listener listener, final String remote) throws IOException { final String adjusted = prepareFile(local, remote); - final RemoteFile rf = engine.open(adjusted, EnumSet.of(OpenMode.WRITE, - OpenMode.CREAT, - OpenMode.TRUNC)); - try { - final InputStream fis = local.getInputStream(); - final RemoteFile.RemoteFileOutputStream rfos = rf.new RemoteFileOutputStream(0, 16); - try { + try (RemoteFile rf = engine.open(adjusted, EnumSet.of(OpenMode.WRITE, OpenMode.CREAT, OpenMode.TRUNC))) { + try (InputStream fis = local.getInputStream(); + RemoteFile.RemoteFileOutputStream rfos = rf.new RemoteFileOutputStream(0, 16)) { new StreamCopier(fis, rfos) .bufSize(engine.getSubsystem().getRemoteMaxPacketSize() - rf.getOutgoingPacketOverhead()) .keepFlushing(false) .listener(listener) .copy(); - } finally { - fis.close(); - rfos.close(); } - } finally { - rf.close(); } return adjusted; } - private String prepareDir(final LocalSourceFile local, final String remote) - throws IOException { - final FileAttributes attrs; + private boolean makeDirIfNotExists(final String remote) throws IOException { try { - attrs = engine.stat(remote); + FileAttributes attrs = engine.stat(remote); + if (attrs.getMode().getType() != FileMode.Type.DIRECTORY) { + throw new IOException(remote + " exists and should be a directory, but was a " + attrs.getMode().getType()); + } + // Was not created, but existed. + return false; } catch (SFTPException e) { if (e.getStatusCode() == StatusCode.NO_SUCH_FILE) { - log.debug("probeDir: {} does not exist, creating", remote); + log.debug("makeDir: {} does not exist, creating", remote); engine.makeDir(remote); - return remote; - } else + return true; + } else { throw e; + } } + } - if (attrs.getMode().getType() == FileMode.Type.DIRECTORY) - if (engine.getPathHelper().getComponents(remote).getName().equals(local.getName())) { - log.debug("probeDir: {} already exists", remote); - return remote; + private boolean isDirectory(final String remote) throws IOException { + try { + FileAttributes attrs = engine.stat(remote); + return attrs.getMode().getType() == FileMode.Type.DIRECTORY; + } catch (SFTPException e) { + if (e.getStatusCode() == StatusCode.NO_SUCH_FILE) { + log.debug("isDir: {} does not exist", remote); + return false; } else { - log.debug("probeDir: {} already exists, path adjusted for {}", remote, local.getName()); - return prepareDir(local, engine.getPathHelper().adjustForParent(remote, local.getName())); + throw e; } - else - throw new IOException(attrs.getMode().getType() + " file already exists at " + remote); + } } private String prepareFile(final LocalSourceFile local, final String remote) @@ -264,8 +294,7 @@ private String prepareFile(final LocalSourceFile local, final String remote) throw e; } if (attrs.getMode().getType() == FileMode.Type.DIRECTORY) { - log.debug("probeFile: {} was directory, path adjusted for {}", remote, local.getName()); - return engine.getPathHelper().adjustForParent(remote, local.getName()); + throw new IOException("Trying to upload file " + local.getName() + " to path " + remote + " but that is a directory"); } else { log.debug("probeFile: {} is a {} file that will be replaced", remote, attrs.getMode().getType()); return remote; @@ -281,5 +310,4 @@ private FileAttributes getAttributes(LocalSourceFile local) } } - } diff --git a/src/test/groovy/com/hierynomus/sshj/sftp/SFTPClientTest.groovy b/src/test/groovy/com/hierynomus/sshj/sftp/SFTPClientTest.groovy new file mode 100644 index 000000000..b972129fa --- /dev/null +++ b/src/test/groovy/com/hierynomus/sshj/sftp/SFTPClientTest.groovy @@ -0,0 +1,244 @@ +/* + * Copyright (C)2009 - SSHJ Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.hierynomus.sshj.sftp + +import com.hierynomus.sshj.test.SshFixture +import com.hierynomus.sshj.test.util.FileUtil +import net.schmizz.sshj.SSHClient +import net.schmizz.sshj.sftp.SFTPClient +import org.junit.Rule +import org.junit.rules.TemporaryFolder +import spock.lang.Specification +import spock.lang.Unroll + +import static org.codehaus.groovy.runtime.IOGroovyMethods.withCloseable + +class SFTPClientTest extends Specification { + + @Rule + public SshFixture fixture = new SshFixture() + + @Rule + public TemporaryFolder temp = new TemporaryFolder() + + @Unroll + def "should copy #sourceType->#targetType if #targetExists with #named name"() { + given: + File src = source + File dest = target + + when: + doUpload(src, dest) + + then: + exists.each { f -> + if (dest.isDirectory()) { + assert new File(dest, f).exists() + } else { + assert dest.exists() + } + } + // Dest is also counted by recursiveCount if it is a dir + exists.size() + (dest.isDirectory() ? 1 : 0) == recursiveCount(dest) + + cleanup: + // Delete the temp directories + recursiveDelete(source.getParentFile()) + recursiveDelete(target.getParentFile()) + + where: + source | target || exists + sourceTree() | existingTargetDir("dest") || ["toto.txt", "tata.txt", "tutu", "tutu/tutu.txt"] + sourceTree() | newTargetDir("dest") || ["toto.txt", "tata.txt", "tutu", "tutu/tutu.txt"] + sourceTree() | existingTargetDir("toto") || ["toto.txt", "tata.txt", "tutu", "tutu/tutu.txt"] + sourceTree() | newTargetDir("toto") || ["toto.txt", "tata.txt", "tutu", "tutu/tutu.txt"] + sourceFile() | existingTargetDir("dest") || ["toto.txt"] + sourceFile() | newTargetFile("toto.txt") || ["toto.txt"] + sourceFile() | newTargetFile("diff.txt") || ["diff.txt"] + + sourceType = source.isDirectory() ? "dir" : "file" + targetType = target.isDirectory() ? "dir" : sourceType + targetExists = target.exists() ? "exists" : "not exists" + named = (target.name == source.name) ? "same" : "different" + } + + + def "should not throw exception on close before disconnect"() { + given: + File file = temp.newFile("source.txt") + FileUtil.writeToFile(file, "This is the source") + + when: + doUpload(file, temp.newFile("dest.txt")) + + then: + noExceptionThrown() + } +// +// def "should copy dir->dir if exists and same name"() { +// given: +// File srcDir = temp.newFolder("toto") +// File destDir = temp.newFolder("dest", "toto") +// FileUtil.writeToFile(new File(srcDir, "toto.txt"), "Toto file") +// +// when: +// doUpload(srcDir, destDir) +// +// then: +// destDir.exists() +// !new File(destDir, "toto").exists() +// new File(destDir, "toto.txt").exists() +// } +// +// def "should copy dir->dir if exists and different name"() { +// given: +// File srcDir = temp.newFolder("toto") +// File destDir = temp.newFolder("dest") +// FileUtil.writeToFile(new File(srcDir, "toto.txt"), "Toto file") +// +// when: +// doUpload(srcDir, destDir) +// +// then: +// destDir.exists() +// new File(destDir, "toto.txt").exists() +// } +// +// def "should copy dir->dir if not exists and same name"() { +// given: +// File dd = temp.newFolder("dest") +// File destDir = new File(dd, "toto") +// +// when: +// doUpload(srcDir, destDir) +// +// then: +// destDir.exists() +// new File(destDir, "toto.txt").exists() +// } +// +// def "should copy dir->dir if not exists and different name"() { +// given: +// File srcDir = temp.newFolder("toto") +// File destDir = new File(temp.getRoot(), "dest") +// FileUtil.writeToFile(new File(srcDir, "toto.txt"), "Toto file") +// +// when: +// doUpload(srcDir, destDir) +// +// then: +// destDir.exists() +// new File(destDir, "toto.txt").exists() +// } + + def "should not merge same name subdirs (GH #252)"() { + given: + File toto = temp.newFolder("toto") + File tutu = mkdir(toto, "tutu") + File toto2 = mkdir(toto, "toto") + File dest = temp.newFolder("dest") + FileUtil.writeToFile(new File(toto, "toto.txt"), "Toto file") + FileUtil.writeToFile(new File(tutu, "tototutu.txt"), "Toto/Tutu file") + FileUtil.writeToFile(new File(toto2, "totototo.txt"), "Toto/Toto file") + + when: + doUpload(toto, dest) + + then: + new File(dest, "toto").exists() + new File(dest, "toto.txt").exists() + new File(dest, "tutu").exists() + new File(dest, "tutu/tototutu.txt").exists() + new File(dest, "toto").exists() + new File(dest, "toto/totototo.txt").exists() + !new File(dest, "totototo.txt").exists() + } + + private void doUpload(File src, File dest) throws IOException { + SSHClient sshClient = fixture.setupConnectedDefaultClient() + sshClient.authPassword("test", "test") + try { + withCloseable(sshClient.newSFTPClient()) { SFTPClient sftpClient -> + sftpClient.put(src.getPath(), dest.getPath()) + } + } finally { + sshClient.disconnect() + } + } + + private File mkdir(File parent, String name) { + File file = new File(parent, name) + file.mkdirs() + return file + } + + private def sourceTree() { + def tempDir = File.createTempDir() + File srcDir = mkdir(tempDir, "toto") + FileUtil.writeToFile(new File(srcDir, "toto.txt"), "Toto file") + FileUtil.writeToFile(new File(srcDir, "tata.txt"), "Tata file") + File tutuDir = mkdir(srcDir, "tutu") + FileUtil.writeToFile(new File(tutuDir, "tutu.txt"), "Tutu file") + return srcDir + } + + private def sourceFile() { + def tempDir = File.createTempDir() + def totoFile = new File(tempDir, "toto.txt") + FileUtil.writeToFile(totoFile, "Bare toto file") + return totoFile + } + + private def existingTargetDir(String name) { + def tempDir = File.createTempDir("sftp", "tmp") + tempDir.deleteOnExit() + return mkdir(tempDir, name) + } + + private def newTargetFile(String name) { + def tempDir = File.createTempDir("sftp", "tmp") + tempDir.deleteOnExit() + return new File(tempDir, name) + } + + private def newTargetDir(String name) { + def tempDir = File.createTempDir("sftp", "tmp") + tempDir.deleteOnExit() + return new File(tempDir, name) + } + + private int recursiveCount(File file) { + if (file.isFile()) { + return 1 + } + File[] files = file.listFiles(); + if (files != null) { + return 1 + (files.collect({ f -> recursiveCount(f) }).sum() as int) + } else { + return 1 + } + } + + private void recursiveDelete(File file) { + File[] files = file.listFiles(); + if (files != null) { + for (File each : files) { + recursiveDelete(each); + } + } + file.delete(); + } +} diff --git a/src/test/java/com/hierynomus/sshj/sftp/SFTPClientTest.java b/src/test/java/com/hierynomus/sshj/sftp/SFTPClientTest.java deleted file mode 100644 index a44716f07..000000000 --- a/src/test/java/com/hierynomus/sshj/sftp/SFTPClientTest.java +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Copyright (C)2009 - SSHJ Contributors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package com.hierynomus.sshj.sftp; - -import com.hierynomus.sshj.test.SshFixture; -import com.hierynomus.sshj.test.util.FileUtil; -import net.schmizz.sshj.SSHClient; -import net.schmizz.sshj.sftp.SFTPClient; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.TemporaryFolder; - -import java.io.File; -import java.io.IOException; - -public class SFTPClientTest { - - @Rule - public SshFixture fixture = new SshFixture(); - - @Rule - public TemporaryFolder temp = new TemporaryFolder(); - - @Test - public void shouldNotThrowExceptionOnCloseBeforeDisconnect() throws IOException { - SSHClient sshClient = fixture.setupConnectedDefaultClient(); - sshClient.authPassword("test", "test"); - SFTPClient sftpClient = sshClient.newSFTPClient(); - File file = temp.newFile("source.txt"); - FileUtil.writeToFile(file, "This is the source"); - try { - try { - sftpClient.put(file.getPath(), temp.newFile("dest.txt").getPath()); - } finally { - sftpClient.close(); - } - } finally { - sshClient.disconnect(); - } - } -}