You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "bozzelliandrea (via GitHub)" <gi...@apache.org> on 2023/06/09 10:19:07 UTC
[GitHub] [commons-lang] bozzelliandrea opened a new pull request, #1063: ObjectUtils caught NullPointerException inside defaultIfNull method to return default value
bozzelliandrea opened a new pull request, #1063:
URL: https://github.com/apache/commons-lang/pull/1063
this is a proposal change for the defaultIfNull method, it could be useful when we want a default value with a list or nested object, for example:
```java
defaultIfNull(list.get(0).getValue(), "helloDefault");
```
where list.get(0) is null, so we skip the null pointer and directly return the default value as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] aherbert commented on a diff in pull request #1063: ObjectUtils: caught NullPointerException inside defaultIfNull method to return default value
Posted by "aherbert (via GitHub)" <gi...@apache.org>.
aherbert commented on code in PR #1063:
URL: https://github.com/apache/commons-lang/pull/1063#discussion_r1224468663
##########
src/main/java/org/apache/commons/lang3/ObjectUtils.java:
##########
@@ -595,7 +595,11 @@ public static short CONST_SHORT(final int v) {
* TODO Rename to getIfNull in 4.0
*/
public static <T> T defaultIfNull(final T object, final T defaultValue) {
- return object != null ? object : defaultValue;
+ try {
Review Comment:
The suggested method would work. However this is outside of the scope of the library. There are many uses of a `Supplier` in the library and we do not want to set a precedent for catching NPE and handling this by returning `null`.
A possible change that may be acceptable is to update `o.a.c.l.function.Suppliers.get` to catch NPE:
```Java
public static <T> T get(final Supplier<T> supplier) {
try {
return supplier == null ? null : supplier.get();
} catch (NullPointerException ignored) {
return null;
}
}
```
Then you can call e.g.:
```Java
List<String> list = Collections.emptyList();
Object object = ObjectUtils.defaultIfNull(Suppliers.get(() -> list.get(0)), "helloDefault");
```
The `Suppliers` class then effectively hides your potential NPE.
I would not recommend this change to `Suppliers`. A NPE is typically generated by a programming error and this type of usage would cover it up. The use case may be that a supplier is provided by an unknown party and you wish to protect against generating NPE. But a third party could throw any type of RTE or error. Only catching NPE is a specific case where you are using your own supplier and are aware it may dereference null. This seems like it should be handled better in the original implementing code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] bozzelliandrea closed pull request #1063: ObjectUtils: caught NullPointerException inside defaultIfNull method to return default value
Posted by "bozzelliandrea (via GitHub)" <gi...@apache.org>.
bozzelliandrea closed pull request #1063: ObjectUtils: caught NullPointerException inside defaultIfNull method to return default value
URL: https://github.com/apache/commons-lang/pull/1063
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] aherbert commented on a diff in pull request #1063: ObjectUtils: caught NullPointerException inside defaultIfNull method to return default value
Posted by "aherbert (via GitHub)" <gi...@apache.org>.
aherbert commented on code in PR #1063:
URL: https://github.com/apache/commons-lang/pull/1063#discussion_r1224218659
##########
src/main/java/org/apache/commons/lang3/ObjectUtils.java:
##########
@@ -595,7 +595,11 @@ public static short CONST_SHORT(final int v) {
* TODO Rename to getIfNull in 4.0
*/
public static <T> T defaultIfNull(final T object, final T defaultValue) {
- return object != null ? object : defaultValue;
+ try {
Review Comment:
This will not catch the exception in your example:
```Java
defaultIfNull(list.get(0).getValue(), "helloDefault");
```
Equivalent to:
```Java
Object o = list.get(0).getValue(); // NPE generated here
ObjectUtils.defaultIfNull(o, "helloDefault");
```
What you require is a method that uses a `Supplier` to create the object. Your supplier would be responsible for catching the NPE if you use `ObjectUtils.getFirstNonNull`:
```Java
List<Object> list = Collections.emptyList();
Object object = ObjectUtils.getFirstNonNull(
() -> { try {
return list.get(0).getValue();
} catch (NullPointerException ignored) {
return null;
}},
() -> "helloDefault"
);
```
In this scenario it makes more sense to check for null using a classic `if` or `ternary` statement to avoid the NPE and let the `ObjectUtils` handle the null from `getValue`:
```Java
Object o = ObjectUtils.defaultIfNull(list.isEmpty() ? null : list.get(0).getValue(),
"helloDefault");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] bozzelliandrea commented on a diff in pull request #1063: ObjectUtils: caught NullPointerException inside defaultIfNull method to return default value
Posted by "bozzelliandrea (via GitHub)" <gi...@apache.org>.
bozzelliandrea commented on code in PR #1063:
URL: https://github.com/apache/commons-lang/pull/1063#discussion_r1224319063
##########
src/main/java/org/apache/commons/lang3/ObjectUtils.java:
##########
@@ -595,7 +595,11 @@ public static short CONST_SHORT(final int v) {
* TODO Rename to getIfNull in 4.0
*/
public static <T> T defaultIfNull(final T object, final T defaultValue) {
- return object != null ? object : defaultValue;
+ try {
Review Comment:
@aherbert thx for the review, yep you right, the supplier could be the only solution to check inside the defaultIfNull the exception.
A possible solution could be overloaded the defaultIfNull method with a supplier as first parameter, like this:
```java
public static <T> T defaultIfNull(final Supplier<T> supplier, final T defaultValue) {
try {
return defaultIfNull(supplier.get(), defaultValue);
} catch (NullPointerException npe) {
return defaultValue;
}
}
```
and an usage like: `ObjectUtils.defaultIfNull(() -> list.get(0).getValue(), "helloDefault");`
in this scenario, the exception will be checked when the get method of the provider is called.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org