Commons IO
  1. Commons IO
  2. IO-215

FileUtils copy methods swallow date modification failures

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0
    • Component/s: Utilities
    • Labels:
      None

      Description

      FileUtils.doCopyDirectory(..) and .FileUtils.doCopyFile(..) both call the setLastModified() method, but fail to check if it succeeded or not.

      Surely if the caller has asked for the date to be preserved, failure to do so should be reported somehow?

      1. IO-215-copy-option-v5.patch
        20 kB
        Niall Pemberton
      2. IO-215-copy-option-v3.patch
        15 kB
        Niall Pemberton

        Activity

        Hide
        Niall Pemberton added a comment -

        I don't see any issue with the current behaviour and am not sure what you're thinking of in terms of "reporting" this - I guess you mean throw an exception - but that would seem over the top for the behaviour of a minor option.

        Show
        Niall Pemberton added a comment - I don't see any issue with the current behaviour and am not sure what you're thinking of in terms of "reporting" this - I guess you mean throw an exception - but that would seem over the top for the behaviour of a minor option.
        Hide
        Sebb added a comment -

        The user is able to specify whether or not to preserve file dates, and I think that failure to do so breaks the API contract - therefore an Exception should be thrown.

        There are some public methods which don't offer the option, but set it true internally.
        These should probably retain the current behaviour.

        Show
        Sebb added a comment - The user is able to specify whether or not to preserve file dates, and I think that failure to do so breaks the API contract - therefore an Exception should be thrown. There are some public methods which don't offer the option, but set it true internally. These should probably retain the current behaviour.
        Hide
        Niall Pemberton added a comment -

        I've used this in the past and what was most important was that the files were copied - it was preferable that the last modified date was preserved, but a small loss of historical info was not critical.

        I accept that your proposal is a valid alternative way of working - but since these methods have worked this way since at least Commons IO 1.1 then I'm against changing the behaviour for current implementations.

        I have improved the documentation of the current methods regarding their behaviour wrt preserving the file dates:

        http://svn.apache.org/viewvc?view=revision&revision=995431

        Show
        Niall Pemberton added a comment - I've used this in the past and what was most important was that the files were copied - it was preferable that the last modified date was preserved, but a small loss of historical info was not critical. I accept that your proposal is a valid alternative way of working - but since these methods have worked this way since at least Commons IO 1.1 then I'm against changing the behaviour for current implementations. I have improved the documentation of the current methods regarding their behaviour wrt preserving the file dates: http://svn.apache.org/viewvc?view=revision&revision=995431
        Hide
        Sebb added a comment -

        OK, point taken.

        How about adding new copyFileXX() and copyDirectoryXX() methods that do report failures?

        I don't think there's any point in adding a boolean reportError parameter as it would only apply if preserveDates is true, so the methods would need new names.
        Any suggestions?

        It's a pity that changing the return from void to boolean is not binary compatible, otherwise that would be an easy solution.
        Maybe consider doing that for a major version update?

        However the private internal copy methods could be changed to return true/false without affecting binary compatibility of the public API.

        Show
        Sebb added a comment - OK, point taken. How about adding new copyFileXX() and copyDirectoryXX() methods that do report failures? I don't think there's any point in adding a boolean reportError parameter as it would only apply if preserveDates is true, so the methods would need new names. Any suggestions? It's a pity that changing the return from void to boolean is not binary compatible, otherwise that would be an easy solution. Maybe consider doing that for a major version update? However the private internal copy methods could be changed to return true/false without affecting binary compatibility of the public API.
        Hide
        Niall Pemberton added a comment -

        I couldn't think of sensible names for new methods - for example copyFilePreserve() would probably be confusing because copyFile(File, File) does "preserve" and it doesn't really reflect that an exception will be thrown if not preserved - which is the difference with copyFile(). Something like copyFileThrowExceptionIfDatesNotPreserved() isn't very pleasant.

        On the "returning boolean" suggestion - it seems slightly unnatural/odd to return a boolean from a copy method that means "dates preserved or not" - the natural meaning in my mind would be to indicate whether a file was copied or not - for example:

        if (FileUtils.copyFile(source, dest)) {
           // ?means dates preserved, not file copied?
        }
        

        Another idea would be to add an enum for the three types of behaviour:

        • Don't Preserve Dates
        • Preserve Dates
        • Preserve Dates and throw exception
        Show
        Niall Pemberton added a comment - I couldn't think of sensible names for new methods - for example copyFilePreserve() would probably be confusing because copyFile(File, File) does "preserve" and it doesn't really reflect that an exception will be thrown if not preserved - which is the difference with copyFile(). Something like copyFileThrowExceptionIfDatesNotPreserved() isn't very pleasant. On the "returning boolean" suggestion - it seems slightly unnatural/odd to return a boolean from a copy method that means "dates preserved or not" - the natural meaning in my mind would be to indicate whether a file was copied or not - for example: if (FileUtils.copyFile(source, dest)) { // ?means dates preserved, not file copied? } Another idea would be to add an enum for the three types of behaviour: Don't Preserve Dates Preserve Dates Preserve Dates and throw exception
        Hide
        Niall Pemberton added a comment -

        Slight error in the previous patch - adding v2

        Show
        Niall Pemberton added a comment - Slight error in the previous patch - adding v2
        Hide
        Sebb added a comment -

        I like the enum option. It's also backward compatible AFAICS.

        BTW, the Javadoc in V2 uses

        @param preserveFileDate ...
        rather than
        @param option ..

        in a couple of places.

        I also don't like if (xxx() == false) - I think that should be if (!xxx()).

        Show
        Sebb added a comment - I like the enum option. It's also backward compatible AFAICS. BTW, the Javadoc in V2 uses @param preserveFileDate ... rather than @param option .. in a couple of places. I also don't like if (xxx() == false) - I think that should be if (!xxx()).
        Hide
        Niall Pemberton added a comment -

        Attaching IO-215-copy-option-v3.patch that fixes the javadoc errors and adds FileUtils.copyFileToDirectory(File, File, CopyOption) method.

        I don't like if (!xxx()) - I think if (xxx() == false) is better. I guess we disagree on this style.

        Currently as this stands the copyDirectory() implementation when called with CopyOption.PreserveDatesThrowError will throw an error when the first File.setLastModified(long) operation fails. This could leave the copy operation in an inconsistent state with the directory only partially copied. IMO I think it would be better to complete the copy operation and throw an exception at the end. WDYT?

        Show
        Niall Pemberton added a comment - Attaching IO-215 -copy-option-v3.patch that fixes the javadoc errors and adds FileUtils.copyFileToDirectory(File, File, CopyOption) method. I don't like if (!xxx()) - I think if (xxx() == false) is better. I guess we disagree on this style. Currently as this stands the copyDirectory() implementation when called with CopyOption.PreserveDatesThrowError will throw an error when the first File.setLastModified(long) operation fails. This could leave the copy operation in an inconsistent state with the directory only partially copied. IMO I think it would be better to complete the copy operation and throw an exception at the end. WDYT?
        Hide
        Sebb added a comment -

        Agreed that failing part-way through a dir copy is not ideal.

        If the private file and dir methods were changed to return false if any setLastModified fails, and true otherwise, then it should be easy to throw the appropriate error at the end.
        Or better yet, the private doCopyDirectory method could return the File that failed, otherwise null. This would allow the Exception to identify the file which caused the first failure.

        Show
        Sebb added a comment - Agreed that failing part-way through a dir copy is not ideal. If the private file and dir methods were changed to return false if any setLastModified fails, and true otherwise, then it should be easy to throw the appropriate error at the end. Or better yet, the private doCopyDirectory method could return the File that failed, otherwise null. This would allow the Exception to identify the file which caused the first failure.
        Hide
        Niall Pemberton added a comment -

        Attaching IO-215-copy-option-v4.patch - I did that, except that I used a count of the number of dates not preserved

        Show
        Niall Pemberton added a comment - Attaching IO-215 -copy-option-v4.patch - I did that, except that I used a count of the number of dates not preserved
        Hide
        Sebb added a comment - - edited

        OK, I wondered about maintaining a count.
        Arguably more useful than the details of the first failed file, but would be nice to have the total count (and perhaps the first file) as well.

        But that can be tweaked later if necessary without changing the API.

        Show
        Sebb added a comment - - edited OK, I wondered about maintaining a count. Arguably more useful than the details of the first failed file, but would be nice to have the total count (and perhaps the first file) as well. But that can be tweaked later if necessary without changing the API.
        Hide
        Niall Pemberton added a comment -

        Attaching IO-215-copy-option-v5.patch - simplified patch (test case)

        Having done this work, I'm still wondering whether its really required. Did you have an actual need for this - or was it just from looking at the code? If its the latter and no-one else has ever raised this, then its probably overcomplicating the API for something that no-one needs.

        Show
        Niall Pemberton added a comment - Attaching IO-215 -copy-option-v5.patch - simplified patch (test case) Having done this work, I'm still wondering whether its really required. Did you have an actual need for this - or was it just from looking at the code? If its the latter and no-one else has ever raised this, then its probably overcomplicating the API for something that no-one needs.
        Hide
        Niall Pemberton added a comment -

        I think we should just leave this at the documentation change.

        Closing as FIXED

        Show
        Niall Pemberton added a comment - I think we should just leave this at the documentation change. Closing as FIXED

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development