You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Mathieu Lirzin <ma...@nereide.fr> on 2018/07/18 15:53:41 UTC
‘MapStack’ weirdness.
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
Re: ‘MapStack’ weirdness.
Posted by Taher Alkhateeb <sl...@gmail.com>.
+1 for the patch, simple and clean.
On Sat, Jul 21, 2018 at 6:58 PM, Mathieu Lirzin
<ma...@nereide.fr> wrote:
> Taher Alkhateeb <sl...@gmail.com> 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
>
Re: ‘MapStack’ weirdness.
Posted by Mathieu Lirzin <ma...@nereide.fr>.
Taher Alkhateeb <sl...@gmail.com> 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.
Re: ‘MapStack’ weirdness.
Posted by Taher Alkhateeb <sl...@gmail.com>.
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 <sc...@hotwaxsystems.com>
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 <ma...@nereide.fr>
> wrote:
>
> > Hello Scott,
> >
> > Scott Gray <sc...@hotwaxsystems.com> 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
> >
>
Re: ‘MapStack’ weirdness.
Posted by Scott Gray <sc...@hotwaxsystems.com>.
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 <ma...@nereide.fr> wrote:
> Hello Scott,
>
> Scott Gray <sc...@hotwaxsystems.com> 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
>
Re: ‘MapStack’ weirdness.
Posted by Mathieu Lirzin <ma...@nereide.fr>.
Hello Scott,
Scott Gray <sc...@hotwaxsystems.com> 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
Re: ‘MapStack’ weirdness.
Posted by Scott Gray <sc...@hotwaxsystems.com>.
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, <sl...@gmail.com>
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
> <ma...@nereide.fr> 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
>
Re: ‘MapStack’ weirdness.
Posted by Taher Alkhateeb <sl...@gmail.com>.
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
<ma...@nereide.fr> 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