You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hop.apache.org by "bamaer (via GitHub)" <gi...@apache.org> on 2026/10/20 12:37:52 UTC
[PR] 9033 (hop)
bamaer opened a new pull request, #9043:
URL: https://github.com/apache/hop/pull/9043
…ath in SFTP
fixes #9033
**Please** add a meaningful description for your change here
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] Run `mvn clean install apache-rat:check` to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
- [x] If you have a group of commits related to the same change, please squash your commits into one and force push your branch using `git rebase -i`.
- [x] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable.
To make clear that you license your contribution under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [x] I hereby declare this contribution to be licensed under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
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: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SFTP slashes for z/Os, improved error messages #9033 (hop)
Posted by "hansva (via GitHub)" <gi...@apache.org>.
hansva merged PR #9043:
URL: https://github.com/apache/hop/pull/9043
--
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: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SFTP slashes for z/Os, improved error messages #9033 (hop)
Posted by "mattcasters (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SFTP slashes for z/Os, improved error messages #9033 (hop)
Posted by "hansva (via GitHub)" <gi...@apache.org>.
hansva commented on PR #9043:
URL: https://github.com/apache/hop/pull/9043#issuecomment-6292634039
**1. Must fix: `SftpClient` still sends `//X` as is.** `chdir()`, `folderExists()`, `createFolder()` (and `getFileType()`, `delete()`, `renameFile()` with absolute paths) don't add the trailing slash, so jsch still turns a single-level data set path into `/X` for `ActionSftp` and `ActionSftpPut`. Move the rule into one helper and use it in `SftpFileObject.remotePath` and in those methods:
```java
public static String keepDataSetPrefix(String path) {
return path != null && path.length() > 2 && path.startsWith("//")
&& path.charAt(2) != '/' && path.indexOf('/', 2) < 0
? path + "/"
: path;
}
```
Add `SftpClient` cases to `SftpDoubleSlashPathTest`. If you'd rather leave this out of the PR, change "fixes #9033" to "addresses #9033" and open a follow-up issue.
**2. Must fix: PR description.** It's still the template text. Add the cause, the fix, and what stays open from #9033: the hint for invalid data set names, and the jsch upstream report.
**3. Should do: check a sequential data set.** Every single-level path now ends in `/`, files included. Read and write one single-level sequential data set on z/OS. If `//HLQ.SEQ/` is refused, say in the comment on `remotePath` that only folders work.
**4. Nit:** add the z/OS sentence of `SftpPut.Error.TargetIsNotAFile` only for `//` paths (`targetFile.getName() instanceof IRootPrefixFileName p && !p.getRootPrefix().isEmpty()`).
--
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: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org