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

        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.
        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
        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.
        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?
        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.
        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.
        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.
        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.
        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
        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.
        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?
        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.
        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.
        Hide
        Pier-Luc Caron St-Pierre added a comment -

        The patch.

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

          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