You are viewing a plain text version of this content. The canonical link for it is here.
Posted to agila-dev@incubator.apache.org by Glenn J Gonzales <gg...@exist.com> on 2005/03/08 09:07:06 UTC

[PATCH] expression evaluation, timer service

Hi,

I'm submitting a patch for evaluation of expressions from parameter 
binding values. It uses commons-jexl and accepts a String expression. 
The code tries to be fault-tolerant by returning the empty string if no 
valid evaluation can be performed. Also here are adjustments to 
NodeContextImpl.java (get/setBoundValue). I've also included a patch for 
TimerService and TimerServiceImpl (set the message token id to the 
passed-in value; also changed the signature to allow the node associated 
with the token to be given data upon execution from the service).

Cheers,
Glenn

Re: [PATCH] expression evaluation, timer service

Posted by Geir Magnusson Jr <ge...@4quarters.com>.
On Mar 9, 2005, at 5:18 AM, Glenn J Gonzales wrote:
>
> Geir Magnusson Jr wrote:
>
>> Comments.  re the resolver, this looks mainly like a refactoring of 
>> what was there in the jexlResolver (which is fine - we should make 
>> this pluggable so other ELs can be used)
>>
>> However,  I'm not sure that we need to be fault tolerant - suppose 
>> you are expecting a string back - then your indication of error, "", 
>> is a valid result, and you'll never know something went wrong.  I 
>> think that we should toss an exception rather than color the 
>> interpretation of the returned result.
>
> That's right. I thought about this too but I saw that 
> get/setBoundValue() in NodeContext interface doesn't declare thrown 
> exceptions, and I thought the intention was to keep the evaluation 
> "clean" (i.e., be fault-tolerant, since getBoundValue() returns the 
> empty string when the evaluation causes an exception),

That's not fault tolerant, IMO - it really is confusing, as it doesn't 
only return strings...

>  because in NodeContextImpl it's the one that handles the exceptions 
> from the evaluation itself. Letting these methods 
> (get/setBoundValue()) declare thrown Exception(s) allows the invoking 
> node to choose its own course of action,which may include a 
> pass-through, or a call to some application-specific service that 
> would know what to do.

Yep

