Should not catch Exception in EntityUtilProperties.getSystemPropertyValue()

[Wei Zhang]

Hi,

I found there is a try/catch block to catch Exception in EntityUtilProperties.getSystemPropertyValue(). Which will catche a NPE (delegator is null) when this method is called in framework/widget/templates/HtmlFormMacroLibrary.ftl but we only get a waring message in the log file.

So I think we should catch GenericEntityException rather than Exception here to expose NPE or other runtime exceptions.

Thanks,

Wei

[Jacques Le Roux]

Thanks Wei,

Please All, refer to OFBIZ-9230 to see how I temporarily handled the issue.

I also agree that using GenericEntityException is better than just a permissive Exception

But I believe we should rather fix the underlying issue I reported there. So I was hesitant to agree with Wei though it's maybe a better way to get to
really fix the issue because it shows it at the UI level and non only in logs.

Jacques


Le 28/02/2017 à 03:35, Wei Zhang a écrit :

> Hi,
>
> I found there is a try/catch block to catch Exception in
> EntityUtilProperties.getSystemPropertyValue(). Which will catche a NPE
> (delegator is null) when this method is called in
> framework/widget/templates/HtmlFormMacroLibrary.ftl but we only get a waring
> message in the log file.
>
> So I think we should catch GenericEntityException rather than Exception here
> to expose NPE or other runtime exceptions.
>
> Thanks,
>
> Wei
>
>
>
> -----
> 程序羊
> --
> View this message in context: http://ofbiz.135035.n4.nabble.com/Should-not-catch-Exception-in-EntityUtilProperties-getSystemPropertyValue-tp4702833.html
> Sent from the OFBiz - Dev mailing list archive at Nabble.com.
>
>

[Rishi Solanki]

+1 for using the GenericEntityException, we did similar effort in
OFBIZ-7539.

Also agree on Jacques concern that, we should also fix the NPE for
delegator coming from HtmlFormMacroLibrary.ftl. So I think for now the fix
provided in the mentioned ticket is fine OFBIZ-9230 and we should also use
the GenericEntityException. Later we should try to figure out why and when
system lose the delegator which causes the NPE. Once we would fix the
reason we should remove the fix added in the EntityUtilProperties class
method.

New fix proposal:

Change method WidgetWorker.getDelegator with following code;

    public static Delegator getDelegator(Map<String, Object> context) {
        Delegator delegator = (Delegator) context.get("delegator");
        if (delegator == null) {
            delegator = DelegatorFactory.getDelegator(delegatorName);
        }
        return delegator;
    }


Please see if this proposal looks fine then we could try and see if it
works to fix the original issue reported in the ticket.

Thanks!


Rishi Solanki
Sr. Manager, Enterprise Software Development
HotWax Systems Pvt. Ltd.
Direct: [phone]
http://www.hotwaxsystems.com

On Tue, Feb 28, 2017 at 1:49 PM, Jacques Le Roux <
[hidden email]> wrote:

> Thanks Wei,
>
> Please All, refer to OFBIZ-9230 to see how I temporarily handled the issue.
>
> I also agree that using GenericEntityException is better than just a
> permissive Exception
>
> But I believe we should rather fix the underlying issue I reported there.
> So I was hesitant to agree with Wei though it's maybe a better way to get
> to really fix the issue because it shows it at the UI level and non only in
> logs.
>
> Jacques
>
>
>
> Le 28/02/2017 à 03:35, Wei Zhang a écrit :
>
>> Hi,
>>
>> I found there is a try/catch block to catch Exception in
>> EntityUtilProperties.getSystemPropertyValue(). Which will catche a NPE
>> (delegator is null) when this method is called in
>> framework/widget/templates/HtmlFormMacroLibrary.ftl but we only get a
>> waring
>> message in the log file.
>>
>> So I think we should catch GenericEntityException rather than Exception
>> here
>> to expose NPE or other runtime exceptions.
>>
>> Thanks,
>>
>> Wei
>>
>>
>>
>> -----
>> 程序羊
>> --
>> View this message in context: http://ofbiz.135035.n4.nabble.
>> com/Should-not-catch-Exception-in-EntityUtilProperties-getSy
>> stemPropertyValue-tp4702833.html
>> Sent from the OFBiz - Dev mailing list archive at Nabble.com.
>>
>>
>>
>

[Jacques Le Roux]

Thanks Rishi,

Yes, good idea!

What do you suggest we use for delegatorName? I guess "default"?

Also for OFBIZ-9230 it should be rather

------------------------------------------------------------------------------------------------------------------------------------------------------

Index: framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityUtilProperties.java
===================================================================
--- framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityUtilProperties.java (revision 1784797)
+++ framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityUtilProperties.java (working copy)
@@ -40,6 +40,7 @@
  import org.apache.ofbiz.base.util.UtilValidate;
  import org.apache.ofbiz.base.util.collections.ResourceBundleMapWrapper;
  import org.apache.ofbiz.entity.Delegator;
+import org.apache.ofbiz.entity.DelegatorFactory;
  import org.apache.ofbiz.entity.GenericEntityException;
  import org.apache.ofbiz.entity.GenericValue;

@@ -62,8 +63,11 @@
          }
          resource = resource.replace(".properties", "");
          if (delegator == null) {
-            Debug.logInfo("Could not get a system property for " + name + ". Reason: the delegator is null", module);
-            return results;
+            delegator = DelegatorFactory.getDelegator("default");
+            if (delegator == null) {
+                Debug.logInfo("Could not get a system property for " + name + ". Reason: the delegator is null", module);
+                return results;
+            }
          }
          try {
              GenericValue systemProperty = EntityQuery.use(delegator)
@@ -82,7 +86,7 @@
                  results.put("value", "");
                  return results;
              }
-        } catch (Exception e) {
+        } catch (GenericEntityException e) {
              Debug.logInfo("Could not get a system property for " + name + " : " + e.getMessage(), module);
          }
          return results;

------------------------------------------------------------------------------------------------------------------------------------------------------

BTW, though it should not hurt, I'm not sure we need a change in WidgetWorker.getDelegator() . And if we do, we still need to handle the case when

delegator = DelegatorFactory.getDelegator("default");

returns null

Jacques


Le 28/02/2017 à 12:49, Rishi Solanki a écrit :

> +1 for using the GenericEntityException, we did similar effort in
> OFBIZ-7539.
>
> Also agree on Jacques concern that, we should also fix the NPE for
> delegator coming from HtmlFormMacroLibrary.ftl. So I think for now the fix
> provided in the mentioned ticket is fine OFBIZ-9230 and we should also use
> the GenericEntityException. Later we should try to figure out why and when
> system lose the delegator which causes the NPE. Once we would fix the
> reason we should remove the fix added in the EntityUtilProperties class
> method.
>
> New fix proposal:
>
> Change method WidgetWorker.getDelegator with following code;
>
>      public static Delegator getDelegator(Map<String, Object> context) {
>          Delegator delegator = (Delegator) context.get("delegator");
>          if (delegator == null) {
>              delegator = DelegatorFactory.getDelegator(delegatorName);
>          }
>          return delegator;
>      }
>
>
> Please see if this proposal looks fine then we could try and see if it
> works to fix the original issue reported in the ticket.
>
> Thanks!
>
>
> Rishi Solanki
> Sr. Manager, Enterprise Software Development
> HotWax Systems Pvt. Ltd.
> Direct: [phone]
> http://www.hotwaxsystems.com
>
> On Tue, Feb 28, 2017 at 1:49 PM, Jacques Le Roux <
> [hidden email]> wrote:
>
>> Thanks Wei,
>>
>> Please All, refer to OFBIZ-9230 to see how I temporarily handled the issue.
>>
>> I also agree that using GenericEntityException is better than just a
>> permissive Exception
>>
>> But I believe we should rather fix the underlying issue I reported there.
>> So I was hesitant to agree with Wei though it's maybe a better way to get
>> to really fix the issue because it shows it at the UI level and non only in
>> logs.
>>
>> Jacques
>>
>>
>>
>> Le 28/02/2017 à 03:35, Wei Zhang a écrit :
>>
>>> Hi,
>>>
>>> I found there is a try/catch block to catch Exception in
>>> EntityUtilProperties.getSystemPropertyValue(). Which will catche a NPE
>>> (delegator is null) when this method is called in
>>> framework/widget/templates/HtmlFormMacroLibrary.ftl but we only get a
>>> waring
>>> message in the log file.
>>>
>>> So I think we should catch GenericEntityException rather than Exception
>>> here
>>> to expose NPE or other runtime exceptions.
>>>
>>> Thanks,
>>>
>>> Wei
>>>
>>>
>>>
>>> -----
>>> 程序羊
>>> --
>>> View this message in context: http://ofbiz.135035.n4.nabble.
>>> com/Should-not-catch-Exception-in-EntityUtilProperties-getSy
>>> stemPropertyValue-tp4702833.html
>>> Sent from the OFBiz - Dev mailing list archive at Nabble.com.
>>>
>>>
>>>

[Wei Zhang]

I think we need to do 3 things.

1. Catch GenericEntityException in EntityUtilProperties.getSystemPropertyValue()
2. Create a new Delegator if it is null in EntityUtilProperties.getSystemPropertyValue()
3. Find out how to get a delegator instance in framework/widget/templates/HtmlFormMacroLibrary.ftl

[Rishi Solanki]

Thank you Wei, Jacques for your reply.

What I was proposing to change method of WidgetWorker.getDelegator()
method. I see whenever any form render the fields and requires delegator
then it always tries to get delegator from WidgetWorker's mentioned method.
Please refer MacroFormRenederer class as mentioned in the OFBIZ-9230 by
Jacques.

If we fix that method, then I think we should be fine and while rendering
system will always have the delegator and this issue should not appear. In
that case we won't required extra delegaor checks in the
EntityUtilProperties class for this particular issue. In case we requied
this check from other place then we need to make sure delegator always
passed to EntityUtilProperties methods, instead of adding condition in the
EntityUtilProperties methods. Because many methods in that class uses the
delegator as parameter.

Finally we should catch the GenericEntityException instead of Exception. In
a way #1 and #3 needs to be work on and for #2 we should
 let it be as is so that in future system will report if somewhere we are
losing the delegator then we could take care. That means what Jacques did
in his last fix should be fine, simply log delegator is missing on console.

This is what I'm proposing, but I'm okay if we want to fix delegator issue
in the EntityUtilProperties class, if so then I would say to fix it for all
methods which uses delegator as parameter.

Thanks!





Rishi Solanki
Sr. Manager, Enterprise Software Development
HotWax Systems Pvt. Ltd.
Direct: [phone]
http://www.hotwaxsystems.com

On Thu, Mar 2, 2017 at 8:02 AM, Wei Zhang <[hidden email]> wrote:

> I think we need to do 3 things.
>
> 1. Catch GenericEntityException in
> EntityUtilProperties.getSystemPropertyValue()
> 2. Create a new Delegator if it is null in
> EntityUtilProperties.getSystemPropertyValue()
> 3. Find out how to get a delegator instance in
> framework/widget/templates/HtmlFormMacroLibrary.ftl
>
>
>
>
> -----
> 程序羊
> --
> View this message in context: http://ofbiz.135035.n4.nabble.
> com/Should-not-catch-Exception-in-EntityUtilProperties-
> getSystemPropertyValue-tp4702833p4702909.html
> Sent from the OFBiz - Dev mailing list archive at Nabble.com.
>

[Jacques Le Roux]

Thanks Wei,

Right, that's it.
My patch does 1  and 2.
3 is another beast to tame...

Jacques

Le 02/03/2017 à 03:32, Wei Zhang a écrit :

> I think we need to do 3 things.
>
> 1. Catch GenericEntityException in
> EntityUtilProperties.getSystemPropertyValue()
> 2. Create a new Delegator if it is null in
> EntityUtilProperties.getSystemPropertyValue()
> 3. Find out how to get a delegator instance in
> framework/widget/templates/HtmlFormMacroLibrary.ftl
>
>
>
>
> -----
> 程序羊
> --
> View this message in context: http://ofbiz.135035.n4.nabble.com/Should-not-catch-Exception-in-EntityUtilProperties-getSystemPropertyValue-tp4702833p4702909.html
> Sent from the OFBiz - Dev mailing list archive at Nabble.com.
>
>

[Jacques Le Roux]

Le 02/03/2017 à 08:00, Rishi Solanki a écrit :

> Thank you Wei, Jacques for your reply.
>
> What I was proposing to change method of WidgetWorker.getDelegator()
> method. I see whenever any form render the fields and requires delegator
> then it always tries to get delegator from WidgetWorker's mentioned method.
> Please refer MacroFormRenederer class as mentioned in the OFBIZ-9230 by
> Jacques.
>
> If we fix that method, then I think we should be fine and while rendering
> system will always have the delegator and this issue should not appear. In
> that case we won't required extra delegaor checks in the
> EntityUtilProperties class for this particular issue.

Actually no. EntityUtilProperties.getSystemPropertyValueI() I change in my patch is only used by EntityUtilProperties methods (getMessage, 2
getPropertyValue, propertyValueEqualsIgnoreCase)
But those are widely used in OFBiz, not only in the context of widget
> In case we requied this check from other place then we need to make sure delegator always
> passed to EntityUtilProperties methods, instead of adding condition in the
> EntityUtilProperties methods. Because many methods in that class uses the
> delegator as parameter.
The EntityUtilProperties methods which needs a delegator are actually those above because they call getSystemPropertyValue() which needs the
delegator, not for other reasons.
> Finally we should catch the GenericEntityException instead of Exception. In
> a way #1 and #3 needs to be work on and for #2 we should
>   let it be as is so that in future system will report if somewhere we are
> losing the delegator then we could take care. That means what Jacques did
> in his last fix should be fine, simply log delegator is missing on console.

So you propose not to use the "default" delegator but simply log the error. I agree this is better.
But we will face issues in the UI like the one reported by 程序羊 in log (try replacing Exception by GenericEntityException and then get to  
https://localhost:8443/partymgr/control/main)
And I don't see a simple and quick fix for the missing delegator :/
> This is what I'm proposing, but I'm okay if we want to fix delegator issue
> in the EntityUtilProperties class, if so then I would say to fix it for all
> methods which uses delegator as parameter.

If you mean EntityUtilProperties  methods then my workaround is "OK".
Actually there are 1 other places where we use
delegator = DelegatorFactory.getDelegator("default");
checkRhsType()
(also one in tests with delegatorCreationUsingFactoryGetDelegator() that we can neglect)

I attach in the Jira the patch I finally propose and we should continue to discuss there IMO

Thanks

Jacques

>
> Thanks!
>
>
>
>
>
> Rishi Solanki
> Sr. Manager, Enterprise Software Development
> HotWax Systems Pvt. Ltd.
> Direct: [phone]
> http://www.hotwaxsystems.com
>
> On Thu, Mar 2, 2017 at 8:02 AM, Wei Zhang <[hidden email]> wrote:
>
>> I think we need to do 3 things.
>>
>> 1. Catch GenericEntityException in
>> EntityUtilProperties.getSystemPropertyValue()
>> 2. Create a new Delegator if it is null in
>> EntityUtilProperties.getSystemPropertyValue()
>> 3. Find out how to get a delegator instance in
>> framework/widget/templates/HtmlFormMacroLibrary.ftl
>>
>>
>>
>>
>> -----
>> 程序羊
>> --
>> View this message in context: http://ofbiz.135035.n4.nabble.
>> com/Should-not-catch-Exception-in-EntityUtilProperties-
>> getSystemPropertyValue-tp4702833p4702909.html
>> Sent from the OFBiz - Dev mailing list archive at Nabble.com.
>>
>

[Rishi Solanki]

Thanks!


Rishi Solanki
Sr. Manager, Enterprise Software Development
HotWax Systems Pvt. Ltd.
Direct: [phone]
http://www.hotwaxsystems.com

On Thu, Mar 2, 2017 at 3:10 PM, Jacques Le Roux <
[hidden email]> wrote:

>
>
> Le 02/03/2017 à 08:00, Rishi Solanki a écrit :
>
>> Thank you Wei, Jacques for your reply.
>>
>> What I was proposing to change method of WidgetWorker.getDelegator()
>> method. I see whenever any form render the fields and requires delegator
>> then it always tries to get delegator from WidgetWorker's mentioned
>> method.
>> Please refer MacroFormRenederer class as mentioned in the OFBIZ-9230 by
>> Jacques.
>>
>> If we fix that method, then I think we should be fine and while rendering
>> system will always have the delegator and this issue should not appear. In
>> that case we won't required extra delegaor checks in the
>> EntityUtilProperties class for this particular issue.
>>
> Actually no. EntityUtilProperties.getSystemPropertyValueI() I change in
> my patch is only used by EntityUtilProperties methods (getMessage, 2
> getPropertyValue, propertyValueEqualsIgnoreCase)
> But those are widely used in OFBiz, not only in the context of widget
>
>> In case we requied this check from other place then we need to make sure
>> delegator always
>> passed to EntityUtilProperties methods, instead of adding condition in the
>> EntityUtilProperties methods. Because many methods in that class uses the
>> delegator as parameter.
>>
> The EntityUtilProperties methods which needs a delegator are actually
> those above because they call getSystemPropertyValue() which needs the
> delegator, not for other reasons.
>
>> Finally we should catch the GenericEntityException instead of Exception.
>> In
>> a way #1 and #3 needs to be work on and for #2 we should
>>   let it be as is so that in future system will report if somewhere we are
>> losing the delegator then we could take care. That means what Jacques did
>> in his last fix should be fine, simply log delegator is missing on
>> console.
>>
>
> So you propose not to use the "default" delegator but simply log the
> error. I agree this is better.
> But we will face issues in the UI like the one reported by 程序羊 in log (try
> replacing Exception by GenericEntityException and then get to
> https://localhost:8443/partymgr/control/main)
> And I don't see a simple and quick fix for the missing delegator :/
>
>> This is what I'm proposing, but I'm okay if we want to fix delegator issue
>> in the EntityUtilProperties class, if so then I would say to fix it for
>> all
>> methods which uses delegator as parameter.
>>
>
> If you mean EntityUtilProperties  methods then my workaround is "OK".
> Actually there are 1 other places where we use
> delegator = DelegatorFactory.getDelegator("default");
> checkRhsType()
> (also one in tests with delegatorCreationUsingFactoryGetDelegator() that
> we can neglect)
>
> I attach in the Jira the patch I finally propose and we should continue to
> discuss there IMO
>
> Thanks
>
> Jacques
>
>
>
>
>> Thanks!
>>
>>
>>
>>
>>
>> Rishi Solanki
>> Sr. Manager, Enterprise Software Development
>> HotWax Systems Pvt. Ltd.
>> Direct: [phone]
>> http://www.hotwaxsystems.com
>>
>> On Thu, Mar 2, 2017 at 8:02 AM, Wei Zhang <[hidden email]>
>> wrote:
>>
>> I think we need to do 3 things.
>>>
>>> 1. Catch GenericEntityException in
>>> EntityUtilProperties.getSystemPropertyValue()
>>> 2. Create a new Delegator if it is null in
>>> EntityUtilProperties.getSystemPropertyValue()
>>> 3. Find out how to get a delegator instance in
>>> framework/widget/templates/HtmlFormMacroLibrary.ftl
>>>
>>>
>>>
>>>
>>> -----
>>> 程序羊
>>> --
>>> View this message in context: http://ofbiz.135035.n4.nabble.
>>> com/Should-not-catch-Exception-in-EntityUtilProperties-
>>> getSystemPropertyValue-tp4702833p4702909.html
>>> Sent from the OFBiz - Dev mailing list archive at Nabble.com.
>>>
>>>
>>
>