Commons VFS
  1. Commons VFS
  2. VFS-245

FileName interface says that it is immutable, however AbstractFileName.setType(FileType type) flouts this rule

    Details

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

      Description

      FileName interface says that it is immutable, however AbstractFileName.setType(FileType type) flouts this rule.

        Activity

        Hide
        Ralph Goers added a comment -

        Made the changes as described above. While this does not make the FileName immutable (as it should be) it does eliminate the consequences of the FileType changing.

        Show
        Ralph Goers added a comment - Made the changes as described above. While this does not make the FileName immutable (as it should be) it does eliminate the consequences of the FileType changing.
        Hide
        Ralph Goers added a comment -

        I've experimented with various options for this.

        I looked at removing the FileType attribute. Unfortunately, resolveName in DefaultFileSystemManager relies on it to determine whether the FileName is a File vs a Folder so removing it would break that.

        I looked at making the FileType immutable and removing the setType method. This is doable but requires that AbstractFileObject change the setType method to create a new FileName when the type is changed instead of modifying the FileName. As a consequence this just moves the immutability problem since FileObjects are cached by their FileName.

        While the assessment above is correct, I disagree with the outcome. appendURI is provided to allow FileName implementations to include information such as the userid and password into the path. Furthermore, the only thing the FileType is used for is to determine whether to append a '/' to the path. Given this it makes more sense to me to modify equals(), hashcode, and compareTo to use a method that generates a URI that contains the results from appendURI but does not try to append the '/' to the path. This will result in these three methods returning constant values regardless of what the FileType might be.

        Show
        Ralph Goers added a comment - I've experimented with various options for this. I looked at removing the FileType attribute. Unfortunately, resolveName in DefaultFileSystemManager relies on it to determine whether the FileName is a File vs a Folder so removing it would break that. I looked at making the FileType immutable and removing the setType method. This is doable but requires that AbstractFileObject change the setType method to create a new FileName when the type is changed instead of modifying the FileName. As a consequence this just moves the immutability problem since FileObjects are cached by their FileName. While the assessment above is correct, I disagree with the outcome. appendURI is provided to allow FileName implementations to include information such as the userid and password into the path. Furthermore, the only thing the FileType is used for is to determine whether to append a '/' to the path. Given this it makes more sense to me to modify equals(), hashcode, and compareTo to use a method that generates a URI that contains the results from appendURI but does not try to append the '/' to the path. This will result in these three methods returning constant values regardless of what the FileType might be.
        Hide
        Vince Bonfanti added a comment -

        Here's what I've discovered about this problem:

        1) The AbstractFileName equals() and hashcode() methods rely on its getRootUri() and getPath() methods.

        2) The getRootUri() method invokes appendRootUri(), which is an abstract method. Since we can't know what the subclass might do within appendRootUri(), it's probably not a good idea for getRootUri() to be used by equals() or hashcode().

        3) The getPath() method might invoke getUriTrailer() based on the setting of VFS.isUriStyle(). The getUriTrailer() method is the one that invokes getType() to determine whether to add a "/" to the path. Therefore, it's probably not a good idea for getPath() to be used by equals() or hashcode().

        I recommend that equals() and hashcode() be modified to rely only on the "scheme" and "absPath" properties, both of which are Strings and both of which are immutable. A patch file with the proposed change is attached.

        Show
        Vince Bonfanti added a comment - Here's what I've discovered about this problem: 1) The AbstractFileName equals() and hashcode() methods rely on its getRootUri() and getPath() methods. 2) The getRootUri() method invokes appendRootUri(), which is an abstract method. Since we can't know what the subclass might do within appendRootUri(), it's probably not a good idea for getRootUri() to be used by equals() or hashcode(). 3) The getPath() method might invoke getUriTrailer() based on the setting of VFS.isUriStyle(). The getUriTrailer() method is the one that invokes getType() to determine whether to add a "/" to the path. Therefore, it's probably not a good idea for getPath() to be used by equals() or hashcode(). I recommend that equals() and hashcode() be modified to rely only on the "scheme" and "absPath" properties, both of which are Strings and both of which are immutable. A patch file with the proposed change is attached.
        Hide
        Sebb added a comment -

        Since FileType is used in equals() and hashCode() this could cause serious problems.

        Show
        Sebb added a comment - Since FileType is used in equals() and hashCode() this could cause serious problems.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development