Re: [xwiki-devs] [Proposal] Change our Script API best practice regarding exception handling
Hi Thomas and all, On 15 Jan 2016 at 17:53:04, Thomas Mortagne ([email protected](mailto:[email protected])) 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, [email protected] 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, [email protected] ([email protected](mailto:[email protected])) 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, [email protected] ([email protected](mailto:[email protected])) wrote:
On 15 Jan 2016 at 13:26:08, [email protected] ([email protected](mailto:[email protected])) wrote:
On 15 Jan 2016 at 13:23:07, [email protected] ([email protected](mailto:[email protected])) 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, [email protected] ([email protected](mailto:[email protected])) 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#HBestPrac... > > > > > > >> > ) > > > > > > >> > > > > > > > >> > 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]
Also note that this proposal below also works when mixing #try() and ExceptionCatchingUberspector. For example: #try() call1() <— raises exception call2() #end Explanation: * call1() is caught by ExceptionCatchingUberspector, which sets the exceptions * when call2() is called, ExceptionCatchingUberspector runs and since the exception has not been handled it’s raised * the TryCatch directive’s catch() is thus called and the error is caught Thanks -Vincent On 16 Jan 2016 at 20:09:50, [email protected] ([email protected](mailto:[email protected])) wrote:
Hi Thomas and all,
On 15 Jan 2016 at 17:53:04, Thomas Mortagne ([email protected](mailto:[email protected])) 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, [email protected] 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, [email protected] ([email protected](mailto:[email protected])) 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, [email protected] ([email protected](mailto:[email protected])) wrote:
On 15 Jan 2016 at 13:26:08, [email protected] ([email protected](mailto:[email protected])) wrote:
On 15 Jan 2016 at 13:23:07, [email protected] ([email protected](mailto:[email protected])) 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, [email protected] ([email protected](mailto:[email protected])) 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#HBestPrac... > > > > > > > >> > ) > > > > > > > >> > > > > > > > > >> > 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]
Ok so after more thinking here’s what’s my take on this topic now (it’s a change from what I was initially proposing as best practice): 1) Using the ExceptionCatcherUberspector I’ve highlighted in my previous mails we’re able to modify the existing script APIs to throw Exception without having to modify any calling script. This is great since it allows our Java code to throw exceptions. This is step 1 of the global plan. 2) However I believe that on the scripting side, the best practice that we should aim at is to use the #try() directive. There are 2 reasons for that: a) it makes coding in Velocity closer to any other scripting language b) it’s the most flexible strategy since it allows to capture exceptions for a single call or for a series of calls while ensuring that there are no side effects (ie no code is executed after an exception is raised, unless it’s voluntary). This wouldn’t be the case if we were advocating for the other approach based on checking the result of each call caught by the ExceptionCatcherUberspector. Here are some examples: Example 1: single statement check #set ($result = $var.call1()) #if ($error) ... do something ... #else ... do something else ... #end #set ($result2 = $var.call2()) ... vs #try() #set ($result = $var.call1()) #end #if ($exception) ... do something ... #else ... do something else ... #end #set ($result2 = $var.call2()) ... On this example the ExceptionCatcherUberspector way is possibly slightly better. Example 2: multiple statements check #set ($result = $var.call1()) #set ($result2 = $var.call2()) #if ($error) <---- problem if an error raised by call1() causes a side effect in the 2nd statement ... do something ... #else ... do something else ... #end ... vs #try() #set ($result = $var.call1()) #set ($result2 = $var.call2()) #end #if ($exception) ... do something ... #else ... do something else ... #end ... On this last example the #try way is definitely the best since call2() is never called. They way to do this with the ExceptionCatcherUberspector way would be to litter each java call with a check, which is too verbose by far and would make the code unreadable. So conclusion: =============== Our best practice for writing velocity scripts should be to use the #try directive when we wish to handle exceptions. In the majority of the cases we won’t need to and the exceptions will be caught either by the MacroTransformation, generating an error block to the user when rendering happens, or it will be caught by the contentvars.vm template, also generating error stack trace to the user. However, before we can get to this, we could have an ExceptionCatcherUberspector implemented, which would allow us to migrate scripting APIs in Java to throw Exceptions while still keeping backward compatibility with scripts that do stuff like the following to handle exceptions (which return null in most of our script APIs ATM): #set ($result = $var.call1()) #if ($result) ... #else ... #end Thus new scripts should be written to not catch anything if they don’t need to handle exceptions in a special way (letting it bubble) or use the #try directive. Existing scripts can continue to use the old way till we decide to migrate them to the new way. At some point in the future, we could decide to remove the ExceptionCatcherUberspector from the default velocity configuration and leave it as something optional. WDYT? Thanks -Vincent On 16 Jan 2016 at 21:16:29, [email protected] ([email protected](mailto:[email protected])) wrote:
Also note that this proposal below also works when mixing #try() and ExceptionCatchingUberspector. For example:
#try() call1() <— raises exception call2() #end
Explanation: * call1() is caught by ExceptionCatchingUberspector, which sets the exceptions * when call2() is called, ExceptionCatchingUberspector runs and since the exception has not been handled it’s raised * the TryCatch directive’s catch() is thus called and the error is caught
Thanks -Vincent
On 16 Jan 2016 at 20:09:50, [email protected] ([email protected](mailto:[email protected])) wrote:
Hi Thomas and all,
On 15 Jan 2016 at 17:53:04, Thomas Mortagne ([email protected](mailto:[email protected])) 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, [email protected] 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, [email protected] ([email protected](mailto:[email protected])) 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, [email protected] ([email protected](mailto:[email protected])) wrote:
On 15 Jan 2016 at 13:26:08, [email protected] ([email protected](mailto:[email protected])) wrote:
> > On 15 Jan 2016 at 13:23:07, [email protected] ([email protected](mailto:[email protected])) 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, [email protected] ([email protected](mailto:[email protected])) 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#HBestPrac... > > > > > > > > >> > ) > > > > > > > > >> > > > > > > > > > >> > 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]
On Sat, Jan 16, 2016 at 8:09 PM, [email protected] <[email protected]> wrote:
Hi Thomas and all,
On 15 Jan 2016 at 17:53:04, Thomas Mortagne ([email protected](mailto:[email protected])) 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.
Not sure why you want to migrate existing code. Modifying existing methods to make them throw exceptions would be an API breakage.
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, [email protected] 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, [email protected] ([email protected](mailto:[email protected])) 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, [email protected] ([email protected](mailto:[email protected])) wrote:
On 15 Jan 2016 at 13:26:08, [email protected] ([email protected](mailto:[email protected])) wrote:
On 15 Jan 2016 at 13:23:07, [email protected] ([email protected](mailto:[email protected])) 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, [email protected] ([email protected](mailto:[email protected])) 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#HBestPrac... > > > > > > > >> > ) > > > > > > > >> > > > > > > > > >> > 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]
devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne
On 18 Jan 2016 at 09:41:02, Thomas Mortagne ([email protected](mailto:[email protected])) wrote:
On Sat, Jan 16, 2016 at 8:09 PM, [email protected] wrote:
Hi Thomas and all,
On 15 Jan 2016 at 17:53:04, Thomas Mortagne ([email protected](mailto:[email protected])) 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.
Not sure why you want to migrate existing code. Modifying existing methods to make them throw exceptions would be an API breakage.
hmm indeed I was thinking in term of Velocity for which it wouldn’t be an api breakage with the ExceptionCatchingUberspector but I forgot the other scripting languages for which it would be a breakage. So the conclusion is that we’re not able to change the server API without changing the client side at the same time, which means it’s almost impossible to change and the new best practice would apply only to new scripting APIs. <side note> If we wanted it to work we would need to have the ScriptServiceManager return dynamic proxies fo the asked Script Service and handle the try/catch in those dynamic proxies. Note that this is probably not hard to do but I don’t think we’d like this best practice and instead I feel it’s a better practice to catch exceptions on the scripting side. </side note> So in conclusion, only point 2) remains (form my last email), i.e. the usage of the #try directive, which already exist. And thus the ExceptionCatchingUberspector is not interesting IMO. I’ll create a jira issue for it and attach the code I have so far for it, and then close the issue as won’t fix (for future reference). I’ll send a new proposal to summarize this properly. Thanks -Vincent
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, [email protected] 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, [email protected] ([email protected](mailto:[email protected])) 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, [email protected] ([email protected](mailto:[email protected])) wrote:
On 15 Jan 2016 at 13:26:08, [email protected] ([email protected](mailto:[email protected])) wrote:
> > On 15 Jan 2016 at 13:23:07, [email protected] ([email protected](mailto:[email protected])) 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, [email protected] ([email protected](mailto:[email protected])) 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#HBestPrac... > > > > > > > > >> > ) > > > > > > > > >> > > > > > > > > > >> > 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]
devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
On 18 Jan 2016 at 10:02:37, [email protected] ([email protected](mailto:[email protected])) wrote:
On 18 Jan 2016 at 09:41:02, Thomas Mortagne ([email protected](mailto:[email protected])) wrote:
On Sat, Jan 16, 2016 at 8:09 PM, [email protected] wrote:
Hi Thomas and all,
On 15 Jan 2016 at 17:53:04, Thomas Mortagne ([email protected](mailto:[email protected])) 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.
Not sure why you want to migrate existing code. Modifying existing methods to make them throw exceptions would be an API breakage.
hmm indeed I was thinking in term of Velocity for which it wouldn’t be an api breakage with the ExceptionCatchingUberspector but I forgot the other scripting languages for which it would be a breakage. So the conclusion is that we’re not able to change the server API without changing the client side at the same time, which means it’s almost impossible to change and the new best practice would apply only to new scripting APIs.
If we wanted it to work we would need to have the ScriptServiceManager return dynamic proxies fo the asked Script Service and handle the try/catch in those dynamic proxies. Note that this is probably not hard to do but I don’t think we’d like this best practice and instead I feel it’s a better practice to catch exceptions on the scripting side.
So in conclusion, only point 2) remains (form my last email), i.e. the usage of the #try directive, which already exist. And thus the ExceptionCatchingUberspector is not interesting IMO. I’ll create a jira issue for it and attach the code I have so far for it, and then close the issue as won’t fix (for future reference).
Done here: http://jira.xwiki.org/browse/XCOMMONS-906
I’ll send a new proposal to summarize this properly.
Also done: http://markmail.org/message/e77r6zn45teghrmz Thanks -Vincent
Thanks -Vincent
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, [email protected] 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, [email protected] ([email protected](mailto:[email protected])) 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, [email protected] ([email protected](mailto:[email protected])) wrote:
> > > > > > On 15 Jan 2016 at 13:26:08, [email protected] ([email protected](mailto:[email protected])) wrote: > > > > > On 15 Jan 2016 at 13:23:07, [email protected] ([email protected](mailto:[email protected])) 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, [email protected] ([email protected](mailto:[email protected])) 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#HBestPrac... > > > > > > > > > >> > ) > > > > > > > > > >> > > > > > > > > > > >> > 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]
participants (2)
-
Thomas Mortagne -
vincent＠massol.net