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, vinc...@massol.net (vinc...@massol.net(mailto:vinc...@massol.net)) 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, 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