>
>>
>> 2) Why do we need to surround expressions with ${}?  Shouldn't we 
>> just have the expression?
>
> I tried that before -- works, and probably makes better sense; 
> however, it requires that you formulate expressions java-like (can get 
> ugly), for example:
>
> <binding name="message" type="static" datatype="String" input="true" 
> output="false"><![CDATA["Task for : " + user.name()]]></binding>
>
> It's similarly awkward in the case of a jdbc-based activity. An sql 
> statement parameter may look like: "update customers set status = '" + 
> cust.getStatus() + "', department = '" + cust.getDept() + "' where 
> customer_id = " + cust.getId(), whereas if we make it EL-like we 
> simply pass: update customers set status = '${cust.getStatus()}', 
> department = '${cust.getDept()}' where customer_id = ${cust.getId()} 
> (of course we can use prepared statements instead, but that would only 
> be for jdbc-based activities of a specific functionality, not for a 
> general jdbc-access activity where you can enter any sql along with 
> other jdbc details).
>
> <binding name="sql" type="el" datatype="String" input="true" 
> output="false"><![CDATA["update customers set status = '" + 
> cust.getStatus() + "', department = '" + cust.getDept() + "' where 
> customer_id = " + cust.getId()]]></binding>
>
> as opposed to:
>
> <binding name="sql" type="el" datatype="String" input="true" 
> output="false"><![CDATA[update customers set status = 
> '${cust.getStatus()}', department = '${cust.getDept()}' where 
> customer_id = ${cust.getId()}]]></binding>

Hm.

>
>>>
>>> Index: org/apache/agila/impl/NodeContextImpl.java
>>> ===================================================================
>>> --- org/apache/agila/impl/NodeContextImpl.java  (revision 156357)
>>> +++ org/apache/agila/impl/NodeContextImpl.java  (working copy)
>>> @@ -25,6 +25,7 @@
>>>  import org.apache.agila.services.notification.NotificationService;
>>>  import org.apache.agila.services.task.TaskService;
>>>  import org.apache.agila.services.TimerService;
>>> +import org.apache.agila.util.Evaluator;
>>>  import org.apache.commons.jexl.Expression;
>>>  import org.apache.commons.jexl.ExpressionFactory;
>>>  import org.apache.commons.jexl.JexlContext;
>>> @@ -83,14 +84,8 @@
>>>                  return binding.getValue();
>>>
>>>              case Binding.EL :
>>> -                try {
>>> -                    return jexlResolver(binding.getValue(), 
>>> this.getInstanceData());
>>> -                }
>>> -                catch(Exception e) {
>>> -                    // TODO somethig useful
>>> -                    e.printStackTrace();
>>> -                    return "";
>>> -                }
>>> +                return Evaluator.evaluate( binding.getValue(), 
>>> this.getInstanceData() );
>>> +
>>>              default :
>>>                  return binding.getValue();
>>>          }
>>> @@ -111,17 +106,25 @@
>>>
>>>              case Binding.EL :
>>>                  try {
>>> -                    /*
>>> -                     * Awful hack - need to fix so exprs can be used
>>> -                     * for now, assume a scalar reference and just 
>>> set it
>>> -                     */
>>>
>>> -                    this.getInstanceData().put(binding.getName(), 
>>> value);
>>> +                    if(value != null) {
>>> +                        
>>> this.getInstanceData().put(binding.getName(), Evaluator.evaluate( 
>>> value.toString(), this.getInstanceData() ));
>>> +                    }
>>> +                    // else no op?
>>>                  }
>>
>>
>> This I don't get.  IIRC the code correctly, here's the situation.  
>> You have a binding
>>
>> suppose foo is the name, and "a.name()" is the expression value.
>>
>> I understand what it means to get the bound value of "foo" - you run 
>> "a.name()" through the resolver, and get the value (or null) 
>> depending on the state of the context, at that time.
>>
>> But to *put* the value for "foo"?  I can't figure out why I'd do
>>
>>   put("foo", "a.name()")
>>
>> if I wanted the expression to be evaluated at that moment, because I 
>> could just do
>>
>>   put("foo", a.name() )
>>
>> right?
>
> Yes, but that's just it -- if the node wanted to evaluate it at that 
> time, then the value is put directly into the instance data, but if 
> the node wants to pass it off for evaluation later, then maybe the 
> expression would be what you want (a scenario would be if the value 
> returned may change from one node execution to the next, like a 
> balance account value -- dunno if that's a good example though :) ).

Right - but your patch evaluates the expression and puts the result in.

>
>>
>> One could argue that a node may want to work with an expression w/in 
>> the context of that node's execution, and put the resulting value in 
>> the context (which is what your proposed patch does), but I don't 
>> think that's right.  I think we lose too much functionality - the 
>> ability to pass an expression onward so it can be evaluated at the 
>> time of execution of another node.
>>
> Argh, I've interchanged the cases. The code above should have been for 
> bindings with type "STATIC", because for these types of bindings, you 
> want to set them to the actual (resolved) value of the expression, 
> while for type "EL", you want to set it for evaluation later (code 
> below). Given that the code above and directly below are interchanged 
> for the cases, I think it fulfills the functionality you mentioned 
> nicely; however, with regards to the "output" dynamics of the binding, 
> this imposes some level of consistency, because if a node has an EL 
> binding that declares an output, it is expected that the value it puts 
> into the instance data will be an expression, and if it's of STATIC 
> type, then the value will be the resolved value.

yes

>
>> if we do need to have nodes deal w/ expressions, we should just make 
>> the expression evaluator available to it directly.
>>
> The evaluator can also be used directly since these are public static 
> methods, but I think the node context should still be able to provide 
> more general evaluation capability.

Remember, the evaluator should be pluggable - a node shouldn't assume 
the kind of expressions, nor should it care.  IOW, an activity 
shouldn't have an import or reference to a given expression evaluator 
class, but rather asked the node context for the appropriate one.  Or 
such.


>
> I have attached a redo.

Ok

>
> Cheers,
> Glenn
>
>>
>>
>>>                  catch(Exception e) {
>>>                      // TODO somethig useful
>>>                      e.printStackTrace();
>>>                  }
>>> +
>>> +                break;
>>> +
>>> +            case Binding.STATIC :
>>> +
>>> +                if(value != null) 
>>> this.getInstanceData().put(binding.getName(), value);
>>> +
>>> +                break;
>>> +
>>>              default :
>>>                  // TODO urg...
>>>          }
>>
>
> Index: org/apache/agila/util/Evaluator.java
> ===================================================================
> --- org/apache/agila/util/Evaluator.java        (revision 0)
> +++ org/apache/agila/util/Evaluator.java        (revision 0)
> @@ -0,0 +1,106 @@
> +package org.apache.agila.util;
> +
> +import org.apache.commons.jexl.Expression;
> +import org.apache.commons.jexl.ExpressionFactory;
> +import org.apache.commons.jexl.JexlContext;
> +import org.apache.commons.jexl.JexlHelper;
> +
> +import java.util.Map;
> +import java.util.ArrayList;
> +import java.util.List;
> +import java.util.Iterator;
> +
> +/**
> + * @author <a href="mailto:ggonzales@exist.com">Glenn J Gonzales.</a>
> + */
> +public class Evaluator {
> +
> +    /**
> +     * Reads a line of text and evaluates it using the data in 
> <code>params</code>. Uses Commons-Jexl
> +     * to execute the evaluation.
> +     *
> +     * @param line
> +     * @param params
> +     * @return If the text is a single EL-like expression, then the 
> corresponding object value of the
> +     * expression is returned, otherwise (i.e., if the text contains 
> template text or more than one
> +     * EL-like expression) this returns a concatenation of the string 
> representation of the values.
> +     */
> +    public static Object evaluate(String line, Map params) throws 
> Exception {
> +
> +        if(line == null) return "";
> +
> +        List store = new ArrayList();
> +
> +        int indexExprStart = line.indexOf("${");
> +
> +        if(indexExprStart != -1) {
> +
> +            if(indexExprStart > 0) store.add(line.substring(0, 
> indexExprStart));
> +
> +            int indexExprEnd = line.indexOf("}", indexExprStart + 2);
> +
> +            if(indexExprEnd != -1) {
> +
> +                String expr = line.substring(indexExprStart + 2, 
> indexExprEnd);
> +
> +                Object value = resolve( expr, params );
> +
> +                store.add(value);
> +
> +                if(indexExprEnd != line.length() - 1) {
> +                    store.add( evaluate(line.substring(indexExprEnd + 
> 1), params) );
> +                }
> +            }
> +            else {
> +                return evaluateStore(store);
> +            }
> +        }
> +        else {
> +            store.add(line);
> +        }
> +
> +        return evaluateStore(store);
> +    }
> +
> +    /**
> +     * If <code>store</code> contains more than one element, then 
> return the concatenation of the string
> +     * representation of the objects; otherwise, return the lone 
> object.
> +     *
> +     * @param store
> +     * @return
> +     */
> +    private static Object evaluateStore(List store) {
> +
> +        if(store.size() > 1) {
> +
> +            StringBuffer buffer = new StringBuffer();
> +
> +            for(Iterator contents = store.iterator(); 
> contents.hasNext(); ) {
> +                buffer.append( "" + contents.next() );
> +            }
> +
> +            return buffer.toString();
> +        }
> +        else {
> +            return store.get(0);
> +        }
> +    }
> +
> +    /**
> +     * Resolve the expression using the <code>params</code> data.
> +     *
> +     * @param expr
> +     * @param params
> +     * @return
> +     */
> +    private static Object resolve(String expr, Map params) throws 
> Exception {
> +
> +        Expression jexlExpr = 
> ExpressionFactory.createExpression(expr);
> +
> +        JexlContext jexlContext = JexlHelper.createContext();
> +
> +        jexlContext.setVars(params);
> +
> +        return jexlExpr.evaluate(jexlContext);
> +    }
> +}
> Index: org/apache/agila/model/node/TestNotification.java
> ===================================================================
> --- org/apache/agila/model/node/TestNotification.java   (revision 
> 156357)
> +++ org/apache/agila/model/node/TestNotification.java   (working copy)
> @@ -38,9 +38,17 @@
>       */
>      public Connection[] doEnd(NodeContext ctx) {
>
> -        String val = (String) ctx.getBoundValue("message");
> +        try {
> +            String val = (String) ctx.getBoundValue("message");
> +            ctx.getNotificationService().notify(new UserID(1), val);
> +        }
> +        catch (Exception e) {
> +            // TODO log?
> +            e.printStackTrace();
> +        }
>
> -        ctx.getNotificationService().notify(new UserID(1), val);
> +        // becomes a no-op in case of an exception in evaluation
> +
>          return this.getOutboundConnections();
>      }
>  }
> Index: org/apache/agila/model/node/SimpleDecision.java
> ===================================================================
> --- org/apache/agila/model/node/SimpleDecision.java     (revision 
> 156357)
> +++ org/apache/agila/model/node/SimpleDecision.java     (working copy)
> @@ -27,7 +27,14 @@
>          /*
>           *  get the value
>           */
> -        Object o = ctx.getBoundValue("value");
> +        Object o = null;
> +
> +        try {
> +            o = ctx.getBoundValue("value");
> +        }
> +        catch (Exception e) {
> +            e.printStackTrace();
> +        }
>
>          boolean b = false;
>
> Index: org/apache/agila/model/node/TestTaskNode.java
> ===================================================================
> --- org/apache/agila/model/node/TestTaskNode.java       (revision 
> 156357)
> +++ org/apache/agila/model/node/TestTaskNode.java       (working copy)
> @@ -59,8 +59,15 @@
>          if ("APPROVE".equals(name.toUpperCase())) {
>              b = true;
>          }
> -        ctx.setBoundValue("approved", new Boolean(b));
>
> +        try {
> +            ctx.setBoundValue("approved", new Boolean(b));
> +        }
> +        catch (Exception e) {
> +            // do output for now
> +            e.printStackTrace();
> +        }
> +
>          return getOutboundConnections();
>      }
>
> Index: org/apache/agila/model/node/HelloWorldActivity.java
> ===================================================================
> --- org/apache/agila/model/node/HelloWorldActivity.java (revision 
> 156357)
> +++ org/apache/agila/model/node/HelloWorldActivity.java (working copy)
> @@ -30,8 +30,15 @@
>      public Connection[] doEnd(NodeContext ctx) {
>          System.out.println("HelloWorldActivity : hello world");
>
> -        String val = (String) ctx.getBoundValue("message");
> +        String val = null;
>
> +        try {
> +            val = (String) ctx.getBoundValue("message");
> +        }
> +        catch (Exception e) {
> +            e.printStackTrace();
> +        }
> +
>          System.out.println("HelloWorldActivity : message = " + val);
>
>          return getOutboundConnections();
> Index: org/apache/agila/example/LeaveApplicationTask.java
> ===================================================================
> --- org/apache/agila/example/LeaveApplicationTask.java  (revision 
> 156357)
> +++ org/apache/agila/example/LeaveApplicationTask.java  (working copy)
> @@ -94,9 +94,16 @@
>           * the bindings
>           */
>
> -        ctx.setBoundValue(NUMDAYS, taskData.numDays);
> -        ctx.setBoundValue(REASON, taskData.reason);
> +        try {
> +            ctx.setBoundValue(NUMDAYS, taskData.numDays);
> +            ctx.setBoundValue(REASON, taskData.reason);
> +        }
> +        catch (Exception e) {
> +            // do output for now
> +            e.printStackTrace();
> +        }
>
> +
>          return getOutboundConnections();
>      }
> Index: org/apache/agila/model/NodeContext.java
> ===================================================================
> --- org/apache/agila/model/NodeContext.java     (revision 156357)
> +++ org/apache/agila/model/NodeContext.java     (working copy)
> @@ -31,8 +31,8 @@
>   */
>  public interface NodeContext {
>
> -    public Object getBoundValue(String name);
> -    public void setBoundValue(String name, Object value);
> +    public Object getBoundValue(String name) throws Exception;
> +    public void setBoundValue(String name, Object value) throws 
> Exception;
>
>      public Map getInstanceData();
> Index: org/apache/agila/impl/NodeContextImpl.java
> ===================================================================
> --- org/apache/agila/impl/NodeContextImpl.java  (revision 156357)
> +++ org/apache/agila/impl/NodeContextImpl.java  (working copy)
> @@ -25,6 +25,7 @@
>  import org.apache.agila.services.notification.NotificationService;
>  import org.apache.agila.services.task.TaskService;
>  import org.apache.agila.services.TimerService;
> +import org.apache.agila.util.Evaluator;
>  import org.apache.commons.jexl.Expression;
>  import org.apache.commons.jexl.ExpressionFactory;
>  import org.apache.commons.jexl.JexlContext;
> @@ -67,7 +68,7 @@
>       * @param name
>       * @return
>       */
> -    public Object getBoundValue(String name) {
> +    public Object getBoundValue(String name) throws Exception {
>
>          Map bm = node.getBindings();
>
> @@ -83,20 +84,14 @@
>                  return binding.getValue();
>
>              case Binding.EL :
> -                try {
> -                    return jexlResolver(binding.getValue(), 
> this.getInstanceData());
> -                }
> -                catch(Exception e) {
> -                    // TODO somethig useful
> -                    e.printStackTrace();
> -                    return "";
> -                }
> +                return Evaluator.evaluate( binding.getValue(), 
> this.getInstanceData() );
> +
>              default :
>                  return binding.getValue();
>          }
>      }
>
> -    public void setBoundValue(String name, Object value) {
> +    public void setBoundValue(String name, Object value) throws 
> Exception {
>
>          Map bm = node.getBindings();
>
> @@ -109,19 +104,16 @@
>
>          switch(binding.getType()) {
>
> +            case Binding.STATIC :
> +                this.getInstanceData().put( binding.getName(), 
> Evaluator.evaluate( value.toString(), this.getInstanceData()) );
> +
> +                break;
> +
>              case Binding.EL :
> -                try {
> -                    /*
> -                     * Awful hack - need to fix so exprs can be used
> -                     * for now, assume a scalar reference and just 
> set it
> -                     */
> +                if(value != null) 
> this.getInstanceData().put(binding.getName(), value);
>
> -                    this.getInstanceData().put(binding.getName(), 
> value);
> -                }
> -                catch(Exception e) {
> -                    // TODO somethig useful
> -                    e.printStackTrace();
> -                }
> +                break;
> +
>              default :
>                  // TODO urg...
>          }
-- 
Geir Magnusson Jr                                  +1-203-665-6437
geir@gluecode.com


Re: [PATCH] expression evaluation, timer service

Posted by Glenn J Gonzales <gg...@exist.com>.
Hi Geir, great to hear from you! Heard you were busy with Geronimo :)

Geir Magnusson Jr wrote:

> Comments.  re the resolver, this looks mainly like a refactoring of 
> what was there in the jexlResolver (which is fine - we should make 
> this pluggable so other ELs can be used)
>
> However,  I'm not sure that we need to be fault tolerant - suppose you 
> are expecting a string back - then your indication of error, "", is a 
> valid result, and you'll never know something went wrong.  I think 
> that we should toss an exception rather than color the interpretation 
> of the returned result.

That's right. I thought about this too but I saw that 
get/setBoundValue() in NodeContext interface doesn't declare thrown 
exceptions, and I thought the intention was to keep the evaluation 
"clean" (i.e., be fault-tolerant, since getBoundValue() returns the 
empty string when the evaluation causes an exception), because in 
NodeContextImpl it's the one that handles the exceptions from the 
evaluation itself. Letting these methods (get/setBoundValue()) declare 
thrown Exception(s) allows the invoking node to choose its own course of 
action,which may include a pass-through, or a call to some 
application-specific service that would know what to do.

>
> 2) Why do we need to surround expressions with ${}?  Shouldn't we just 
> have the expression?

I tried that before -- works, and probably makes better sense; however, 
it requires that you formulate expressions java-like (can get ugly), for 
example:

<binding name="message" type="static" datatype="String" input="true" 
output="false"><![CDATA["Task for : " + user.name()]]></binding>

 It's similarly awkward in the case of a jdbc-based activity. An sql 
statement parameter may look like: "update customers set status = '" + 
cust.getStatus() + "', department = '" + cust.getDept() + "' where 
customer_id = " + cust.getId(), whereas if we make it EL-like we simply 
pass: update customers set status = '${cust.getStatus()}', department = 
'${cust.getDept()}' where customer_id = ${cust.getId()} (of course we 
can use prepared statements instead, but that would only be for 
jdbc-based activities of a specific functionality, not for a general 
jdbc-access activity where you can enter any sql along with other jdbc 
details).

<binding name="sql" type="el" datatype="String" input="true" 
output="false"><![CDATA["update customers set status = '" + 
cust.getStatus() + "', department = '" + cust.getDept() + "' where 
customer_id = " + cust.getId()]]></binding>

as opposed to:

<binding name="sql" type="el" datatype="String" input="true" 
output="false"><![CDATA[update customers set status = 
'${cust.getStatus()}', department = '${cust.getDept()}' where 
customer_id = ${cust.getId()}]]></binding>

>>
>> Index: org/apache/agila/impl/NodeContextImpl.java
>> ===================================================================
>> --- org/apache/agila/impl/NodeContextImpl.java  (revision 156357)
>> +++ org/apache/agila/impl/NodeContextImpl.java  (working copy)
>> @@ -25,6 +25,7 @@
>>  import org.apache.agila.services.notification.NotificationService;
>>  import org.apache.agila.services.task.TaskService;
>>  import org.apache.agila.services.TimerService;
>> +import org.apache.agila.util.Evaluator;
>>  import org.apache.commons.jexl.Expression;
>>  import org.apache.commons.jexl.ExpressionFactory;
>>  import org.apache.commons.jexl.JexlContext;
>> @@ -83,14 +84,8 @@
>>                  return binding.getValue();
>>
>>              case Binding.EL :
>> -                try {
>> -                    return jexlResolver(binding.getValue(), 
>> this.getInstanceData());
>> -                }
>> -                catch(Exception e) {
>> -                    // TODO somethig useful
>> -                    e.printStackTrace();
>> -                    return "";
>> -                }
>> +                return Evaluator.evaluate( binding.getValue(), 
>> this.getInstanceData() );
>> +
>>              default :
>>                  return binding.getValue();
>>          }
>> @@ -111,17 +106,25 @@
>>
>>              case Binding.EL :
>>                  try {
>> -                    /*
>> -                     * Awful hack - need to fix so exprs can be used
>> -                     * for now, assume a scalar reference and just 
>> set it
>> -                     */
>>
>> -                    this.getInstanceData().put(binding.getName(), 
>> value);
>> +                    if(value != null) {
>> +                        
>> this.getInstanceData().put(binding.getName(), Evaluator.evaluate( 
>> value.toString(), this.getInstanceData() ));
>> +                    }
>> +                    // else no op?
>>                  }
>
>
> This I don't get.  IIRC the code correctly, here's the situation.  You 
> have a binding
>
> suppose foo is the name, and "a.name()" is the expression value.
>
> I understand what it means to get the bound value of "foo" - you run 
> "a.name()" through the resolver, and get the value (or null) depending 
> on the state of the context, at that time.
>
> But to *put* the value for "foo"?  I can't figure out why I'd do
>
>   put("foo", "a.name()")
>
> if I wanted the expression to be evaluated at that moment, because I 
> could just do
>
>   put("foo", a.name() )
>
> right?

Yes, but that's just it -- if the node wanted to evaluate it at that 
time, then the value is put directly into the instance data, but if the 
node wants to pass it off for evaluation later, then maybe the 
expression would be what you want (a scenario would be if the value 
returned may change from one node execution to the next, like a balance 
account value -- dunno if that's a good example though :) ).

>
> One could argue that a node may want to work with an expression w/in 
> the context of that node's execution, and put the resulting value in 
> the context (which is what your proposed patch does), but I don't 
> think that's right.  I think we lose too much functionality - the 
> ability to pass an expression onward so it can be evaluated at the 
> time of execution of another node.
>
Argh, I've interchanged the cases. The code above should have been for 
bindings with type "STATIC", because for these types of bindings, you 
want to set them to the actual (resolved) value of the expression, while 
for type "EL", you want to set it for evaluation later (code below). 
Given that the code above and directly below are interchanged for the 
cases, I think it fulfills the functionality you mentioned nicely; 
however, with regards to the "output" dynamics of the binding, this 
imposes some level of consistency, because if a node has an EL binding 
that declares an output, it is expected that the value it puts into the 
instance data will be an expression, and if it's of STATIC type, then 
the value will be the resolved value.

> if we do need to have nodes deal w/ expressions, we should just make 
> the expression evaluator available to it directly.
>
The evaluator can also be used directly since these are public static 
methods, but I think the node context should still be able to provide 
more general evaluation capability.

I have attached a redo.

Cheers,
Glenn

>
>
>>                  catch(Exception e) {
>>                      // TODO somethig useful
>>                      e.printStackTrace();
>>                  }
>> +
>> +                break;
>> +
>> +            case Binding.STATIC :
>> +
>> +                if(value != null) 
>> this.getInstanceData().put(binding.getName(), value);
>> +
>> +                break;
>> +
>>              default :
>>                  // TODO urg...
>>          }
>


Re: [PATCH] expression evaluation, timer service

Posted by Geir Magnusson Jr <ge...@4quarters.com>.
Comments.  re the resolver, this looks mainly like a refactoring of 
what was there in the jexlResolver (which is fine - we should make this 
pluggable so other ELs can be used)

However,  I'm not sure that we need to be fault tolerant - suppose you 
are expecting a string back - then your indication of error, "", is a 
valid result, and you'll never know something went wrong.  I think that 
we should toss an exception rather than color the interpretation of the 
returned result.

2) Why do we need to surround expressions with ${}?  Shouldn't we just 
have the expression?

More inline.  I don't like the expression/node context change for "put" 
(see below) and love the timer suggestion.  Will commit that.

geir


On Mar 8, 2005, at 4:07 AM, Glenn J Gonzales wrote:

> Hi,
>
> I'm submitting a patch for evaluation of expressions from parameter 
> binding values. It uses commons-jexl and accepts a String expression. 
> The code tries to be fault-tolerant by returning the empty string if 
> no valid evaluation can be performed. Also here are adjustments to 
> NodeContextImpl.java (get/setBoundValue). I've also included a patch 
> for TimerService and TimerServiceImpl (set the message token id to the 
> passed-in value; also changed the signature to allow the node 
> associated with the token to be given data upon execution from the 
> service).
>
> Cheers,
> Glenn
> Index: org/apache/agila/util/Evaluator.java
> ===================================================================
> --- org/apache/agila/util/Evaluator.java        (revision 0)
> +++ org/apache/agila/util/Evaluator.java        (revision 0)
> @@ -0,0 +1,118 @@
> +package org.apache.agila.util;
> +
> +import org.apache.commons.jexl.Expression;
> +import org.apache.commons.jexl.ExpressionFactory;
> +import org.apache.commons.jexl.JexlContext;
> +import org.apache.commons.jexl.JexlHelper;
> +
> +import java.util.Map;
> +import java.util.ArrayList;
> +import java.util.List;
> +import java.util.Iterator;
> +
> +/**
> + * @author <a href="mailto:ggonzales@exist.com">Glenn J Gonzales.</a>
> + */
> +public class Evaluator {
> +
> +    /**
> +     * Reads a line of text and evaluates it using the data in 
> <code>params</code>. Uses Commons-Jexl
> +     * to execute the evaluation.
> +     *
> +     * @param line
> +     * @param params
> +     * @return If the text is a single EL-like expression, then the 
> corresponding object value of the
> +     * expression is returned, otherwise (i.e., if the text contains 
> template text or more than one
> +     * EL-like expression) this returns a concatenation of the string 
> representation of the values.
> +     */
> +    public static Object evaluate(String line, Map params) {
> +
> +        if(line == null) return "";
> +
> +        List store = new ArrayList();
> +
> +        int indexExprStart = line.indexOf("${");
> +
> +        if(indexExprStart != -1) {
> +
> +            if(indexExprStart > 0) store.add(line.substring(0, 
> indexExprStart));
> +
> +            int indexExprEnd = line.indexOf("}", indexExprStart + 2);
> +
> +            if(indexExprEnd != -1) {
> +
> +                String expr = line.substring(indexExprStart + 2, 
> indexExprEnd);
> +
> +                Object value = resolve( expr, params );
> +
> +                store.add(value);
> +
> +                if(indexExprEnd != line.length() - 1) {
> +                    Object returnObj = 
> evaluate(line.substring(indexExprEnd + 1), params);
> +                    store.add( returnObj != null ? returnObj : "" );
> +                }
> +            }
> +            else {
> +                return evaluateStore(store);
> +            }
> +        }
> +        else {
> +            store.add(line);
> +        }
> +
> +        return evaluateStore(store);
> +    }
> +
> +    /**
> +     * If <code>store</code> contains more than one element, then 
> return the concatenation of the string
> +     * representation of the objects; otherwise, return the lone 
> object.
> +     *
> +     * @param store
> +     * @return
> +     */
> +    private static Object evaluateStore(List store) {
> +
> +        if(store.size() > 1) {
> +
> +            StringBuffer buffer = new StringBuffer();
> +
> +            for(Iterator contents = store.iterator(); 
> contents.hasNext(); ) {
> +                buffer.append( contents.next().toString() );
> +            }
> +
> +            return buffer.toString();
> +        }
> +        else {
> +            return store.get(0);
> +        }
> +    }
> +
> +    /**
> +     * Resolve the expression using the <code>params</code> data.
> +     *
> +     * @param expr
> +     * @param params
> +     * @return
> +     */
> +    private static Object resolve(String expr, Map params) {
> +
> +        // make it fault tolerant
> +        Object obj = new String("");
> +
> +        try {
> +            Expression jexlExpr = 
> ExpressionFactory.createExpression(expr);
> +
> +            JexlContext jexlContext = JexlHelper.createContext();
> +
> +            jexlContext.setVars(params);
> +
> +            obj = jexlExpr.evaluate(jexlContext);
> +        }
> +        catch (Exception e) {
> +            // TODO log?
> +            //e.printStackTrace();
> +        }
> +
> +        return obj;
> +    }
> +}
> Index: org/apache/agila/impl/NodeContextImpl.java
> ===================================================================
> --- org/apache/agila/impl/NodeContextImpl.java  (revision 156357)
> +++ org/apache/agila/impl/NodeContextImpl.java  (working copy)
> @@ -25,6 +25,7 @@
>  import org.apache.agila.services.notification.NotificationService;
>  import org.apache.agila.services.task.TaskService;
>  import org.apache.agila.services.TimerService;
> +import org.apache.agila.util.Evaluator;
>  import org.apache.commons.jexl.Expression;
>  import org.apache.commons.jexl.ExpressionFactory;
>  import org.apache.commons.jexl.JexlContext;
> @@ -83,14 +84,8 @@
>                  return binding.getValue();
>
>              case Binding.EL :
> -                try {
> -                    return jexlResolver(binding.getValue(), 
> this.getInstanceData());
> -                }
> -                catch(Exception e) {
> -                    // TODO somethig useful
> -                    e.printStackTrace();
> -                    return "";
> -                }
> +                return Evaluator.evaluate( binding.getValue(), 
> this.getInstanceData() );
> +
>              default :
>                  return binding.getValue();
>          }
> @@ -111,17 +106,25 @@
>
>              case Binding.EL :
>                  try {
> -                    /*
> -                     * Awful hack - need to fix so exprs can be used
> -                     * for now, assume a scalar reference and just 
> set it
> -                     */
>
> -                    this.getInstanceData().put(binding.getName(), 
> value);
> +                    if(value != null) {
> +                        this.getInstanceData().put(binding.getName(), 
> Evaluator.evaluate( value.toString(), this.getInstanceData() ));
> +                    }
> +                    // else no op?
>                  }

