Maven
  1. Maven
  2. MNG-3047

DefaultArtifactVersion compareTo inconsistent with equals

    Details

    • Patch Submitted:
      Yes
    • Number of attachments :
      1

      Description

      Over the course of investigating MNG-3046, I discovered DefaultArtifactVersion's implementation of Comparable.compareTo() is inconsistent with its equals(Object). (DefaultArtifactVersion doesn't implement equals(...); it's using the instance equals it gets from Object.) The contract for Comparable.compareTo()[1] states, while it's not strictly required the behavior between compareTo and equals be consistent, breaking it should be an overt and visible decision. In the case of DefaultArtifactVersion, there's really no reason not to implement equals and hashCode.

      I have a fix-I'll attach a patch shortly-that implements equals and hashCode following the recipes from Bloch's "Effective Java." In fact, equals now uses a cleaned-up compareTo, ensuring consistency across these methods.

      Since the interface ArtifactVersion extends Comparable (as opposed to DefaultArtifactVersion implementing both ArtifactVersion and Comparable) I assume the intent is to be able to compare different ArtifactVersions regardless of implementation. Therefore, I added the equals and hashCode declaration to the interface and made the equals and compareTo implementations work with all ArtifactVersions.

      Note that this work obviates the patch for MNG-3046. I made that patch small and surgical to fix a major issue. This fix is less urgent-still important, imho-but I wasn't sure if the interface changes were right for the whole project, if such a big change is warranted, etc. The bottom line is: only that patch or this one need be applied, not both.

      [1] http://java.sun.com/javase/6/docs/api/java/lang/Comparable.html#compareTo(T)

        Issue Links

          Activity

          Hide
          David Julian added a comment -

          Changes to org.apache.maven.artifact.versioning:

          ArtifactVersion
          DefaultArtifactVersion
          DefaultArtifactVersionTest

          Show
          David Julian added a comment - Changes to org.apache.maven.artifact.versioning: ArtifactVersion DefaultArtifactVersion DefaultArtifactVersionTest
          Hide
          John Casey added a comment -

          Applied. Thanks!

          Show
          John Casey added a comment - Applied. Thanks!

            People

            • Assignee:
              John Casey
              Reporter:
              David Julian
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: