FtpServer
  1. FtpServer
  2. FTPSERVER-253

Enhance the Ftplet.afterCommand to provide more information about the result of command execution

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.1.0
    • Component/s: Core, Ftplets
    • Labels:
      None

      Description

      It would be nice to enhance the afterCommand method in the Ftplet to provide additional details about the result of command execution. Currently the afterCommand method of an Ftplet is called back with the following parameters -

      FtpSession, FtpRequest and FtpReply. The FtpReply parameter contains only the reply code and the reply string that was sent. The Ftplets may want to know a little more information on what exactly happened to the command that was executed. For example, the afterCommand for RNTO command might want to know the from file, the to file and if the command was successful or not.

      More information on this can be found at http://www.mail-archive.com/ftpserver-users@mina.apache.org/msg00512.html.

        Activity

        Hide
        Jiri Kuhn added a comment -

        The current API is really too simple. If you want to do an action with an uploaded file (which is quite common, I think), you need its name. But the STOU command provide no way how to pass generated unique file name to a ftplet.

        This issue should be solved soon. It degrades the power of ftplets.

        Show
        Jiri Kuhn added a comment - The current API is really too simple. If you want to do an action with an uploaded file (which is quite common, I think), you need its name. But the STOU command provide no way how to pass generated unique file name to a ftplet. This issue should be solved soon. It degrades the power of ftplets.
        Hide
        Niklas Gustavsson added a comment -

        Agreed. The issue is scheduled for 1.1 and I believe Sai has planned to work on it (if not, I'll pick it up and do it).

        Show
        Niklas Gustavsson added a comment - Agreed. The issue is scheduled for 1.1 and I believe Sai has planned to work on it (if not, I'll pick it up and do it).
        Hide
        Sai Pullabhotla added a comment -

        Okay, I think I'm ready to work on this and need your inputs in finalizing the approach. Below is one of the emails I sent a while ago :

        ----- BEGIN EMAIL -----
        Change the signature of the Command.execute to return the FtpReply
        (this may not be needed, but I think it makes more sense to do it this
        way rather than storing the "lastReply" in the FtpSession. I
        definitely prefer this approach.

        Create subclasses of FtpReply where Ftplets may want to know
        additional information about the result of a command. For example, the
        RNTO command returns a RenameFtpReply would contain the fromFile and
        toFile information.

        I don't think we will have too many subclasses of the DefaultFtpReply,
        but I will see what I can do when I dig into the code.

        One thing I'm still debating on is - if we really should use FtpReply
        objects to provide this additional information or create yet another
        abstract class (call it Result) and call back the Ftplet afterCommand
        method with the Result instead of the FtpReply. This Result would
        contain the FtpReply plus any other information. I prefer this solely
        based on the logical structure/naming of the objects. Both approaches
        would work fine, except the backward compatibility issue if we decide
        to go with the new Result object. To be more specific, the Result
        class would look like -

        public abstract class Result

        { private FtpReply reply; private boolean success; private long startTime; private long endTime; }

        A sub class of Result, RenameResult would look like -

        public abstract class RenameResult extends Result

        { private FtpFile from; private FtpFile to; }

        ----- END EMAIL -----

        I'm leaning towards the second approach of creating a Result object which holds all the information about the execution result, which includes the FtpReply we sent.

        We cannot use the described soultion #1 (extending DefaultFtpReply) as is because we have a subclass of DefaultFtpReply which is the LocalizedFtpReply. So, if we want to go with solution #1 we have to extend the new Reply classes such as RenameFtpReply from LocalizedFtpReply or get rid of the LocalizedFtpReply and make the DefaultFtpReply support both English and Localized messages.

        Please let me know what you guys think and we will go from there.

        Show
        Sai Pullabhotla added a comment - Okay, I think I'm ready to work on this and need your inputs in finalizing the approach. Below is one of the emails I sent a while ago : ----- BEGIN EMAIL ----- Change the signature of the Command.execute to return the FtpReply (this may not be needed, but I think it makes more sense to do it this way rather than storing the "lastReply" in the FtpSession. I definitely prefer this approach. Create subclasses of FtpReply where Ftplets may want to know additional information about the result of a command. For example, the RNTO command returns a RenameFtpReply would contain the fromFile and toFile information. I don't think we will have too many subclasses of the DefaultFtpReply, but I will see what I can do when I dig into the code. One thing I'm still debating on is - if we really should use FtpReply objects to provide this additional information or create yet another abstract class (call it Result) and call back the Ftplet afterCommand method with the Result instead of the FtpReply. This Result would contain the FtpReply plus any other information. I prefer this solely based on the logical structure/naming of the objects. Both approaches would work fine, except the backward compatibility issue if we decide to go with the new Result object. To be more specific, the Result class would look like - public abstract class Result { private FtpReply reply; private boolean success; private long startTime; private long endTime; } A sub class of Result, RenameResult would look like - public abstract class RenameResult extends Result { private FtpFile from; private FtpFile to; } ----- END EMAIL ----- I'm leaning towards the second approach of creating a Result object which holds all the information about the execution result, which includes the FtpReply we sent. We cannot use the described soultion #1 (extending DefaultFtpReply) as is because we have a subclass of DefaultFtpReply which is the LocalizedFtpReply. So, if we want to go with solution #1 we have to extend the new Reply classes such as RenameFtpReply from LocalizedFtpReply or get rid of the LocalizedFtpReply and make the DefaultFtpReply support both English and Localized messages. Please let me know what you guys think and we will go from there.
        Hide
        Niklas Gustavsson added a comment -

        I don't see that we can do the change in the Command interface as that would break our API and force a FtpServer 2.0, which we're not really ready for

        > I'm leaning towards the second approach of creating a Result object which holds all the information about the execution result, which includes the FtpReply we sent.
        > We cannot use the described soultion #1 (extending DefaultFtpReply) as is because we have a subclass of DefaultFtpReply which is the LocalizedFtpReply. So, if we want to go with solution #1 we have to extend the new Reply classes such
        > as RenameFtpReply from LocalizedFtpReply or get rid of the LocalizedFtpReply and make the DefaultFtpReply support both English and Localized messages.

        I still prefer using FtpReply for this. The inheritance hierarchy would all be in the interfaces (extensions from FtpReply) and thus we could implement them all in LocalizedFtpReply if we would like to (not saying it's a good idea .

        /niklas

        Show
        Niklas Gustavsson added a comment - I don't see that we can do the change in the Command interface as that would break our API and force a FtpServer 2.0, which we're not really ready for > I'm leaning towards the second approach of creating a Result object which holds all the information about the execution result, which includes the FtpReply we sent. > We cannot use the described soultion #1 (extending DefaultFtpReply) as is because we have a subclass of DefaultFtpReply which is the LocalizedFtpReply. So, if we want to go with solution #1 we have to extend the new Reply classes such > as RenameFtpReply from LocalizedFtpReply or get rid of the LocalizedFtpReply and make the DefaultFtpReply support both English and Localized messages. I still prefer using FtpReply for this. The inheritance hierarchy would all be in the interfaces (extensions from FtpReply) and thus we could implement them all in LocalizedFtpReply if we would like to (not saying it's a good idea . /niklas
        Hide
        Sai Pullabhotla added a comment -

        Can't wait for 2.0 . I think I figured out what needs to be done for the
        most part. I will have a patch out for 1.1 this weekend.

        Regards,

        Sai Pullabhotla
        Phone: (402) 408-5753
        Fax: (402) 408-6861
        www.jMethods.com

        On Sun, Mar 29, 2009 at 1:38 PM, Niklas Gustavsson (JIRA)

        Show
        Sai Pullabhotla added a comment - Can't wait for 2.0 . I think I figured out what needs to be done for the most part. I will have a patch out for 1.1 this weekend. Regards, Sai Pullabhotla Phone: (402) 408-5753 Fax: (402) 408-6861 www.jMethods.com On Sun, Mar 29, 2009 at 1:38 PM, Niklas Gustavsson (JIRA)
        Hide
        Sai Pullabhotla added a comment -

        Attached is the first draft of the patch. There is probably more to do regarding this, but I first wanted you to review it to ensure we are on the right track.

        Below is what I've done so far:

        Added two new methods in the FtpReply interface - one that returns the time the reply was sent and another that tells if the reply is a positive reply

        Implemented the above two methods int he DefaultFtpReply.

        Added a new method in the FtpRequest interface that returns the time at which the request was received.

        Implemented the above method in the DefaultFtpRequest.

        The above 4 items can be used by the Ftplet implementations to determine if the request was sucessfully processed or not and the total time it took to process the request.

        Added a new method named getPhysicalFile in the FtpFile interface. This method returns an Object.

        The above method is already implemented in the NativeFtpFile.

        The above two are needed as most often the Ftplets want to know the physical location to do any further processing on the file or just to log the physical file.

        Created a new Interface org.apache.ftpserver.ftplet.DataTransferFtpReply

        Created a new Interface org.apache.ftpserver.ftplet.RenameFtpReply.

        Created org.apache.ftpserver.impl.LocalizedDataTransferFtpReply which exteds LocalizedFtpReply and implements DataTransferFtpReply

        Created org.apache.ftpserver.impl.LocalizedRenameFtpReply which extends LocalizedFtpReply and implements RenameFtpReply

        Updated the APPE, LIST, RETR, STOR, STOU commands to send LocalizedDataTransferFtpReply. I still have to update the MLSD command I believe.

        Updated the RNTO command to send LocalizedRenameFtpReply.

        Created a new utility class named FtpReplyTranslator where all methods that expand variables and translate messages are kept. These methods are used by the Localized*FtpReply classes.

        Below is what needs to be done if you all like what has been done above:

        Create one more extension of FtpReply (may be call it FileActionFtpReply) which is sent on all commands that act on a file (no data transfer is involved). These commands include MKD, RMD, DELE, CWD etc. Update these commands to send the new type of FtpReply.

        Other observations:

        While working on the above I noticed a couple of things that may need to be changed/fixed:

        1. All data transfer commands (such as LIST, RETR etc) have a block of code that checks to see if a PASV/PORT command was issued. This code should probably go into the AbstractCommand instead of having the very same code in 7 different command implementations. Also, the reply sent when this check fails may need to be translated.

        2. getUniqueFile in STOU command may need to be synchronized. Even if we synchronize it, there is no gurantee that two different FTP sessions get the same file as we do not create the file in this method. Perhaps, we should use File.createTempFile() method to do this.

        Hope it all makes sense. Please let me know what you think or need any clarification.

        Regards,
        Sai Pullabhotla

        Show
        Sai Pullabhotla added a comment - Attached is the first draft of the patch. There is probably more to do regarding this, but I first wanted you to review it to ensure we are on the right track. Below is what I've done so far: Added two new methods in the FtpReply interface - one that returns the time the reply was sent and another that tells if the reply is a positive reply Implemented the above two methods int he DefaultFtpReply. Added a new method in the FtpRequest interface that returns the time at which the request was received. Implemented the above method in the DefaultFtpRequest. The above 4 items can be used by the Ftplet implementations to determine if the request was sucessfully processed or not and the total time it took to process the request. Added a new method named getPhysicalFile in the FtpFile interface. This method returns an Object. The above method is already implemented in the NativeFtpFile. The above two are needed as most often the Ftplets want to know the physical location to do any further processing on the file or just to log the physical file. Created a new Interface org.apache.ftpserver.ftplet.DataTransferFtpReply Created a new Interface org.apache.ftpserver.ftplet.RenameFtpReply. Created org.apache.ftpserver.impl.LocalizedDataTransferFtpReply which exteds LocalizedFtpReply and implements DataTransferFtpReply Created org.apache.ftpserver.impl.LocalizedRenameFtpReply which extends LocalizedFtpReply and implements RenameFtpReply Updated the APPE, LIST, RETR, STOR, STOU commands to send LocalizedDataTransferFtpReply. I still have to update the MLSD command I believe. Updated the RNTO command to send LocalizedRenameFtpReply. Created a new utility class named FtpReplyTranslator where all methods that expand variables and translate messages are kept. These methods are used by the Localized*FtpReply classes. Below is what needs to be done if you all like what has been done above: Create one more extension of FtpReply (may be call it FileActionFtpReply) which is sent on all commands that act on a file (no data transfer is involved). These commands include MKD, RMD, DELE, CWD etc. Update these commands to send the new type of FtpReply. Other observations: While working on the above I noticed a couple of things that may need to be changed/fixed: 1. All data transfer commands (such as LIST, RETR etc) have a block of code that checks to see if a PASV/PORT command was issued. This code should probably go into the AbstractCommand instead of having the very same code in 7 different command implementations. Also, the reply sent when this check fails may need to be translated. 2. getUniqueFile in STOU command may need to be synchronized. Even if we synchronize it, there is no gurantee that two different FTP sessions get the same file as we do not create the file in this method. Perhaps, we should use File.createTempFile() method to do this. Hope it all makes sense. Please let me know what you think or need any clarification. Regards, Sai Pullabhotla
        Hide
        David Latorre added a comment -

        Wow Sai

        Are you subscribed to the developers mailing list? So we can move the discussion there.

        I hadn't thought much about these changes myself but your work looks pretty good. The only thing I don't quite get is why we would use FTPFile.getPhysicalFile ... Since it returns an Object, the coder developing the FTPLet should know which 'PhysicalFile' object (s)he is using. This would mean in most situations that they know which FTPFile implementation they're using and hence, they could use casting to their appropriate type. And I guess it's possible to have an implementation of FTPFile that doesn't use a "PhysicalFile" object ... So what's your use case for this addition? I'm so tired i can't clearly think...

        Another problem we have is that it is becoming quite difficult to develop pluggable components (be it an UserManager ,a Command or a ftplet) with only the API documentation. How would a user know which types of FTPReply he should use if overwriting a command? - I don't have a solution for this at all. It's just something we could think about.

        Agree we should add a bug report for getUniqueFile(), using createTempFile would be simpler but it is probably better to generate an unique filename and check for permission before creating the actual file (so all these should be under a static lock) ; although of course checking only parent directory permissions we are good to go right now.

        Show
        David Latorre added a comment - Wow Sai Are you subscribed to the developers mailing list? So we can move the discussion there. I hadn't thought much about these changes myself but your work looks pretty good. The only thing I don't quite get is why we would use FTPFile.getPhysicalFile ... Since it returns an Object, the coder developing the FTPLet should know which 'PhysicalFile' object (s)he is using. This would mean in most situations that they know which FTPFile implementation they're using and hence, they could use casting to their appropriate type. And I guess it's possible to have an implementation of FTPFile that doesn't use a "PhysicalFile" object ... So what's your use case for this addition? I'm so tired i can't clearly think... Another problem we have is that it is becoming quite difficult to develop pluggable components (be it an UserManager ,a Command or a ftplet) with only the API documentation. How would a user know which types of FTPReply he should use if overwriting a command? - I don't have a solution for this at all. It's just something we could think about. Agree we should add a bug report for getUniqueFile(), using createTempFile would be simpler but it is probably better to generate an unique filename and check for permission before creating the actual file (so all these should be under a static lock) ; although of course checking only parent directory permissions we are good to go right now.

          People

          • Assignee:
            Unassigned
            Reporter:
            Sai Pullabhotla
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development