Issue #246 deprecate file replace option, retain minor version #258

GregBragg · 2022-04-30T13:29:44Z

"For the current one I suggest the plan like this:

Bring back your SmbMessageHandler, but set replaceFile on the provided SmbSessionFactory, not SmbRemoteFileTemplate.
Deprecate those options on the SmbSessionFactory to discourage end-users to use them explicitly.
Leave the logic as is in the SmbSession as is.

This way we should nail both birds: have options deprecated and retain the old behavior in both write() and rename()."

…e overwrite

artembilan · 2022-05-02T14:03:04Z

spring-integration-smb/README.md

@@ -18,7 +18,7 @@ Put the following block into pom.xml if using Maven:
    <dependency>
        <groupId>org.springframework.integration</groupId>
        <artifactId>spring-integration-smb</artifactId>
-        <version>1.2.0.RELEASE</version>
+        <version>1.3.0.RELEASE</version>


I don't think we need to start a new version at all.
We don't do a breaking change and any new components can be introduced in any version: they are fully not a breaking change.

Plus I think we need to remove this version from this README at all.
It does not reflect the reality of the project.
The actual version is controlled from the gradle.properties file.
I'm not sure that we can use that value in some placeholder for README: it is fully static content, so it cannot resolve any external variables.

So you want me to remove the documentation on how to add the dependency for a Maven project? Or leave the version as is or change it to something else?

I think just remove the <version> is enough.

Ok so what version will we be building here then? I need this for my current changes in my services so that I can pick up the latest JCIFS library in this project.

The gradle.properties has this content:

version=1.2.2.BUILD-SNAPSHOT

So, when we release, it is going to be 1.2.2.RELEASE.

artembilan · 2022-05-02T14:04:34Z

...ration-smb/src/main/java/org/springframework/integration/smb/outbound/SmbMessageHandler.java

+		super(remoteFileTemplate);
+	}
+
+	public SmbMessageHandler(SmbRemoteFileTemplate remoteFileTemplate, FileExistsMode mode) {


I think we need to disable SmbRemoteFileTemplate-based ctors for time being.
Just for the case to not encourage end-users to use those deprecated properties!

Not sure I understand... you don't want anybody to use SmbRemoteFileTemplate? I'm just following the pattern for FTP channel adapter in some respects...

That's OK to use it outside of this SmbMessageHandler.
The point is that setReplaceFile(true) has to be called on the SmbSessionFactory and if we expose a SmbRemoteFileTemplate-based ctor, that means end-user will have to call that deprecated setReplaceFile() manually.
If we don't have this kind of ctors, people will just use an existing one and and won't call a deprecated API, because, yeah, it is deprecated.

Does it make sense?

We are extending FileTransferringMessageHandler so I have to support all the constructors in it... How am I going to do that unless I change SmbRemoteFileTemplate like I first proposed in the other branch?

Why is that?
There is just enough to have a one ctor based on one super or delegating to another super?
We are not forced to implement all the super ctors.

artembilan · 2022-05-02T14:06:45Z

...ration-smb/src/main/java/org/springframework/integration/smb/outbound/SmbMessageHandler.java

+	public SmbMessageHandler(SessionFactory<SmbFile> sessionFactory) {
+		this(new SmbRemoteFileTemplate(sessionFactory));
+		SmbSessionFactory smbSessionFactory = (SmbSessionFactory) sessionFactory;
+		smbSessionFactory.setReplaceFile(true);


I think this one throws a deprecation warning, therefore this ctor has to be marked with the @SuppressWarning("deprecation").
It is totally fine to use a deprecated API in the library code since we need to retain the current behavior.
What we only need to avoid is to let end-users call those deprecated setters from their code: we are going to remove them in the next version anyway.

artembilan · 2022-05-02T15:04:23Z

...ration-smb/src/main/java/org/springframework/integration/smb/outbound/SmbMessageHandler.java

+
+	public SmbMessageHandler(SessionFactory<SmbFile> sessionFactory) {
+		this(new SmbRemoteFileTemplate(sessionFactory));
+		SmbSessionFactory smbSessionFactory = (SmbSessionFactory) sessionFactory;


I think we need to have a type check here.
There is a CachingSessionFactory which may wrap an original one. So, we will fail here with class cast exception.

This is kinda why I wanted to clean up this code to remove the unneeded flags and go to a major version... this is getting messy now. When you see the latest edits you will understand what I mean...

Yeah...
If you feel so uncomfortable with such a mess, we definitely can look into a new major version.
But that has to go to Spring Integration project already as a module: #254

I just don't want to support two major versions at the same time...

WDYT?

Yeah understood,,, I'm working on trying to get this as clean as possible.

artembilan · 2022-05-02T16:30:04Z

...rc/test/java/org/springframework/integration/smb/outbound/SmbSendingMessageHandlerTests.java

@@ -62,7 +62,7 @@ public void prepare() {
 		smbSessionFactory.setUsername("sambaguest");
 		smbSessionFactory.setPassword("sambaguest");
 		smbSessionFactory.setShareAndDir("smb-share/");
-		//smbSessionFactory.setReplaceFile(true);  // deprecated as of 1.3
+		//smbSessionFactory.setReplaceFile(true);  // deprecated as of 1.2.2


Why do you still keep this code over here?
I really see it only removed altogether.
Having it commented out does not give us too much value.

...ration-smb/src/main/java/org/springframework/integration/smb/outbound/SmbMessageHandler.java

artembilan · 2022-05-02T20:06:55Z

...ration-smb/src/main/java/org/springframework/integration/smb/outbound/SmbMessageHandler.java

+	@SuppressWarnings("deprecation")
+	public SmbMessageHandler(SessionFactory<SmbFile> sessionFactory, FileExistsMode mode) {
+		super(new SmbRemoteFileTemplate(sessionFactory));
+		Assert.isTrue(mode == FileExistsMode.REPLACE, "Only FileExistsMode.REPLACE is supported.");


Why is that?
The mode is not a session factory responsibility.
This is exactly what FileTransferringMessageHandler does for us.

What only I would like to see in this ctor is a delegation to the super one like you do now, but with this mode propagation.

In addition it would be a plus to call setReplaceFile() if the provided mode is a FileExistsMode.REPLACE.

I'm not trying to argue with you but we can't support any mode other than FileExistsMode.REPLACE, why wouldn't we want to inform the developer that only that mode is supported?

Not sure in your conclusion. Just follow the this.remoteFileTemplate.send(message, this.mode) code and see how that mode is used.
There is a logic like in the RemoteFileTemplate:

if (FileExistsMode.REPLACE.equals(mode)) { session.write(stream, tempFilePath); } else if (FileExistsMode.APPEND.equals(mode)) { session.append(stream, tempFilePath); }

And this has nothing to do with an SMB protocol. We just call respective session method and so on.

I get that... but to fix this bug replaceFile always has to be true. There are a few ways to fix it, only one is backwards compatible and will not drive a major version change.

default replaceFile=true in SmbConfig

remove all instances of replaceFile code in SmbSession so that the existing file deletes as intended on the SMB share in the write and rename methods

default replaceFile=true in all constructors for SmbMessageHandler

All the code for replaceFile and even useTempFile is redundant now, it predates any of this code for FileExistsMode.

I still don't understand what use case/requirement you are trying to solution for here with this new public SmbMessageHandler(SessionFactory sessionFactory, FileExistsMode mode).

My point is to retain the FileTransferringMessageHandler functionality as much as possible.
And that one opens for us only if we expose a ctor based on the FileExistsMode.
There is nothing special to do for the SmbSession since it supports an APPEND as is even right now without any changes.

artembilan

I think your changes are very close to what I would expect as a fix for the issue.

I'll pull it locally today for final review and probably will merge after.

Thank you!

Feel free to look into moving this project as a module to the https://github.com/spring-projects/spring-integration with respective overhaul afterwards for a new major version over there.

GregBragg · 2022-05-03T14:22:47Z

I think your changes are very close to what I would expect as a fix for the issue.

I'll pull it locally today for final review and probably will merge after.

Thank you!

Feel free to look into moving this project as a module to the https://github.com/spring-projects/spring-integration with respective overhaul afterwards for a new major version over there.

Awesome! Let me know when you can do a release build as I am waiting on these changes for my services to use.
I'd be interested in moving this to the main SI project folder and maintaining it there going forward.

Thanks!

artembilan · 2022-05-03T16:13:43Z

Merged as 9ef8e04 after some minor clean ups.

I'm going to release that version today.

Thank you for contribution, @GregBragg ; looking forward for more!

GregBragg added 2 commits April 29, 2022 16:07

Fix for Issue spring-projects#246, replaceFile must be true for a fil…

5505b04

…e overwrite

Updates after further unit testing

4b96d2c

artembilan requested changes May 2, 2022

View reviewed changes

artembilan mentioned this pull request May 2, 2022

Issue #246 deprecate file replace option from smb session factory #257

Closed

artembilan requested changes May 2, 2022

View reviewed changes

Updated for initial PR review requested changes

b9fee42

artembilan requested changes May 2, 2022

View reviewed changes

removed comment to satisfy PR review

9ca1fcb

artembilan requested changes May 2, 2022

View reviewed changes

...ration-smb/src/main/java/org/springframework/integration/smb/outbound/SmbMessageHandler.java Show resolved Hide resolved

Updated to satisfy PR review requests for changes

f6efe67

artembilan requested changes May 2, 2022

View reviewed changes

GregBragg added 2 commits May 2, 2022 16:50

More updates after further PR review

8ce7264

Updated again based on PR review comments

15c89ca

artembilan approved these changes May 3, 2022

View reviewed changes

artembilan closed this May 3, 2022

GregBragg deleted the Issue_#246_Deprecate_file_replace_option branch May 22, 2022 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #246 deprecate file replace option, retain minor version #258

Issue #246 deprecate file replace option, retain minor version #258

GregBragg commented Apr 30, 2022 •

edited

Loading

artembilan May 2, 2022

GregBragg May 2, 2022 •

edited

Loading

artembilan May 2, 2022

GregBragg May 2, 2022

artembilan May 2, 2022

artembilan May 2, 2022

GregBragg May 2, 2022

artembilan May 2, 2022

GregBragg May 2, 2022

artembilan May 2, 2022

artembilan May 2, 2022

artembilan May 2, 2022

GregBragg May 2, 2022

artembilan May 2, 2022

GregBragg May 2, 2022

artembilan May 2, 2022

GregBragg May 2, 2022

artembilan May 2, 2022

GregBragg May 2, 2022

artembilan May 2, 2022

GregBragg May 3, 2022

artembilan May 3, 2022

artembilan left a comment

GregBragg commented May 3, 2022

artembilan commented May 3, 2022

Issue #246 deprecate file replace option, retain minor version #258

Issue #246 deprecate file replace option, retain minor version #258

Conversation

GregBragg commented Apr 30, 2022 • edited Loading

Choose a reason for hiding this comment

GregBragg May 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

GregBragg commented May 3, 2022

artembilan commented May 3, 2022

GregBragg commented Apr 30, 2022 •

edited

Loading

GregBragg May 2, 2022 •

edited

Loading