Opened 7 years ago

Closed 5 years ago

Last modified 15 months ago

#4515 enhancement closed fixed (fixed)

support the FTP FEAT and OPTS requests

Reported by: zooko Owned by: Asheesh Laroia
Priority: normal Milestone:
Component: ftp Keywords: ftp, patch, unicode, i18n, encoding, utf-8, utf8
Cc: david-sarah@…, zookog@…, francois@…, davidsarah, zooko@…, Adi Roiban Branch: branches/add-ftp-feat-4515
branch-diff, diff-cov, branch-cov, buildbot
Author: jerub

Description

There is some code proposed in a Tahoe-LAFS ticket to add support for the FEAT request to Twisted's FTP implementation:

http://tahoe-lafs.org/trac/tahoe-lafs/ticket/682#comment:18

Please see that ticket for motivation and details.

Attachments (4)

4515-add-ftp-feat-1.diff (10.5 KB) - added by Adi Roiban 6 years ago.
4515-add-ftp-feat-2.diff (10.7 KB) - added by Adi Roiban 6 years ago.
Add news file.
ftp-feat-uses-double-quotes.patch (2.5 KB) - added by Asheesh Laroia 5 years ago.
Adjusting docstrings to style guide
ftp-feat-more-test-docs.patch (2.9 KB) - added by Asheesh Laroia 5 years ago.
Add docstrings for new tests (and also improve a few other tests per style guide)

Download all attachments as: .zip

Change History (25)

comment:1 Changed 7 years ago by zooko

Cc: zooko@… added
Component: loreftp
Owner: changed from spiv to itamarst

comment:2 Changed 7 years ago by davidsarah

Here's a slight adaptation of that code to be more suitable as an addition to source:trunk/twisted/protocols/ftp.py:

def ftp_FEAT(self, arg=None):
    if arg is not None:
        return defer.fail(CmdSyntaxError('FEAT does not take any argument'))

    if hasattr(self.shell, 'feat'):
        d = defer.maybeDeferred(self.shell.feat)
    else:
        d = defer.succeed([])

    def _reply(features):
        self.sendLine('211- Features')
        for f in features:
            self.sendLine(' ' + f)
        return SYS_STATUS_OR_HELP_REPLY
    d.addCallback(_reply)
    return d

Obviously it needs unit tests and documentation.

Lack of FEAT support was briefly mentioned in #3596, but not fixed there, and I wasn't able to find any other relevant tickets. The FEAT command is defined in RFC 2389.

comment:3 Changed 7 years ago by davidsarah

RFC 2389 says: "The OPTS command MUST be implemented whenever the FEAT command is implemented."

OPTS only has a single-line response, so its implementation could be just:

def ftp_OPTS(self, args=''):
    if hasattr(self.shell, 'opts'):
        return defer.maybeDeferred(self.shell.opts, args)
    else:
        return defer.fail(CmdNotImplementedError('OPTS'))

comment:4 Changed 7 years ago by davidsarah

Summary: support the FTP FEAT requestsupport the FTP FEAT and OPTS requests

comment:5 Changed 7 years ago by Adi Roiban

Cc: Adi Roiban added

There is one problem with getting OPTS and FEAT only from the shell:

Some features like UTF-8 can be implemented at the shell level, but AUTH, PBSZ and PROT, SIZE .. etc are implemented at the FTP transport level.

Since FEAT command described additional commands that are available in the transport level, I think that it would make sense to get the features list from the transport and not from the shell.

Same for OPTS command, I think that it would make sense to implement the OPTS command at the FTP transport layer and the ftp_OPTS command should first get the options from the tranport layer only proxy the OPTS to the shell only as a fallback.

In the same time, I can imagine that FEAT and OPTS command will be used in some custom transport that are extending the default FTP transport implementation.


If someone has time to be a mentor for this bug, I can dedicate my time to implement and write the required tests.

comment:6 Changed 7 years ago by Jean-Paul Calderone

It seems right that the FTP protocol implementation should be participating more in determining the response here. The response to a FEAT command might therefore be a combination of what the shell contributes and what the protocol implementation contributes.

