Issue #246 deprecate file replace option from smb session factory

[GregBragg]

Fixes\mitigates Issue #246, implemented custom channel adapter for SMB outbound.

[GregBragg]

FileExistsMode.REPLACE mode came after this code that was written for SMB extension over 10 years ago... it won't be easy to rip it apart to follow current SI design patterns. We would have to go with a full refactor of this extension library to follow current SI design patterns.

The code to replace a file is heavily embedded in the SmbSession class for methods such as write and rename as part of it's internal code. I'm open to suggestions on how to refactor the code base to follow SI code base, however it will require breaking backwards functionality.

[artembilan]

No, that's not going to break anything. If we just deprecate those options on the SmbSessionFactory, then it will be a signal for end-user to move to the "in favor of" suggestions. I don't say to remove the code, just deprecate and with the default false for those replaceFile and useTempFile on the SmbConfig, the logic is going to the branch we need for FileTransferringMessageHandler support with its FileExistsMode.REPLACE. That's existing code and working functionality.

Only what we have to do as part of this fix is to deprecate misleading options and discourage new user to look into them. Existing users are OK so far until we remove those options and their logic. But this one is really going to a different issue.

[GregBragg]

Ok trying to understand what you mean/want... I can certainly mark the methods in SmbConfig as deprecated.

This is the current default settings in SmbConfig: private boolean replaceFile = false; private boolean useTempFile = false;

Are you fine with the rest of the changes for the implementation of the new SmbMessageHandler class?

[GregBragg]

Let me look into this more... and try to come up with a better fix.

[artembilan]

Well, I don't see an SmbMessageHandler implementation justification yet. That FileExistsMode.REPLACE is really a default one for the FileTransferringMessageHandler. Therefore I see only a deprecation for options on the SmbConfig as a fix for this issue.

[GregBragg]

Well, I don't see an SmbMessageHandler implementation justification yet. That FileExistsMode.REPLACE is really a default one for the FileTransferringMessageHandler. Therefore I see only a deprecation for options on the SmbConfig as a fix for this issue.

The problem is that you still need to set that in the configuration to true the way the code was written... if (this.smbShare.isReplaceFile() && smbFileTo.exists()) { smbFileTo.delete(); }

I'd have to take out that piece of logic to check for true and this piece of logic in SmbSessionFactory in the createSession method: smbShare.setReplaceFile(this.isReplaceFile()); smbShare.setUseTempFile(this.isUseTempFile());

I agree with you that the way the code was written 10 years ago does not make sense today for this extension. Probably best to rip off the bandaid, remove the code that is no longer needed and go with a version 2.0

[artembilan]

The problem is that you still need to set that in the configuration to true the way the code was written

Why is that? Why the plain write into the remote file output stream is not enough to have replace functionality? Please, confirm with some testing that without calling that smbFileTo.delete(); we cannot have a replace feature provided for us by the FileTransferringMessageHandler.

[GregBragg]

The problem is that you still need to set that in the configuration to true the way the code was written

Why is that? Why the plain write into the remote file output stream is not enough to have replace functionality? Please, confirm with some testing that without calling that smbFileTo.delete(); we cannot have a replace feature provided for us by the FileTransferringMessageHandler.

Yeah I did go through all that in the code, and via testing. Those values for replaceFile are expecting a true to do an actual delete of the file... it overrides the base class behaviors. The bug/issue will still remain if we just mark those methods as deprecated, might even be worse because we mark them as "don't use", so nobody sets those flags as true and it fails with an exception.

Give me a day... I'm going to come up with a better fix for this weird, legacy behavior.

[artembilan]

Right. That's my question: why do we need to delete the target file if we are just going to replace its content? If we deal with temporary file and then would like to rename, it is indeed expected to fail if the target file already exists. That's how it works with (S)FTP protocol as well.

[GregBragg]

Right. That's my question: why do we need to delete the target file if we are just going to replace its content? If we deal with temporary file and then would like to rename, it is indeed expected to fail if the target file already exists. That's how it works with (S)FTP protocol as well.

