Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor FileTransferringMessageHandler impls and resepctive Java DSL components #3092

Closed
artembilan opened this issue Oct 29, 2019 · 13 comments · Fixed by #3304
Closed

Refactor FileTransferringMessageHandler impls and resepctive Java DSL components #3092

artembilan opened this issue Oct 29, 2019 · 13 comments · Fixed by #3304

Comments

@artembilan
Copy link
Member

The FileTransferringMessageHandler hierarchy possible refactoring:

  1. Pursue a consistency: an SftpMessageHandler always comes only with the SftpRemoteFileTemplate, so it looks like an FtpMessageHandler has to require only its FtpRemoteFileTemplate. Therefore FtpMessageHandler(RemoteFileTemplate<FTPFile> remoteFileTemplate, FileExistsMode mode) is deprecated and must be replaced with the FtpRemoteFileTemplate-based one.
  2. Since SftpMessageHandler cannot be created from the basic RemoteFileTemplate, its SftpMessageHandlerSpec must require a proper SftpRemoteFileTemplate. Therefore those ctors must be deprecated in favor of newly introduced for SftpRemoteFileTemplate
  3. Do same for the FtpMessageHandlerSpec
  4. I think existing ctors in the FileTransferringMessageHandlerSpec, and therefore - FileTransferringMessageHandler should exist for a while. There might be some use-cases when we can fully rely on the generic RemoteFileTemplate.
@joaquinjsb
Copy link
Contributor

I'm going to give a look to this as well!

@artembilan artembilan added this to the 5.4.x milestone Feb 27, 2020
@artembilan
Copy link
Member Author

Kinda breaking change in the binary. Moving to the next release.

@deepak5127
Copy link
Contributor

Hi artembilan ,

I am newbie to GitHub contributions. Please let me know if I can take a look into this issue. Thanks

@artembilan
Copy link
Member Author

Hi @deepak5127 ,

Yes, you definitely welcome to take a look into this.
Please, make yourself familiar with our contribution process first of all: https://github.com/spring-projects/spring-integration/blob/master/CONTRIBUTING.adoc

Feel free to ask more question here if any.

Thank you for consider to fix it!

@deepak5127
Copy link
Contributor

Sure, thanks @artembilan

@deepak5127
Copy link
Contributor

@artembilan Started looking into FtpMessageHandlerSpec refacotring. I see RemoteFileTemplate is used in all three specs - inbound, outbound and gateway.

The issue says to refactor only FtpMessageHandlerSpec. So should I keep FtpOutboundGatewaySpec and FtpStreamingInboundChannelAdapterSpec as - is ?

@artembilan
Copy link
Member Author

Let's fix FtpMessageHandler and its FtpMessageHandlerSpec first - Same way we have an SftpMessageHandler! Then we can take a look into gateways and inbound channel adapters.

@deepak5127
Copy link
Contributor

Sure, thanks

@deepak5127
Copy link
Contributor

Sorry one more question, FtpMessageHandlerSpec and SftpMessageHandlerSpec are called from Ftp and Sftp classes. Should i deprecate methods which has (RemoteFileTemplate remoteFileTemplate) and (RemoteFileTemplate remoteFileTemplate, FileExistsMode fileExistsMode) in favor of their respective SftpRemoteFileTemplate and FtpRemoteFileTemplate ?

@artembilan
Copy link
Member Author

@deepak5127 ,

That's correct. I'm fully agree that we need to follow that change anyway.

@deepak5127
Copy link
Contributor

deepak5127 commented Jun 11, 2020

Thanks @artembilan . I have completed the code changes in my local branch.

Couple of questions,

  1. In FtpMessageHandlerSpec, for FtpMessageHandlerSpec(FtpRemoteFileTemplate ftpRemoteFileTemplate) method, it is internally calling FtpMessageHandler(SessionFactory sessionFactory) . This again creates new FtpRemoteFileTemplate. I think instead of calling FtpMessageHandler(SessionFactory sessionFactory), we can call FtpMessageHandler(FtpRemoteFileTemplate ftpRemoteFileTemplate, FileExistsMode mode) by sending default FtpRemoteFileTemplate.ExistsMode.NLST ?
  2. I dont see Junit written for these outbound adapters in SFTP and FTP tests. Should i write new Test cases in FtpTests and SftpTests ?

@artembilan
Copy link
Member Author

Your observation is correct:

  1. We definitely pursue a goal to avoid that FtpRemoteFileTemplate recreation.
  2. Yes, more test coverage would be beneficial

Thanks; looking forward for Pull Request from you!

deepak5127 added a commit to deepak5127/spring-integration that referenced this issue Jun 11, 2020
@deepak5127
Copy link
Contributor

@artembilan Created PR

@artembilan artembilan modified the milestones: 5.4.x, 5.4 M1 Jun 15, 2020
deepak5127 added a commit to deepak5127/spring-integration that referenced this issue Jun 15, 2020
deepak5127 added a commit to deepak5127/spring-integration that referenced this issue Jun 15, 2020
deepak5127 added a commit to deepak5127/spring-integration that referenced this issue Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants