mattcasters commented on code in PR #9043:
URL: https://github.com/apache/hop/pull/9043#discussion_r4341272194
##########
plugins/tech/sftp/src/main/resources/org/apache/hop/pipeline/transforms/sftpput/messages/messages_en_US.properties:
##########
@@ -64,6 +64,7 @@ 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. On z/OS this is an
existing data set the server lists as a folder, or a name other data sets are
stored under.
Review Comment:
**[nit]** The z/OS sentence is appended for every non-file target, including
an ordinary directory on a Unix SFTP server.
**Suggestion:** Keep the first sentence for every target. Add the z/OS
clause only when the name is a `//` path (`targetFile.getName()` is an
`IRootPrefixFileName` with a non-empty root prefix).
##########
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 + "/"
Review Comment:
**[suggestion]** `keepDataSetPrefix` appends `/` to every single-level `//`
path, and `SftpFileObject.remotePath` plus `SftpClient` stat/delete/rename/cd
all use it. jsch then sends `//HLQ.SEQ/` for a sequential data set opened as a
file, not only for a prefix used as a folder. The javadoc says that file form
has not been verified on z/OS; a trailing slash is the directory/prefix
spelling, so a read, write, or `rm` can miss `//HLQ.SEQ` or address a different
name. Previously glob turned it into `/HLQ.SEQ`, so this is not a regression of
a working path, but it is a different path.
**Suggestion:** Read and write one single-level sequential data set on z/OS.
If `//HLQ.SEQ/` is accepted, say so here. If it is refused, say in this comment
that only a folder/prefix is covered, and do not treat file get/put/rm of
`//HLQ.SEQ` as fixed.
##########
plugins/tech/sftp/src/main/java/org/apache/hop/pipeline/transforms/sftpput/SftpPut.java:
##########
@@ -267,6 +270,35 @@ 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()) {
+ throw new HopException(
+ BaseMessages.getString(PKG, "SftpPut.Error.TargetIsNotAFile",
targetUri));
+ }
+ } 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 + "Root cause: " +
ExceptionUtils.getRootCauseMessage(e);
Review Comment:
**[nit]** `Root cause:` is a new string shown on the error row, and it is
hardcoded in English while the other SFTP Put errors go through the message
bundle.
**Suggestion:** Move it to `messages_en_US.properties` (for example
`SftpPut.Error.RootCause=Root cause: {0}`) and pass
`ExceptionUtils.getRootCauseMessage(e)` as the argument.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
