Commons IO
  1. Commons IO
  2. IO-291

Add new function FileUtils.directoryContains

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.2
    • Component/s: Utilities
    • Labels:

      Description

      I added a function that determines whether the specified leaf is contains by the specified composite.

      1. io-291-v5.patch
        10 kB
        Pier-Luc Caron St-Pierre
      2. io-291-simple.diff
        8 kB
        Gary Gregory

        Activity

        Gary Gregory made changes -
        Fix Version/s 2.2 [ 12318448 ]
        Gary Gregory made changes -
        Status Reopened [ 4 ] Closed [ 6 ]
        Resolution Fixed [ 1 ]
        Hide
        Gary Gregory added a comment -

        Version 2.2 has been released and addresses this issue.

        Show
        Gary Gregory added a comment - Version 2.2 has been released and addresses this issue.
        Sebb made changes -
        Fix Version/s 2.2 [ 12318448 ]
        Gary Gregory committed 1234902 (3 files)
        Reviews: none

        Refactor new directortContains method for https://issues.apache.org/jira/browse/IO-291. Change behavior: an unrealized child File is NOT contained in a directory in FileUtils. If you want to test with file names, use FilenameUtils#directoryContains(String, String).

        Hide
        Niall Pemberton added a comment - - edited

        Also the meat of the method seems more suited to FilenameUtils:

        // Canonicalize paths (normalizes relative paths)
        String canonicalParent = directory.getCanonicalPath();
        String canonicalChild = child.getCanonicalPath();
        
        if (IOCase.SYSTEM.checkEquals(canonicalParent, canonicalChild)) {
            return false;
        }
        
        return IOCase.SYSTEM.checkStartsWith(canonicalChild, canonicalParent);
        
        Show
        Niall Pemberton added a comment - - edited Also the meat of the method seems more suited to FilenameUtils: // Canonicalize paths (normalizes relative paths) String canonicalParent = directory.getCanonicalPath(); String canonicalChild = child.getCanonicalPath(); if (IOCase.SYSTEM.checkEquals(canonicalParent, canonicalChild)) { return false ; } return IOCase.SYSTEM.checkStartsWith(canonicalChild, canonicalParent);
        Hide
        Niall Pemberton added a comment -

        Seems very odd for the new method to return true if the file doesn't exist - I added a test case showing this:

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

        Show
        Niall Pemberton added a comment - Seems very odd for the new method to return true if the file doesn't exist - I added a test case showing this: http://svn.apache.org/viewvc?view=revision&revision=1228554
        Niall Pemberton committed 1228554 (1 file)
        Reviews: none

        IO-291 Add test case which shows FileUtils.directoryContains() returns true for a file which doesn't exist

        Hide
        Gary Gregory added a comment -

        Committed revision 1201713. This is a simple implementation that seems to make the most sense to me. Please discuss.

        Show
        Gary Gregory added a comment - Committed revision 1201713. This is a simple implementation that seems to make the most sense to me. Please discuss.
        Henri Yandell made changes -
        Fix Version/s 2.2 [ 12318448 ]
        Gary Gregory made changes -
        Summary Add new function FileUtils.isContained Add new function FileUtils.directoryContains
        Gary Gregory made changes -
        Fix Version/s 2.1 [ 12316027 ]
        Hide
        Pier-Luc Caron St-Pierre added a comment - - edited

        I built on top of your patch. So if you apply my patch (io-291-v5.patch) from scratch, you should have our combined changes.

        • I have implemented the directoryContains(final File directory, final File child)
        • I have added some test case for the newly implemented function.
        • I have added some javadoc, renamed some parameters
        • I have corrected the code style of the test to match the original style.

        We have some issues to address :

        • Actually the test case fail on testSameFile() because the case is tester on a file. It fail the rule that a directory cannot be a file
        • Does a directory contain itself? I do not have strong opinions about that.
        • Does a file contain itself? I do not have strong opinions about that.
        • In the scope of the method "directoryContains(final File directory, final File child)", what will happens if the files is an empty collection?
        Show
        Pier-Luc Caron St-Pierre added a comment - - edited I built on top of your patch. So if you apply my patch (io-291-v5.patch) from scratch, you should have our combined changes. I have implemented the directoryContains(final File directory, final File child) I have added some test case for the newly implemented function. I have added some javadoc, renamed some parameters I have corrected the code style of the test to match the original style. We have some issues to address : Actually the test case fail on testSameFile() because the case is tester on a file. It fail the rule that a directory cannot be a file Does a directory contain itself? I do not have strong opinions about that. Does a file contain itself? I do not have strong opinions about that. In the scope of the method "directoryContains(final File directory, final File child)", what will happens if the files is an empty collection?
        Pier-Luc Caron St-Pierre made changes -
        Attachment io-291-v5.patch [ 12502866 ]
        Pier-Luc Caron St-Pierre made changes -
        Attachment io-291-v4.patch [ 12502692 ]
        Pier-Luc Caron St-Pierre made changes -
        Attachment FileUtils.isContained.patch [ 12502590 ]
        Gary Gregory made changes -
        Attachment io-291-simple.diff [ 12502764 ]
        Hide
        Gary Gregory added a comment -

        Let's try again:

        Attached is a much simpler implementation that works with:

        Unrealized File objects
        No recursion.
        Correct Javadoc
        No tabs.

        Show
        Gary Gregory added a comment - Let's try again: Attached is a much simpler implementation that works with: Unrealized File objects No recursion. Correct Javadoc No tabs.
        Gary Gregory made changes -
        Attachment io-291-simple.diff [ 12502673 ]
        Gary Gregory made changes -
        Attachment io-291.diff [ 12502597 ]
        Hide
        Sebb added a comment -

        No tabs.

        The file io-291-simple.diff still contains some tabs.

        By the way, when uploading a replacement patch, please delete any file(s) that are totally superseded by the new patch.
        Unfortunately JIRA does not have any way to mark patches as obsolete.

        Show
        Sebb added a comment - No tabs. The file io-291-simple.diff still contains some tabs. By the way, when uploading a replacement patch, please delete any file(s) that are totally superseded by the new patch. Unfortunately JIRA does not have any way to mark patches as obsolete.
        Pier-Luc Caron St-Pierre made changes -
        Attachment io-291-v4.patch [ 12502692 ]
        Hide
        Pier-Luc Caron St-Pierre added a comment - - edited

        Thanks for you input.

        The initial use case is that I am exposing all the content of a folder to my user by a web service. I trust the lib to sanitize correctly the parameters, but I am afraid that my function will be used in other context.

        • I have make some coding style fix in the io-291-simple.patch.
        • I have removed the author tag.
        Show
        Pier-Luc Caron St-Pierre added a comment - - edited Thanks for you input. The initial use case is that I am exposing all the content of a folder to my user by a web service. I trust the lib to sanitize correctly the parameters, but I am afraid that my function will be used in other context. I have make some coding style fix in the io-291-simple.patch. I have removed the author tag.
        Gary Gregory made changes -
        Attachment io-291-simple.diff [ 12502673 ]
        Hide
        Gary Gregory added a comment -

        Attached is a much simpler implementation that works with:

        • Unrealized File objects
        • No recursion.
        • Correct Javadoc
        • No tabs.
        Show
        Gary Gregory added a comment - Attached is a much simpler implementation that works with: Unrealized File objects No recursion. Correct Javadoc No tabs.
        Gary Gregory made changes -
        Attachment io-291-simple.diff [ 12502672 ]
        Gary Gregory made changes -
        Attachment io-291-simple.diff [ 12502672 ]
        Hide
        Gary Gregory added a comment -

        Attached is a much simpler implementation that works with:

        • Unrealized File objects
        • No recursion.
        • Correct Javadoc
        Show
        Gary Gregory added a comment - Attached is a much simpler implementation that works with: Unrealized File objects No recursion. Correct Javadoc
        Gary Gregory made changes -
        Attachment io-291-simple.diff [ 12502671 ]
        Gary Gregory made changes -
        Attachment io-291-simple.diff [ 12502671 ]
        Hide
        Gary Gregory added a comment -

        Attached is a much simpler implementation that works with:

        • Unrealized File objects
        • No recursion.
        Show
        Gary Gregory added a comment - Attached is a much simpler implementation that works with: Unrealized File objects No recursion.
        Hide
        Sebb added a comment -

        What is the original use case?
        Maybe knowing that would help inform decisions on whether files and/or directories contain themselves and whether parameters need to exist or not.

        Note that the behaviour of getCanonicalFile() may depend on whether or not the file exists, from the Javadoc:

        The canonical form of the pathname of a nonexistent file or directory may be different from the canonical form of the same pathname after the file or directory is created. Similarly, the canonical form of the pathname of an existing file or directory may be different from the canonical form of the same pathname after the file or directory is deleted.

        I don't like the recursive implementation; also it should not be necessary to call getCanonicalFile() multiple times.
        It's also unsafe to call it multiple times as the representation may potentially change because of the above.

        ==
        The io-291.diff patch contains tabs, and is an Eclipse workspace-relative patch so is difficult for anyone else to apply.

        Both patches contain @author tags, which we discourage.

        Show
        Sebb added a comment - What is the original use case? Maybe knowing that would help inform decisions on whether files and/or directories contain themselves and whether parameters need to exist or not. Note that the behaviour of getCanonicalFile() may depend on whether or not the file exists, from the Javadoc: The canonical form of the pathname of a nonexistent file or directory may be different from the canonical form of the same pathname after the file or directory is created. Similarly, the canonical form of the pathname of an existing file or directory may be different from the canonical form of the same pathname after the file or directory is deleted. I don't like the recursive implementation; also it should not be necessary to call getCanonicalFile() multiple times. It's also unsafe to call it multiple times as the representation may potentially change because of the above. == The io-291.diff patch contains tabs, and is an Eclipse workspace-relative patch so is difficult for anyone else to apply. Both patches contain @author tags, which we discourage.
        Gary Gregory made changes -
        Attachment io-291.diff [ 12502597 ]
        Hide
        Gary Gregory added a comment - - edited

        Attaching my version of the patch with discussed changes. What is not clear to me is if this is the best choice:

            public void testUnrealizedContainment() throws IOException {
        	final File dir = new File("DOESNOTEXIST");
        	final File file = new File(dir, "DOESNOTEXIST2");
                assertFalse(dir.exists());	
                assertFalse(file.exists());	
                assertFalse(FileUtils.contains(dir, file));
            }
        

        as opposed to:

        assertTrue(FileUtils.contains(dir, file));
        

        The relationship in b/w parent and child is valid but unrealized on disk. Shouldn't contain value the relationship as valid even if unrealized? I am inclined to say yes.

        Thoughts?

        Show
        Gary Gregory added a comment - - edited Attaching my version of the patch with discussed changes. What is not clear to me is if this is the best choice: public void testUnrealizedContainment() throws IOException { final File dir = new File( "DOESNOTEXIST" ); final File file = new File(dir, "DOESNOTEXIST2" ); assertFalse(dir.exists()); assertFalse(file.exists()); assertFalse(FileUtils.contains(dir, file)); } as opposed to: assertTrue(FileUtils.contains(dir, file)); The relationship in b/w parent and child is valid but unrealized on disk. Shouldn't contain value the relationship as valid even if unrealized? I am inclined to say yes. Thoughts?
        Gary Gregory made changes -
        Assignee Gary D. Gregory [ garydgregory ]
        Hide
        Gary Gregory added a comment -

        I've applied the patch to my working copy and am working on small changes...

        Show
        Gary Gregory added a comment - I've applied the patch to my working copy and am working on small changes...
        Hide
        Gary Gregory added a comment -

        I think a containsAll(Collection<Files>) method should also be in this patch.

        Show
        Gary Gregory added a comment - I think a containsAll(Collection<Files>) method should also be in this patch.
        Hide
        Gary Gregory added a comment -

        I do not think having candidate in the name helps.

        Show
        Gary Gregory added a comment - I do not think having candidate in the name helps.
        Hide
        Pier-Luc Caron St-Pierre added a comment -

        Does the prefix 'candidate' for the parameters names add some value or should I remove it?

        Show
        Pier-Luc Caron St-Pierre added a comment - Does the prefix 'candidate' for the parameters names add some value or should I remove it?
        Hide
        Gary Gregory added a comment -

        For these kinds of methods I prefer, "contains(parent, child)" where the first argument is the receiver of the verb and the following arguments are the actual arguments. Which would translate to "parent.contains(child)". The is like the Collection.contains method.

        The next issue is whether "contains(file, file)" should return true or false. Does a file contain itself? Does a directory contains itself?

        If you think of a directory as a collection of files and directories, it does not, unless "." is listed.

        Show
        Gary Gregory added a comment - For these kinds of methods I prefer, "contains(parent, child)" where the first argument is the receiver of the verb and the following arguments are the actual arguments. Which would translate to "parent.contains(child)". The is like the Collection.contains method. The next issue is whether "contains(file, file)" should return true or false. Does a file contain itself? Does a directory contains itself? If you think of a directory as a collection of files and directories, it does not, unless "." is listed.
        Sebb made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Sebb added a comment -

        A bug needs to be left open until it has been completed; provision of a patch is only one stage in the process.

        The patch may be rejected, or may require amendment.

        Show
        Sebb added a comment - A bug needs to be left open until it has been completed; provision of a patch is only one stage in the process. The patch may be rejected, or may require amendment.
        Sebb made changes -
        Labels newbie patch patch
        Pier-Luc Caron St-Pierre made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Resolved [ 5 ]
        Pier-Luc Caron St-Pierre made changes -
        Field Original Value New Value
        Attachment FileUtils.isContained.patch [ 12502590 ]
        Hide
        Pier-Luc Caron St-Pierre added a comment -

        The patch.

        Show
        Pier-Luc Caron St-Pierre added a comment - The patch.
        Pier-Luc Caron St-Pierre created issue -

          People

          • Assignee:
            Gary Gregory
            Reporter:
            Pier-Luc Caron St-Pierre
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development