16 Jan
2016
16 Jan
'16
8:09 p.m.
Hi Thomas and all,
On 15 Jan 2016 at 17:53:04, Thomas Mortagne
(thomas.mortagne@xwiki.com(mailto:thomas.mortagne@xwiki.com)) wrote:
What about making possible to enable/disable as a
{{velocity}} macro
parameter (disabled by default) ?
It’s a very good idea but it raises questions for migrating existing code to this
strategy. I think we could do better with the proposal below.
Wanted behavior:
Mandatory:
* Throw Exceptions from script APIs.
* Easily handle raised exceptions from Velocity.
* Early exception-raising if exception is not handled, i.e. if we have 2 statements in
Velocity and the first one raises an exception we don’t want the second one to execute,
unless the velocity code tests for it.
Nice to have:
* Ability to modify existing script APIs so that they all throw exceptions without having
to touch client scripts using those script APIs
Let’s see some examples.
* UC1: Server code is currently catching exception and calling code is in a velocity macro
(or in a template)
** Example of what we have now:
Server:
public Object something()
{
try {
…
} catch (…) {
return null;
}
}
Calling code:
{{velocity}}
#set ($result = $var.something())
#if ($result)
...
{{/velocity}}
** Example of what we want to move to:
Server:
public Object something() throws Exception
{
…
}
Calling code:
The same code as now
** How it could work:
The Exception Catching uberspector catches the exception raised by something(), stores it
into a velocity binding and returns null. Thus the calling code doesn’t need to be
modified, we’re good.
* UC2: Server code is currently throwing exception and calling code is in a velocity macro
(or in a template)
** Example of what we have now:
Server:
public Object something() throws Exception
{
...
}
Calling code:
{{velocity}}
#set ($result = $var.something())
...
{{/velocity}}
** Example of what we want to move to:
Server:
The same code as now
Calling code:
The same code as now
** How it could work:
Currently if the calling code is in a velocity macro then the velocity macro will throw
the Exception and the MacroTransformation will catch it and display an error box. If the
calling code is in a template then it’s captured with the #try directive and displayed.
With the Exception Catching uberspector, it’s caught and doesn’t bubble up.
One solution for this use case is to wrap the captured Exception into an ExceptionResult
object such as:
ExceptionResult
{
Exception getException();
boolean hasException();
boolean isHandled();
}
And when getException() is called then isHandled() returns true.
Then modify:
* VelocityMacro so that if an ExceptionResult object exists in the Velocity Context and
isHandled() is false then raise the exception. And thus MacroTransformation will also get
it and will display an error box as before.
* Similarly, modify the Template Manager code to also throw an exception if an unhandled
exception is found
This let the script writer to decide if he wants to handle the exception or let it bubble
up.
* UC3: Several consecutive lines with an exception in calling code
** Example of what we have now:
Server code:
Any code
Calling code:
{{velocity}}
#set ($result = $var.something1()) <— throws an Exception
#set ($result = $var.something2())
...
{{/velocity}}
** Example of what we want to move to:
Server:
The same code as now
Calling code:
The same code as now
** How it could work:
Right now the velocity macro or template will display the first Exception that happens in
an error box and the second line (something2()) will not get called. We need to do the
same.
The idea is simply to check in the Exception-catching uberspector is an unhandled
exception exists and if so, then raise it.
* Conclusion:
I think this strategy would not require to configuration at the level of the velocity
macro and that it could work for all cases.
Do you see a case where it would not work?
WDYT of it?
Thanks
-Vincent
FTR (and as a backup for me ;)) before I start modifying the code here are what I had so
far for TryCatchDirective and ExceptionCatchingUberspector:
* https://gist.github.com/vmassol/ec3c407f264091eeb4b3 and https://gist.github.com/vmassol/83db5df84a4041ec018f
* https://gist.github.com/vmassol/c887cb92f76afa5fba4d
On Fri, Jan 15, 2016 at 5:41 PM, vincent(a)massol.net
wrote:
> I need to think more about this because it means velocity macros won’t show an error
box anymore in case of error….
>
> Thanks
> -Vincent
>
>
> On 15 Jan 2016 at 17:20:21, vincent(a)massol.net
(vincent@massol.net(mailto:vincent@massol.net)) wrote:
>
>> Ok I’ve finished the implementation and will be committing soon.
>>
>>
>> Note that this could change slightly what the user sees.
>>
>> For example in the past if you had this velocity script:
>>
>> {{velocity}}
>> $someclass.someMethodThrowingException()
>> … rest of the script here...
>> {{/velocity}}
>>
>>
>> The user would see a stack trace on his screen before upgrading to 8.0 and after
upgrading to 8.0 he’ll see something different which depends on what the “… rest of the
script here…” does.
>>
>> What I don’t like too much is that this could lead to unwanted side effects. For
example, imagine that we have the following:
>>
>>
>> {{velocity}}
>> #set ($docReference =
$someclass.someMethodComputingTheReferenceButThrowingException())
>> …
>> $xwiki.getDocument($docReference).save()
>> ...
>> {{/velocity}}
>>
>> Then if an exception is raised in
someMethodComputingTheReferenceButThrowingException() it used to display an exception but
will now do something else. It happens that in this case it’ll report a NPE (AFAICS in the
code) but we could imagine it would create a wrong document in the wiki, etc.
>>
>> Any thoughts about this? Should I still go ahead (I think so).
>>
>> Thanks
>> -Vincent
>>
>>
>> On 15 Jan 2016 at 13:37:39, vincent(a)massol.net
(vincent@massol.net(mailto:vincent@massol.net)) wrote:
>>
>> >
>> >
>> >
>> >
>> >
>> > On 15 Jan 2016 at 13:26:08, vincent(a)massol.net
(vincent@massol.net(mailto:vincent@massol.net)) wrote:
>> >
>> > >
>> > > On 15 Jan 2016 at 13:23:07, vincent(a)massol.net
(vincent@massol.net(mailto:vincent@massol.net)) wrote:
>> > >
>> > > > Forget what I’ve said regarding the #try directive.
>> > > >
>> > > > Actually the way to use it is:
>> > > >
>> > > > #try()
>> > > > … wrap some script snipper, one or several calls…
>> > > > #end
>> > > > .. here only you can access the $exception binding.
>> > > >
>> > > > Thus:
>> > > >
>> > > > * Wrapping the whole content to evaluate won’t work
>> > > > * The Exception Catching Uberspector is really what we need. It’s
more fine-grained than the #try directive (or it’s equivalent if you use #try() for each
line of script calling methods).
>> > > >
>> > > > In addition:
>> > > >
>> > > > * We provide script APIs in xwiki-commons too so if we want this
best practice of throwing exceptions in script APIs, we also need to make this work in
xwiki-commons when used outside of XWiki. Thus we need a solution there too.
>> > > > ** A) One solution would be to use XWiki’s Context and make the
last exception available with $xcontext.lastVelocityException but it’s not very natural, I
hope we can find a better solution
>> > > > ** B) Another solution is to move VelocityManager to commons in
xwiki-commons-velocity, offer a basic impl without skin support and override it in
xwiki-platform to add the platform-specific stuff.
>> > > >
>> > > > I’ll start exploring B). Let me know if you don’t agree or if you
have a better idea.
>> > >
>> > > Actually VelocityManager is already in commons, it’s the impl that is
in platform. So right now it will work for us; it just won’t work for users who use just
commons. Like if you use the Rendering in standalone mode.
>> >
>> > Actually forget that, I don’t need to use VelocityManager in practice (and
it would be costly to do so). I just need to use the Execution component:
>> >
>> > VelocityContext vcontext =
>> > (VelocityContext) this.execution.getContext().getProperty(
>> > VelocityExecutionContextInitializer.VELOCITY_CONTEXT_ID);
>> >
>> >
>> > Thanks
>> > -Vincent
>> >
>> > Ps: Sorry for the live design noise...
>> >
>> > >
>> > > Thanks
>> > > -Vincent
>> > >
>> > > > Thanks
>> > > > -Vincent
>> > > >
>> > > >
>> > > > On 15 Jan 2016 at 12:15:41, vincent(a)massol.net
(vincent@massol.net(mailto:vincent@massol.net)) wrote:
>> > > >
>> > > > >
>> > > > > [snip]
>> > > > >
>> > > > > > > > > > >> > Hi devs,
>> > > > > > > > > > >> >
>> > > > > > > > > > >> > Right now our
strategy is for script services and script APIs in general
>> > > > > > > > > > >> > to catch exceptions,
store them and offer a getLastError() method to get
>> > > > > > > > > > >> > them (see
>> > > > > > > > > > >> >
http://extensions.xwiki.org/xwiki/bin/view/Extension/Script+Module#HBestPra…
>> > > > > > > > > > >> > )
>> > > > > > > > > > >> >
>> > > > > > > > > > >> > However it would be
much nicer to:
>> > > > > > > > > > >> > * Let our script
services generate exceptions
>> > > > > > > > > > >> > * Offer a velocity
script service to get the last exception raised by a
>> > > > > > > > > > >> > java call from
velocity
>> > > > > > > > > > >> > * Implement this
uberspector to catch the exceptions and to set them in
>> > > > > > > > > > >> > the execution
context
>> > > > > > > > > > >> >
>> > > > > > > > > > >> > That should be quite
easy to implement IMO.
>> > > > > > > > > > >> >
>> > > > > > > > > > >> > WDYT?
>> > > > > > > > > > >> >
>> > > > > > > > > > >>
>> > > > > > > > > > >> +1, it's a pain to
call setLastError() everywhere there can be an exception
>> > > > > > > > > > >> thrown, and we almost
always forget to do it (for this reason).
>> > > > > > > > > > >>
>> > > > > > > > > > >> Note that we also have the
#try() directive now.
>> > > > > > > > > > >
>> > > > > > > > > > > Yes, I should have mentioned
that there’s indeed also this possibility:
>> > > > > > > > > > > * Have script API throw
Exceptions
>> > > > > > > > > > > * Force velocity script users
to wrap their code with the try directive when they need to catch exceptions
>> > > > > > > > > > >
>> > > > > > > > > > > I still believe that the use
of the Exception-catching uberspector is better.
>> > > > > > > > > > >
>> > > > > > > > > > > WDYT?
>> > > > > > > > > >
>> > > > > > > > > > Does it mean you plan to get rid of
new #try directive ? Because it
>> > > > > > > > > > will be broken with this new
uberspector.
>> > > > > > > > >
>> > > > > > > > > That’s a good point, I had not thought
about the implementation at this stage.
>> > > > > > > > >
>> > > > > > > > > I think this could still work. When the
#try directive is used I’d just have to setup some flag somewhere in Velocity and in the
uberspector I could check if this flag is set and if so then don’t catch the exception.
>> > > > > > > >
>> > > > > > > >
>> > > > > > > > Actually, thinking more, I think you’re right
and that the #try directive plays exactly the same role as an Exception-catching
uberspector and I don’t see the need for the #try directive if we provide an uberspector.
>> > > > > > > >
>> > > > > > > > So I’m proposing to deprecate it but still
keep it for backward compatibility for now (probably a full cycle).
>> > > > > > > >
>> > > > > > > > WDYT?
>> > > > > > >
>> > > > > > > Note that I’d like to change a bit the proposal
and instead of making the exception available from a script service, I’d prefer to make it
available as a known velocity binding such as $lastException. There’s no reason to use a
script service since that would mean it would work for all scripts and in this case we
only want it to work for Velocity.
>> > > > > > >
>> > > > > > > Since there’s no way to get the Velocity Context
from within an uberspector, I’ll get it by using our Component Manager and get the
VelocityManager component and call getVelocityContext()… If you know a better way, let me
know.
>> > > > > >
>> > > > > > hmm… this would mean that I’d need to put this new
uberspector in xwiki-platform since VelocityManager is in platform ATM… (@Thomas: our
discussion of yesterday ;)).
>> > > > >
>> > > > >
>> > > > > There’s an alternative, which is to modify our
implementation of VelocityEngine.evaluate() and decorate the source with a #try()
directive so that it’s always called (and make sure that calling it nested won’t affect it
for backward compat).
>> > > > >
>> > > > > This could be simpler to implement and doesn’t force us to
move some velocity code to platform.
>> > > > >
>> > > > > WDYT?
>> > > > >
>> > > > > Thanks
>> > > > > -Vincent
>> > > > >
>> > > > > [snip]