ftpd returns 0 for all timestamps

[zooko]

It appears that the FTP server, unlike the SFTP server, always returns Jan 1 1970 for all timestamps.

[lebek]

That just fixes the modified time. The underlying Twisted implementation doesn't handle create or last access times.

[zooko]

lebek: thank you for the patch! It needs a unit test before we can commit it to trunk. Would you be willing to write on?

[lebek]

Eh, ignore the comment on that attachment.

[lebek]

fix_ftpd_mtime.dpatch​ is obsolete, fix_ftpd_mtime_with_unit_test.dpatch​ and fix_ftpd_size_type.dpatch​ should be applied together.

[davidsarah]

These look excellent, thanks. Comments:

• Instead of the constant 626644800 in expected_root, use a class attribute, e.g. self.FALL_OF_THE_BERLIN_WALL.
• "if name not in children: raise NoSuchChildError(name)" in _modifier is not covered, and not needed (it's ok to let it raise a KeyError on children[name]).
• The case where "linkmotime" is in the metadata is not tested. (I saw this by looking at the test coverage [*] and seeing that the relevant line "value = metadata["tahoe"]["linkmotime"]" of src/allmydata/frontends/ftpd.py was not run. Of course many other lines of ftpd.py are also not tested yet, but this is a line added by the fix.)
• It is not tested that a file added via FTP (rather than by the internal Python API) is created with the right timestamp. This ticket can be closed without that test as long as it is documented on #512 that it is needed.
• Add a comment above the relevant line in expected_root saying that the timestamp is expected to be 0 if no timestamp metadata is present.

[*] wiki:HowToWriteTests , which is currently being written, will describe how to generate test coverage reports.

[lebek]

---

Content-Disposition: form-data; name="replace"

on

[lebek]

Thanks for the review davidsarah.

Replying to davidsarah:

• Instead of the constant 626644800 in expected_root, use a class attribute, e.g. self.FALL_OF_THE_BERLIN_WALL.

Done, also added a self.TURN_OF_MILLENIUM in order to differentiate the output in the case where both mtime and linkmotime are set.

• "if name not in children: raise NoSuchChildError(name)" in _modifier is not covered, and not needed (it's ok to let it raise a KeyError on children[name]).

Agreed.

• The case where "linkmotime" is in the metadata is not tested. (I saw this by looking at the test coverage [*] and seeing that the relevant line "value = metadata["tahoe"]["linkmotime"]" of src/allmydata/frontends/ftpd.py was not run. Of course many other lines of ftpd.py are also not tested yet, but this is a line added by the fix.)

Fixed.

• It is not tested that a file added via FTP (rather than by the internal Python API) is created with the right timestamp. This ticket can be closed without that test as long as it is documented on #512 that it is needed.

I have added a note to #512.

• Add a comment above the relevant line in expected_root saying that the timestamp is expected to be 0 if no timestamp metadata is present.

Done.

[davidsarah]

Excellent! Good tests.

[davidsarah]

Note that fix_ftpd_size_type.dpatch​ is equivalent to warner's patch on #680, which we dithered about applying for three years because of an incompatibility with one FTP client, curlftpfs (although it isn't clear that the change breaks anything that wasn't already broken). We eventually decided to apply it, but then missed the 1.9.0 deadline.

[davidsarah]

Fixed by 14a50f258a0cb249, be1fd9d2b501ade9 and 7f6ee7e9180377bc.

[davidsarah]

with a pyflakes fix in 74cfa65f0d35ea12.