FtpServer
  1. FtpServer
  2. FTPSERVER-375

[FindBugs] NativeFtpFile implements equals(), but does not - hashCode()

    Details

    • Type: Wish Wish
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.0.6, 1.1.0
    • Component/s: Core
    • Labels:

      Description

      In fact, i can't find why it need to implement equals() in first place. If for any map - then hashCode() will also be required. For example, this.file.getCanonicalFile().hashCode()

      Anyway, could we please have a comment why do we need equals() method?

        Activity

        Hide
        Sebb added a comment -

        file.getCanonicalPath().hashCode() would do just as well here and in equals(), and avoids creating the File object.

        However, both of these methods may involve file access, so may be slow.

        Also, the canonical name may depend on whether the file exists or not - see the Javadoc [1]

        Seems risky to me to use a base for the hashcode which may vary.

        As the originator wrote - does the class need an equals() method?

        [1] http://download.oracle.com/javase/6/docs/api/java/io/File.html#getCanonicalPath()

        Show
        Sebb added a comment - file.getCanonicalPath().hashCode() would do just as well here and in equals(), and avoids creating the File object. However, both of these methods may involve file access, so may be slow. Also, the canonical name may depend on whether the file exists or not - see the Javadoc [1] Seems risky to me to use a base for the hashcode which may vary. As the originator wrote - does the class need an equals() method? [1] http://download.oracle.com/javase/6/docs/api/java/io/File.html#getCanonicalPath( )
        Hide
        Niklas Gustavsson added a comment -

        Yes, we do use equals() so that's required to work as before. Not so worried about speed, as the equals (currently at least) check goes hand in hand with a lot of other I/O.

        Agreed that the hashCode() implementation can be simplified, will fix.

        On the case that the canonical file might change, that seems consistent with that equals() will also change in this case.

        All of this said, we do not currently use FtpFile in a hash map as far as I know, so it's a bit theoretical.

        Show
        Niklas Gustavsson added a comment - Yes, we do use equals() so that's required to work as before. Not so worried about speed, as the equals (currently at least) check goes hand in hand with a lot of other I/O. Agreed that the hashCode() implementation can be simplified, will fix. On the case that the canonical file might change, that seems consistent with that equals() will also change in this case. All of this said, we do not currently use FtpFile in a hash map as far as I know, so it's a bit theoretical.
        Hide
        Sebb added a comment -

        However, equals() and hashCode() are not calculated simulataneously, so if the external conditions change between calling the two methods, they may disagree.

        Further, a hashCode should not change if the object does not change (and should not change at all if used in some kinds of map).

        It would be safer to fetch the canonical path once, and base the methods on that.

        This would need to be stored in a volatile field and made package-protected (or better with a package-protected getter) for access by equals().

        Should use getCanonicalPath() - not File() - which is a String, so is immutable.

        But again - is the equals method needed?
        And if so, what does it mean to be equal to another instance?
        This affects how the methods need to be coded.

        Also I agree that the equals method needs to be documented if it is kept.

        Show
        Sebb added a comment - However, equals() and hashCode() are not calculated simulataneously, so if the external conditions change between calling the two methods, they may disagree. Further, a hashCode should not change if the object does not change (and should not change at all if used in some kinds of map). It would be safer to fetch the canonical path once, and base the methods on that. This would need to be stored in a volatile field and made package-protected (or better with a package-protected getter) for access by equals(). Should use getCanonicalPath() - not File() - which is a String, so is immutable. But again - is the equals method needed? And if so, what does it mean to be equal to another instance? This affects how the methods need to be coded. Also I agree that the equals method needs to be documented if it is kept.
        Hide
        Niklas Gustavsson added a comment -

        > However, equals() and hashCode() are not calculated simulataneously, so if the external conditions change between calling the two methods,
        > they may disagree.
        >
        > Further, a hashCode should not change if the object does not change (and should not change at all if used in some kinds of map).
        >
        > It would be safer to fetch the canonical path once, and base the methods on that.

        I still think this discussion is getting a bit theoretical for this particular case, but I'll bite. As far as I know, the hashCode() contract says that it might change, if the result of equals changes. Since, in some file systems, the canonical path might change when the file is changed (created/deleted), both equals and hashCode might the change. From our usage, this seems reasonable. From the Javadoc: "Whenever it is invoked on the same object more than once during an execution of a Java application, the hashCode method must consistently return the same integer, provided no information used in equals comparisons on the object is modified"

        And again, yes, the equals method is needed. It is for example used in RMD to ensure that we do not delete the cwd.

        I would be happy to add documentation to specify the contract in FtpFile.equals(), feel free to open an issue for this.

        Show
        Niklas Gustavsson added a comment - > However, equals() and hashCode() are not calculated simulataneously, so if the external conditions change between calling the two methods, > they may disagree. > > Further, a hashCode should not change if the object does not change (and should not change at all if used in some kinds of map). > > It would be safer to fetch the canonical path once, and base the methods on that. I still think this discussion is getting a bit theoretical for this particular case, but I'll bite. As far as I know, the hashCode() contract says that it might change, if the result of equals changes. Since, in some file systems, the canonical path might change when the file is changed (created/deleted), both equals and hashCode might the change. From our usage, this seems reasonable. From the Javadoc: "Whenever it is invoked on the same object more than once during an execution of a Java application, the hashCode method must consistently return the same integer, provided no information used in equals comparisons on the object is modified" And again, yes, the equals method is needed. It is for example used in RMD to ensure that we do not delete the cwd. I would be happy to add documentation to specify the contract in FtpFile.equals(), feel free to open an issue for this.
        Hide
        Sebb added a comment -

        Patch to use canonicalPath rather than canonicalFile.
        Also adds Javadoc for equals() method.

        Show
        Sebb added a comment - Patch to use canonicalPath rather than canonicalFile. Also adds Javadoc for equals() method.
        Hide
        Sebb added a comment -

        I think there's a possible bug in the equals method - it only takes account of the file field, and ignores the user and fileName fields.

        Show
        Sebb added a comment - I think there's a possible bug in the equals method - it only takes account of the file field, and ignores the user and fileName fields.
        Hide
        Niklas Gustavsson added a comment -

        Patch looks good, feel free to commit.

        As for equals (and hashCode()) ignoring user and fileName, this is on purpose. equals only attempts to see if the physical file is the same, not the user scoped file (which user/fileName represents).

        Show
        Niklas Gustavsson added a comment - Patch looks good, feel free to commit. As for equals (and hashCode()) ignoring user and fileName, this is on purpose. equals only attempts to see if the physical file is the same, not the user scoped file (which user/fileName represents).
        Hide
        Sebb added a comment -

        Patch added in #1138633 and #1138635

        Show
        Sebb added a comment - Patch added in #1138633 and #1138635
        Hide
        Niklas Gustavsson added a comment -

        Closing with Sebbs patch in place

        Show
        Niklas Gustavsson added a comment - Closing with Sebbs patch in place

          People

          • Assignee:
            Niklas Gustavsson
            Reporter:
            Sergey Vladimirov
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development