On Fri, Mar 14, 2014 at 7:56 PM, Adam Murdoch <adam.murd...@gradleware.com>wrote:
> > Yet, there is plenty to be done on error handling and making sure all > resources are being released even in error scenarios - so the boring but > important parts. > > I have one question about error handling. There are three types of > failures listed in the spec: invalid credentials, cannot connect to the > server and everything else (called server exception). I think that we > probably want to extract all permission related errors (when both resolving > and uploading) as a separate error type so that the error message provides > user an idea that the error is permission related as it feels that it might > be a common error. What do you think? > > > The key is to make the error message informative at this stage, which we > don't necessarily need a new type for. If we formalise 'not authorised' > problems in the resource APIs then we would also need to update the file > and http backed resource implementations to. So, I'd leave this as a later > refactoring. > What I meant here by a new type is not a new exception class but to differentiate permission errors from generic errors when it comes to what message is being shown when such an error occurs. At the moment I'm throwing a new runtime exception of type SftpException with a message saying what has happened (invalid credentials, connection error) and where (host and port part of the url). I was asking if we should check if we have permissions to perform a given operation (file download or upload, directory creation, etc) and if not throw a SftpException with a message saying that permission was denied to do it. > > > Also, DefaultExternalResourceRepository requires an implementation of an > ExternalResourceLister yet it is currently not exercised by any test - to > be honest I haven't looked into how it's used and what kind of tests would > be needed for it so any pointers would be appreciated. Do we want to add > some tests to the initial story for that? > > > It's used whenever there's a dynamic version in a dependency declaration. > So, if you've got one of these in one of the tests somewhere, then we're > good. > I don't have a test like that yet so I will add some. Thanks for pointing me in the right direction. > > > > On Thu, Mar 6, 2014 at 3:56 AM, Adam Murdoch > <adam.murd...@gradleware.com>wrote: > >> >> On 6 Mar 2014, at 7:33 am, Marcin Erdmann <marcin.erdm...@proxerd.pl> >> wrote: >> >> I finally got my head around the contract of ExternalResourceAccessor and >> after doing some reverse engineering of HttpResourceAccessor and >> Ivy's SFTPResolver as well as skimming SFTP protocol specs the following >> are my findings and questions: >> >> Error handling is far from perfect in both clients - they only return a >> generic error when you try to get file metadata for a file that doesn't >> exist but there seems to be a specific substatus for such situation called >> SSH_FX_NO_SUCH_FILE. >> I can see that this substatus is returned by SSHD's implementation of SFTP >> server when accessing files that don't exist even though the spec( >> http://tools.ietf.org/id/draft-ietf-secsh-filexfer-00.txt) cryptically >> describes it as "is returned when a reference is made to a file which >> should exist but doesn't" - this should but doesn't part baffles me a >> bit. Anyway I will assume that this substatus is returned whenever trying >> to stat a non-existing file. >> >> I'm going to use SSHD's client as it seems to be better maintained and I >> like it more. I will be also able to easily change it's behavior when it >> comes to handling the aforementioned SSH_FX_NO_SUCH_FILE substatus. >> >> We will probably want to throw a specific contextual exception when >> credentials are incorrect, right? >> >> >> At this stage, I think it's sufficient that the error message gives the >> user some idea that the credentials are incorrect. Eventually, it would be >> nice to tell the difference, if possible. >> >> >> It feels like we want to have separate ssh sessions for getting metadata >> and getting the file - the fact that you request an ExternalResource >> doesn't mean that you will ever read it and thus close it, but you need to >> open a ssh session to get the metadata. So the idea here is to open a >> session, get metadata to see if the resource exists and close the session >> possibly passing the metadata to the created ExternalResource if the >> resource exists. Then, if resource contents are requested open a new >> session and close it when close() is called on the ExternalResource. Are we >> ok with such approach? >> >> >> Yes, for now. I think at some point we will rework >> the ExternalResourceAccessor interface so that it can better deal with the >> fact that for some transports (file, sftp), the meta-data for a resource >> and the content for a resource have to be fetched as separate operations. >> The interface at the moment assumes that it's cheap-ish to get the >> meta-data at the same time the content is requested. >> >> This way, the caller can decide whether it needs the meta-data or not, >> and invoke the appropriate operations - get meta-data, get content or get >> meta-data and content (if supported). >> >> >> -- >> Adam Murdoch >> Gradle Co-founder >> http://www.gradle.org >> VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting >> http://www.gradleware.com >> >> >> >> > > > -- > Adam Murdoch > Gradle Co-founder > http://www.gradle.org > VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting > http://www.gradleware.com > > > >
