exceptionfactory commented on code in PR #10405:
URL: https://github.com/apache/nifi/pull/10405#discussion_r2450256601
##########
nifi-extension-bundles/nifi-extension-utils/nifi-file-transfer/src/main/java/org/apache/nifi/processor/util/file/transfer/FetchFileTransfer.java:
##########
@@ -116,6 +116,21 @@ public abstract class FetchFileTransfer extends
AbstractProcessor {
.required(false)
.build();
+ public static final PropertyDescriptor MOVE_CONFLICT_RESOLUTION = new
PropertyDescriptor.Builder()
+ .name("Move Conflict Resolution")
+ .description(String.format("Determines how to handle filename
collisions when '%s' is '%s'. " +
+ "This setting controls behavior when the target file
exists in the %s.",
Review Comment:
A multiline string with `"""description""".formatted()` can also work and
make this a bit easier to read.
##########
nifi-extension-bundles/nifi-extension-utils/nifi-file-transfer/src/main/java/org/apache/nifi/processor/util/file/transfer/FileTransferConflictUtil.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.processor.util.file.transfer;
+
+import org.apache.nifi.flowfile.FlowFile;
+import org.apache.nifi.logging.ComponentLog;
+
+import java.io.IOException;
+
+public final class FileTransferConflictUtil {
+ private FileTransferConflictUtil() {
+ }
+
+ /**
+ * Attempts to generate a unique filename by prefixing with an
incrementing integer followed by a dot.
+ * Returns null if a unique name could not be found within 99 attempts.
+ */
+ public static String generateUniqueFilename(final FileTransfer transfer,
+ final String path,
+ final String baseFileName,
+ final FlowFile flowFile,
+ final ComponentLog logger)
throws IOException {
+ boolean uniqueNameGenerated;
+ String candidate = null;
+ for (int i = 1; i < 100; i++) {
Review Comment:
This loop could result in issuing a number of remote file commands. Rather
than a loop of 100 retries, it seems better to generate a filename using a
`UUID` to avoid the need for looping and retrying.
##########
nifi-extension-bundles/nifi-extension-utils/nifi-file-transfer/src/main/java/org/apache/nifi/processor/util/file/transfer/FetchFileTransfer.java:
##########
@@ -363,7 +379,45 @@ private void performCompletionStrategy(final FileTransfer
transfer, final Proces
transfer.ensureDirectoryExists(flowFile, new
File(absoluteTargetDirPath));
}
- final String destinationPath = String.format("%s/%s",
absoluteTargetDirPath, simpleFilename);
+ String destinationFileName = simpleFilename;
+ final FileInfo remoteFileInfo =
transfer.getRemoteFileInfo(flowFile, absoluteTargetDirPath,
destinationFileName);
+ if (remoteFileInfo != null) {
+ final String strategy =
context.getProperty(MOVE_CONFLICT_RESOLUTION).getValue();
+ getLogger().info("Detected filename conflict moving remote
file for {} so handling using configured Conflict Resolution of {}", flowFile,
strategy);
Review Comment:
Recommend removing this log since one of the other strategy options should
log the particular details.
##########
nifi-extension-bundles/nifi-extension-utils/nifi-file-transfer/src/main/java/org/apache/nifi/processor/util/file/transfer/FetchFileTransfer.java:
##########
@@ -363,7 +379,45 @@ private void performCompletionStrategy(final FileTransfer
transfer, final Proces
transfer.ensureDirectoryExists(flowFile, new
File(absoluteTargetDirPath));
}
- final String destinationPath = String.format("%s/%s",
absoluteTargetDirPath, simpleFilename);
+ String destinationFileName = simpleFilename;
+ final FileInfo remoteFileInfo =
transfer.getRemoteFileInfo(flowFile, absoluteTargetDirPath,
destinationFileName);
+ if (remoteFileInfo != null) {
+ final String strategy =
context.getProperty(MOVE_CONFLICT_RESOLUTION).getValue();
+ getLogger().info("Detected filename conflict moving remote
file for {} so handling using configured Conflict Resolution of {}", flowFile,
strategy);
+ switch (strategy.toUpperCase()) {
+ case FileTransfer.CONFLICT_RESOLUTION_REPLACE:
+ try {
+ transfer.deleteFile(flowFile,
absoluteTargetDirPath, destinationFileName);
+ } catch (final IOException deleteEx) {
+ getLogger().warn("Failed to delete existing
destination file {} on {}:{} due to {}. Move will be attempted regardless.",
+ destinationFileName, host, port,
deleteEx.toString(), deleteEx);
Review Comment:
The `due to` syntax should be avoided, the inclusion of the exception
provides both the message and the stack trace.
```suggestion
getLogger().warn("Failed to delete existing
destination file {} on {}:{}. Move will be attempted regardless.",
destinationFileName, host, port,
deleteEx);
```
##########
nifi-extension-bundles/nifi-extension-utils/nifi-file-transfer/src/main/java/org/apache/nifi/processor/util/file/transfer/FetchFileTransfer.java:
##########
@@ -363,7 +379,45 @@ private void performCompletionStrategy(final FileTransfer
transfer, final Proces
transfer.ensureDirectoryExists(flowFile, new
File(absoluteTargetDirPath));
}
- final String destinationPath = String.format("%s/%s",
absoluteTargetDirPath, simpleFilename);
+ String destinationFileName = simpleFilename;
+ final FileInfo remoteFileInfo =
transfer.getRemoteFileInfo(flowFile, absoluteTargetDirPath,
destinationFileName);
+ if (remoteFileInfo != null) {
+ final String strategy =
context.getProperty(MOVE_CONFLICT_RESOLUTION).getValue();
+ getLogger().info("Detected filename conflict moving remote
file for {} so handling using configured Conflict Resolution of {}", flowFile,
strategy);
+ switch (strategy.toUpperCase()) {
+ case FileTransfer.CONFLICT_RESOLUTION_REPLACE:
+ try {
+ transfer.deleteFile(flowFile,
absoluteTargetDirPath, destinationFileName);
+ } catch (final IOException deleteEx) {
+ getLogger().warn("Failed to delete existing
destination file {} on {}:{} due to {}. Move will be attempted regardless.",
+ destinationFileName, host, port,
deleteEx.toString(), deleteEx);
+ }
+ break;
+ case FileTransfer.CONFLICT_RESOLUTION_RENAME:
+ final String unique =
FileTransferConflictUtil.generateUniqueFilename(transfer,
absoluteTargetDirPath, destinationFileName, flowFile, getLogger());
+ if (unique != null) {
+ destinationFileName = unique;
+ } else {
+ getLogger().warn("Could not determine a unique
name after 99 attempts for {}. Move will not be performed.", flowFile);
+ return;
+ }
+ break;
+ case FileTransfer.CONFLICT_RESOLUTION_IGNORE:
+ getLogger().info("Configured to IGNORE move
conflict for {}. Original remote file will be left in place.", flowFile);
Review Comment:
For the ignore strategy, the `debug` level seems more appropriate than
`info` for logging.
--
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]
