You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Adam Heath <do...@brainfood.com> on 2010/01/31 04:30:14 UTC
Re: svn commit: r904962 - in /ofbiz/trunk/framework: base/src/org/ofbiz/base/util/collections/FlexibleMapAccessor.java
base/src/org/ofbiz/base/util/string/UelUtil.java minilang/src/org/ofbiz/minilang/method/ContextAccessor.java
adrianc@apache.org wrote:
> Author: adrianc
> Date: Sun Jan 31 03:24:58 2010
> New Revision: 904962
>
> URL: http://svn.apache.org/viewvc?rev=904962&view=rev
> Log:
> Based on advice from Adam Heath, made FlexibleMapAccessor and UelUtil methods syntactically correct. Methods that read Maps take read-only Map arguments, and methods that write Maps take writable Map arguments.
>
>
> Modified:
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/collections/FlexibleMapAccessor.java
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java
> ofbiz/trunk/framework/minilang/src/org/ofbiz/minilang/method/ContextAccessor.java
>
> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/collections/FlexibleMapAccessor.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/collections/FlexibleMapAccessor.java?rev=904962&r1=904961&r2=904962&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/collections/FlexibleMapAccessor.java (original)
> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/collections/FlexibleMapAccessor.java Sun Jan 31 03:24:58 2010
> @@ -170,7 +170,7 @@
> * @param base the Map to remove from
> * @return the object removed
> */
> - public T remove(Map<String, ? extends Object> base) {
> + public T remove(Map<String, Object> base) {
Heh, not quite correct. You are allowed to remove items from a map
with ? extends.
If you recall, ? extends means that the exact class of the value is
unknown. Since you don't know what the concrete class is, it's not
possible to store any new values into the map, as you might break the
generics contract.
However, removing an item does not break the generics contract. So,
this change is not correct.
> if (this.isEmpty()) {
> return null;
> }
> @@ -179,8 +179,7 @@
> return null;
> }
> try {
> - Map<String, Object> writableMap = UtilGenerics.cast(base);
> - UelUtil.removeValue(writableMap, getExpression(base));
> + UelUtil.removeValue(base, getExpression(base));
> } catch (Exception e) {
> Debug.logError("UEL exception while removing value: " + e + ", original = " + this.original, module);
> }
>
> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java?rev=904962&r1=904961&r2=904962&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java (original)
> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java Sun Jan 31 03:24:58 2010
> @@ -82,7 +82,7 @@
> */
> @SuppressWarnings("unchecked")
> public static Object evaluate(Map<String, ? extends Object> context, String expression, Class expectedType) {
> - ELContext elContext = new BasicContext(context);
> + ELContext elContext = new ReadOnlyContext(context);
> ValueExpression ve = exprFactory.createValueExpression(elContext, expression, expectedType);
> return ve.getValue(elContext);
> }
> @@ -122,8 +122,29 @@
> protected static class BasicContext extends ELContext {
> protected final Map<String, Object> variables;
> protected final VariableMapper variableMapper;
> - public BasicContext(Map<String, ? extends Object> context) {
> + public BasicContext(Map<String, Object> context) {
> this.variableMapper = new BasicVariableMapper(this);
> + this.variables = context;
> + }
> + @Override
> + public ELResolver getELResolver() {
> + return defaultResolver;
> + }
> + @Override
> + public FunctionMapper getFunctionMapper() {
> + return UelFunctions.getFunctionMapper();
> + }
> + @Override
> + public VariableMapper getVariableMapper() {
> + return this.variableMapper;
> + }
> + }
> +
> + protected static class ReadOnlyContext extends ELContext {
> + protected final Map<String, ? extends Object> variables;
> + protected final VariableMapper variableMapper;
> + public ReadOnlyContext(Map<String, ? extends Object> context) {
> + this.variableMapper = new ReadOnlyVariableMapper(this);
> this.variables = UtilGenerics.cast(context);
> }
> @Override
> @@ -138,6 +159,24 @@
> public VariableMapper getVariableMapper() {
> return this.variableMapper;
> }
> + protected static class ReadOnlyVariableMapper extends VariableMapper {
> + protected final ReadOnlyContext elContext;
> + protected ReadOnlyVariableMapper(ReadOnlyContext elContext) {
> + this.elContext = elContext;
> + }
> + @Override
> + public ValueExpression resolveVariable(String variable) {
> + Object obj = UelUtil.resolveVariable(variable, this.elContext.variables, null);
> + if (obj != null) {
> + return new ReadOnlyExpression(obj);
> + }
> + return null;
> + }
> + @Override
> + public ValueExpression setVariable(String variable, ValueExpression expression) {
> + throw new PropertyNotWritableException();
> + }
> + }
> }
>
> protected static class BasicVariableMapper extends VariableMapper {
> @@ -449,7 +488,7 @@
> return result;
> }
>
> - public static Object resolveVariable(String variable, Map<String, Object> variables, Locale locale) {
> + public static Object resolveVariable(String variable, Map<String, ? extends Object> variables, Locale locale) {
> Object obj = null;
> String createObjectType = null;
> String name = variable;
>
> Modified: ofbiz/trunk/framework/minilang/src/org/ofbiz/minilang/method/ContextAccessor.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/minilang/src/org/ofbiz/minilang/method/ContextAccessor.java?rev=904962&r1=904961&r2=904962&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/minilang/src/org/ofbiz/minilang/method/ContextAccessor.java (original)
> +++ ofbiz/trunk/framework/minilang/src/org/ofbiz/minilang/method/ContextAccessor.java Sun Jan 31 03:24:58 2010
> @@ -91,7 +91,7 @@
> }
>
> /** Based on name remove from Map or from List in Map */
> - public T remove(Map<String, ? extends Object> theMap, MethodContext methodContext) {
> + public T remove(Map<String, Object> theMap, MethodContext methodContext) {
> return fma.remove(theMap);
> }
Same explanation as above.
Re: svn commit: r904962 - in /ofbiz/trunk/framework: base/src/org/ofbiz/base/util/collections/FlexibleMapAccessor.java base/src/org/ofbiz/base/util/string/UelUtil.java minilang/src/org/ofbiz/minilang/method/ContextAccessor.java
Posted by Adrian Crum <ad...@yahoo.com>.
--- On Sat, 1/30/10, Adam Heath <do...@brainfood.com> wrote:
> From: Adam Heath <do...@brainfood.com>
> Subject: Re: svn commit: r904962 - in /ofbiz/trunk/framework: base/src/org/ofbiz/base/util/collections/FlexibleMapAccessor.java base/src/org/ofbiz/base/util/string/UelUtil.java minilang/src/org/ofbiz/minilang/method/ContextAccessor.java
> To: dev@ofbiz.apache.org
> Cc: commits@ofbiz.apache.org
> Date: Saturday, January 30, 2010, 7:30 PM
> adrianc@apache.org
> wrote:
> > Author: adrianc
> > Date: Sun Jan 31 03:24:58 2010
> > New Revision: 904962
> >
> > URL: http://svn.apache.org/viewvc?rev=904962&view=rev
> > Log:
> > Based on advice from Adam Heath, made
> FlexibleMapAccessor and UelUtil methods syntactically
> correct. Methods that read Maps take read-only Map
> arguments, and methods that write Maps take writable Map
> arguments.
> >
> >
> > Modified:
> >
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/collections/FlexibleMapAccessor.java
> >
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java
> >
> ofbiz/trunk/framework/minilang/src/org/ofbiz/minilang/method/ContextAccessor.java
> >
> > Modified:
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/collections/FlexibleMapAccessor.java
> > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/collections/FlexibleMapAccessor.java?rev=904962&r1=904961&r2=904962&view=diff
> >
> ==============================================================================
> > ---
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/collections/FlexibleMapAccessor.java
> (original)
> > +++
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/collections/FlexibleMapAccessor.java
> Sun Jan 31 03:24:58 2010
> > @@ -170,7 +170,7 @@
> > * @param base the Map
> to remove from
> > * @return the object
> removed
> > */
> > - public T remove(Map<String, ?
> extends Object> base) {
> > + public T remove(Map<String,
> Object> base) {
>
> Heh, not quite correct. You are allowed to remove
> items from a map
> with ? extends.
>
> If you recall, ? extends means that the exact class of the
> value is
> unknown. Since you don't know what the concrete class
> is, it's not
> possible to store any new values into the map, as you might
> break the
> generics contract.
>
> However, removing an item does not break the generics
> contract. So,
> this change is not correct.
I did a partial revert and had FMA's remove method convert the read-only Map to a writable Map. The UEL spec doesn't include a "remove" function, so I have to simulate it by replacing the value with null.
This is as close as I can get to the ideal. If anyone else wants to take a stab at it, they are more than welcome.