OFBiz
  1. OFBiz
  2. OFBIZ-4427

Runtime errors with UtilValidate.isEmpty(Object) and IsNotEmpty should be caught during compilation

    Details

    • Type: Improvement Improvement
    • Status: Reopened
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: Trunk
    • Fix Version/s: None
    • Component/s: framework
    • Sprint:
      Bug Crush Event - 21/2/2015

      Description

      Hence we need to remove the UtilValidate.isEmpty(Object) method and provide methods that accept explicit types.

      Adrian then (http://markmail.org/message/37rtrnulhmyzxfoo) suggested that

      Scripting languages should use a facade class that provides methods for working with generic Objects or providing default behaviors.

      1. OFBIZ-4427.patch
        1 kB
        Paul Foxworthy
      2. OFBIZ-4427_isEmpty.patch
        88 kB
        Paul Foxworthy

        Issue Links

          Activity

          Hide
          Paul Foxworthy added a comment -

          I like the principle, but the current implementation specifically refuses to even log a warning for commonly used classes like Number and DateTime. A comment says this is to avoid "flooding" the log. See https://fisheye6.atlassian.com/browse/~br=trunk/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java?r=1076384#to774 . For other classes the logging severity level was reduced from warning to verbose, presumably again because of the volume of log entries.
          Could someone get a number on how many places are affected before we decide how to proceed?

          Show
          Paul Foxworthy added a comment - I like the principle, but the current implementation specifically refuses to even log a warning for commonly used classes like Number and DateTime. A comment says this is to avoid "flooding" the log. See https://fisheye6.atlassian.com/browse/~br=trunk/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java?r=1076384#to774 . For other classes the logging severity level was reduced from warning to verbose, presumably again because of the volume of log entries. Could someone get a number on how many places are affected before we decide how to proceed?
          Hide
          Adrian Crum added a comment -

          According to Eclipse:

          166 references in Java code
          25 references in scripts (bsh, groovy, xml)

          Show
          Adrian Crum added a comment - According to Eclipse: 166 references in Java code 25 references in scripts (bsh, groovy, xml)
          Hide
          Paul Foxworthy added a comment -

          I tried commenting out isEmpty( Object ) and isNotEmpty(Object), and here's a patch for the first few problems I found. Plenty more to be done!

          Show
          Paul Foxworthy added a comment - I tried commenting out isEmpty( Object ) and isNotEmpty(Object), and here's a patch for the first few problems I found. Plenty more to be done!
          Hide
          Paul Foxworthy added a comment -

          https://fisheye6.atlassian.com/browse/~br=trunk/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilHttp.java?r=1170525#to614 has a call to UtilValidate.isEmpty with appDefaultLocale, which was declared as Object. Can it be tightened to String? The call at https://fisheye6.atlassian.com/browse/ofbiz/trunk/applications/product/src/org/ofbiz/product/store/ProductStoreWorker.java?r=1044931#to111 passes a String.

          And thanks, Adrian. At a guess most of the 166 calls will be OK so there are a manageable number to fix.

          Show
          Paul Foxworthy added a comment - https://fisheye6.atlassian.com/browse/~br=trunk/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilHttp.java?r=1170525#to614 has a call to UtilValidate.isEmpty with appDefaultLocale, which was declared as Object. Can it be tightened to String? The call at https://fisheye6.atlassian.com/browse/ofbiz/trunk/applications/product/src/org/ofbiz/product/store/ProductStoreWorker.java?r=1044931#to111 passes a String. And thanks, Adrian. At a guess most of the 166 calls will be OK so there are a manageable number to fix.
          Hide
          Jacques Le Roux added a comment -

          It does not solve anything but I found interesting sizeIsEmpty in commons-collections CollectionUtils.java: www.jarvana.com/jarvana/view/commons-collections/commons-collections/3.2/commons-collections-3.2-sources.jar!/org/apache/commons/collections/CollectionUtils.java?format=ok

          Show
          Jacques Le Roux added a comment - It does not solve anything but I found interesting sizeIsEmpty in commons-collections CollectionUtils.java: www.jarvana.com/jarvana/view/commons-collections/commons-collections/3.2/commons-collections-3.2-sources.jar!/org/apache/commons/collections/CollectionUtils.java?format=ok
          Hide
          Paul Foxworthy added a comment -

          Hi Jacques,

          In what way interesting? It seems similar to ObjectType.isEmpty (https://fisheye6.atlassian.com/browse/~br=trunk/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java?r=1076384#to765) and the various overloaded UtilValidate.isEmpty methods.

          Cheers

          Paul Foxworthy

          Show
          Paul Foxworthy added a comment - Hi Jacques, In what way interesting? It seems similar to ObjectType.isEmpty ( https://fisheye6.atlassian.com/browse/~br=trunk/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java?r=1076384#to765 ) and the various overloaded UtilValidate.isEmpty methods. Cheers Paul Foxworthy
          Hide
          Jacques Le Roux added a comment -

          Yes, I think we could add also the array type we don't have. I was hoping to find some inspiration there or even a replacement but I must say it's similar than ours

          Show
          Jacques Le Roux added a comment - Yes, I think we could add also the array type we don't have. I was hoping to find some inspiration there or even a replacement but I must say it's similar than ours
          Hide
          Paul Foxworthy added a comment -

          I've been doing some more thinking about this. I think UtilValidate.isEmpty() should work with an array, as Jacques said. Currently it doesn't.

          There is at least one spot where isEmpty is being used with an array: https://fisheye6.atlassian.com/browse/ofbiz/trunk/specialpurpose/ebaystore/src/org/ofbiz/ebaystore/EbayStore.java?hb=true#to1441

          That is very misleading code. Anyone reading it would be entitled to assume isEmpty would test for null and for an empty array.

          If we do want to support arrays, and we want to remove UtilValidate.isEmpty for any Object in general, then we need to add eight overloaded versions of UtilValidate.isEmpty for arrays of each of the primitive types, plus one more for Object[].

          Is that OK in order to achieve the goal of this Jira, or should we give up on compile time checking of calls to isEmpty?

          Show
          Paul Foxworthy added a comment - I've been doing some more thinking about this. I think UtilValidate.isEmpty() should work with an array, as Jacques said. Currently it doesn't. There is at least one spot where isEmpty is being used with an array: https://fisheye6.atlassian.com/browse/ofbiz/trunk/specialpurpose/ebaystore/src/org/ofbiz/ebaystore/EbayStore.java?hb=true#to1441 That is very misleading code. Anyone reading it would be entitled to assume isEmpty would test for null and for an empty array. If we do want to support arrays, and we want to remove UtilValidate.isEmpty for any Object in general, then we need to add eight overloaded versions of UtilValidate.isEmpty for arrays of each of the primitive types, plus one more for Object[]. Is that OK in order to achieve the goal of this Jira, or should we give up on compile time checking of calls to isEmpty?
          Hide
          Jacques Le Roux added a comment -

          Hi Paul,

          Coincidentally, I was working on the style of last OFBIZ-4580 patch. I wanted to fix/improve the way the error is returned/logged when GenericEntityException is catched there. For that I wanted to use something like

          String errMsg = UtilProperties.getMessage("CommonUiLabels", "CommonDatabaseProblem", messageMap, (Locale) context.get("locale"));
          

          I found 2 issues I'm working on:

          1. CommonDatabaseProblem does not receive an errMessage parameter thought it's used as is OOTB, which is unrelated to our issue
          2. Looking into <<public static String getMessage(String resource, String name, Object[] arguments, Locale locale)>> I wanted to use UtilValidate.isEmpty(arguments) in/instead of <<if (arguments != null && arguments.length > 0) {>> and then remembered your proposition. I think it's a good idea, and would appreciate a patch

          Thanks for your help

          Show
          Jacques Le Roux added a comment - Hi Paul, Coincidentally, I was working on the style of last OFBIZ-4580 patch. I wanted to fix/improve the way the error is returned/logged when GenericEntityException is catched there. For that I wanted to use something like String errMsg = UtilProperties.getMessage( "CommonUiLabels" , "CommonDatabaseProblem" , messageMap, (Locale) context.get( "locale" )); I found 2 issues I'm working on: CommonDatabaseProblem does not receive an errMessage parameter thought it's used as is OOTB, which is unrelated to our issue Looking into <<public static String getMessage(String resource, String name, Object[] arguments, Locale locale)>> I wanted to use UtilValidate.isEmpty(arguments) in/instead of <<if (arguments != null && arguments.length > 0) {>> and then remembered your proposition. I think it's a good idea, and would appreciate a patch Thanks for your help
          Hide
          Paul Foxworthy added a comment -

          Here's a new patch.
          I have:

          • deprecated UtilValidate.isEmpty(Object) and UtilValidate.isNotEmpty(Object)
          • added overloaded variants of these two methods with an Object[] parameter
          • Removed isEmpty(String) and isNotEmpty(String), because String derives from CharSequence, and there is already a CharSequence overload for these methods
          • detected and fixed many calls to isEmpty and isNotEmpty when all that was needed was a comparison with null

          There are still many calls remaining, but they will set off a deprecation warning, so if several of us work at resolving those we can eventually eliminate the methods

          Show
          Paul Foxworthy added a comment - Here's a new patch. I have: deprecated UtilValidate.isEmpty(Object) and UtilValidate.isNotEmpty(Object) added overloaded variants of these two methods with an Object[] parameter Removed isEmpty(String) and isNotEmpty(String), because String derives from CharSequence, and there is already a CharSequence overload for these methods detected and fixed many calls to isEmpty and isNotEmpty when all that was needed was a comparison with null There are still many calls remaining, but they will set off a deprecation warning, so if several of us work at resolving those we can eventually eliminate the methods
          Hide
          Paul Foxworthy added a comment -

          I have decided to only write isEmpty(Object[]) and isNotEmpty(Object[]) for the moment.

          If anyone ever wants to use these methods with an array of one of the primitive types, that would not compile. We would need to add two more methods each with a parameter of the primitive array type. I think we're better off doing that when it's needed, rather than writing a complete set of sixteen methods, most of which will never be used.

          Show
          Paul Foxworthy added a comment - I have decided to only write isEmpty(Object[]) and isNotEmpty(Object[]) for the moment. If anyone ever wants to use these methods with an array of one of the primitive types, that would not compile. We would need to add two more methods each with a parameter of the primitive array type. I think we're better off doing that when it's needed, rather than writing a complete set of sixteen methods, most of which will never be used.
          Hide
          Jacques Le Roux added a comment -

          Hi Paul,

          Now that you have commit rights, why not commit those changes?

          Show
          Jacques Le Roux added a comment - Hi Paul, Now that you have commit rights, why not commit those changes?
          Hide
          Jacques Le Roux added a comment -

          I tried to apply the patch. There are 11 rejected hunks. So I decided to go by hand for those but stumbled upon this in InvoiceWorker.java (1st hunk I tried to handle by hand)

          @@ -519,12 +519,12 @@
                           conversionRate = acctgTransEntry.getBigDecimal("amount").divide(acctgTransEntry.getBigDecimal("origAmount"), new MathContext(100)).setScale(decimals,rounding);
                       }
                       // check if a payment is applied and use the currency conversion from there
          -            if (UtilValidate.isEmpty(conversionRate)) {
          +            if (conversionRate == null) {
                           List<GenericValue> paymentAppls = invoice.getRelated("PaymentApplication");
                           for (GenericValue paymentAppl : paymentAppls) {
                               GenericValue payment = paymentAppl.getRelatedOne("Payment");
          -                    if (UtilValidate.isNotEmpty(payment.getBigDecimal("actualCurrencyAmount"))) {
          -                        if (UtilValidate.isEmpty(conversionRate)) {
          +                    if (payment.getBigDecimal("actualCurrencyAmount") != null) {
          +                        if (conversionRate == null) {
                                       conversionRate = payment.getBigDecimal("amount").divide(payment.getBigDecimal("actualCurrencyAmount"),new MathContext(100)).setScale(decimals,rounding);
          
                                   } else {
                                       conversionRate = conversionRate.add(payment.getBigDecimal("amount").divide(payment.getBigDecimal("actualCurrencyAmount"),new MathContext(100))).divide(new BigDecimal("2"),new MathContext(100)).setScale(decimals,rounding);
          

          Obviously the second test on "conversionRate == null" does not make sense because conversionRate would be necessarily null in the whole block. But I guess that the 2 "conversionRate = ....." cases were implemented for a reason. So I did not dig furter and decided to stop there and to close this as won't fix, better to not introduce troubles...

          Show
          Jacques Le Roux added a comment - I tried to apply the patch. There are 11 rejected hunks. So I decided to go by hand for those but stumbled upon this in InvoiceWorker.java (1st hunk I tried to handle by hand) @@ -519,12 +519,12 @@ conversionRate = acctgTransEntry.getBigDecimal( "amount" ).divide(acctgTransEntry.getBigDecimal( "origAmount" ), new MathContext(100)).setScale(decimals,rounding); } // check if a payment is applied and use the currency conversion from there - if (UtilValidate.isEmpty(conversionRate)) { + if (conversionRate == null ) { List<GenericValue> paymentAppls = invoice.getRelated( "PaymentApplication" ); for (GenericValue paymentAppl : paymentAppls) { GenericValue payment = paymentAppl.getRelatedOne( "Payment" ); - if (UtilValidate.isNotEmpty(payment.getBigDecimal( "actualCurrencyAmount" ))) { - if (UtilValidate.isEmpty(conversionRate)) { + if (payment.getBigDecimal( "actualCurrencyAmount" ) != null ) { + if (conversionRate == null ) { conversionRate = payment.getBigDecimal( "amount" ).divide(payment.getBigDecimal( "actualCurrencyAmount" ), new MathContext(100)).setScale(decimals,rounding); } else { conversionRate = conversionRate.add(payment.getBigDecimal( "amount" ).divide(payment.getBigDecimal( "actualCurrencyAmount" ), new MathContext(100))).divide( new BigDecimal( "2" ), new MathContext(100)).setScale(decimals,rounding); Obviously the second test on " conversionRate == null " does not make sense because conversionRate would be necessarily null in the whole block. But I guess that the 2 " conversionRate = ..... " cases were implemented for a reason. So I did not dig furter and decided to stop there and to close this as won't fix, better to not introduce troubles...
          Hide
          Paul Foxworthy added a comment -

          Hi Jacques,

          I think there's no need to be scared . I agree with your analysis: conversionRate will be null, so the inner if statement can be removed.

          I suggest: change the else part of the inner if to throw an exception, then create a unit test to verify the exception never happens.

          Show
          Paul Foxworthy added a comment - Hi Jacques, I think there's no need to be scared . I agree with your analysis: conversionRate will be null, so the inner if statement can be removed. I suggest: change the else part of the inner if to throw an exception, then create a unit test to verify the exception never happens.
          Hide
          Paul Foxworthy added a comment - - edited

          Jacques says in the description: "Scripting languages should use a facade class that provides methods for working with generic Objects or providing default behaviors"

          Are you suggesting that UtilValidate.isEmpty( Object ) should be available to scripting languages, but not to Java?

          The whole point of this issue is to enable isEmpty for types where emptiness is relevant , and not for other types where isEmpty is misleading and we should just use == null. I'd argue that if "emptiness" does not make sense, then isEmpty should not work, even from a scripting language.

          In Groovy you can retrofit an interface onto a class using metaclasses without rewriting the code for that class. So we can add the org.apache.ofbiz.base.lang.IsEmpty interface to everything we need, and still deprecate it for Objects in general.

          Show
          Paul Foxworthy added a comment - - edited Jacques says in the description: "Scripting languages should use a facade class that provides methods for working with generic Objects or providing default behaviors" Are you suggesting that UtilValidate.isEmpty( Object ) should be available to scripting languages, but not to Java? The whole point of this issue is to enable isEmpty for types where emptiness is relevant , and not for other types where isEmpty is misleading and we should just use == null. I'd argue that if "emptiness" does not make sense, then isEmpty should not work, even from a scripting language. In Groovy you can retrofit an interface onto a class using metaclasses without rewriting the code for that class. So we can add the org.apache.ofbiz.base.lang.IsEmpty interface to everything we need, and still deprecate it for Objects in general.
          Hide
          Jacques Le Roux added a comment -

          Paul,

          Actually it was Adrian's writing (I should have quoted him) http://markmail.org/message/37rtrnulhmyzxfoo which was an answer to Scott Gray previous message. Summary: they agreed about removing UtilValidate.isEmpty(Object) but Adrian suggested the sentence above, it's about minilang

          Show
          Jacques Le Roux added a comment - Paul, Actually it was Adrian's writing (I should have quoted him) http://markmail.org/message/37rtrnulhmyzxfoo which was an answer to Scott Gray previous message. Summary: they agreed about removing UtilValidate.isEmpty(Object) but Adrian suggested the sentence above, it's about minilang
          Hide
          Paul Foxworthy added a comment -

          OK, thanks Jacques.

          ObjectType.isEmpty should remain for minilang, querying and other framework code that needs it. So minilang should continue to work without any change.

          It's just UtilValidate.isEmpty(Object) and UtilValidate.isNotEmpty(Object) that are going away, so service code in Java and Groovy will get better checking.

          Show
          Paul Foxworthy added a comment - OK, thanks Jacques. ObjectType.isEmpty should remain for minilang, querying and other framework code that needs it. So minilang should continue to work without any change. It's just UtilValidate.isEmpty(Object) and UtilValidate.isNotEmpty(Object) that are going away, so service code in Java and Groovy will get better checking.
          Hide
          Jacques Le Roux added a comment -

          I Paul, I agree so I reopen. BTW I forgot to mention in the title: the same is true for isNotEmpty(Object) which was introduced with OFBIZ-785. isEmpty(Object) was pre Apache era.

          Will you handle the rest with a new patch?

          Show
          Jacques Le Roux added a comment - I Paul, I agree so I reopen. BTW I forgot to mention in the title: the same is true for isNotEmpty(Object) which was introduced with OFBIZ-785 . isEmpty(Object) was pre Apache era. Will you handle the rest with a new patch?
          Hide
          Paul Foxworthy added a comment -

          OK.

          The patch should be considerably smaller because OFBIZ-8471 has already cleaned up use of IsEmpty with GenericValue objects.

          Show
          Paul Foxworthy added a comment - OK. The patch should be considerably smaller because OFBIZ-8471 has already cleaned up use of IsEmpty with GenericValue objects.
          Hide
          Jacques Le Roux added a comment -

          Sounds good, looking forward

          Show
          Jacques Le Roux added a comment - Sounds good, looking forward

            People

            • Assignee:
              Paul Foxworthy
              Reporter:
              Jacques Le Roux
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:

                Development

                  Agile