OFBiz
  1. OFBiz
  2. OFBIZ-4427

Possible runtime errors with UtilValidate.isEmpty(Object) should be rather caught during compilation

    Details

      Description

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

      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

        Activity

        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
        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
        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 -

        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 -

        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 -

        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 -

        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 -

        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
        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
        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 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?

          People

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

            Dates

            • Created:
              Updated:

              Development