ofbiz-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jacques Le Roux (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (OFBIZ-9230) Missing reference to the delegator in framework/widget/templates/HtmlFormMacroLibrary.ftl
Date Fri, 03 Mar 2017 12:03:45 GMT

    [ https://issues.apache.org/jira/browse/OFBIZ-9230?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15893856#comment-15893856
] 

Jacques Le Roux edited comment on OFBIZ-9230 at 3/3/17 12:03 PM:
-----------------------------------------------------------------

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

{code}
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;
     }
 }
{code}

In the convo, you suggested
{code}
delegator = DelegatorFactory.getDelegator(delegatorName);
{code}
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()
{code}
delegator = DelegatorFactory.getDelegator("default");
{code}
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
{code}
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;
{code}

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!




was (Author: jacques.le.roux):
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

{code}
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;
     }
 }
{code}

In the convo, you suggested
{code}
delegator = DelegatorFactory.getDelegator(delegatorName);
{code}
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()
{code}
delegator = DelegatorFactory.getDelegator("default");
{code}
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
{code}
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
1785155)
+++ framework/entity/src/main/java/org/apache/ofbiz/entity/condition/EntityExpr.java	(working
copy)
@@ -18,11 +18,13 @@
  *******************************************************************************/
 package org.apache.ofbiz.entity.condition;

+import java.io.IOException;
 import java.util.Collection;
 import java.util.List;
 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 +163,7 @@
         visitor.acceptEntityExpr(this);
     }

-    public void checkRhsType(ModelEntity modelEntity, Delegator delegator) {
+    public void checkRhsType(ModelEntity modelEntity, Delegator delegator) throws IOException,
GeneralException {
         if (this.rhs == null || this.rhs == GenericEntity.NULL_FIELD || modelEntity == null)
return;

         Object value = this.rhs;
@@ -179,9 +181,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;
{code}

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!



> Missing reference to the delegator in framework/widget/templates/HtmlFormMacroLibrary.ftl
> -----------------------------------------------------------------------------------------
>
>                 Key: OFBIZ-9230
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-9230
>             Project: OFBiz
>          Issue Type: Bug
>          Components: ALL COMPONENTS
>    Affects Versions: Trunk
>            Reporter: Wei Zhang
>            Assignee: Jacques Le Roux
>            Priority: Minor
>         Attachments: OFBIZ-9230.patch
>
>
> 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
> {quote}
> 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
> {quote}



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

Mime
View raw message