‘MapStack’ weirdness.

[Mathieu Lirzin]

Hello,

‘MapStack’ is a specialization of the ‘MapContext’ class which is an
implementation of ‘Map’ interface that has the particularity to store
its key-values in separate sub-maps that can mask each others.

The whole point of ‘MapStack’ implementation is to override the Map
accessors to ensure that the key "context" is associated with ‘this’
meaning the ‘MapStack’ instance.

--8<---------------cut here---------------start------------->8---
    @Override
    public Object get(Object key) {
        if ("context".equals(key)) {
            return this;
        }
        return super.get(key);
    }

    @Override
    public Object get(String name, Locale locale) {
        if ("context".equals(name)) {
            return this;
        }
        return super.get(name, locale);
    }

    @Override
    public Object put(K key, Object value) {
        if ("context".equals(key)) {
            if (value == null || this != value) {
                Debug.logWarning("Putting a value in a MapStack with key [context] that is not this MapStack, will be hidden by the current MapStack self-reference: " + value, module);
            }
        }
        return super.put(key, value);
    }
--8<---------------cut here---------------end--------------->8---

I don't understand the purpose of such thing.  So if someone has some
rationale to share, I would be grateful.

Thanks.

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[taher]

I didn't get into it before, but my first gut reaction is .. delete
that thing! :)

Anyway, it seems to have to many references, so I would advise
investigating how it is constructed to understand what is being pulled
in. Just begin from the container loader and work your way down with a
debugger until you hit exactly the reason why this check was in place.

I find this code a bit ugly, and I'd rather get rid of it if doesn't
add any value (and I think it doesn't)

On Wed, Jul 18, 2018 at 6:53 PM, Mathieu Lirzin
<[hidden email]> wrote:

> Hello,
>
> ‘MapStack’ is a specialization of the ‘MapContext’ class which is an
> implementation of ‘Map’ interface that has the particularity to store
> its key-values in separate sub-maps that can mask each others.
>
> The whole point of ‘MapStack’ implementation is to override the Map
> accessors to ensure that the key "context" is associated with ‘this’
> meaning the ‘MapStack’ instance.
>
> --8<---------------cut here---------------start------------->8---
>     @Override
>     public Object get(Object key) {
>         if ("context".equals(key)) {
>             return this;
>         }
>         return super.get(key);
>     }
>
>     @Override
>     public Object get(String name, Locale locale) {
>         if ("context".equals(name)) {
>             return this;
>         }
>         return super.get(name, locale);
>     }
>
>     @Override
>     public Object put(K key, Object value) {
>         if ("context".equals(key)) {
>             if (value == null || this != value) {
>                 Debug.logWarning("Putting a value in a MapStack with key [context] that is not this MapStack, will be hidden by the current MapStack self-reference: " + value, module);
>             }
>         }
>         return super.put(key, value);
>     }
> --8<---------------cut here---------------end--------------->8---
>
> I don't understand the purpose of such thing.  So if someone has some
> rationale to share, I would be grateful.
>
> Thanks.
>
> --
> Mathieu Lirzin
> GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Scott Gray-3]

I think it relates to the "context" variable that is frequently used in
groovy data prep scripts for the script output.

I'm not in front of a computer, but looking at it in that light may help.

Regards
Scott

On Thu, 19 Jul 2018, 09:24 Taher Alkhateeb, <[hidden email]>
wrote:

> I didn't get into it before, but my first gut reaction is .. delete
> that thing! :)
>
> Anyway, it seems to have to many references, so I would advise
> investigating how it is constructed to understand what is being pulled
> in. Just begin from the container loader and work your way down with a
> debugger until you hit exactly the reason why this check was in place.
>
> I find this code a bit ugly, and I'd rather get rid of it if doesn't
> add any value (and I think it doesn't)
>
> On Wed, Jul 18, 2018 at 6:53 PM, Mathieu Lirzin
> <[hidden email]> wrote:
> > Hello,
> >
> > ‘MapStack’ is a specialization of the ‘MapContext’ class which is an
> > implementation of ‘Map’ interface that has the particularity to store
> > its key-values in separate sub-maps that can mask each others.
> >
> > The whole point of ‘MapStack’ implementation is to override the Map
> > accessors to ensure that the key "context" is associated with ‘this’
> > meaning the ‘MapStack’ instance.
> >
> > --8<---------------cut here---------------start------------->8---
> >     @Override
> >     public Object get(Object key) {
> >         if ("context".equals(key)) {
> >             return this;
> >         }
> >         return super.get(key);
> >     }
> >
> >     @Override
> >     public Object get(String name, Locale locale) {
> >         if ("context".equals(name)) {
> >             return this;
> >         }
> >         return super.get(name, locale);
> >     }
> >
> >     @Override
> >     public Object put(K key, Object value) {
> >         if ("context".equals(key)) {
> >             if (value == null || this != value) {
> >                 Debug.logWarning("Putting a value in a MapStack with key
> [context] that is not this MapStack, will be hidden by the current MapStack
> self-reference: " + value, module);
> >             }
> >         }
> >         return super.put(key, value);
> >     }
> > --8<---------------cut here---------------end--------------->8---
> >
> > I don't understand the purpose of such thing.  So if someone has some
> > rationale to share, I would be grateful.
> >
> > Thanks.
> >
> > --
> > Mathieu Lirzin
> > GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37
>

[Mathieu Lirzin]

Taher Alkhateeb <[hidden email]> writes:

> I didn't get into it before, but my first gut reaction is .. delete
> that thing! :)

:-)

