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, vinc...@massol.net (vinc...@massol.net(mailto:vinc...@massol.net)) wrote: > Hi Thomas and all, > > On 15 Jan 2016 at 17:53:04, Thomas Mortagne > (thomas.morta...@xwiki.com(mailto:thomas.morta...@xwiki.com)) wrote: > > > What about making possible to enable/disable as a {{velocity}} macro > > parameter (disabled by default) ? > > It’s a very good idea but it raises questions for migrating existing code to > this strategy. I think we could do better with the proposal below. > > Wanted behavior: > > Mandatory: > * Throw Exceptions from script APIs. > * Easily handle raised exceptions from Velocity. > * Early exception-raising if exception is not handled, i.e. if we have 2 > statements in Velocity and the first one raises an exception we don’t want > the second one to execute, unless the velocity code tests for it. > > Nice to have: > * Ability to modify existing script APIs so that they all throw exceptions > without having to touch client scripts using those script APIs > > Let’s see some examples. > > * UC1: Server code is currently catching exception and calling code is in a > velocity macro (or in a template) > > ** Example of what we have now: > > Server: > public Object something() > { > try { > … > } catch (…) { > return null; > } > } > > Calling code: > {{velocity}} > #set ($result = $var.something()) > #if ($result) > ... > {{/velocity}} > > > ** Example of what we want to move to: > > Server: > public Object something() throws Exception > { > … > } > > Calling code: > The same code as now > > ** How it could work: > > The Exception Catching uberspector catches the exception raised by > something(), stores it into a velocity binding and returns null. Thus the > calling code doesn’t need to be modified, we’re good. > > * UC2: Server code is currently throwing exception and calling code is in a > velocity macro (or in a template) > > ** Example of what we have now: > > Server: > public Object something() throws Exception > { > ... > } > > Calling code: > {{velocity}} > #set ($result = $var.something()) > ... > {{/velocity}} > > ** Example of what we want to move to: > > Server: > The same code as now > > Calling code: > The same code as now > > ** How it could work: > > Currently if the calling code is in a velocity macro then the velocity macro > will throw the Exception and the MacroTransformation will catch it and > display an error box. If the calling code is in a template then it’s captured > with the #try directive and displayed. With the Exception Catching > uberspector, it’s caught and doesn’t bubble up. > > One solution for this use case is to wrap the captured Exception into an > ExceptionResult object such as: > > ExceptionResult > { > Exception getException(); > boolean hasException(); > boolean isHandled(); > } > > And when getException() is called then isHandled() returns true. > > Then modify: > * VelocityMacro so that if an ExceptionResult object exists in the Velocity > Context and isHandled() is false then raise the exception. And thus > MacroTransformation will also get it and will display an error box as before. > * Similarly, modify the Template Manager code to also throw an exception if > an unhandled exception is found > > This let the script writer to decide if he wants to handle the exception or > let it bubble up. > > * UC3: Several consecutive lines with an exception in calling code > > ** Example of what we have now: > > Server code: > Any code > > Calling code: > {{velocity}} > #set ($result = $var.something1()) <— throws an Exception > #set ($result = $var.something2()) > ... > {{/velocity}} > > ** Example of what we want to move to: > > Server: > The same code as now > > Calling code: > The same code as now > > > ** How it could work: > > Right now the velocity macro or template will display the first Exception > that happens in an error box and the second line (something2()) will not get > called. We need to do the same. > > The idea is simply to check in the Exception-catching uberspector is an > unhandled exception exists and if so, then raise it. > > * Conclusion: > > I think this strategy would not require to configuration at the level of the > velocity macro and that it could work for all cases. > > Do you see a case where it would not work? > > WDYT of it? > > Thanks > -Vincent > > FTR (and as a backup for me ;)) before I start modifying the code here are > what I had so far for TryCatchDirective and ExceptionCatchingUberspector: > * https://gist.github.com/vmassol/ec3c407f264091eeb4b3 and > https://gist.github.com/vmassol/83db5df84a4041ec018f > * https://gist.github.com/vmassol/c887cb92f76afa5fba4d > > > On Fri, Jan 15, 2016 at 5:41 PM, vinc...@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, vinc...@massol.net > > > (vinc...@massol.net(mailto:vinc...@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, vinc...@massol.net > > >> (vinc...@massol.net(mailto:vinc...@massol.net)) wrote: > > >> > > >> > > > >> > > > >> > > > >> > > > >> > > > >> > On 15 Jan 2016 at 13:26:08, vinc...@massol.net > > >> > (vinc...@massol.net(mailto:vinc...@massol.net)) wrote: > > >> > > > >> > > > > >> > > On 15 Jan 2016 at 13:23:07, vinc...@massol.net > > >> > > (vinc...@massol.net(mailto:vinc...@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, vinc...@massol.net > > >> > > > (vinc...@massol.net(mailto:vinc...@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#HBestPractices > > >> > > > > > > > > > >> > ) > > >> > > > > > > > > > >> > > > >> > > > > > > > > > >> > 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 devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs
