spring-projects / spring-integration

Spring Integration provides an extension of the Spring programming model to support the well-known Enterprise Integration Patterns (EIP)
http://projects.spring.io/spring-integration/
Apache License 2.0
1.55k stars 1.11k forks source link

FtpSession does not allow list or listNames without a path [INT-3919] #7866

Closed spring-operator closed 8 years ago

spring-operator commented 8 years ago

Chris Gillespie opened INT-3919 and commented

org.springframework.integration.ftp.session.FtpSession requires a path be passed to both the "list" and "listNames" methods. The path must have text.

The underlying Apache FTP client allows a file listing to be performed without a path (so it is performed in the default folder after logging in to the ftp server). The FTP protocol also does allow a "LIST" command to be performed without an explicit folder path.

I am currently integrating with two commercial FTP servers that require LIST to be performed without a folder path, but I can't achieve that with the current version of FTPSession.

I'm happy to make the code enhancement myself and perform a pull request. For reference I would change lines 66 to 75 to be:

    public FTPFile[] list() throws IOException {
        return ftpClient.listFiles();
    }

    @Override
    public FTPFile[] list(String path) throws IOException {
        if (StringUtils.hasText(path)) {
            return ftpClient.listFiles(path);
        }

        return list();
    }

    public String[] listNames() throws IOException {
        return ftpClient.listNames();
    }

    @Override
    public String[] listNames(String path) throws IOException {
        if ( StringUtils.hasText(path) ) {
            return ftpClient.listNames(path);
        }

        return listNames();
    }

Affects: 4.1.6

Reference URL: http://grepcode.com/file/repo1.maven.org/maven2/org.springframework.integration/spring-integration-ftp/4.1.6.RELEASE/org/springframework/integration/ftp/session/FtpSession.java?av=f

Issue Links:

Referenced from: pull request https://github.com/spring-projects/spring-integration/pull/1704

spring-operator commented 8 years ago

Gary Russell commented

Thanks for the offer to contribute please sign the CLA as described in the Contribution Guidelines.

Note that we added the getClientInstance() method to allow users to invoke features on the underlying client that are not exposed on the session.

For even more convenience, the FtpRemoteFileTemplate has an execute method and will take care of closing the session etc after a user-operation is complete.

spring-operator commented 8 years ago

Artem Bilan commented

Please, look the solution in the PR. I didn't introduce new methods, since we have a base Session<F> contract and looks like SFTP doesn't allow null for the path. Therefore I've just ended up with the solution like "allow null for the list(path) method".

spring-operator commented 8 years ago

Gary Russell commented

It sounds like this solution won't solve the problem that Chris Gillespie reports, when using an outbound gateway or inbound channel adapter:

return ((FTPClient) session.getClientInstance()).printWorkingDirectory();

I am currently integrating with two commercial FTP servers that require LIST to be performed without a folder path, but I can't achieve that with the current version of FTPSession.

(my emphasis)

spring-operator commented 8 years ago

Artem Bilan commented

According the FTP LIST command specification we have:

Returns information of a file or directory if specified, else information of the current working directory is returned.

https://en.wikipedia.org/wiki/List_of_FTP_commands.

Not sure what the problem should be with an explicit workingDirectory...

Although I agree that Crhis should confirm that the solution is good for him.

My big concern there do pass null to the FTP Client that we than end up with some NPE problems downstream, when we can't pass remoteDirectory to the headers, because it is null. That's why the algorithm to fallback to the workingDirectory from client should be good compromise to meet this JIRA requirements and don't introduce any unexpected breaking changes.

spring-operator commented 8 years ago

Gary Russell commented

meet this JIRA requirements

I understand your concern but I don't think it does meet the requirements.

... if specified ...

The spec allows no path to be provided.

The current implementation provides no way to do that.

@Test
public void testNullList() throws Exception {
    DefaultFtpSessionFactory sf = new DefaultFtpSessionFactory();
    sf.setHost("10.0.0.3");
    sf.setUsername("ftptest");
    sf.setPassword("ftptest");
    FtpSession session = sf.getSession();
    FTPFile[] list = session.list(null);
    System.out.println(Arrays.asList(list));
    list = session.list(session.getClientInstance().printWorkingDirectory());
    System.out.println(Arrays.asList(list));
}

Produces the same results (as you would expect) but the LIST command is different:

LIST\r\n

Vs

LIST /home/ftptest\r\n

Again, as you would expect.

The issue is the server in this case does not allow a path in the LIST command.

Since we are releasing M2 next Tuesday, we either need to fix this or push it to M2.

spring-operator commented 8 years ago

Chris Gillespie commented

Hi both,

I have a workaround for this so am happy for it to be pushed back if needed. I have a customised InboundFileSynchronizer and FtpSession to meet my needs. I'd be happy to contribute the code for either or both if that would help.

The problematic FTP servers that I'm working with don't tolerate anything passed as the workingDirectory. I don't know who the vendor of the software is, but it is affecting 2 out of 35 commercial FTP servers that we are connecting to.

Thank you both for your time and effort on this. It seemed like a simple fix at the outset!

Chris.

spring-operator commented 8 years ago

Gary Russell commented

Chris Gillespie Thanks for the update; we think we have a way forward, it's just a bit more work than we expected because some of the higher layers presume a non-null path.