Also, I don't think that a list of strings is a good interface for asking the shell for its contribution. Each of these strings has some associated semantics (presumably ;). So, instead, there should probably be a separate method (perhaps each on a separate interface) for each possibility.

It looks like a lot of the FEAT options are independent of the shell, actually. Maybe it would make sense to start off only supporting it at the FTP protocol level and then add integration with the shell later, for those options which require it. That could be done to resolve new, more specific tickets like Add support for RFC 2640 filename internationalization.

I don't know if the same goes for OPTS, because the RFC is rather vague on its usage.

comment:7 Changed 7 years ago by Adi Roiban

I have created ticket #5100 for UTF-8 filenames.

From my experience I only used the OPTS command for UTF-8 support.

To move on with solving this bug, I think that as a starting point FEAT and OPTS can be implemented as part of UTF-8 filename support and later extended as needed ( ie . as part of REST, APPE, FTPS support).

In the same time, the bug from http://tahoe-lafs.org/ is only complaining about UTF-8 and not a generic FEAT 'framework'

Does it make sense to you to start by fixing bug #5100 or you think that we should first at a generic mechanism for advertising FEAT and OPTS to the clients?

comment:8 Changed 7 years ago by Jean-Paul Calderone

It's good to go from the specific to the general, rather than the reverse. So feel free to pursue #5100 first if you see how to do it now. (This doesn't follow my suggestion to start off without getting help from the shell (since UTF-8 encoded filesystems is something the shell will have to claim -- or not -- to support), but that doesn't really matter; it was just an idea to make it easier to get started on something.)

comment:9 in reply to:  5 Changed 7 years ago by davidsarah

Replying to adiroiban:

There is one problem with getting OPTS and FEAT only from the shell:

Some features like UTF-8 can be implemented at the shell level, but AUTH, PBSZ and PROT, SIZE .. etc are implemented at the FTP transport level.

True, but IIUC, those are commands that Twisted would need to be aware of to define relevant interface methods. Features like UTF-8, on the other hand, don't need any awareness from Twisted (which is also why the shell can/should just provide a list of strings). If and when Twisted gains support for AUTH etc., the relevant feature strings can automagically be added to the list returned by FEAT whenever the transport supports those methods.

exarkun wrote:

It's good to go from the specific to the general, rather than the reverse. So feel free to pursue #5100 first if you see how to do it now.

+1

Changed 6 years ago by Adi Roiban

Attachment: 4515-add-ftp-feat-1.diff added

comment:10 Changed 6 years ago by Adi Roiban

Keywords: review added

Hi,

I have attached a patch for adding FEAT and OPTS ftp commands.

For now, OPTS will always return a "502 Option 'X' not implemented." since the server does not support any specific options. When implementing UTF-8 support 'OPT UTF8' will be added.

FEAT command is implemented only in the FTP protocol and does not rely on the FTPShell. If the FTPShell will start supporting various options, the FEAT command could query the FTPShell for extra features and append them to the list of features.

FEAT command should also be allowed before 'USER/PASS'. I have added PUBLIC_COMMANDS to FTP protocol, and this can be filled will public commands.

I have also added some docstring for some commands that I touched while working at this patch. Please let me know if you want them removed.

While working for UTF-8 support I plan to extend the FEAT and OPTS to query the FTPShell for support info.

For now I used simple string lists for public command and features. The list can be easily appended by various FTP implementations. For me FTP is a static protocol and I don't see that in the near future people will start to heavily extend FTP by adding many new commands and features. Jean-Paul suggested to define interfaces and other helper methods, but I fill this is over-engineering.

Please let me know any changes are required.

Cheers, Adi

Changed 6 years ago by Adi Roiban

Attachment: 4515-add-ftp-feat-2.diff added

Add news file.

comment:11 in reply to:  10 ; Changed 6 years ago by davidsarah

Replying to adiroiban:

