dan-s1 commented on code in PR #8503:
URL: https://github.com/apache/nifi/pull/8503#discussion_r1526344159
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java:
##########
@@ -110,6 +110,14 @@ private static String
convertFactorySetToString(Set<String> factorySetNames) {
.collect(Collectors.joining(", "));
}
+ private static String buildFullPath(String path, String filename) {
+ return (path == null) ? filename : (path.endsWith("/")) ? path +
filename : path + "/" + filename;
+ }
+
+ private static boolean identifiesDirectory(FileAttributes attributes) {
Review Comment:
The usual convention to name methods that return boolean is to ask a
question in the name usually by using is/has as the prefix.
```suggestion
private static boolean isDirectory(FileAttributes attributes) {
```
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java:
##########
@@ -110,6 +110,14 @@ private static String
convertFactorySetToString(Set<String> factorySetNames) {
.collect(Collectors.joining(", "));
}
+ private static String buildFullPath(String path, String filename) {
+ return (path == null) ? filename : (path.endsWith("/")) ? path +
filename : path + "/" + filename;
+ }
Review Comment:
Nested ternary operators are hard to read. Write out the logic.
```suggestion
private static String buildFullPath(String path, String filename) {
if (path == null) {
return filename;
} else if (path.endsWith("/")) {
return path + filename
} else {
return path + "/" + filename;
}
}
```
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java:
##########
@@ -694,20 +708,12 @@ public FileInfo getRemoteFileInfo(final FlowFile
flowFile, final String path, St
}
}
- RemoteResourceInfo matchingEntry = null;
- for (final RemoteResourceInfo entry : remoteResources) {
- if (entry.getName().equalsIgnoreCase(filename)) {
- matchingEntry = entry;
- break;
- }
- }
-
// Previously JSCH would perform a listing on the full path (path +
filename) and would get an exception when it wasn't
// a file and then return null, so to preserve that behavior we return
null if the matchingEntry is a directory
- if (matchingEntry != null && matchingEntry.isDirectory()) {
+ if (fileAttributes == null || identifiesDirectory(fileAttributes)) {
Review Comment:
Per earlier comment
```suggestion
if (fileAttributes == null || isDirectory(fileAttributes)) {
```
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java:
##########
@@ -443,17 +455,18 @@ private FileInfo newFileInfo(final RemoteResourceInfo
entry, String path) {
appendPermission(permsBuilder, permissions, FilePermission.OTH_X, "x");
final FileInfo.Builder builder = new FileInfo.Builder()
- .filename(entry.getName())
+ .filename(filename)
.fullPathFileName(newFullForwardPath)
- .directory(entry.isDirectory())
- .size(entry.getAttributes().getSize())
- .lastModifiedTime(entry.getAttributes().getMtime() * 1000L)
+ .directory(identifiesDirectory(attributes))
Review Comment:
Per earlier comment
```suggestion
.directory(isDirectory(attributes))
```
--
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]