> Anyway, it seems to have to many references, so I would advise
> investigating how it is constructed to understand what is being pulled
> in. Just begin from the container loader and work your way down with a
> debugger until you hit exactly the reason why this check was in place.

I have put a breakpoint inside the if branch of the overidden ‘get’
method and started to randomly navigate OFBiz.  I have found some
callers of this “feature” which can be triggered for example via the
<https://localhost:8443/facility/control/EditFacility?facilityId=WebStoreWarehouse>
URI.

The reason is that some Freemarker template files are referencing the
‘context’ map explicitly.  I have no experience writing “.ftl” files so
I am not sure if this is a good thing or not.  It seems required since
my attempts to replace those ‘context’ references by globals have
failed.

As shown by next patch, I would like to replace the hardcoded special
case in ‘MapStack’ by explicitly adding the ‘context’ Map in the
FreeMarker model at rendering time.


diff --git framework/base/src/main/java/org/apache/ofbiz/base/util/collections/MapStack.java framework/base/src/main/java/org/apache/ofbiz/base/util/collections/MapStack.java
index 547e9942a3..347d6cbd3e 100644
--- framework/base/src/main/java/org/apache/ofbiz/base/util/collections/MapStack.java
+++ framework/base/src/main/java/org/apache/ofbiz/base/util/collections/MapStack.java
@@ -84,42 +84,4 @@ public class MapStack<K> extends MapContext<K, Object> {
         standAloneChild.push();
         return standAloneChild;
     }
-
-    /* (non-Javadoc)
-     * @see java.util.Map#get(java.lang.Object)
-     */
-    @Override
-    public Object get(Object key) {
-        if ("context".equals(key)) {
-            return this;
-        }
-
-        return super.get(key);
-    }
-
-    /* (non-Javadoc)
-     * @see org.apache.ofbiz.base.util.collections.LocalizedMap#get(java.lang.String, java.util.Locale)
-     */
-    @Override
-    public Object get(String name, Locale locale) {
-        if ("context".equals(name)) {
-            return this;
-        }
-
-        return super.get(name, locale);
-    }
-
-    /* (non-Javadoc)
-     * @see java.util.Map#put(java.lang.Object, java.lang.Object)
-     */
-    @Override
-    public Object put(K key, Object value) {
-        if ("context".equals(key)) {
-            if (value == null || this != value) {
-                Debug.logWarning("Putting a value in a MapStack with key [context] that is not this MapStack, will be hidden by the current MapStack self-reference: " + value, module);
-            }
-        }
-
-        return super.put(key, value);
-    }
 }
