#4930 defect closed fixed (fixed)
FTP server lacks tests for RNTO and RNFR.
Reported by: | Adi Roiban | Owned by: | spiv |
---|---|---|---|
Priority: | low | Milestone: | |
Component: | ftp | Keywords: | |
Cc: | itamarst, Adi Roiban, Thijs Triemstra | Branch: |
branches/ftp-rnfr-tests-4930
branch-diff, diff-cov, branch-cov, buildbot |
Author: | spiv |
Description (last modified by )
calling RNTO while being authenticated, without calling RNFR will generate an unhandled exceptions since self._fromName is only defined in RNFR.
Here is a patch that identifies the problem:
Please let me know if you agree with with fix and I could provide a full patch together with the required tests.
-
TwistedCore-10.2.0/twisted/protocols/ftp.py
=== modified file 'TwistedCore-10.2.0/twisted/protocols/ftp.py'
752 752 return BAD_CMD_SEQ, "PASS required after USER" 753 753 754 754 elif self.state == self.AUTHED: 755 if cmd == 'RNTO': 756 return BAD_CMD_SEQ, "RNFR required before RNTO" 755 757 method = getattr(self, "ftp_" + cmd, None) 756 758 if method is not None: 757 759 return method(*params)
Attachments (3)
Change History (18)
comment:1 Changed 11 years ago by
Cc: | itamarst added |
---|
comment:2 Changed 11 years ago by
Description: | modified (diff) |
---|
Changed 10 years ago by
Attachment: | 4930-rnto-1.diff added |
---|
comment:3 Changed 10 years ago by
Keywords: | review added |
---|
comment:4 Changed 10 years ago by
Cc: | Adi Roiban added |
---|
comment:5 Changed 10 years ago by
Cc: | Thijs Triemstra added |
---|---|
Keywords: | review removed |
Owner: | set to Adi Roiban |
Thanks. Before this can land the testcase names:
- should start with
test_
- need a docstring
Changed 10 years ago by
Attachment: | 4930-rnto-2.diff added |
---|
comment:6 Changed 10 years ago by
Keywords: | review added |
---|
Hi,
I have attached a new diff.
Sorry for missing the docstring and the test_ prefix.
I have also renamed testSYST to test_SYST and added a docstring since I used it as an example for starting the new test.
For test_RNFRandRNTO I have also added an 'integration' check for file renaming on disk.
Please let me know if it needs other changes. Cheers
Changed 10 years ago by
Attachment: | 4930-rnto-3.diff added |
---|
comment:7 Changed 10 years ago by
Owner: | Adi Roiban deleted |
---|
comment:8 Changed 10 years ago by
Owner: | set to spiv |
---|
comment:9 Changed 10 years ago by
Status: | new → assigned |
---|
comment:10 Changed 10 years ago by
Keywords: | review removed |
---|---|
Summary: | RNTO without prior calling RNFR should send a bad command sequence response → FTP server lacks tests for RNTO and RNFR. |
As the original goal of this ticket has been satisfied I've updated the summary to reflect the goal of the current patches.
userHome=self.directory,
It's a bit of a misfeature that the anonymous user implies read-only access, as it leads to ugliness like adding cruft about logging-in non-anonymous users to write tests for RNFR/RNTO. The capabilities of the client should be orthogonal to how the login was done. Oh well, something to worry about in another ticket.
371 def test_RNFRandRNTO(self): 372 """ 373 Sending the RNFR command followed by RNTO should perform a 374 successful rename operation. 375 """
Say "will perform" rather than "should perform". Be assertive in test docstrings :)
Also, the claim isn't strictly true: it also requires that the source and destination filenames are valid (or even more pedantically, that IFTPShell implementation accepts the rename(src, dest)
call). So I'd say "Sending the RNFR command followed by RNTO, with valid filenames, will perform a successful rename operation".
["250 Requested File Action Completed OK"],
This test highlights that the Title Cased Phrase here is pretty weird and inconsistent with the other messages. Not the fault of this patch, but if you're inclined to please submit a patch to tidy this up.
As my only comment requesting action is a tweak to a docstring, I'll go ahead and submit this (assuming the tests actually pass).
comment:11 Changed 10 years ago by
Author: | → spiv |
---|---|
Branch: | → branches/ftp-rnfr-tests-4930 |
(In [34797]) Branch to ftp-rnfr-tests-4930.
comment:13 Changed 10 years ago by
I forgot to add: we could do with even more tests here, e.g. exercising what happens when the server encounters an error when tries to perform the rename.
comment:14 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:15 Changed 10 years ago by
Hi spiv and many thanks for the review.
Your comments are very helpful.
Hope it the future the patches will be perfect :)
Cheers!
It looks like this was already implemented in latest Twisted version.
I was not able to find the test for server so I start adding a test for this case.
While working at this, I also found that there is no test for a proper RNFR and RNTO for the server. There are many tests for the client.
I have attached a patch containing the 2 tests.