SFTP slashes for z/Os, improved error messages #9033

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<SftpPutMeta, SftpPutData> {
@@ -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.

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,15 +298,15 @@ 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);
     }
   }
 
   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;
       }

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.
+   *
+   * <p>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.
+   *
+   * <p>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;
+  }
 }

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.
+   *
+   * <p>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);
     }

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.
 

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);