Re: [xwiki-devs] [Proposal] Change our Script API best practice regarding exception handling

On 18 Jan 2016 at 09:41:02, Thomas Mortagne (thomas.mortagne@xwiki.com(mailto:thomas.mortagne@xwiki.com)) wrote: 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). 

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, 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]