Mailing-List: contact users-help@camel.apache.org; run by ezmlm
Precedence: bulk
Reply-To: users@camel.apache.org
Received-SPF: pass (nike.apache.org: domain of bengt.rodehav@gmail.com
 designates 209.85.192.180 as permitted sender)
MIME-Version: 1.0
Sender: bengt.rodehav@gmail.com
In-Reply-To: 
 <CAF8t5XteKOdeVoo9YQ7zAZP4Wom41CUdJX0WG2yTbj0L9gZ9Rw@mail.gmail.com>
References: 
 <CAJ0TPGJ6gdaEuYgjpKG42HR-ozbyC0e5Z=mi2xRDmnonBtY0Eg@mail.gmail.com>
	<5183D4BB.8010001@gmail.com>
	<CAJ0TPG+baX814g7F2nc5PvjoKVZoVEQwbXLaEWXJTSvsTEc3bw@mail.gmail.com>
	<CAJ0TPGJ5-Mwo1u+ndM2yz69jq6RLdfbdfVB34WxaaWio4WtVhg@mail.gmail.com>
	<CAF8t5XteKOdeVoo9YQ7zAZP4Wom41CUdJX0WG2yTbj0L9gZ9Rw@mail.gmail.com>
Date: Thu, 30 May 2013 10:23:48 +0200
Message-ID: 
 <CAJ0TPGJz58LZPH5s2qq8xUB-SKRisLDueOi3um2DNYskk5=GbQ@mail.gmail.com>
Subject: Re: Sftp bug in Camel 2.11.0
From: Bengt Rodehav <bengt@rodehav.com>
To: users@camel.apache.org
Content-Type: multipart/alternative; boundary=047d7b6781b0bb213b04ddeb3770

--047d7b6781b0bb213b04ddeb3770
Content-Type: text/plain; charset=ISO-8859-1

Hello Aki - sorry for the late reply.

Yes, I do realize that the reason for this code is to do a stepwise change
of directories when going back to the "home" directory. What I don't
understand is why this is necessary. It is only done in sftp - not in
ftp/ftps.

I have never been in any situation where stepwise is needed at all. But,
since stepwise by default is true, I assume that these situations exist
(but I think the default should be not to use stepwise...). But, why would
you have to change directories stepwise when going back up only in sftp but
not in ftp/ftps?

Also, I always disconnect after each poll (I've found it much more stable)
which means that going back up is totally unnecessary since I will
re-connect on next poll and therefore start in the "home"directory again.

Of course, fixing the logic might be preferrable to removing it if there is
a need for this kind of logic...

BTW, do you know why the stepwise option exists? What scenario requires us
to change to a directory in a stepwise fashion instead of changing to that
directory directly?

/Bengt


2013/5/27 Aki Yoshida <elakito@gmail.com>

> Hi Bengt,
> the quoted code itself is needed for the normal case to ensure the stepwise
> operation moves the path upwards from the current directory to where it
> started without touching its upper directory which you potentially have no
> authorization to access.
>
> but the code does not handle the case when the path is at the root, as you
> describe. In that case, this code needs to be skipped as it is not needed
> (because you started out from the root and you have authorization down from
> the root to the current directory).
>
> I commented to the ticket.
>
> regards, aki
>
>
> 2013/5/6 Bengt Rodehav <bengt@rodehav.com>
>
> > JIRA created:
> >
> > https://issues.apache.org/jira/browse/CAMEL-6335
> >
> > /Bengt
> >
> >
> > 2013/5/6 Bengt Rodehav <bengt@rodehav.com>
> >
> > > 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 <hzbarcea@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
> > >>>
> > >>>
> > >
> >
>

--047d7b6781b0bb213b04ddeb3770--