You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Bill Horsman <bi...@logicalcobwebs.co.uk> on 2004/01/10 20:21:58 UTC
[PATCH] Re: [jexl] - checking for unresolved variables
Jexl Patch:
I've made some changes to allow for strict checking of unknown variables
inside an expression. Please see attached patch plus one new class
(UnknownVariableException). The changes were simple enough. The
JexlHelper.createContext()
method is now overridden with a new
JexlHelper.createContext(boolean strict)
method. The old one just calls the new one with strict = false. So the
default behaviour is unchanged.
Then I decided to make HashMapContext use the composition pattern to
contain a HashMap rather than extend it. This allowed me to ensure that
everything else used my new getVar() method rather than getting stuff
directly from the Map. The getVar() method contains the code that
decides whether to throw an Exception (strict) or return a null
(lenient).
This meant changes to JexlContext and quite a few changes (mostly to
test classes) that were using jc.getVars() to get direct access to the
Map. IMHO, it is a bad idea to expose the map in that way.
Finally, I added a test method called testUnknownVariables() to JexlTest
to check it all works. And it does :)
-
Bill
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by Bill Horsman <bi...@logicalcobwebs.co.uk>.
On Sun, 2004-01-11 at 21:27, peter royal wrote:
> A new patch would be easiest :)
Here it is. Some documentation added. And I also took the liberty of
adding a clear() method to JexlContext.
> If you're working against an anon CVS checkout, -N to 'cvs diff' will
> include new files.
That only works if you use "cvs add" first. And I can't do that with
anon access. So, in the meantime, I continue with clumsy additional file
:)
-
Bill
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by peter royal <pr...@pace2020.com>.
On Jan 11, 2004, at 3:03 PM, Bill Horsman wrote:
> Do you want me to resubmit the patch? BTW, my patch file didn't include
> the new file I added. Is there anyway of including the new files too,
> or
> do I just need to include them separately?
A new patch would be easiest :)
If you're working against an anon CVS checkout, -N to 'cvs diff' will
include new files.
-pete
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by Bill Horsman <Bi...@logicalcobwebs.co.uk>.
On Sun, 2004-01-11 at 20:47, peter royal wrote:
> Looks good. My only suggestion is that you likely want to check for
> existence of the key in the map in order to known when to throw the
> exception, as opposed to testing for null. That way you can have valid
> variables that are null.
Argh, you're right. To be honest, I'd forgotten that a map's value could
be null. I've made the change as you suggest and changed the unit test
to use a null value and it passes.
Do you want me to resubmit the patch? BTW, my patch file didn't include
the new file I added. Is there anyway of including the new files too, or
do I just need to include them separately?
-
Bill
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by peter royal <pr...@apache.org>.
On Jan 10, 2004, at 2:21 PM, Bill Horsman wrote:
> I've made some changes to allow for strict checking of unknown
> variables
> inside an expression. Please see attached patch plus one new class
> (UnknownVariableException). The changes were simple enough. The
>
> JexlHelper.createContext()
>
> method is now overridden with a new
>
> JexlHelper.createContext(boolean strict)
>
> method. The old one just calls the new one with strict = false. So the
> default behaviour is unchanged.
Looks good. My only suggestion is that you likely want to check for
existence of the key in the map in order to known when to throw the
exception, as opposed to testing for null. That way you can have valid
variables that are null.
-pete
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by Bill Horsman <bi...@logicalcobwebs.co.uk>.
On Mon, 2004-01-12 at 16:28, Geir Magnusson Jr wrote:
> To be clear, I *really* don't think it's a good idea to break usage.
That sounds reasonable. It's a judgement call on when it's a good time
to break backwards compatibility and I don't know enough about Jexl.
> Now, I certainly agree that this would be darn useful, and I see two
> ways (offhand) to approach it :
>
> 1) Your solution, where the context implementation has to understand
> and know about all this magic jexl stuff
Not quite. in my solution the context is actually pretty stupid. It just
throws an exception when it's asked something it doesn't know.
> Initial comments - you need to be sure that you don't get fooled by a
> sequence of expressions that will resolve ok - IOW, an assignment
>
> a = b;
>
> w/ 'b' in the context should result in 'OK' for an unresolved check
> even though a isn't in the context to start.
I didn't realise you could do that. But by happy coincidence my solution
does cope with that. The JexlContext is never asked for the value of "a"
so it doesn't complain.
> The other would be to have a checker that takes an expression and
> figures out if it will go given a context. That leads to a two step
> execution process, where you'd first run the check and then
> evaluate()....
I like this idea. Sounds more complicated to code but could be worth it.
It would be cool to provide other information too. Like, give me a list
of all the unknown variables (not just the first one you came across).
Or, give me a list of all the variables used in this expression.
One thing occurs to me. If we do end up holding some state during the
checking process then we have to make sure that it is thread safe. I can
see the same JexlContext being applied to different Expressions and the
same Expression being passed different JexlContexts.
I remain, as ever, open minded :)
-
Bill
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by Bill Horsman <bi...@logicalcobwebs.co.uk>.
On Tue, 2004-01-13 at 17:22, Bill Horsman wrote:
> (Yet) another solution:
>
> Get the JexlContext to track which variables it was asked for. Note
> which ones were known and which were not known. Then you can ask
> JexlContext for the list at the end.
I got so enthused by this idea that I decided to just do it. There is a
new method on Expression:
CheckResults check(JexlContext context) throws Exception;
And CheckResults contains:
Object getAnswer();
String[] getKnownVariables();
String[] getUnknownVariables();
String[] getUsedVariables();
(It's documented better in the patch).
Good things:
It doesn't break the current API
It gives more information than before: a list of all the unknown
variables (not just the first one encountered), and a list of all the
used variables (this is a nice to have for my current app)
Bad things:
It needs to clone the JexlContext to make it thread-safe. This is done
transparently, but it might effect performance. But then again, I don't
think it's significant. (Actually, as I write this I realise that it
doesn't need to clone it - just wrap it. But the shallow clone of Map is
not too lengthy anyway).
It's not a simple configuration change to make it lenient or strict. You
need to call a different method (check instead of evaluate). I suppose
you could make evaluate do something similar (and just throw an
exception that contained the CheckResults or something).
The patch is an tar archive because I still haven't found a way of
bundling the new files in the patch file. The archive contains two new
Java files and the patch file.
- Bill
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by Bill Horsman <bi...@logicalcobwebs.co.uk>.
Hi Pete,
On Tue, 2004-01-13 at 16:41, peter royal wrote:
> lets leverage the map! the map knows if a variable exists. So how about
> JexlExpression.setExplicitVariableDeclaration( true ); and then Jexl
> just does (in ASTIdentifier, and I think that might be the only
> place..)
That would work. My only argument against it is that if someone adds
another usage for JexlContext.getVars().get() then they might forget to
add that check. So it's a maintenance issue.
To be honest, I don't understand what ASTIdentifier does (not that it is
opaque, just that I haven't explored yet). All I know is that there are
41 other implementations of SimpleNode.value().
(Yet) another solution:
Get the JexlContext to track which variables it was asked for. Note
which ones were known and which were not known. Then you can ask
JexlContext for the list at the end.
It's good because it doesn't break the API and it doesn't mean delving
into Jexl to find all the places that it needs to be done in.
It's not thread-safe. But, we can clone a JexlContext easily enough. How
about:
Expression.check(JexlContext);
And the implementation looks *something* like:
public String[] check(JexlContext context) {
// Clone the context
CheckingContext checkingContext = new CheckingContext(context);
// Evaluate as normal
evaluate(checkingContext);
// Return the list of unknown variables
return checkingContext.getUnknownVariables();
}
Well, we can't be accused of being short of ideas! Not sure of the
performance problems here - but I think, like XML parsing, people are
happy to accept that if they want to be strict.
- Bill
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by peter royal <pr...@apache.org>.
On Jan 12, 2004, at 10:28 AM, Geir Magnusson Jr wrote:
> 2) A different approach, where the context is simple (allowing
> recycling w/o much effort of other backing impls) and Jexl is smart
>
> Regarding #2, I can see 2 approaches.
>
> One is to do it during evaluate(), like you proposed, which will
> change the state of some of the objects in the context, and not all if
> something is wrong :
>
> nuclearReactor.start(); nuclearReactor.setTemparature(temp);
> nuclearReactor.stop();
>
> Granted, I know that you can't always ensure you get through, but
> still, adding another way causes concern. On the flipside, it's
> simple....
>
> The other would be to have a checker that takes an expression and
> figures out if it will go given a context. That leads to a two step
> execution process, where you'd first run the check and then
> evaluate()....
>
> Comments?
an alternative approach..
lets leverage the map! the map knows if a variable exists. So how about
JexlExpression.setExplicitVariableDeclaration( true ); and then Jexl
just does (in ASTIdentifier, and I think that might be the only
place..)
if( isExplicitVariableDeclaration )
{
final Map vars = jc.getVars();
if( vars.doesKeyExist( val ) )
{
return vars.get( val );
}
else
{
throw new UnknownVariableException( val );
}
}
else
{
return jc.getVars().get( val );
}
-pete
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by Geir Magnusson Jr <ge...@4quarters.com>.
On Jan 11, 2004, at 3:28 PM, peter royal wrote:
> On Jan 11, 2004, at 2:54 PM, Bill Horsman wrote:
>> On Sun, 2004-01-11 at 20:46, peter royal wrote:
>>
>>>> So if implement the Map interface (or expose it) then you run the
>>>> risk
>>>> of another class just calling get() instead of getVar() and
>>>> bypassing
>>>> my
>>>> strict variable check.
>>>
>>> Unless the new exception was a subclass of RuntimeException :)
>>
>> Yep, that's true. Well, I can be persuaded either way. To extend Map
>> or
>> just to use it. My gut instinct still says the composition is better.
>
> Your gut is right, I just worry about the change to the client
> contract.. This would no longer be a "drop-in" upgrade. I'll ping Geir
> and get his opinion on this too..
Ok - I read the thread, and I think it's not a good idea to totally
break usage because Map can't throw an exception (and no, I don't think
a RuntimeException is the right way either, as that also has far
reaching complications for dropping into existing uses....)
To be clear, I *really* don't think it's a good idea to break usage.
:) Having getVars() return a j.u.Map means that it's easy to use and
simple to implement for alternative backing stores....
Now, I certainly agree that this would be darn useful, and I see two
ways (offhand) to approach it :
1) Your solution, where the context implementation has to understand
and know about all this magic jexl stuff
or
2) A different approach, where the context is simple (allowing
recycling w/o much effort of other backing impls) and Jexl is smart
Regarding #2, I can see 2 approaches.
One is to do it during evaluate(), like you proposed, which will change
the state of some of the objects in the context, and not all if
something is wrong :
nuclearReactor.start(); nuclearReactor.setTemparature(temp);
nuclearReactor.stop();
Granted, I know that you can't always ensure you get through, but
still, adding another way causes concern. On the flipside, it's
simple....
The other would be to have a checker that takes an expression and
figures out if it will go given a context. That leads to a two step
execution process, where you'd first run the check and then
evaluate()....
Comments?
geir
--
Geir Magnusson Jr 203-247-1713(m)
geir@4quarters.com
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by peter royal <pr...@apache.org>.
On Jan 11, 2004, at 2:54 PM, Bill Horsman wrote:
> On Sun, 2004-01-11 at 20:46, peter royal wrote:
>
>>> So if implement the Map interface (or expose it) then you run the
>>> risk
>>> of another class just calling get() instead of getVar() and bypassing
>>> my
>>> strict variable check.
>>
>> Unless the new exception was a subclass of RuntimeException :)
>
> Yep, that's true. Well, I can be persuaded either way. To extend Map or
> just to use it. My gut instinct still says the composition is better.
Your gut is right, I just worry about the change to the client
contract.. This would no longer be a "drop-in" upgrade. I'll ping Geir
and get his opinion on this too..
-pete
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by Bill Horsman <Bi...@logicalcobwebs.co.uk>.
On Sun, 2004-01-11 at 20:46, peter royal wrote:
> > So if implement the Map interface (or expose it) then you run the risk
> > of another class just calling get() instead of getVar() and bypassing
> > my
> > strict variable check.
>
> Unless the new exception was a subclass of RuntimeException :)
Yep, that's true. Well, I can be persuaded either way. To extend Map or
just to use it. My gut instinct still says the composition is better.
-
Bill
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by peter royal <pr...@apache.org>.
On Jan 10, 2004, at 5:56 PM, Bill Horsman wrote:
>> Do you mean the methods of the java.util.Map interface constrain the
>> implementation ?
>
> That's not what I meant, but actually it does :) My getVar() method
> throws an UnknownVariableException and in order to maintain that API I
> would have to make the Map's get() method also throw that Exception.
> Only you can't do that.
>
> So if implement the Map interface (or expose it) then you run the risk
> of another class just calling get() instead of getVar() and bypassing
> my
> strict variable check.
Unless the new exception was a subclass of RuntimeException :)
-pete
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by Bill Horsman <bi...@logicalcobwebs.co.uk>.
On Sat, 2004-01-10 at 23:43, Paul Libbrecht wrote:
> Do you mean the methods of the java.util.Map interface constrain the
> implementation ?
That's not what I meant, but actually it does :) My getVar() method
throws an UnknownVariableException and in order to maintain that API I
would have to make the Map's get() method also throw that Exception.
Only you can't do that.
So if implement the Map interface (or expose it) then you run the risk
of another class just calling get() instead of getVar() and bypassing my
strict variable check.
For completeness, my original point was that the Map interface wasn't
restrictive enough. We can always add additional Map-like methods if
people need them: clear() for instance.
-
Bill
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by Paul Libbrecht <pa...@activemath.org>.
On 10-Jan-04, at 23:02 Uhr, Bill Horsman wrote:
> Another advantage of not implementing the MAP interface directly is
> that
> you then have the flexibility of choosing another storage method
> without
> breaking the JexlContext implementation.
Do you mean the methods of the java.util.Map interface constrain the
implementation ?
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by Bill Horsman <bi...@logicalcobwebs.co.uk>.
Hi Paul,
On Sat, 2004-01-10 at 22:39, Paul Libbrecht wrote:
> But this getVars could return a map that still uses your getVars, or ?
> It's more work, I know, but it's the power of interfaces in principle.
>
> I think getVars() can be quite of use in some context.
Yep, I suppose so. But I like the idea of a very small API and only
adding methods as required. IMHO anyway.
Another advantage of not implementing the MAP interface directly is that
you then have the flexibility of choosing another storage method without
breaking the JexlContext implementation.
I appreciate your feedback - much better than being ignored :)
-
Bill
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [PATCH] Re: [jexl] - checking for unresolved variables
Posted by Paul Libbrecht <pa...@activemath.org>.
On 10-Jan-04, at 20:21 Uhr, Bill Horsman wrote:
> [...] Then I decided to make HashMapContext use the composition
> pattern to
> contain a HashMap rather than extend it. This allowed me to ensure that
> everything else used my new getVar() method rather than getting stuff
> directly from the Map. The getVar() method contains the code that
> decides whether to throw an Exception (strict) or return a null
> (lenient).
Let me say that such a thing really tastes good to me. I've been
complaining immensely about the too lax tolerance of null and I think
the major reason is that of undefined variables.
> This meant changes to JexlContext and quite a few changes (mostly to
> test classes) that were using jc.getVars() to get direct access to the
> Map. IMHO, it is a bad idea to expose the map in that way.
But this getVars could return a map that still uses your getVars, or ?
It's more work, I know, but it's the power of interfaces in principle.
I think getVars() can be quite of use in some context.
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org