FEAT command is implemented only in the FTP protocol and does not rely on the FTPShell. If the FTPShell will start supporting various options, the FEAT command could query the FTPShell for extra features and append them to the list of features.

[...]

While working for UTF-8 support I plan to extend the FEAT and OPTS to query the FTPShell for support info.

Is there any reason to wait for UTF-8 support to do that? In the case of Tahoe-LAFS' FTP server, we already support UTF-8; we just don't have any way to declare that we do. It would be possible to get the FTP instance and add to its FEATURES list, but that seems a bit like relying on implementation details. Maybe I'm just being impatient and you will soon add a feat method or similar to IFTPShell, but may I ask whether that is likely to be in the same release as this patch?

comment:12 Changed 5 years ago by Thijs Triemstra

Owner: itamarst deleted

comment:13 in reply to:  11 Changed 5 years ago by Adi Roiban

Replying to davidsarah:

Replying to adiroiban:

FEAT command is implemented only in the FTP protocol and does not rely on the FTPShell. If the FTPShell will start supporting various options, the FEAT command could query the FTPShell for extra features and append them to the list of features.

[...]

While working for UTF-8 support I plan to extend the FEAT and OPTS to query the FTPShell for support info.

Is there any reason to wait for UTF-8 support to do that? In the case of Tahoe-LAFS' FTP server, we already support UTF-8; we just don't have any way to declare that we do. It would be possible to get the FTP instance and add to its FEATURES list, but that seems a bit like relying on implementation details. Maybe I'm just being impatient and you will soon add a feat method or similar to IFTPShell, but may I ask whether that is likely to be in the same release as this patch?

Hi, Sorry for this late reply. Not sure why I missed this comment.

I don't plan to add IFTPShell.FEATURES in this patch since without UTF-8 support there will be nothing to export. Beside UTF-8 all other features (SIZE, MDTM, ect) are tied to the protocol.FTP implementation.

We can imagine a mechanism for allowing the IFTPShell to handle arbitrary FTP commands, but I think that this will make things unnecessarily complicated.

Cheers

comment:14 Changed 5 years ago by Stephen Thorne

Author: jerub
Branch: branches/add-ftp-feat-4515

(In [35768]) Create branch for ticket #4515, adding ftp FEAT command.

comment:15 Changed 5 years ago by Stephen Thorne

Owner: set to Stephen Thorne
Status: newassigned

comment:16 Changed 5 years ago by Stephen Thorne

Owner: changed from Stephen Thorne to Asheesh Laroia
Status: assignednew

paul, can you please update the docstrings to use doublequotes?

comment:17 Changed 5 years ago by Asheesh Laroia

I'm adding a patch that converts the docstrings to use """ not .

However, I can't find a reference in http://twistedmatrix.com/trac/browser/trunk/doc/core/development/policy/coding-standard.xhtml?format=raw that indicates that docstrings need to use """ not .

Changed 5 years ago by Asheesh Laroia

Adjusting docstrings to style guide

comment:18 Changed 5 years ago by Stephen Thorne

(In [35770]) Apply ftp-feat-uses-double-quotes.patch and other changes by paulproteus

Refs #4515

Changed 5 years ago by Asheesh Laroia

Add docstrings for new tests (and also improve a few other tests per style guide)

comment:19 Changed 5 years ago by Stephen Thorne

(In [35771]) Add docstrings for new tests (and also improve a few other tests per style guide)

ftp-feat-more-test-docs.patch by paulproteus

Refs #4515

comment:20 Changed 5 years ago by Stephen Thorne

Resolution: fixed
Status: newclosed

(In [35772]) Merge add-ftp-feat-4515: Add FEAT and OPTS methods to ftp

Author: adiroiban, paulproteus Reviewer: jerub Fixes: #4515

The OPTS method by default will just say 'unsupported' for any argument. If you implement OPTS you must implement FEAT, so both are added here.

In the future this will allow us to add support for UTF8 on FTP more easily.

comment:21 Changed 15 months ago by hawkowl

Keywords: review removed

[mass edit] Removing review from closed tickets.

Note: See TracTickets for help on using tickets.