Commons Lang
  1. Commons Lang
  2. LANG-753

Document v3.x changes to Validate API

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0, 3.0.1, 3.1
    • Fix Version/s: 3.2
    • Component/s: lang.*
    • Labels:
      None

      Description

      Validate.notNull() throws an IllegalArgumentException in commons-lang 2.6, but in 3.0.1 it throws a NullPointerException. I can't find any note about this in the release notes. I think this is a regression, because it could break others code.

        Activity

        Hide
        Matt Benson added a comment -

        Committed revision 1354616.

        Show
        Matt Benson added a comment - Committed revision 1354616.
        Hide
        Matt Benson added a comment -

        It's a documentation issue. If you need an IllegalArgumentException you can use some variant of Validate.isTrue(arg != null). As Erhan pointed out early on, the behavior cannot be changed for [lang] v3.x without introducing worse BC issues than what you've experienced upgrading from v2.x (bearing in mind that the package change was an intentional break for just this reason).

        Show
        Matt Benson added a comment - It's a documentation issue. If you need an IllegalArgumentException you can use some variant of Validate.isTrue(arg != null) . As Erhan pointed out early on, the behavior cannot be changed for [lang] v3.x without introducing worse BC issues than what you've experienced upgrading from v2.x (bearing in mind that the package change was an intentional break for just this reason).
        Hide
        Oliver Siegmar added a comment -

        Ignorance is Not a Solution

        Any news on this matter?

        Show
        Oliver Siegmar added a comment - Ignorance is Not a Solution Any news on this matter?
        Hide
        Max Ullinger added a comment -

        We are in the same boat here.

        We are NOT interested in WHAT is wrong with an object, we just want to know if it is correct according to our validation rules, and if not abandon current processing.
        That means having to catch more than one exception is tedious, enflates code and adds nothing in our case (and is therefore something we will not do with our rather large codebase).

        If we want to know exactly why validation fails it does not help us to know what kind of exception was thrown, the object in question needs to be examined field per field anyway.

        It would be easy enough to have a wrapper class for Validate that changes the type of the thrown exception to IllegalArgumentException (and we might do that as a workaround), but I am still not convinced that going with more than one exception is what people need from Validate.

        Changing the behaviour back would therefore make sense, I feel.

        Still, keep up the good work!

        Show
        Max Ullinger added a comment - We are in the same boat here. We are NOT interested in WHAT is wrong with an object, we just want to know if it is correct according to our validation rules, and if not abandon current processing. That means having to catch more than one exception is tedious, enflates code and adds nothing in our case (and is therefore something we will not do with our rather large codebase). If we want to know exactly why validation fails it does not help us to know what kind of exception was thrown, the object in question needs to be examined field per field anyway. It would be easy enough to have a wrapper class for Validate that changes the type of the thrown exception to IllegalArgumentException (and we might do that as a workaround), but I am still not convinced that going with more than one exception is what people need from Validate. Changing the behaviour back would therefore make sense, I feel. Still, keep up the good work!
        Hide
        J. Nathaniel Sloan added a comment -

        +1 for Oliver here. By making this change, you've abandoned me at version 2.6. The first test case I write for most methods is simply that it behaves rationally when presented with null input, especially where null is disallowed by the API. Therefore, I have an entire suite of tests written to look for IllegalArgumentException.

        Naturally, I could change the tests, but why would I do so? Unless I'm willing to rewrite them all to check the message text, I can't differentiate between a NullPointerException thrown by the Validate method and one generated by making a method call on the null reference.

        Version 2.6 performs two tasks: provides a readable message, which is good for log files and defect analysis, and changes the exception type from what you would otherwise get, which is good for automated testing. Version 3.x does only the former.

        Finally, +1 again for the idea that just because Oracle does something doesn't make it a best practice. It was wrong when they wrote it into the spec for Comparator, and it's wrong now, for exactly the reasons in Oliver's first comment. NullPointerException should be reserved exclusively for making method calls using null references. If I wanted a NullPointerException, I'd just omit the call to Validate and let the code continue.

        I hope you'll reconsider, and thanks for listening.

        Show
        J. Nathaniel Sloan added a comment - +1 for Oliver here. By making this change, you've abandoned me at version 2.6. The first test case I write for most methods is simply that it behaves rationally when presented with null input, especially where null is disallowed by the API. Therefore, I have an entire suite of tests written to look for IllegalArgumentException. Naturally, I could change the tests, but why would I do so? Unless I'm willing to rewrite them all to check the message text, I can't differentiate between a NullPointerException thrown by the Validate method and one generated by making a method call on the null reference. Version 2.6 performs two tasks: provides a readable message, which is good for log files and defect analysis, and changes the exception type from what you would otherwise get, which is good for automated testing. Version 3.x does only the former. Finally, +1 again for the idea that just because Oracle does something doesn't make it a best practice. It was wrong when they wrote it into the spec for Comparator, and it's wrong now, for exactly the reasons in Oliver's first comment. NullPointerException should be reserved exclusively for making method calls using null references. If I wanted a NullPointerException, I'd just omit the call to Validate and let the code continue. I hope you'll reconsider, and thanks for listening.
        Hide
        Oliver Siegmar added a comment -

        Well, I still think a NPE is wrong - either in org.apache.commons.lang3.Validate and in java.util.Objects. The latter one doesn't makes it better. But if you do not want to change that, I'm okay with a correction in the documentation you offered to do. Thanks!

        Show
        Oliver Siegmar added a comment - Well, I still think a NPE is wrong - either in org.apache.commons.lang3.Validate and in java.util.Objects. The latter one doesn't makes it better. But if you do not want to change that, I'm okay with a correction in the documentation you offered to do. Thanks!
        Hide
        Paul Benedict added a comment -

        Thanks Oliver. It's a shame it wasn't mentioned in that documentation. Unless you object, I can taken on the burden of modifying that file. You okay with me doing that?

        Show
        Paul Benedict added a comment - Thanks Oliver. It's a shame it wasn't mentioned in that documentation. Unless you object, I can taken on the burden of modifying that file. You okay with me doing that?
        Hide
        Oliver Siegmar added a comment -

        It should be explicitly mentioned in "What's new in Commons Lang 3.0?" for example.

        Show
        Oliver Siegmar added a comment - It should be explicitly mentioned in " What's new in Commons Lang 3.0? " for example.
        Hide
        Paul Benedict added a comment -

        If there are compatibility notes (are there?), this change should be mentioned. I +1 increasing the documentation where possible.

        Show
        Paul Benedict added a comment - If there are compatibility notes (are there?), this change should be mentioned. I +1 increasing the documentation where possible.
        Hide
        Paul Benedict added a comment -

        This is not a bug nor a regression. Lang 3 is totally independent of Lang 2. Henry and I went through this many times and agreed this is what we wanted to do. The javadoc was appropriately updated to document that NPE is thrown both in the class header and in the individual methods.

        Show
        Paul Benedict added a comment - This is not a bug nor a regression. Lang 3 is totally independent of Lang 2. Henry and I went through this many times and agreed this is what we wanted to do. The javadoc was appropriately updated to document that NPE is thrown both in the class header and in the individual methods.
        Hide
        Oliver Siegmar added a comment -

        Then please document this breaking API change, because an undocumented API change is a regression bug.

        Show
        Oliver Siegmar added a comment - Then please document this breaking API change, because an undocumented API change is a regression bug.
        Hide
        Paul Benedict added a comment -

        This is not a bug. We changed notNull() to throw NPE in 3.0 to align with the JDK which is the "best practice". JDK 7 includes a similar public method in java.util.Objects and also throws NPE on null.

        Show
        Paul Benedict added a comment - This is not a bug. We changed notNull() to throw NPE in 3.0 to align with the JDK which is the "best practice". JDK 7 includes a similar public method in java.util.Objects and also throws NPE on null.
        Hide
        Oliver Siegmar added a comment -

        Consider that there are currently many more 2.6 users than 3.0 users. So breaking 3.0 compatibility is not as bad as breaking 2.6 compatibility if the change would be made now. This breaking changed should be documented very well, of course. Probably it could also be a 3.1 verison.

        From my point of view, a NullPointerException is the wrong type of Exception anyway. A NullPointerException should be thrown by the JVM only if an operation is executed on a null reference accidently. If I want to ensure that a method argument is required (must not be null) I'd like to throw an IllegalArgumentException because that describes what really happend - an illegal argument (null in this case) was passed to the method.

        Show
        Oliver Siegmar added a comment - Consider that there are currently many more 2.6 users than 3.0 users. So breaking 3.0 compatibility is not as bad as breaking 2.6 compatibility if the change would be made now . This breaking changed should be documented very well, of course. Probably it could also be a 3.1 verison. From my point of view, a NullPointerException is the wrong type of Exception anyway. A NullPointerException should be thrown by the JVM only if an operation is executed on a null reference accidently. If I want to ensure that a method argument is required (must not be null) I'd like to throw an IllegalArgumentException because that describes what really happend - an illegal argument (null in this case) was passed to the method.
        Hide
        Erhan Bagdemir added a comment - - edited

        well.
        i think that it can not be reverted so easily since there could be also applications which use version 3.0+ and expect NullPointerException.
        To throw an IllegalArgumentException in this case (3.0+) can also break these applications. I think, it's obviously a mistake in design which breaks backward compatibility "silently" using an another unchecked Exception. However, the optional "values" parameter in the newer versions of notNull(...) would let 2.6 developers upgrade his framework version upto 3+ without any bother although he supplies just two params.

        My suggestion, is to add a hint in javadocs of method and to accept officially that the 3+ is not backward compatible.
        Even, it is considerable to change the method signatur to notNull(T object, String message, Object [] values) so that the IDEs catch this incompatibility in later versions.

        Show
        Erhan Bagdemir added a comment - - edited well. i think that it can not be reverted so easily since there could be also applications which use version 3.0+ and expect NullPointerException. To throw an IllegalArgumentException in this case (3.0+) can also break these applications. I think, it's obviously a mistake in design which breaks backward compatibility "silently" using an another unchecked Exception. However, the optional "values" parameter in the newer versions of notNull(...) would let 2.6 developers upgrade his framework version upto 3+ without any bother although he supplies just two params. My suggestion, is to add a hint in javadocs of method and to accept officially that the 3+ is not backward compatible. Even, it is considerable to change the method signatur to notNull(T object, String message, Object [] values) so that the IDEs catch this incompatibility in later versions.

          People

          • Assignee:
            Unassigned
            Reporter:
            Oliver Siegmar
          • Votes:
            3 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development