Re: PathMatchingFilter and configuration

[Kalle Korhonen <ka...@gmail.com>]

Just FYI, I implemented the simplified logic for Tapestry/Shiro
integration as described. I'm quite happy how it turned out and in the
process I was able to obsolete big chunks of path handling code (for
example, see http://svn.codehaus.org/tynamo/trunk/tapestry-security/src/test/java/org/tynamo/security/testapp/services/AppModule.java).
I think the same approach would be quite valuable to the Shiro
community at large as well, though I personally have perhaps less
incentive now to push the changes upstream. Since the path matching
and path specific configuration is specific to Shiro filters, the
concerns Les raised earlier don't quite apply - obviously you can
share filter instances between different filter chains, it's just that
chain specific configuration is much simpler (and will perform better)
if you don't. If we decide to keep the (Shiro) issue open, perhaps we
can bring the topic up again when planning for 2.0.

Kalle


On Sat, Feb 12, 2011 at 5:10 PM, Kalle Korhonen
<ka...@gmail.com> wrote:
> On Sat, Feb 12, 2011 at 3:25 PM, Les Hazlewood <lh...@apache.org> wrote:
>> Actually, in thinking about this more, I don't think we should create
>> new Filter instances per chain by default, and here's why:
>> 1.  Shiro filter chains can contain _any_ javax.servlet.Filter
>
> Sure, which may the be exact problem. By the servlet spec, the filters
> are configured in web.xml. We've talked about that before and Shiro
> would also be a better fit for Play framework if these filters weren't
> servlet filters.
>
>> 2.  Creating a new Filter instance per chain can be a dangerous thing
>> to do - there are many filters that, upon startup/init, go through
>
> Sure it could, but that's certainly addressable if there were any such
> filters with significant initialization logic. Even then, it's
> one-time cost instead of recurring cost.
>
>> I don't know what Tapestry Security supports, but if you're allowed to
>> configure any javax.servlet.Filter like you can in Shiro's INI, I
>
> Currently it does, but I just opened an issue for it and specifically
> noted that they shouldn't be allowed anymore
> (http://jira.codehaus.org/browse/TYNAMO-76). BTW, Tapestry is
> implemented as a filter as well, so obviously you can use *standard*
> servlet filters together with Tapestry as you see fit.
>
>> Finally, and please correct me if I'm wrong, but your main concern in
>> this thread isn't really about new instances vs pooled instances (if
>> it was, I'm assuming you'd argue that the Servlet containers are doing
>> it incorrectly as well, since we just retain the same behavior).  It
>> is almost entirely driven by your concern about performance due to
>> executing path matching too often, correct?
>
> Right about the performance, that's what the PathMatchingFilter issue
> all is about. Neither issue I opened says anything about the pool
> either way. Absolutely wrong assumption on the servlet container
> behavior: that comparison is off - there's no behavior such as filter
> chains. By the spec, each filter includes the mapping configuration in
> itself and they are all being compared to, similar to executing them
> all as separate chains.
>
> As is, please provide closing comments to
> https://issues.apache.org/jira/browse/SHIRO-256 for future reference.
>
> Kalle
>

Re: PathMatchingFilter and configuration

[Kalle Korhonen <ka...@gmail.com>]

On Tue, May 31, 2011 at 8:01 PM, Les Hazlewood <le...@hazlewood.com> wrote:
> Sounds good to me - can you please add this to the 2.0 brainstorming page?

Done.

Re: PathMatchingFilter and configuration

[Les Hazlewood <le...@hazlewood.com>]

Sounds good to me - can you please add this to the 2.0 brainstorming page?

I've got some ideas around how to eliminate the path matching behavior
per filter that also makes filter logic more 'pluggable' as well
(without forcing subclassing).

Best,

Les

On Tue, May 31, 2011 at 4:57 PM, Kalle Korhonen
<ka...@gmail.com> wrote:
> Just FYI, I implemented the simplified logic for Tapestry/Shiro
> integration as described. I'm quite happy how it turned out and in the
> process I was able to obsolete big chunks of path handling code (for
> example, see http://svn.codehaus.org/tynamo/trunk/tapestry-security/src/test/java/org/tynamo/security/testapp/services/AppModule.java).
> I think the same approach would be quite valuable to the Shiro
> community at large as well, though I personally have perhaps less
> incentive now to push the changes upstream. Since the path matching
> and path specific configuration is specific to Shiro filters, the
> concerns Les raised earlier don't quite apply - obviously you can
> share filter instances between different filter chains, it's just that
> chain specific configuration is much simpler (and will perform better)
> if you don't. If we decide to keep the (Shiro) issue open, perhaps we
> can bring the topic up again when planning for 2.0.
>
> Kalle
>
>
> On Sat, Feb 12, 2011 at 5:10 PM, Kalle Korhonen
> <ka...@gmail.com> wrote:
>> On Sat, Feb 12, 2011 at 3:25 PM, Les Hazlewood <lh...@apache.org> wrote:
>>> Actually, in thinking about this more, I don't think we should create
>>> new Filter instances per chain by default, and here's why:
>>> 1.  Shiro filter chains can contain _any_ javax.servlet.Filter
>>
>> Sure, which may the be exact problem. By the servlet spec, the filters
>> are configured in web.xml. We've talked about that before and Shiro
>> would also be a better fit for Play framework if these filters weren't
>> servlet filters.
>>
>>> 2.  Creating a new Filter instance per chain can be a dangerous thing
>>> to do - there are many filters that, upon startup/init, go through
>>
>> Sure it could, but that's certainly addressable if there were any such
>> filters with significant initialization logic. Even then, it's
>> one-time cost instead of recurring cost.
>>
>>> I don't know what Tapestry Security supports, but if you're allowed to
>>> configure any javax.servlet.Filter like you can in Shiro's INI, I
>>
>> Currently it does, but I just opened an issue for it and specifically
>> noted that they shouldn't be allowed anymore
>> (http://jira.codehaus.org/browse/TYNAMO-76). BTW, Tapestry is
>> implemented as a filter as well, so obviously you can use *standard*
>> servlet filters together with Tapestry as you see fit.
>>
>>> Finally, and please correct me if I'm wrong, but your main concern in
>>> this thread isn't really about new instances vs pooled instances (if
>>> it was, I'm assuming you'd argue that the Servlet containers are doing
>>> it incorrectly as well, since we just retain the same behavior).  It
>>> is almost entirely driven by your concern about performance due to
>>> executing path matching too often, correct?
>>
>> Right about the performance, that's what the PathMatchingFilter issue
>> all is about. Neither issue I opened says anything about the pool
>> either way. Absolutely wrong assumption on the servlet container
>> behavior: that comparison is off - there's no behavior such as filter
>> chains. By the spec, each filter includes the mapping configuration in
>> itself and they are all being compared to, similar to executing them
>> all as separate chains.
>>
>> As is, please provide closing comments to
>> https://issues.apache.org/jira/browse/SHIRO-256 for future reference.
>>
>> Kalle
>>
>