diff --git framework/base/src/main/java/org/apache/ofbiz/base/util/template/FreeMarkerWorker.java framework/base/src/main/java/org/apache/ofbiz/base/util/template/FreeMarkerWorker.java
index 129bcaf799..1f53fd9fba 100644
--- framework/base/src/main/java/org/apache/ofbiz/base/util/template/FreeMarkerWorker.java
+++ framework/base/src/main/java/org/apache/ofbiz/base/util/template/FreeMarkerWorker.java
@@ -199,6 +199,8 @@ public final class FreeMarkerWorker {
     public static Environment renderTemplate(Template template, Map<String, Object> context, Appendable outWriter) throws TemplateException, IOException {
         // make sure there is no "null" string in there as FreeMarker will try to use it
         context.remove("null");
+        // Allow the template to refer to the context explicitly.
+        context.put("context", context);
         // Since the template cache keeps a single instance of a Template that is shared among users,
         // and since that Template instance is immutable, we need to create an Environment instance and
         // use it to process the template with the user's settings.


WDYT?

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Mathieu Lirzin]

Hello Scott,

Scott Gray <[hidden email]> writes:

> I think it relates to the "context" variable that is frequently used in
> groovy data prep scripts for the script output.
>
> I'm not in front of a computer, but looking at it in that light may help.

As shown by this snippet from ‘GroovyUtil.java’, the “context” binding
is added explicitly so it doesn't require the ‘MapStack’ special case
for the "context" key.  In fact since the context is copied to an
‘HashMap’ before being passed to Groovy, the ‘MapStack’ implementation
is not used at all from Groovy.

--8<---------------cut here---------------start------------->8---
    public static Binding getBinding(Map<String, Object> context, String expression) {
        Map<String, Object> vars = new HashMap<>();
        if (context != null) {
            vars.putAll(context);
            if (UtilValidate.isNotEmpty(expression)) {
              ...;
            }
            vars.put("context", context);
            ...;
        }
        return new Binding(vars);
    }

    public static Object runScriptAtLocation(String location, String methodName, Map<String, Object> context) throws GeneralException {
        Script script = InvokerHelper.createScript(getScriptClassFromLocation(location), getBinding(context));
        Object result = null;
        if (UtilValidate.isEmpty(methodName)) {
            result = script.run();
        } else {
            result = script.invokeMethod(methodName, new Object[] { context });
        }
        return result;
    }
--8<---------------cut here---------------end--------------->8---

What is nice is that the ‘getBinding’ method has a javadoc explaining
why the “context” binding is added explicitly.  Thanks to Adrian Crum
(RIP) for providing it.

--8<---------------cut here---------------start------------->8---
The ‘context’ Map is added to the ‘Binding’ as a variable called
"context" so that variables can be passed back to the caller.
--8<---------------cut here---------------end--------------->8---

Thanks for your input.

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[taher]

+1 for the patch, simple and clean.

On Sat, Jul 21, 2018 at 6:58 PM, Mathieu Lirzin
<[hidden email]> wrote:

> Taher Alkhateeb <[hidden email]> writes:
>
>> I didn't get into it before, but my first gut reaction is .. delete
>> that thing! :)
>
> :-)
>
>> Anyway, it seems to have to many references, so I would advise
>> investigating how it is constructed to understand what is being pulled
>> in. Just begin from the container loader and work your way down with a
>> debugger until you hit exactly the reason why this check was in place.
>
> I have put a breakpoint inside the if branch of the overidden ‘get’
> method and started to randomly navigate OFBiz.  I have found some
> callers of this “feature” which can be triggered for example via the
> <https://localhost:8443/facility/control/EditFacility?facilityId=WebStoreWarehouse>
> URI.
>
> The reason is that some Freemarker template files are referencing the
> ‘context’ map explicitly.  I have no experience writing “.ftl” files so
> I am not sure if this is a good thing or not.  It seems required since
> my attempts to replace those ‘context’ references by globals have
> failed.
>
> As shown by next patch, I would like to replace the hardcoded special
> case in ‘MapStack’ by explicitly adding the ‘context’ Map in the
> FreeMarker model at rendering time.
>
>
>
> WDYT?
>
> --
> Mathieu Lirzin
> GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37
>

[Scott Gray-3]

I looked back through an old copy of the pre-ASF repo and found that the
"context" references were added in 2006 and it had something to do with
making MapStack a LocalizedMap for screen/form widget internalization
rendering.  The commit message and a small explanation in the mailing lists
didn't explain that specific addition unfortunately.  Could be that it
isn't used there anymore, at the time freemarker macros weren't used for
widget rendering, so it's possible that a fix in the freemarker render
could remove the need for it.

Regards
Scott

On 21 July 2018 at 17:12, Mathieu Lirzin <[hidden email]> wrote:

> Hello Scott,
>
> Scott Gray <[hidden email]> writes:
>
> > I think it relates to the "context" variable that is frequently used in
> > groovy data prep scripts for the script output.
> >
> > I'm not in front of a computer, but looking at it in that light may help.
>
> As shown by this snippet from ‘GroovyUtil.java’, the “context” binding
> is added explicitly so it doesn't require the ‘MapStack’ special case
> for the "context" key.  In fact since the context is copied to an
> ‘HashMap’ before being passed to Groovy, the ‘MapStack’ implementation
> is not used at all from Groovy.
>
> --8<---------------cut here---------------start------------->8---
>     public static Binding getBinding(Map<String, Object> context, String
> expression) {
>         Map<String, Object> vars = new HashMap<>();
>         if (context != null) {
>             vars.putAll(context);
>             if (UtilValidate.isNotEmpty(expression)) {
>               ...;
>             }
>             vars.put("context", context);
>             ...;
>         }
>         return new Binding(vars);
>     }
>
>     public static Object runScriptAtLocation(String location, String
> methodName, Map<String, Object> context) throws GeneralException {
>         Script script = InvokerHelper.createScript(
> getScriptClassFromLocation(location), getBinding(context));
>         Object result = null;
>         if (UtilValidate.isEmpty(methodName)) {
>             result = script.run();
>         } else {
>             result = script.invokeMethod(methodName, new Object[] {
> context });
>         }
>         return result;
>     }
> --8<---------------cut here---------------end--------------->8---
>
> What is nice is that the ‘getBinding’ method has a javadoc explaining
> why the “context” binding is added explicitly.  Thanks to Adrian Crum
> (RIP) for providing it.
>
> --8<---------------cut here---------------start------------->8---
> The ‘context’ Map is added to the ‘Binding’ as a variable called
> "context" so that variables can be passed back to the caller.
> --8<---------------cut here---------------end--------------->8---
>
> Thanks for your input.
>
> --
> Mathieu Lirzin
> GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37
>

[taher]

Wow great information digging Scott. Maybe we have many other bits of
redundant code that are just sitting out there for historical reasons.

Anyway I think this settles the mystery and perhaps we should work on
refactoring the code to attempt to get rid of it?

On Fri, Jul 27, 2018, 12:49 AM Scott Gray <[hidden email]>
wrote:

> I looked back through an old copy of the pre-ASF repo and found that the
> "context" references were added in 2006 and it had something to do with
> making MapStack a LocalizedMap for screen/form widget internalization
> rendering.  The commit message and a small explanation in the mailing lists
> didn't explain that specific addition unfortunately.  Could be that it
> isn't used there anymore, at the time freemarker macros weren't used for
> widget rendering, so it's possible that a fix in the freemarker render
> could remove the need for it.
>
> Regards
> Scott
>
> On 21 July 2018 at 17:12, Mathieu Lirzin <[hidden email]>
> wrote:
>
> > Hello Scott,
> >
> > Scott Gray <[hidden email]> writes:
> >
> > > I think it relates to the "context" variable that is frequently used in
> > > groovy data prep scripts for the script output.
> > >
> > > I'm not in front of a computer, but looking at it in that light may
> help.
> >
> > As shown by this snippet from ‘GroovyUtil.java’, the “context” binding
> > is added explicitly so it doesn't require the ‘MapStack’ special case
> > for the "context" key.  In fact since the context is copied to an
> > ‘HashMap’ before being passed to Groovy, the ‘MapStack’ implementation
> > is not used at all from Groovy.
> >
> > --8<---------------cut here---------------start------------->8---
> >     public static Binding getBinding(Map<String, Object> context, String
> > expression) {
> >         Map<String, Object> vars = new HashMap<>();
> >         if (context != null) {
> >             vars.putAll(context);
> >             if (UtilValidate.isNotEmpty(expression)) {
> >               ...;
> >             }
> >             vars.put("context", context);
> >             ...;
> >         }
> >         return new Binding(vars);
> >     }
> >
> >     public static Object runScriptAtLocation(String location, String
> > methodName, Map<String, Object> context) throws GeneralException {
> >         Script script = InvokerHelper.createScript(
> > getScriptClassFromLocation(location), getBinding(context));
> >         Object result = null;
> >         if (UtilValidate.isEmpty(methodName)) {
> >             result = script.run();
> >         } else {
> >             result = script.invokeMethod(methodName, new Object[] {
> > context });
> >         }
> >         return result;
> >     }
> > --8<---------------cut here---------------end--------------->8---
> >
> > What is nice is that the ‘getBinding’ method has a javadoc explaining
> > why the “context” binding is added explicitly.  Thanks to Adrian Crum
> > (RIP) for providing it.
> >
> > --8<---------------cut here---------------start------------->8---
> > The ‘context’ Map is added to the ‘Binding’ as a variable called
> > "context" so that variables can be passed back to the caller.
> > --8<---------------cut here---------------end--------------->8---
> >
> > Thanks for your input.
> >
> > --
> > Mathieu Lirzin
> > GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37
> >
>