GH-9988: Add FileExistsMode expression support #10019
Conversation
Fixes: spring-projects#9988 Issue link: spring-projects#9988 This change allows dynamic determination of FileExistsMode using SpEL expressions, making the component more flexible when handling file existence conflicts. * Add fileExistsModeExpression field and setter methods * Use resolveFileExistsMode in put and get operations * Add changes to the docs Signed-off-by: Jooyoung Pyoung <[email protected]>
Improve runtime behavior by ignoring temporary filename settings when file exists mode is APPEND. Now, in FileExistsMode.APPEND mode, content is always appended directly to the original file regardless of useTemporaryFileName setting. In RemoteFileTemplate: - Remove exception validation when APPEND mode is used with temporary filenames - Modify logic to skip applying temporaryFileSuffix in APPEND mode In AbstractRemoteFileOutboundGateway: - Remove logic that disabled temporary filenames when setting APPEND mode Signed-off-by: Jooyoung Pyoung <[email protected]>
Signed-off-by: Jooyoung Pyoung <[email protected]>
There was a problem hiding this comment.
Please, consider to describe this new feature in the: https://docs.spring.io/spring-integration/reference/ftp/rft.html#page-title. (src/reference/antora/modules/ROOT/pages/sftp/rft.adoc file)
And also mention new feature in the whats-new.adoc.
And as a bonus we also can consider to expose this new option on the RemoteFileOutboundGatewaySpec for Java DSL.
Thanks
| String tempFilePath = tempRemoteFilePath + (this.useTemporaryFileName ? this.temporaryFileSuffix : ""); | ||
| String tempFilePath = tempRemoteFilePath; | ||
| if (!FileExistsMode.APPEND.equals(mode)) { | ||
| tempFilePath += this.useTemporaryFileName ? this.temporaryFileSuffix : ""; |
There was a problem hiding this comment.
Why do we need to append empty string to the end if not this.useTemporaryFileName?
How about this instead:
if (!FileExistsMode.APPEND.equals(mode) && this.useTemporaryFileName) {
tempFilePath += this.temporaryFileSuffix;
}
There was a problem hiding this comment.
You're right, there's no need to append an empty string. Thanks!
| private FileExistsMode resolveFileExistsMode(Message<?> message) { | ||
| if (this.fileExistsModeExpression != null) { | ||
| EvaluationContext evaluationContext = ExpressionUtils.createStandardEvaluationContext(getBeanFactory()); | ||
| evaluationContext.setVariable("fileExistsMode", this.fileExistsMode); |
There was a problem hiding this comment.
Why do we need this variable in the expression?
In most cases no one is going to care about whatever is there by default in the gateway setting.
And with that we could use a global EvaluationContext created only once in the doInit().
There was a problem hiding this comment.
I've removed the unnecessary variable and implemented a global EvaluationContext that's created once in doInit().
Additionally, I was thinking that in the long term, it might be beneficial to move the Expression evaluation logic to the IntegrationObjectSupport class. This could standardize expression evaluation across Spring Integration and reduce duplicate code in each component. Would this be something worth considering?
| if (this.fileExistsModeExpression != null) { | ||
| EvaluationContext evaluationContext = ExpressionUtils.createStandardEvaluationContext(getBeanFactory()); | ||
| evaluationContext.setVariable("fileExistsMode", this.fileExistsMode); | ||
| return this.fileExistsModeExpression.getValue(evaluationContext, message, FileExistsMode.class); |
There was a problem hiding this comment.
I also think we should be able to allow to evaluate expression to just string and then try to resolve enum manually.
This way our contract for this expression would be much friendly for end-users.
There was a problem hiding this comment.
I should have considered user-friendliness more! The expression can now return either a FileExistsMode enum value or a String representation (case-insensitive). Would it be better not to support case-insensitive matching?
| @@ -303,8 +304,6 @@ public String send(Message<?> message, String subDirectory, FileExistsMode... mo | |||
|
|
|||
| private String send(Message<?> message, String subDirectory, FileExistsMode mode) { | |||
| Assert.notNull(this.directoryExpressionProcessor, "'remoteDirectoryExpression' is required"); | |||
| Assert.isTrue(!FileExistsMode.APPEND.equals(mode) || !this.useTemporaryFileName, | |||
| "Cannot append when using a temporary file name"); | |||
There was a problem hiding this comment.
I suggest to make a warning message in the doInit() when this condition is true.
We still gong to ignore, but users would know that it does not make sense to use both useTemporaryFileName and FileExistsMode.APPEND
There was a problem hiding this comment.
I've added a warning message in doInit() when APPEND mode is configured with useTemporaryFileName is true. Thanks for the good suggestion!
- Optimize EvaluationContext usage by creating it once in doInit() - Enhance expression evaluation to support String representation of FileExistsMode - Optimize temporary filename handling logic in RemoteFileTemplate - Add warning message for incompatible APPEND mode with temporary filenames - Rename method to setFileExistsModeExpressionString for consistency - Update Java DSL support in RemoteFileOutboundGatewaySpec - Update reference documentation and release notes Signed-off-by: Jooyoung Pyoung <[email protected]>
|
I've implemented all these suggestions:
Thank you for the helpful guidance! Please let me know if any further adjustments are needed. |
Fixes #9988
In
AbstractRemoteFileOutboundGateway:fileExistsModeExpressionfield and setter methodsresolveFileExistsModein put and get operationsAPPENDmodeIn
RemoteFileTemplate: