Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-9230

Missing reference to the delegator in framework/widget/templates/HtmlFormMacroLibrary.ftl

    Details

      Description

      To reproduce

      1. Load test data <SystemProperty systemResourceId="widget" systemPropertyId="widget.autocompleter.defaultMinLength" systemPropertyValue="3"/>
      2. Open https://localhost:8443/partymgr/control/main
      3. You will still get warning below in log file

      2017-02-24 12:56:16,348 |http-nio-8443-exec-7 |EntityUtilProperties |I| Could not get a system property for widget.autocompleter.defaultMinLength : null

      1. OFBIZ-9230.patch
        2 kB
        Jacques Le Roux

        Issue Links

          Activity

          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          Thanks Zhang for report,

          The reason is in the context of a FTL macro we lose the delegator (in context) when calling
          executeMacro(writer, sr.toString());
          from
          MacroFormRenderer.renderTextField()
          So we need to find a way to pass the delegator to be used when evaluating the Freemarker template. For now I have simply added a clearer explanation of the reason at r1784259

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited Thanks Zhang for report, The reason is in the context of a FTL macro we lose the delegator (in context) when calling executeMacro(writer, sr.toString()); from MacroFormRenderer.renderTextField() So we need to find a way to pass the delegator to be used when evaluating the Freemarker template. For now I have simply added a clearer explanation of the reason at r1784259
          Hide
          tzngvi Wei Zhang added a comment - - edited

          Hi Jacques,

          I found you commit in revision 1784259. Maybe we should catch GenericEntityException rather than Exception in line 85. If so, you can got a NPE in log file.

          Kind Regards,

          Wei

          Show
          tzngvi Wei Zhang added a comment - - edited Hi Jacques, I found you commit in revision 1784259. Maybe we should catch GenericEntityException rather than Exception in line 85. If so, you can got a NPE in log file. Kind Regards, Wei
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Hi Wei,

          Yes maybe indeed. Since nobody else interacted here I suggest we discuss that with the community on the dev ML. Could you please start the thread?

          Show
          jacques.le.roux Jacques Le Roux added a comment - Hi Wei, Yes maybe indeed. Since nobody else interacted here I suggest we discuss that with the community on the dev ML. Could you please start the thread?
          Show
          tzngvi Wei Zhang added a comment - See http://ofbiz.135035.n4.nabble.com/Should-not-catch-Exception-in-EntityUtilProperties-getSystemPropertyValue-td4702833.html
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I put a proposition in the thread

          Show
          jacques.le.roux Jacques Le Roux added a comment - I put a proposition in the thread
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          This patch is my proposition after we discussed in dev ML: https://s.apache.org/yaqx

          Maybe we should rather show the error at the UI level and fix it, but I don't want to shave the yak alone...

          Show
          jacques.le.roux Jacques Le Roux added a comment - This patch is my proposition after we discussed in dev ML: https://s.apache.org/yaqx Maybe we should rather show the error at the UI level and fix it, but I don't want to shave the yak alone...
          Hide
          rishisolankii Rishi Solanki added a comment -

          Thanks for putting all things together and I'm good with the fix provided. The patch attached will fix the issue reported. Also I like the idea of logging the message that system lost the delegator and now using the default delegator, Thanks!

          ==========================================================================================
          Debug.logError("Could not get delegator using the 'default' delegator", module);
          ==========================================================================================

          Apart from the patch provided, can we consider the WidgetWorker class fix suggested over ML. I think it would cover the #3 point mentioned by Wei, that means systel will have the capability to get the delegator again if some how it drops due to some reason. I don't have very strong recommendations see if we can consider this.

          Show
          rishisolankii Rishi Solanki added a comment - Thanks for putting all things together and I'm good with the fix provided. The patch attached will fix the issue reported. Also I like the idea of logging the message that system lost the delegator and now using the default delegator, Thanks! ========================================================================================== Debug.logError("Could not get delegator using the 'default' delegator", module); ========================================================================================== Apart from the patch provided, can we consider the WidgetWorker class fix suggested over ML. I think it would cover the #3 point mentioned by Wei, that means systel will have the capability to get the delegator again if some how it drops due to some reason. I don't have very strong recommendations see if we can consider this.
          Hide
          rishisolankii Rishi Solanki added a comment -

          Not sure how this assigned to me. Assigning back.

          Show
          rishisolankii Rishi Solanki added a comment - Not sure how this assigned to me. Assigning back.
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          Let's think about the situation in all its aspects (and be ready to dive in ).

          First your proposition for WidgetWorker would be actually something like this

          Index: framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java
          ===================================================================
          --- framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java	(revision 1785155)
          +++ framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java	(working copy)
          @@ -35,6 +35,7 @@
           import org.apache.ofbiz.base.util.UtilHttp;
           import org.apache.ofbiz.base.util.UtilValidate;
           import org.apache.ofbiz.entity.Delegator;
          +import org.apache.ofbiz.entity.DelegatorFactory;
           import org.apache.ofbiz.service.LocalDispatcher;
           import org.apache.ofbiz.webapp.control.ConfigXMLReader;
           import org.apache.ofbiz.webapp.control.RequestHandler;
          @@ -410,6 +411,16 @@
          
               public static Delegator getDelegator(Map<String, Object> context) {
                   Delegator delegator = (Delegator) context.get("delegator");
          +        if (delegator == null) {
          +            // this will be the common case for now as the delegator isn't available where we want to do this
          +            // we'll cheat a little here and assume the default delegator
          +            Debug.logError("Could not get a delegator using the 'default' delegator", module);
          +            delegator = DelegatorFactory.getDelegator("default");
          +            if (delegator == null) {
          +                Debug.logError("Could not get a delegator even trying with the 'default' delegator", module);
          +                return null;
          +            }
          +        }
                   return delegator;
               }
           }
          

          In the convo, you suggested

          delegator = DelegatorFactory.getDelegator(delegatorName);
          

          I asked you what should be in delegatorName, but you did not answer and that's the crux of the problem. We will see that later
          I suggested to use in EntityUtilProperties.getSystemPropertyValue()

          delegator = DelegatorFactory.getDelegator("default");
          

          because it fixes the problem at hand while the patch above for getDelegator() does not (simply try it w/o the getSystemPropertyValue() patch).

          So far so good, we saw that we have the same kind of think in checkRhsType(), which should then actually be patched with

          Index: framework/entity/src/main/java/org/apache/ofbiz/entity/condition/EntityExpr.java
          ===================================================================
          --- framework/entity/src/main/java/org/apache/ofbiz/entity/condition/EntityExpr.java	(revision 1785276)
          +++ framework/entity/src/main/java/org/apache/ofbiz/entity/condition/EntityExpr.java	(working copy)
          @@ -23,6 +23,7 @@
           import java.util.Map;
          
           import org.apache.ofbiz.base.util.Debug;
          +import org.apache.ofbiz.base.util.GeneralException;
           import org.apache.ofbiz.base.util.ObjectType;
           import org.apache.ofbiz.base.util.UtilGenerics;
           import org.apache.ofbiz.entity.Delegator;
          @@ -161,7 +162,7 @@
                   visitor.acceptEntityExpr(this);
               }
          
          -    public void checkRhsType(ModelEntity modelEntity, Delegator delegator) {
          +    public void checkRhsType(ModelEntity modelEntity, Delegator delegator) throws GeneralException {
                   if (this.rhs == null || this.rhs == GenericEntity.NULL_FIELD || modelEntity == null) return;
          
                   Object value = this.rhs;
          @@ -179,9 +180,13 @@
                   }
          
                   if (delegator == null) {
          -            // this will be the common case for now as the delegator isn't available where we want to do this
          -            // we'll cheat a little here and assume the default delegator
                       delegator = DelegatorFactory.getDelegator("default");
          +            Debug.logError("Could not get a delegator using the 'default' delegator", module);
          +            if (delegator == null) {
          +                Debug.logError("Could not get a delegator even trying with the 'default' delegator", module);
          +                GeneralException exception = new GeneralException("Could not get a delegator even trying with the 'default' delegator");
          +                throw exception;
          +            }
                   }
          
                   String fieldName = null;
          

          And now I get to the reason for this analysis: multitenant! My patch for getSystemPropertyValue() works in a simple tenant context. But in a multitenant context it will always refer to the "default" tenant, which can be a problem. Same for the other patches above including the checkRhsType() one. BTW the change introduced there is prior (2008) to multitenant insertion (2010), so is also an issue. Now if you look at OFBIZ-6884, which broke the current issue, it's motivated by multitenant, and EntityUtilProperties is mostly useful in a multitenant context, see the problem!

          So to summarise:

          1. My getSystemPropertyValue() patch fixes the issue Wei reported, but is just a workaround, and is not enough and even wrong in a multitenant context.
          2. Same for my proposition for checkRhsType(). At first glance it's slightly better than what we have now because it's throws an error in log in case 'default' is not there. But it's a rare case in a simple tenant context (not a problem OOTB, works since 2008, but could be a problem with custom changes in the EntityEngine...). Anyway using default defeat the purpose of multitenant and is an issue by itself.
          3. Your proposition for getDelegator() is not related with the problem at hand. I'm not sure it helps in other contexts. So I'd refrain to commit it since it inserts a problem in a multitenant context.

          So all those solutions are only working in a simple tenant context. I believe we should refrain to commit any and look for a real solution, ie why the delegator misses here.
          Now there are 2 temporary solutions:

          • The one I committed already, which hide the problem in UI and only shows it in log
          • Reverting my commit and replaces the Exception handling by a GenericEntityException but then the error will show in the UI. Try it, not sure it's better!

          So I suggest we let things as is and close is as unresolved. And create 2 new Jira issues:

          1. One for checkRhsType() which is wrong in a multitenant context.
          2. Another to properly fixes the issue at hand

          In both we can refer to the analysis here to explain what we should do. I'm really yet unsure about what to do for checkRhsType() :/

          To put is simply, even if there are easy workarounds in a simple tenant context, this is a big worry for the multitenant feature which heavilyu relies on EntityUtilProperties.getSystemPropertyValueI(). To a point that I wonder if there are no other issues which makes the multitenant feature so fragile that it either needs to be totally fixed or removed!

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited Let's think about the situation in all its aspects (and be ready to dive in ). First your proposition for WidgetWorker would be actually something like this Index: framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java =================================================================== --- framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java (revision 1785155) +++ framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java (working copy) @@ -35,6 +35,7 @@ import org.apache.ofbiz.base.util.UtilHttp; import org.apache.ofbiz.base.util.UtilValidate; import org.apache.ofbiz.entity.Delegator; + import org.apache.ofbiz.entity.DelegatorFactory; import org.apache.ofbiz.service.LocalDispatcher; import org.apache.ofbiz.webapp.control.ConfigXMLReader; import org.apache.ofbiz.webapp.control.RequestHandler; @@ -410,6 +411,16 @@ public static Delegator getDelegator(Map< String , Object > context) { Delegator delegator = (Delegator) context.get( "delegator" ); + if (delegator == null ) { + // this will be the common case for now as the delegator isn't available where we want to do this + // we'll cheat a little here and assume the default delegator + Debug.logError( "Could not get a delegator using the ' default ' delegator" , module); + delegator = DelegatorFactory.getDelegator( " default " ); + if (delegator == null ) { + Debug.logError( "Could not get a delegator even trying with the ' default ' delegator" , module); + return null ; + } + } return delegator; } } In the convo, you suggested delegator = DelegatorFactory.getDelegator(delegatorName); I asked you what should be in delegatorName, but you did not answer and that's the crux of the problem. We will see that later I suggested to use in EntityUtilProperties.getSystemPropertyValue() delegator = DelegatorFactory.getDelegator( " default " ); because it fixes the problem at hand while the patch above for getDelegator() does not (simply try it w/o the getSystemPropertyValue() patch). So far so good, we saw that we have the same kind of think in checkRhsType(), which should then actually be patched with Index: framework/entity/src/main/java/org/apache/ofbiz/entity/condition/EntityExpr.java =================================================================== --- framework/entity/src/main/java/org/apache/ofbiz/entity/condition/EntityExpr.java (revision 1785276) +++ framework/entity/src/main/java/org/apache/ofbiz/entity/condition/EntityExpr.java (working copy) @@ -23,6 +23,7 @@ import java.util.Map; import org.apache.ofbiz.base.util.Debug; + import org.apache.ofbiz.base.util.GeneralException; import org.apache.ofbiz.base.util.ObjectType; import org.apache.ofbiz.base.util.UtilGenerics; import org.apache.ofbiz.entity.Delegator; @@ -161,7 +162,7 @@ visitor.acceptEntityExpr( this ); } - public void checkRhsType(ModelEntity modelEntity, Delegator delegator) { + public void checkRhsType(ModelEntity modelEntity, Delegator delegator) throws GeneralException { if ( this .rhs == null || this .rhs == GenericEntity.NULL_FIELD || modelEntity == null ) return ; Object value = this .rhs; @@ -179,9 +180,13 @@ } if (delegator == null ) { - // this will be the common case for now as the delegator isn't available where we want to do this - // we'll cheat a little here and assume the default delegator delegator = DelegatorFactory.getDelegator( " default " ); + Debug.logError( "Could not get a delegator using the ' default ' delegator" , module); + if (delegator == null ) { + Debug.logError( "Could not get a delegator even trying with the ' default ' delegator" , module); + GeneralException exception = new GeneralException( "Could not get a delegator even trying with the ' default ' delegator" ); + throw exception; + } } String fieldName = null ; And now I get to the reason for this analysis: multitenant ! My patch for getSystemPropertyValue() works in a simple tenant context. But in a multitenant context it will always refer to the "default" tenant, which can be a problem. Same for the other patches above including the checkRhsType() one. BTW the change introduced there is prior (2008) to multitenant insertion (2010), so is also an issue. Now if you look at OFBIZ-6884 , which broke the current issue, it's motivated by multitenant, and EntityUtilProperties is mostly useful in a multitenant context, see the problem! So to summarise: My getSystemPropertyValue() patch fixes the issue Wei reported, but is just a workaround, and is not enough and even wrong in a multitenant context. Same for my proposition for checkRhsType(). At first glance it's slightly better than what we have now because it's throws an error in log in case 'default' is not there. But it's a rare case in a simple tenant context (not a problem OOTB, works since 2008, but could be a problem with custom changes in the EntityEngine...). Anyway using default defeat the purpose of multitenant and is an issue by itself. Your proposition for getDelegator() is not related with the problem at hand. I'm not sure it helps in other contexts. So I'd refrain to commit it since it inserts a problem in a multitenant context. So all those solutions are only working in a simple tenant context. I believe we should refrain to commit any and look for a real solution, ie why the delegator misses here. Now there are 2 temporary solutions: The one I committed already, which hide the problem in UI and only shows it in log Reverting my commit and replaces the Exception handling by a GenericEntityException but then the error will show in the UI. Try it, not sure it's better! So I suggest we let things as is and close is as unresolved. And create 2 new Jira issues: One for checkRhsType() which is wrong in a multitenant context. Another to properly fixes the issue at hand In both we can refer to the analysis here to explain what we should do. I'm really yet unsure about what to do for checkRhsType() :/ To put is simply, even if there are easy workarounds in a simple tenant context, this is a big worry for the multitenant feature which heavilyu relies on EntityUtilProperties.getSystemPropertyValueI(). To a point that I wonder if there are no other issues which makes the multitenant feature so fragile that it either needs to be totally fixed or removed!
          Hide
          pfm.smits Pierre Smits added a comment -

          I am a little confused: don't we have the delegator in the context?

          Show
          pfm.smits Pierre Smits added a comment - I am a little confused: don't we have the delegator in the context?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Pierre,
          No, and that is the problem

          Show
          jacques.le.roux Jacques Le Roux added a comment - Pierre, No, and that is the problem
          Hide
          rishisolankii Rishi Solanki added a comment -

          Jacques Le Roux: Thanks for the details, it helps me to understand the exact problem and what/why you are trying to achieve. I really need the details and thank you for that. And sorry for not digging into it before your last detailed comment.

          I think (also checked in the code) that we will always have the delegator name in the session which actually point to the current tenant (refer ControlServlet.java line160 and LoginWorker.java line530). I guess in the template/FTL context or in the screen context whenever we get the delegator as null we can use this delegatorName first from session and if it is empty then we could try to use default delegator.
          IMO, this should fix the problem if in the context we don't have the delegator but we have the delegatroName in the session.

          Note: I couldn't tried this, but I think this should work and should also fix the problem.

          Show
          rishisolankii Rishi Solanki added a comment - Jacques Le Roux : Thanks for the details, it helps me to understand the exact problem and what/why you are trying to achieve. I really need the details and thank you for that. And sorry for not digging into it before your last detailed comment. I think (also checked in the code) that we will always have the delegator name in the session which actually point to the current tenant (refer ControlServlet.java line160 and LoginWorker.java line530). I guess in the template/FTL context or in the screen context whenever we get the delegator as null we can use this delegatorName first from session and if it is empty then we could try to use default delegator. IMO, this should fix the problem if in the context we don't have the delegator but we have the delegatroName in the session. Note: I couldn't tried this, but I think this should work and should also fix the problem.
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          Hi Rishi,

          I think (also checked in the code) that we will always have the delegator name in the session which actually point to the current tenant (refer ControlServlet.java line160 and LoginWorker.java line530).

          Actually we also always have the delegator in the context.

          I guess in the template/FTL context or in the screen context whenever we get the delegator as null we can use this delegatorName first from session and if it is empty then we could try to use default delegator. IMO, this should fix the problem if in the context we don't have the delegator but we have the delegatroName in the session.

          Actually in the template/FTL context or in the screen context when we have the delegatorName we have also the delegator. The problem we face here is when executing a macro in the widget form the context and session are always null. But I could use your idea of the delegatorName and decided to pass it to the macro. I had though to create an EntityUtilProperties.getPropertyValueFromDelegatorName() method to eventually pass the delegatorName. Two macros were concerned renderLookupField (reported here) and renderTextField.

          It's fixed in trunk r1785882
          If nobody beats me on it, I'll backport later...

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited Hi Rishi, I think (also checked in the code) that we will always have the delegator name in the session which actually point to the current tenant (refer ControlServlet.java line160 and LoginWorker.java line530). Actually we also always have the delegator in the context. I guess in the template/FTL context or in the screen context whenever we get the delegator as null we can use this delegatorName first from session and if it is empty then we could try to use default delegator. IMO, this should fix the problem if in the context we don't have the delegator but we have the delegatroName in the session. Actually in the template/FTL context or in the screen context when we have the delegatorName we have also the delegator. The problem we face here is when executing a macro in the widget form the context and session are always null. But I could use your idea of the delegatorName and decided to pass it to the macro. I had though to create an EntityUtilProperties.getPropertyValueFromDelegatorName() method to eventually pass the delegatorName. Two macros were concerned renderLookupField (reported here) and renderTextField. It's fixed in trunk r1785882 If nobody beats me on it, I'll backport later...
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Completed for other macro types at revision: 1785925
          Backported both in
          R16.11 r1785926

          Show
          jacques.le.roux Jacques Le Roux added a comment - Completed for other macro types at revision: 1785925 Backported both in R16.11 r1785926

            People

            • Assignee:
              jacques.le.roux Jacques Le Roux
              Reporter:
              tzngvi Wei Zhang
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development