Opened 4 years ago

Closed 4 years ago

Last modified 8 months ago

#6284 defect closed fixed (fixed)

FTP.ftp_STOR() should handle IsADirectoryError error

Reported by: Adi Roiban Owned by: Tom Prince
Priority: normal Milestone:
Component: ftp Keywords:
Cc: itamarst Branch: branches/ftp_STOR_isadirectory-6284
branch-diff, diff-cov, branch-cov, buildbot
Author: tomprince

Description

This is the second part of #5441

Right now, the FileHandler raised a IsADirectory error when trying to write over an existing folder.

The FTP.ftp_STOR() handle IsNotADirectoryError so the IsADirectory error remains unhandled.

I will attach the diff

Attachments (2)

6248-1.diff (4.0 KB) - added by Adi Roiban 4 years ago.
6284-2.diff (1.7 KB) - added by Adi Roiban 4 years ago.
applied on top of 7fd78ad tomprince 2013-03-18 Apply 6248-ftp-stor-folder.diff from adiroiban.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: itamarst added

Changed 4 years ago by Adi Roiban

Attachment: 6248-1.diff added

comment:2 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Hi,

I have attached the patch.

The error is due to a copy/paste from ftp_RMD .

I have also added docstring to callbed and tried to re-order them to improve reading the code.

There is also a live diff here:

https://github.com/chevah/twisted/compare/chevah:master...chevah:6248-ftp-stor-folder

comment:3 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Adi Roiban

Thanks for your work on this.

  • ebOpened wants to log errors that aren't FTPCmdError, at least. It probably does want to keep the white-listing behaviour, since there are a bunch of FTPCmdErrors that shouldn't be happening at that point (and thus are unexpected).
  • ebSent looking at the code that could lead to ebSent, it doesn't look like there is any expected error, so I think any error there should be logged.
  • Since you are changing how errors are handled in all he callbacks, you should add tests for all the cases. (coverage reports at least one with the current code)

Please resubmit after addressing the above points.

comment:4 in reply to:  3 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: changed from Adi Roiban to Tom Prince

Hi and thanks for the review,

Replying to tom.prince:

Thanks for your work on this.

  • ebOpened wants to log errors that aren't FTPCmdError, at least. It probably does want to keep the white-listing behaviour, since there are a bunch of FTPCmdErrors that shouldn't be happening at that point (and thus are unexpected).

I have added a log for non-FTPCmdError exceptions.

  • ebSent: looking at the code that could lead to ebSent, it doesn't look like there is any expected error, so I think any error there should be logged.

The current DTP protocol implementation is very simple and it does not signal errors to the FTP protocol.

Custom implemtations of DTP protocol could raise a FTPCmdError instead of the current: raise Exception("Crap damn crap damn crap damn")

https://github.com/chevah/twisted/blob/6248-ftp-stor-folder/twisted/protocols/ftp.py#L470

This is why I chose to not log FTPCmdError and they should not be considered "unexpected" errors.

The code from master for ebSent is just returning FTPCmdError, but I don't see where that returned error is handled. This is why I handle FTPCmdError in ebSent.

  • Since you are changing how errors are handled in all he callbacks, you should add tests for all the cases. (coverage reports at least one with the current code)

Please resubmit after addressing the above points.

I added coverage tests for error handling.

Nice diff is here: https://github.com/chevah/twisted/compare/chevah:master...chevah:6248-ftp-stor-folder

Can always be downloaded by appending .diff to the url

https://github.com/chevah/twisted/compare/chevah:master...chevah:6248-ftp-stor-folder.diff

Let me know if you want to to upload a new diff file.

comment:5 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: changed from Tom Prince to Adi Roiban
  • ebSent: looking at the code that could lead to ebSent, it doesn't look like there is any expected error, so I think any error there should be logged.

The current DTP protocol implementation is very simple and it does not signal errors to the FTP protocol.

Custom implemtations of DTP protocol could raise a FTPCmdError instead of the current: raise Exception("Crap damn crap damn crap damn")

There is no way to use a custom dtp implementation. It is an implementation detail, so we don't need to worry about handling that case. Also, the errback is only going to get called if the transfer aborted, so that is the error we should return.

Otherwise, it looks good.

comment:6 Changed 4 years ago by Tom Prince

Author: tomprince
Branch: branches/ftp_STOR_isadirectory-6284

(In [37562]) Branching to ftp_STOR_isadirectory-6284.

comment:7 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: changed from Adi Roiban to Tom Prince

t.w.p.FTP.dtpFactory is documented as a public method and it can be replaced with custom implementations.

Is there anyone using the default DTP implementation in production?!?!

With code like 'raise Exception("Crap damn crap damn crap damn")' this looks to me only like an example toy implementation.

Cheers, Adi

comment:8 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: changed from Tom Prince to Adi Roiban

If you look at https://github.com/tomprince/twisted/blob/trunk/twisted/protocols/ftp.py#L894 and https://github.com/tomprince/twisted/blob/trunk/twisted/protocols/ftp.py#L894 which are the two place where dtpFactory is assigned, doesn't give a user any place to override the factory before creating it. In any case, it isn't an interesting protocol, to implement a different way.

Regarding the exception, that code has been there from 2005. And, that error is not going to be propagated beyond ebSent.

comment:9 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: changed from Adi Roiban to Tom Prince

Hi,

Thanks for the review and sorry for the trouble with my previous comment, in which I was wrong.

I have reverted ebSent implementation to the current code and adapted the new tests.

Diff is here: https://github.com/chevah/twisted/compare/chevah:master...chevah:6248-ftp-stor-folder

Looks like GitHub got confused by this diff and it does not look to great. A side by side diff would help here.

Can always be downloaded by appending .diff to the url

https://github.com/chevah/twisted/compare/chevah:master...chevah:6248-ftp-stor-folder.diff

Let me know if you want it attached as a file.

Cheers, Adi

comment:10 Changed 4 years ago by Tom Prince

The linked patch doesn't apply against the branch.

Changed 4 years ago by Adi Roiban

Attachment: 6284-2.diff added

applied on top of 7fd78ad tomprince 2013-03-18 Apply 6248-ftp-stor-folder.diff from adiroiban.

comment:11 Changed 4 years ago by Adi Roiban

Hi. I have attached a new diff on top of the branch dedicated to this ticket.

comment:12 Changed 4 years ago by Tom Prince

(In [37942]) Apply 6284-2.diff from adiroiban.

Refs: #6284

comment:13 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: newclosed

(In [37947]) Merge ftp_STOR_isadirectory-6284: Properly handle IsADirectoryError in ftp_STOR.

Author: adiroiban Reviewers: tom.prince Fixes: #6284

For some reason, it was handling IsNotADirectoryError, instead. Now any FTPCmdError is passed through, rather than being logged.

comment:14 Changed 8 months ago by hawkowl

Keywords: review removed

[mass edit] Removing review from closed tickets.

Note: See TracTickets for help on using tickets.