diff --git a/plugins/tech/sftp/src/main/java/org/apache/hop/pipeline/transforms/sftpput/SftpPut.java b/plugins/tech/sftp/src/main/java/org/apache/hop/pipeline/transforms/sftpput/SftpPut.java index 29ce23bf3e4..6acd768ccac 100644 --- a/plugins/tech/sftp/src/main/java/org/apache/hop/pipeline/transforms/sftpput/SftpPut.java +++ b/plugins/tech/sftp/src/main/java/org/apache/hop/pipeline/transforms/sftpput/SftpPut.java @@ -19,6 +19,7 @@ import java.io.ByteArrayInputStream; import java.io.InputStream; import java.io.OutputStream; +import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.commons.vfs2.FileObject; import org.apache.commons.vfs2.FileSystemException; import org.apache.commons.vfs2.NameScope; @@ -36,6 +37,7 @@ import org.apache.hop.pipeline.transforms.sftpput.SftpPutMeta.AfterSftpPut; import org.apache.hop.vfs.sftp.SftpConnections; import org.apache.hop.vfs.sftp.metadata.SftpConnection; +import org.apache.hop.vfs.sftp.provider.IRootPrefixFileName; /** Uploads files or field content to the server of a named SFTP connection. */ public class SftpPut extends BaseTransform { @@ -110,9 +112,9 @@ public boolean processRow() throws HopException { if (isDebug()) { logDebug( BaseMessages.getString( - PKG, "SftpPut.Log.RowSentToErrorStream", getLinesRead(), e.getMessage())); + PKG, "SftpPut.Log.RowSentToErrorStream", getLinesRead(), errorDescription(e))); } - putError(data.inputRowMeta, row, 1, e.toString(), null, "SFTPPUT001"); + putError(data.inputRowMeta, row, 1, errorDescription(e), null, "SFTPPUT001"); } else { logError(BaseMessages.getString(PKG, "SftpPut.Log.ErrorInTransform"), e); setErrors(1); @@ -243,9 +245,11 @@ private void copyToRemote(InputStream inputStream, String remoteFolder, String r long startTime = System.currentTimeMillis(); long bytes; - try (FileObject targetFile = HopVfs.getFileObject(targetUri, this); - OutputStream outputStream = HopVfs.getOutputStream(targetFile, false)) { - bytes = inputStream.transferTo(outputStream); + try (FileObject targetFile = HopVfs.getFileObject(targetUri, this)) { + checkTargetIsAFile(targetFile, targetUri); + try (OutputStream outputStream = HopVfs.getOutputStream(targetFile, false)) { + bytes = inputStream.transferTo(outputStream); + } } data.uploadedFiles++; data.uploadedBytes += bytes; @@ -267,6 +271,42 @@ private void copyToRemote(InputStream inputStream, String remoteFolder, String r } } + /** + * Something that is there but isn't a file can't be overwritten. Left to VFS, that ends in a bare + * "Could not create file" - and on the way there in an attempt to delete it. A z/OS server lists + * a data set which other data sets are named below as a folder, so this does happen. + */ + static void checkTargetIsAFile(FileObject targetFile, String targetUri) throws HopException { + try { + if (targetFile.exists() && !targetFile.isFile()) { + String message = BaseMessages.getString(PKG, "SftpPut.Error.TargetIsNotAFile", targetUri); + if (targetFile.getName() instanceof IRootPrefixFileName prefixed + && !prefixed.getRootPrefix().isEmpty()) { + message += " " + BaseMessages.getString(PKG, "SftpPut.Error.TargetIsNotAFile.DataSet"); + } + throw new HopException(message); + } + } catch (FileSystemException e) { + throw new HopException( + BaseMessages.getString(PKG, "SftpPut.Error.UnableToUpload", targetUri), e); + } + } + + /** + * What goes to the error stream: the message, and the deepest cause, which is usually the answer + * of the server. + */ + static String errorDescription(Throwable e) { + Throwable rootCause = ExceptionUtils.getRootCause(e); + if (rootCause == null || rootCause == e) { + return e.toString(); + } + return e.toString() + + Const.CR + + BaseMessages.getString( + PKG, "SftpPut.Error.RootCause", ExceptionUtils.getRootCauseMessage(e)); + } + private void createRemoteFolder(String remoteFolder) throws HopException { if (Utils.isEmpty(remoteFolder)) { // No folder given: the file lands in the folder the connection starts in. diff --git a/plugins/tech/sftp/src/main/java/org/apache/hop/vfs/sftp/client/SftpClient.java b/plugins/tech/sftp/src/main/java/org/apache/hop/vfs/sftp/client/SftpClient.java index 770ba97e996..230eea0d250 100644 --- a/plugins/tech/sftp/src/main/java/org/apache/hop/vfs/sftp/client/SftpClient.java +++ b/plugins/tech/sftp/src/main/java/org/apache/hop/vfs/sftp/client/SftpClient.java @@ -17,6 +17,8 @@ package org.apache.hop.vfs.sftp.client; +import static org.apache.hop.vfs.sftp.provider.SftpFileNameParser.keepDataSetPrefix; + import com.google.common.annotations.VisibleForTesting; import com.jcraft.jsch.Channel; import com.jcraft.jsch.ChannelSftp; @@ -178,7 +180,7 @@ public void login(String password) throws HopWorkflowException { public void chdir(String dirToChangeTo) throws HopWorkflowException { try { - c.cd(dirToChangeTo.replace("\\\\", "/").replace("\\", "/")); + c.cd(keepDataSetPrefix(dirToChangeTo.replace("\\\\", "/").replace("\\", "/"))); } catch (SftpException e) { throw new HopWorkflowException(e); } @@ -262,7 +264,7 @@ public void put(FileObject fileObject, String remoteFile, boolean preserveTimest public void delete(String file) throws HopWorkflowException { try { - c.rm(file); + c.rm(keepDataSetPrefix(file)); } catch (SftpException e) { throw new HopWorkflowException(e); } @@ -283,6 +285,7 @@ public void createFolder(String path) throws HopWorkflowException { for (String f : folders) { if (!f.isEmpty() && !folderExists(f)) { + // jsch sends a mkdir as is, so unlike the other requests it needs no trailing slash c.mkdir(f); } } @@ -295,7 +298,7 @@ public void createFolder(String path) throws HopWorkflowException { public void renameFile(String sourcefilename, String destinationfilename) throws HopWorkflowException { try { - c.rename(sourcefilename, destinationfilename); + c.rename(keepDataSetPrefix(sourcefilename), keepDataSetPrefix(destinationfilename)); } catch (SftpException e) { throw new HopWorkflowException(e); } @@ -303,7 +306,7 @@ public void renameFile(String sourcefilename, String destinationfilename) public FileType getFileType(String filename) throws HopWorkflowException { try { - SftpATTRS attrs = c.stat(filename); + SftpATTRS attrs = c.stat(keepDataSetPrefix(filename)); if (attrs == null) { return FileType.IMAGINARY; } @@ -325,7 +328,7 @@ public FileType getFileType(String filename) throws HopWorkflowException { public boolean folderExists(String folderName) { boolean retval = false; try { - SftpATTRS attrs = c.stat(folderName); + SftpATTRS attrs = c.stat(keepDataSetPrefix(folderName)); if (attrs == null) { return false; } diff --git a/plugins/tech/sftp/src/main/java/org/apache/hop/vfs/sftp/provider/SftpFileNameParser.java b/plugins/tech/sftp/src/main/java/org/apache/hop/vfs/sftp/provider/SftpFileNameParser.java index ca677bc6a37..4ba11818a88 100644 --- a/plugins/tech/sftp/src/main/java/org/apache/hop/vfs/sftp/provider/SftpFileNameParser.java +++ b/plugins/tech/sftp/src/main/java/org/apache/hop/vfs/sftp/provider/SftpFileNameParser.java @@ -127,4 +127,29 @@ public static String extractRootPrefix(StringBuilder path) { } return ""; } + + /** + * Gets a data set name of a single level, such as {@code //HLQ.NAME}, to the server intact. + * + *

jsch runs almost every path through its glob handling, which splits the path at the last + * slash and so turns {@code //HLQ.NAME} into {@code /HLQ.NAME} - the USS path. With a trailing + * slash the last slash comes after the data set name, and {@code //HLQ.NAME/} reaches the server + * as is. Paths of more than one level, and every path not starting with exactly two slashes, are + * left alone. + * + *

Verified on z/OS for a data set name prefix used as a folder. A single-level sequential data + * set read or written as a file gets the trailing slash too, which hasn't been verified yet. + * + * @param path a path as it goes to jsch + * @return the path, with a trailing slash if it's a data set name of a single level + */ + public static String keepDataSetPrefix(String path) { + return path != null + && path.length() > 2 + && path.startsWith("//") + && path.charAt(2) != '/' + && path.indexOf('/', 2) < 0 + ? path + "/" + : path; + } } diff --git a/plugins/tech/sftp/src/main/java/org/apache/hop/vfs/sftp/provider/SftpFileObject.java b/plugins/tech/sftp/src/main/java/org/apache/hop/vfs/sftp/provider/SftpFileObject.java index b17101d9534..a76e0581ebb 100644 --- a/plugins/tech/sftp/src/main/java/org/apache/hop/vfs/sftp/provider/SftpFileObject.java +++ b/plugins/tech/sftp/src/main/java/org/apache/hop/vfs/sftp/provider/SftpFileObject.java @@ -117,23 +117,35 @@ protected SftpFileObject(final AbstractFileName fileName, final SftpFileSystem f * USS path. Absolute is deliberate: jsch passes a path starting with a slash to the server * untouched, while a relative one is resolved against a working directory the server would have * normalised for us. + * + *

A data set name of a single level gets a trailing slash, or jsch would take the second slash + * off again - see {@link SftpFileNameParser#keepDataSetPrefix}. */ private static String remotePath(final AbstractFileName fileName, final SftpFileSystem fileSystem) throws FileSystemException { final String rootPrefix = fileName instanceof IRootPrefixFileName prefixed ? prefixed.getRootPrefix() : ""; if (!rootPrefix.isEmpty()) { - return rootPrefix + UriParser.decode(fileName.getPath()); + return SftpFileNameParser.keepDataSetPrefix( + rootPrefix + UriParser.decode(fileName.getPath())); } return UriParser.decode(fileSystem.getRootName().getRelativeName(fileName)); } - /** Creates this file as a folder. */ + /** + * Creates this file as a folder. jsch sends a mkdir as is, unlike the requests it globs, so a + * data set name goes without the trailing slash of {@link #relPath} - see {@link + * SftpFileNameParser#keepDataSetPrefix}. + */ @Override protected void doCreateFolder() throws Exception { + final String folderPath = + relPath.length() > 3 && relPath.startsWith("//") && relPath.endsWith("/") + ? relPath.substring(0, relPath.length() - 1) + : relPath; final ChannelSftp channel = getAbstractFileSystem().getChannel(); try { - channel.mkdir(relPath); + channel.mkdir(folderPath); } finally { putChannel(channel); } diff --git a/plugins/tech/sftp/src/main/resources/org/apache/hop/pipeline/transforms/sftpput/messages/messages_en_US.properties b/plugins/tech/sftp/src/main/resources/org/apache/hop/pipeline/transforms/sftpput/messages/messages_en_US.properties index 126387ebbdb..07b011a3eed 100644 --- a/plugins/tech/sftp/src/main/resources/org/apache/hop/pipeline/transforms/sftpput/messages/messages_en_US.properties +++ b/plugins/tech/sftp/src/main/resources/org/apache/hop/pipeline/transforms/sftpput/messages/messages_en_US.properties @@ -64,6 +64,9 @@ SftpPut.Error.RemoteFilenameEmpty=The remote file name is empty SftpPut.Error.CanNotFindRemoteFolder=Unable to find remote folder [{0}] SftpPut.Error.MoveToDestinationFolderIsEmpty=The folder to move the source file to is empty SftpPut.Error.UnableToUpload=Unable to upload to [{0}] +SftpPut.Error.TargetIsNotAFile=Unable to upload to [{0}]: it already exists on the server and isn''t a file, so it can''t be overwritten. +SftpPut.Error.TargetIsNotAFile.DataSet=On z/OS this is an existing data set the server lists as a folder, or a name other data sets are stored under. +SftpPut.Error.RootCause=Root cause: {0} SftpPut.Error.UnableToMove=Unable to move [{0}] to [{1}] SftpPut.Error.ConnectionNotRegistered=The SFTP connection [{0}] is not available as a VFS scheme in this run. Please make sure it''s part of the metadata of the project this pipeline runs in. diff --git a/plugins/tech/sftp/src/test/java/org/apache/hop/pipeline/transforms/sftpput/SftpPutTest.java b/plugins/tech/sftp/src/test/java/org/apache/hop/pipeline/transforms/sftpput/SftpPutTest.java index e6890f5c382..4c58d4827ae 100644 --- a/plugins/tech/sftp/src/test/java/org/apache/hop/pipeline/transforms/sftpput/SftpPutTest.java +++ b/plugins/tech/sftp/src/test/java/org/apache/hop/pipeline/transforms/sftpput/SftpPutTest.java @@ -22,9 +22,16 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; +import static org.mockito.Mockito.withSettings; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import org.apache.commons.vfs2.FileName; +import org.apache.commons.vfs2.FileObject; import org.apache.hop.core.HopEnvironment; import org.apache.hop.core.exception.HopException; import org.apache.hop.core.logging.ILoggingObject; @@ -38,10 +45,12 @@ import org.apache.hop.pipeline.transforms.sftpput.SftpPutMeta.AfterSftpPut; import org.apache.hop.vfs.sftp.SftpConnectionFileProvider; import org.apache.hop.vfs.sftp.metadata.SftpConnection; +import org.apache.hop.vfs.sftp.provider.IRootPrefixFileName; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; /** * What the transform makes of its configuration before it touches the server: every one of these @@ -182,6 +191,69 @@ void testAFailingRowStopsTheTransform() throws Exception { assertFalse(transform.processRow(), "the transform should have stopped"); } + /** A folder where the file should go can't be overwritten, and the message says so. */ + @Test + void testATargetWhichIsAFolderIsReported(@TempDir Path tempDir) throws Exception { + String uri = tempDir.toUri().toString(); + try (FileObject folder = HopVfs.getFileObject(uri)) { + HopException e = + assertThrows(HopException.class, () -> SftpPut.checkTargetIsAFile(folder, uri)); + assertTrue(e.getMessage().contains("isn't a file"), e.getMessage()); + assertFalse(e.getMessage().contains("z/OS"), "not a data set: " + e.getMessage()); + } + } + + /** Only a data set path gets the z/OS explanation. */ + @Test + void testADataSetWhichIsAFolderIsExplained() throws Exception { + FileName name = mock(FileName.class, withSettings().extraInterfaces(IRootPrefixFileName.class)); + when(((IRootPrefixFileName) name).getRootPrefix()).thenReturn("/"); + FileObject dataSet = mock(FileObject.class); + when(dataSet.exists()).thenReturn(true); + when(dataSet.isFile()).thenReturn(false); + when(dataSet.getName()).thenReturn(name); + + HopException e = + assertThrows( + HopException.class, + () -> SftpPut.checkTargetIsAFile(dataSet, "prod:////HLQ.NAME/MEMBER")); + assertTrue(e.getMessage().contains("isn't a file"), e.getMessage()); + assertTrue(e.getMessage().contains("z/OS"), e.getMessage()); + } + + /** A file, or nothing at all, is fine. */ + @Test + void testATargetWhichIsAFileOrMissingIsAccepted(@TempDir Path tempDir) throws Exception { + Path file = Files.writeString(tempDir.resolve("existing.txt"), "x"); + try (FileObject existing = HopVfs.getFileObject(file.toUri().toString()); + FileObject missing = HopVfs.getFileObject(tempDir.resolve("new.txt").toUri().toString())) { + SftpPut.checkTargetIsAFile(existing, "existing"); + SftpPut.checkTargetIsAFile(missing, "missing"); + } + } + + /** The error stream gets the deepest cause too: that's where the server's answer is. */ + @Test + void testTheErrorDescriptionHasTheRootCause() { + Exception e = + new HopException( + "Unable to upload", + new IOException("Could not create file", new IOException("4: Failure"))); + + String description = SftpPut.errorDescription(e); + + assertTrue(description.contains("Unable to upload"), description); + assertTrue(description.contains("Root cause: IOException: 4: Failure"), description); + } + + /** Without a cause there's nothing to add. */ + @Test + void testTheErrorDescriptionWithoutACause() { + Exception e = new HopException("Unable to find remote folder"); + + assertEquals(e.toString(), SftpPut.errorDescription(e)); + } + private SftpPutMeta meta() { SftpPutMeta meta = new SftpPutMeta(); meta.setConnection(CONNECTION_NAME); diff --git a/plugins/tech/sftp/src/test/java/org/apache/hop/vfs/sftp/provider/SftpDoubleSlashPathTest.java b/plugins/tech/sftp/src/test/java/org/apache/hop/vfs/sftp/provider/SftpDoubleSlashPathTest.java index 59588fab99c..d3ed0d9a3bf 100644 --- a/plugins/tech/sftp/src/test/java/org/apache/hop/vfs/sftp/provider/SftpDoubleSlashPathTest.java +++ b/plugins/tech/sftp/src/test/java/org/apache/hop/vfs/sftp/provider/SftpDoubleSlashPathTest.java @@ -30,6 +30,7 @@ import java.io.File; import java.io.IOException; import java.io.InputStream; +import java.net.InetAddress; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.InvalidPathException; @@ -44,6 +45,7 @@ import org.apache.commons.vfs2.FileSystemException; import org.apache.commons.vfs2.FileSystemManager; import org.apache.commons.vfs2.FileSystemOptions; +import org.apache.commons.vfs2.FileType; import org.apache.commons.vfs2.NameScope; import org.apache.commons.vfs2.RandomAccessContent; import org.apache.commons.vfs2.auth.StaticUserAuthenticator; @@ -60,6 +62,7 @@ import org.apache.hop.vfs.sftp.SftpConnectionFileNameParser; import org.apache.hop.vfs.sftp.SftpConnectionFileProvider; import org.apache.hop.vfs.sftp.SftpConnections; +import org.apache.hop.vfs.sftp.client.SftpClient; import org.apache.hop.vfs.sftp.metadata.SftpConnection; import org.apache.sshd.common.file.virtualfs.VirtualFileSystemFactory; import org.apache.sshd.server.SshServer; @@ -74,6 +77,7 @@ import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.ValueSource; /** @@ -283,6 +287,27 @@ void aRemoteFolderOfANamedConnectionReachesTheDataSet() throws Exception { "the server should have been asked for a //-path, but got: " + paths); } + /** + * A data set name of a single level, the remote folder of the SFTP Put transform for instance. + * jsch would turn {@code //read-tests} into {@code /read-tests}; the trailing slash keeps both. + */ + @Test + void aSingleLevelDataSetKeepsTheSecondSlash() throws Exception { + final String uri = SftpConnections.buildUri(namedConnection(), "//read-tests/", null); + REQUESTED_PATHS.clear(); + try (FileObject folder = manager.resolveFile(uri, authOptions())) { + assertEquals(FileType.FOLDER, folder.getType()); + } + final List paths = new ArrayList<>(REQUESTED_PATHS); + + assertTrue( + paths.contains("//read-tests/"), + "the server should have been asked for the data set, but got: " + paths); + assertFalse( + paths.contains("/read-tests"), + "the server was asked for the single-slash path: " + paths); + } + /** A random-access read opens the same file a plain read does: the data set. */ @Test void aRandomAccessReadAsksForTheDataSet() throws Exception { @@ -360,6 +385,121 @@ void childrenAndParentsKeepTheSecondSlash() throws Exception { } } + /** + * The workflow actions talk to jsch through {@link SftpClient} rather than through VFS, with the + * folder as the user typed it, so the second slash of a single-level data set name needs the same + * protection there. + */ + @Nested + @DisplayName("A data set through the SftpClient of the workflow actions") + class SftpClientDataSets { + + @ParameterizedTest(name = "{0} -> {1}") + @CsvSource({ + "//A.B.C, //A.B.C/", + "//A.B.C/, //A.B.C/", + "//A.B.C/MEMBER, //A.B.C/MEMBER", + "/A.B.C, /A.B.C", + "///A.B.C, ///A.B.C", + "//, //", + "A.B.C, A.B.C" + }) + void onlyASingleLevelDataSetGetsATrailingSlash(String path, String expected) { + assertEquals(expected, SftpFileNameParser.keepDataSetPrefix(path)); + } + + @Test + void folderExistsAsksForTheDataSet() throws Exception { + final List paths = + withClient(client -> assertTrue(client.folderExists("//read-tests"))); + + assertAskedForTheDataSet(paths, "read-tests"); + } + + @Test + void getFileTypeAsksForTheDataSet() throws Exception { + final List paths = + withClient(client -> assertEquals(FileType.FOLDER, client.getFileType("//read-tests"))); + + assertAskedForTheDataSet(paths, "read-tests"); + } + + /** + * A cd asks for the real path first and then looks at whatever the server answered, here the + * single-slash path of the test server: only the first request is ours. + */ + @Test + void chdirAsksForTheDataSet() throws Exception { + final List paths = withClient(client -> client.chdir("//read-tests")); + + assertFalse(paths.isEmpty(), "the server wasn't asked anything"); + assertEquals("//read-tests/", paths.get(0), "paths asked for: " + paths); + } + + /** + * The existence check needs the trailing slash, the mkdir - which jsch sends as is - doesn't. + */ + @Test + void createFolderAsksForTheDataSet() throws Exception { + final File created = new File(getTestDirectoryFile(), "new-data-set"); + try { + final List paths = withClient(client -> client.createFolder("//new-data-set")); + + assertTrue(created.isDirectory(), "the folder should have been created"); + assertAskedForTheDataSet(paths, "new-data-set"); + assertTrue(paths.contains("//new-data-set"), "no mkdir of the data set in: " + paths); + } finally { + Files.deleteIfExists(created.toPath()); + } + } + + /** The same goes for creating a folder through VFS. */ + @Test + void aVfsFolderIsCreatedWithoutTheTrailingSlash() throws Exception { + final File created = new File(getTestDirectoryFile(), "new-vfs-data-set"); + try (FileObject folder = + manager.resolveFile( + SftpConnections.buildUri(namedConnection(), "//new-vfs-data-set", null), + authOptions())) { + REQUESTED_PATHS.clear(); + folder.createFolder(); + final List paths = new ArrayList<>(REQUESTED_PATHS); + + assertTrue(created.isDirectory(), "the folder should have been created"); + assertTrue(paths.contains("//new-vfs-data-set"), "no mkdir of the data set in: " + paths); + } finally { + Files.deleteIfExists(created.toPath()); + } + } + + private void assertAskedForTheDataSet(List paths, String name) { + assertTrue( + paths.contains("//" + name + "/"), + "the server should have been asked for the data set, but got: " + paths); + assertFalse( + paths.contains("/" + name), "the server was asked for the single-slash path: " + paths); + } + + /** Log in to the test server, run the action, and return the raw paths the server saw. */ + private List withClient(ClientAction action) throws Exception { + final SftpClient client = + new SftpClient(InetAddress.getByName("localhost"), serverPort, USER); + try { + client.login(PASSWORD); + REQUESTED_PATHS.clear(); + action.run(client); + return new ArrayList<>(REQUESTED_PATHS); + } finally { + client.disconnect(); + } + } + } + + @FunctionalInterface + private interface ClientAction { + void run(SftpClient client) throws Exception; + } + /** * Windows UNC names ({@code \\server\share\...}) are the one place where Commons VFS keeps a * leading double slash, and the way it does that is the way out for the mainframe as well: the