Hello Hadrian,
I'm not really interested in the blame issue. I'm really glad that there
are so many competent people developing Camel which I think is a great
product. It might be interesting, though, to see how long the bug has been
there and what versions are affected.
I've also been thinking why it hasn't been picked up by any tests. I
realize that it must be very difficult to design tests for integration
components like sftp. Ideally you would have a number of ftp/ftps/sftp
servers to perform integration tests against. But I guess that is hard to
maintain. I don't think unit tests (with mocking) will catch bugs like the
above. How are ftp/ftps/sftp integrations tested?
I have noticed that some sftp servers silently ignore attempts to change
directory to a non-existing directory - you won't get there but you won't
get an error code either. But some will give you an error code (like the
one we are integrating to right now).
I will create a JIRA. As for patching I guess the most important part is to
determine if it is necessary to change back to the home directory in a
stepwise fashion or not. If not, then I think we can just remove that code.
If it is necessary, then we must instead fix it.
/Bengt
2013/5/3 Hadrian Zbarcea <hzbar...@gmail.com>
> Bengt, many thanks for reporting this.
>
> Either svn or git tells you pretty quickly how that code got there (git
> blame). Do you mind raising a jira for this embarrassing bug? If you want
> to contribute a patch it'd be highly appreciated. That aside, I'll look
> into it in the afternoon. I'll need to check if that piece of code was
> meant to address another usecase or was an oversight and insufficient
> testing.
>
> Cheers,
> Hadrian
>
>
>
> On 05/03/2013 10:35 AM, Bengt Rodehav wrote:
>
>> Unfortunately I seem to have found another bug in Camel 2.11.0 regarding
>> sftp. I noticed it when I removed the "stepwise=false" argument from my
>> routes thus enabling the default stepwise way of changing directory.
>>
>> The problematic section begins at line 404 in SftpOperations.java:
>>
>> *404:* if (getCurrentDirectory().**startsWith(path)) {
>> *405:* // use relative path
>> *406:* String p = getCurrentDirectory().**
>> substring(path.length());
>> *407:* if (p.length() == 0) {
>> *408:* return;
>> *409:* }
>> *410:* // the first character must be '/' and hence removed
>> *411:* path =
>> UP_DIR_PATTERN.matcher(p).**replaceAll("/..").substring(1)**;
>> *412:* }
>>
>>
>>
>> The problem does not arise when stepping down into the directory to poll
>> but when going back to the directory where we started. Assume that the
>> home
>> directory in the sftp server is "/" and I want to poll the directory "in"
>> relative to the root. My endpoint would look something like this:
>>
>> sftp://user@server/in?**password=secret
>>
>> What happens is that the "i" in "in" is removed and Camel attempts to
>> change to directory "n". Looking at the code above, the following happens:
>>
>> - The current directory is "/in" since we have previously succeeded in
>> moving there and polling for files. We are now attempting to go back to
>> where we started, therefore "path" is "/".
>> - On line 406, the variable "p" will be set to "in", thus removing the
>> leading "/".
>> - On line 411 an incorrect assumption is made: It is assumed that the
>> first
>> character must be "/" but the leading "/" was removed on line 406. The
>> result is therefore that "path" is set to "n" instead of "in".
>>
>> I haven't investigated when this error was introduced. I did however
>> compare with the corresponding logic in FtpOperations.java. In that file
>> the section quoted above does not exist at all. It seems to me that the
>> purpose with this code is to, stepwise, change directory up to ".." - one
>> directory at a time instead of going straight to "/". It seems very
>> unnecessary to me but I assume there is a reason for it. Otherwise, the
>> easiest change would be to just remove the code above and use the same
>> logic as in FtpOperations.java.
>>
>> Currently my only workaround is to use "stepwise=false".
>>
>> /Bengt
>>
>>
