On Sat, Jan 16, 2016 at 8:09 PM, vinc...@massol.net <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.
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, 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
--
Thomas Mortagne
_______________________________________________
devs mailing list
devs@xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
