On 15 Mar 2014, at 6:31 am, Marcin Erdmann <marcin.erdm...@proxerd.pl> wrote:
> I've been quiet for a while but made some progress on this. I have all > resolving, some error and one uploading test passing from the first story > described in the design doc. Great. > 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. > > 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. > > > 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