This I don't get.  IIRC the code correctly, here's the situation.  You 
have a binding

suppose foo is the name, and "a.name()" is the expression value.

I understand what it means to get the bound value of "foo" - you run 
"a.name()" through the resolver, and get the value (or null) depending 
on the state of the context, at that time.

But to *put* the value for "foo"?  I can't figure out why I'd do

   put("foo", "a.name()")

if I wanted the expression to be evaluated at that moment, because I 
could just do

   put("foo", a.name() )

right?

One could argue that a node may want to work with an expression w/in 
the context of that node's execution, and put the resulting value in 
the context (which is what your proposed patch does), but I don't think 
that's right.  I think we lose too much functionality - the ability to 
pass an expression onward so it can be evaluated at the time of 
execution of another node.

if we do need to have nodes deal w/ expressions, we should just make 
the expression evaluator available to it directly.



>                  catch(Exception e) {
>                      // TODO somethig useful
>                      e.printStackTrace();
>                  }
> +
> +                break;
> +
> +            case Binding.STATIC :
> +
> +                if(value != null) 
> this.getInstanceData().put(binding.getName(), value);
> +
> +                break;
> +
>              default :
>                  // TODO urg...
>          }Index: org/apache/agila/services/TimerService.java
> ===================================================================
> --- org/apache/agila/services/TimerService.java (revision 156357)
> +++ org/apache/agila/services/TimerService.java (working copy)
> @@ -18,6 +18,8 @@
>
>  import org.apache.agila.engine.TokenID;
>
> +import java.io.Serializable;
> +
>  /**
>   *
>   * @author <a href="mailto:geir@gluecode.com">Geir Magnusson Jr.</a>
> @@ -25,5 +27,5 @@
>   */
>  public interface TimerService {
>
> -    public void setExecutionContinue(TokenID tokenID, int seconds);
> +    public void setExecutionContinue(TokenID tokenID, int seconds, 
> String dataKey, Serializable appData);
>  }
> Index: org/apache/agila/impl/memory/TimerServiceImpl.java
> ===================================================================
> --- org/apache/agila/impl/memory/TimerServiceImpl.java  (revision 
> 156357)
> +++ org/apache/agila/impl/memory/TimerServiceImpl.java  (working copy)
> @@ -23,6 +23,8 @@
>  import org.apache.agila.impl.EngineMessageImpl;
>  import EDU.oswego.cs.dl.util.concurrent.ClockDaemon;
>
> +import java.io.Serializable;
> +
>  /**
>   *
>   * @author <a href="mailto:geir@gluecode.com">Geir Magnusson Jr.</a>
> @@ -37,7 +39,7 @@
>          this.queue = qs;
>      }
>
> -    public void setExecutionContinue(final TokenID tokenID, int 
> seconds) {
> +    public void setExecutionContinue(final TokenID tokenID, int 
> seconds, final String dataKey, final Serializable data) {
>
>          daemon.executeAfterDelay(1000L * seconds,
>
> @@ -46,7 +48,12 @@
>                      public void run() {
>                          EngineMessage em = new EngineMessageImpl();
>
> -                        em.setCurrentTokenID(null);
> +                        //em.setCurrentTokenID(null);
> +
> +                        em.addAppData(dataKey, data);
> +
> +                        em.setCurrentTokenID(tokenID);
> +
>                          queue.enqueue(em);
>                      }
>                  }
-- 
Geir Magnusson Jr                                  +1-203-665-6437
geir@gluecode.com