Yes totally agree... this SMB stuff is so old, probably written when the base classes didn't handle it themselves, IDK...

[GregBragg]

Ok code has been checked in, it is no longer backwards compatible with the version 1.2.x code base. The flag replaceFile was embedded everywhere and is now considered redundant with the latest versions of Spring Integration base classes.

[GregBragg]

Why "remove"? We've just discussed "deprecate" for a smooth migration guide.

I'm not convinced yet about a new major version...

I tend to agree but my first pass at this would have been considered backwards compatible by getting around the spiderweb of this flag in multiple classes... it wouldn't have been easy just to mark it as deprecated without removing the code that this flag was affecting.

[artembilan]

I don't ask about removal of those flags functionality, just deprecate their setters to let people know that they are intended to be removed in the future. The deprecation doesn't mean changing existing behavior. We just mark them with the @Deprecated and respective JavaDoc to mention same functionality based on the FileTransferringMessageHandler.

[GregBragg]

I don't ask about removal of those flags functionality, just deprecate their setters to let people know that they are intended to be removed in the future. The deprecation doesn't mean changing existing behavior. We just mark them with the @Deprecated and respective JavaDoc to mention same functionality based on the FileTransferringMessageHandler.

Yes normally I would agree.. but the flags would need to be defaulted to true for the actual version 1.2.x code to work for this issue... that is the underlying problem. This SMB extension code predates anything that has subsequently been implemented for the same functionality in the current version of Spring Integration. It really is now considered redundant... I can't just mark the flags as deprecated unless I change the behavior of the implementation to get around this issue.

[artembilan]

Why it has to be set to true? Looks like in your current change you just removed its code altogether and the logic is left like it would be if we wouldn't set those flags to true. What do I miss, please?

[GregBragg]

Why it has to be set to true? Looks like in your current change you just removed its code altogether and the logic is left like it would be if we wouldn't set those flags to true. What do I miss, please?

It expects a value of true so that it can actually delete the file, which the base class implementation already does by default, however it overrides the base class behavior for the write and rename methods in SmbSession.

[artembilan]

It expects a value of true so that it can actually delete the file, which the base class implementation already does by default,

Where is that code, please?

[GregBragg]

The change might be OK for the next version, but I still would like to see a deprecation one for the current version for a smooth migration onto a new version in the future.

All the changes you did and confirmed with that Main really realigned with my expectations when replaceFile and useTempFile are false on the SessionFactory. Which is a default value for them. Therefore retaining their logic and marking them as @Deprecated is good compromise for the current version. We can come back to removal when we start version 2.0.0.

Not to argue, but that still doesn't fix the underlying problem for this issue which will still cause an exception when the replaceFile is still false and nobody sets it to true in SmbSessionFactory. The useTempFile flag is also redundant because Spring Integration handles this behind the scenes.

I would still have to update the implementation to fix this bug which will break backwards compatibility, which from my perspective when you break the behavior of an existing implementation it should always drive a major version change.

If the only change we are going to do to fix this issue is mark the methods in SmbConfig as deprecated, it does not tell the developer of their code that they must set replaceFile=true for it to work properly. Wasn't that the root cause of this issue?

[artembilan]

But your Main confirms that we don't need replaceFile=true and everything works well with the FileTransferringMessageHandler. Why do we need replaceFile=true in the current version and we don't need it at all in your new version? As far as I see you don't call targetFile.delete() at all, anywhere. And that is fully equivalent to my expectations when replaceFile==false which is default and would be very safe to have it @Deprecated.

[GregBragg]

But your Main confirms that we don't need replaceFile=true and everything works well with the FileTransferringMessageHandler. Why do we need replaceFile=true in the current version and we don't need it at all in your new version? As far as I see you don't call targetFile.delete() at all, anywhere. And that is fully equivalent to my expectations when replaceFile==false which is default and would be very safe to have it @Deprecated.

The code in SmbSession that I highlighted and removed is what is causing the issue, once that code for checking the value of replaceFile flag is deleted, the bug is fixed, however that changes the behavior of the implementation.

This line: if (targetFile.exists() && _this.smbShare.isReplaceFile()_)**

[artembilan]

The code there right now is like this:

            if (this.smbShare.isUseTempFile()) {

                String tempFileName = _path + this.smbShare.newTempFileSuffix();
                SmbFile tempFile = createSmbFileObject(tempFileName);
                tempFile.createNewFile();
                Assert.isTrue(tempFile.canWrite(), "Temporary file [" + tempFileName + "] is not writable.");

                FileCopyUtils.copy(_inputStream, tempFile.getOutputStream());

                if (targetFile.exists() && this.smbShare.isReplaceFile()) {
                    targetFile.delete();
                }
                tempFile.renameTo(targetFile);
            }
            else {
                FileCopyUtils.copy(_inputStream, targetFile.getOutputStream());
            }

So, we don't go inside an if branch when useTempFile == false which is a default value and deprecating it is safe. That this.smbShare.isReplaceFile() won't be true by default anyway. Therefore targetFile.delete(); is not performed at all.

You current fix only presents that FileCopyUtils.copy(_inputStream, targetFile.getOutputStream()); which is indeed going to be the same in the current version when we just don't set useTempFile to true.

So, why do we need to have replaceFile be set to true when we use FileTransferringMessageHandler? I believe the original issue was really to set replaceFile to true which is confusing and misleading. Or as it turned out use the proper solution based on the FileTransferringMessageHandler. But definitely not both. And your current code really confirms that.

Even if we deprecate these option your current logic in the target project is based on the replaceFile = true, it is still going to work. Having a deprecation warning will lead you to read deprecation migration guide and eventually you'll rework a manual replaceFile = true to the FileTransferringMessageHandler.

[GregBragg]

It fails at the rename method here when replaceFile=false, it won't delete the original file before the rename which is where the exception gets thrown:

@Override
public void rename(String _pathFrom, String _pathTo) throws IOException {
    try {

        SmbFile smbFileFrom = createSmbFileObject(_pathFrom);
        SmbFile smbFileTo = createSmbFileObject(_pathTo);

----------> if (this.smbShare.isReplaceFile() && smbFileTo.exists()) { smbFileTo.delete(); } smbFileFrom.renameTo(smbFileTo);

    }
    catch (SmbException _ex) {
        throw new NestedIOException("Failed to rename [" + _pathFrom + "] to [" + _pathTo + "].", _ex);
    }
    if (logger.isInfoEnabled()) {
        logger.info("Successfully renamed remote resource [" + _pathFrom + "] to [" + _pathTo + "].");
    }

}

The if (this.smbShare.isUseTempFile()) { } code block is redundant, it brings no extra value to use a different temp file name when the Spring Integration framework already does it behind the scenes when it already creates a temporary file with a ".writing" file extension name by default (setAutoCreateDirectory is default true in SI)

[artembilan]

I see. So, you talk about a rename() not write(). Thank you for pointing into this! The FtpSession does that removal unconditionally:

    public void rename(String pathFrom, String pathTo) throws IOException {
        this.client.deleteFile(pathTo);
        boolean completed = this.client.rename(pathFrom, pathTo);

Perhaps this is definitely what we also need to do over here? But yeah... May be in the next version.

For the current one I suggest the plan like this:

1. Bring back your SmbMessageHandler, but set replaceFile on the provided SmbSessionFactory, not SmbRemoteFileTemplate.
2. Deprecate those options on the SmbSessionFactory to discourage end-users to use them explicitly.
3. 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(). WDYT?

[GregBragg]

Ok cancel this PR please... I'll have to create a new fresh branch. This one is too convoluted now and I need to retain some of the previous code changes for this fix and for the future 2.0 version.

Thanks!

[artembilan]

Closed in favor of #258