Re SLING-221 and SLING-222

[Felix Meschberger <fm...@gmail.com>]

Hi,

In Rev. 618697 [0]the following main chaines are introduced:

(1) Replace SlingScriptHelper.include(String path,
RequestDispatcherOptions options) signature by
SlingScriptHelper.include(String path, String options).

(2) The implementation of the include method uses a request attribute to
hand over the RequestDispatcherOptions, because there is a
RequestDispatcher getter for Resource and RequestDispatcherOptions but
not a getter for String and RequestDispatcherOptions

(3) SlingRequestDispatcher.include cuts off extensions (duplicating
ResourceResolver.resolve() code) and causes errors, which should not be
caused.


I would like to propose to change this as follows:


(1) I do not understand the reason to change the method signature and
further more I do not understand why a special option (resource type
override) should be special and treated as default. I would like to
revert that change.


(2) Instead of using a request attribute to hand over the
RequestDispatcherOptions, I suggest the addition of a
SlingHttpServletRequest method:

         RequestDispatcher getRequestDispatcher(String path,
                     RequestDispatcherOptions options);



(3) Instead of cutting off extensions and calling
ResourceResolver.getResource, the include method should just call
ResourceResolver.resource(String path) method, which applies the exact
same algorithm as ResourceResolver.resolve(HttpServletRequest) - except
that the former returns null if the resource cannot be found. This
avoids duplicate code. And of course, the include method should just log
problematic situations, end do nothing instead of throwing (if the
response object has the wrong type) or sending an error (if no resource
can be found).


WDYT ?

Regards
Felix

[0] http://svn.apache.org/viewvc?view=rev&revision=618697
[1] http://issues.apache.org/jira/browse/SLING-221
[2] http://issues.apache.org/jira/browse/SLING-222

Re: Re SLING-221 and SLING-222

[Tobias Bocanegra <to...@day.com>]

> > ..SlingScriptHelper.include(String path, String options)....
>
> > ...(1) I do not understand the reason to change the method signature...
>
> The SlingScriptHelper is available in scripts as the "sling" variable,
> so to call sling.include with options the natural way in scripts is to
> do
>
>   sling.include("somePath", "forceResourceType: my/widget");
>
> The code before my changes required the second parameter to be of type
> RequestDispatcherOptions, which is IMHO not practical in scripts.
>
> I don't mind keeping the old method as well if it's useful, but I
> think the new method (String path, String options) is needed as a
> script helper.
>
> > ...further more I do not understand why a special option (resource type
> > override) should be special and treated as default. I would like to
> > revert that change....
>
> The RequestDispatcherOptions allow several options: override resource
> type, replace selectors, etc... but I think in 90% of the cases people
> will just need to override the resource type.
>
> Hence the shortcut that I implemented
>
>   sling.include(somePath,"my/widgets");
>
> which is equivalent to
>
>   sling.include(somePath,"forceResourceType:my/widgets");
>
> I think it's convenient, and not risky as automated tests verify both syntaxes.
>
> > (2) The implementation of the include method uses a request attribute to
> > hand over the RequestDispatcherOptions...
>
> >... (2) Instead of using a request attribute to hand over the
> > RequestDispatcherOptions, I suggest the addition of a
> > SlingHttpServletRequest method...
>
> No problem with what you suggest.
>
> >... (3) SlingRequestDispatcher.include cuts off extensions (duplicating
> > ResourceResolver.resolve() code) and causes errors, which should not be
> > caused....
>
> > (3) Instead of cutting off extensions and calling
> > ResourceResolver.getResource, the include method should just call
> > ResourceResolver.resource(String path) method...
the resolution should work the same way as if it was an toplevel
request. so the dispatcher
must forget all about the previous selectors, extensions, etc. i think
the only thing that is 'saved'
are the request attributes.

> Ok with that - I wanted to get the tests running yesterday, and they
> did, but as I noted in the code there's probably a cleaner way to
> solve the problem than what I did. Feel free to fix that, the "final"
> tests to watch are in the IncludeTest class of the launchpad-webapp.
>
> > ...And of course, the include method should just log
> > problematic situations, end do nothing instead of throwing ...
>
> I'd be ok with that, but before my changes, a resource not found in
> include caused infinite loops. Either way is fine as long as runtime
> problems are easily identifiable by log warnings or exceptions, and do
> not cause infinite loops.

sidetnote: currently it is a bug problem to indetifying errors that
happen in includes. if the response was already committed they never
show up in the response. unfortunately the log show not good
explanation neither. mostly a huge stacktrace of NPEs. for example are
javac compile error of includes almost untracable - you need to attach
a debugger and catch the actuall error to get the error message.

regard, toby
-- 
-----------------------------------------< tobias.bocanegra@day.com >---
Tobias Bocanegra, Day Management AG, Barfuesserplatz 6, CH - 4001 Basel
T +41 61 226 98 98, F +41 61 226 98 97
-----------------------------------------------< http://www.day.com >---

Re: Re SLING-221 and SLING-222

[Bertrand Delacretaz <bd...@apache.org>]

Hi Felix,

On Feb 6, 2008 9:04 AM, Felix Meschberger <fm...@gmail.com> wrote:

> ..SlingScriptHelper.include(String path, String options)....

> ...(1) I do not understand the reason to change the method signature...

The SlingScriptHelper is available in scripts as the "sling" variable,
so to call sling.include with options the natural way in scripts is to
do

  sling.include("somePath", "forceResourceType: my/widget");

The code before my changes required the second parameter to be of type
RequestDispatcherOptions, which is IMHO not practical in scripts.

I don't mind keeping the old method as well if it's useful, but I
think the new method (String path, String options) is needed as a
script helper.

> ...further more I do not understand why a special option (resource type
> override) should be special and treated as default. I would like to
> revert that change....

The RequestDispatcherOptions allow several options: override resource
type, replace selectors, etc... but I think in 90% of the cases people
will just need to override the resource type.

Hence the shortcut that I implemented

  sling.include(somePath,"my/widgets");

which is equivalent to

  sling.include(somePath,"forceResourceType:my/widgets");

I think it's convenient, and not risky as automated tests verify both syntaxes.

> (2) The implementation of the include method uses a request attribute to
> hand over the RequestDispatcherOptions...

>... (2) Instead of using a request attribute to hand over the
> RequestDispatcherOptions, I suggest the addition of a
> SlingHttpServletRequest method...

No problem with what you suggest.

>... (3) SlingRequestDispatcher.include cuts off extensions (duplicating
> ResourceResolver.resolve() code) and causes errors, which should not be
> caused....

> (3) Instead of cutting off extensions and calling
> ResourceResolver.getResource, the include method should just call
> ResourceResolver.resource(String path) method...

Ok with that - I wanted to get the tests running yesterday, and they
did, but as I noted in the code there's probably a cleaner way to
solve the problem than what I did. Feel free to fix that, the "final"
tests to watch are in the IncludeTest class of the launchpad-webapp.

> ...And of course, the include method should just log
> problematic situations, end do nothing instead of throwing ...

I'd be ok with that, but before my changes, a resource not found in
include caused infinite loops. Either way is fine as long as runtime
problems are easily identifiable by log warnings or exceptions, and do
not cause infinite loops.

-Bertrand