Re: Override PathMatchingFilter

[Les Hazlewood <lh...@apache.org>]

On Sat, Feb 5, 2011 at 10:24 PM, Kalle Korhonen
<ka...@gmail.com> wrote:
> Thanks Barry. Been looking into it since I saw your report and there's
> a lot of stuff that is rotten there. First of all, initializing
> IniShiroFilter, creating the IniFilterChainResolverFactory, setting
> the FilterChainResolver and instantiating the default filters are all
> strongly coupled together so there's no easy way to override the
> default behavior. PathMatchingFilterChainResolver allows setting a
> different PatternMatcher but a PathMatchingFilter doesn't and worse
> yet, they are using different pattern matchers.

"Rotten"? "Worse yet"?  That's a bit harsh regarding something that
has been working well for the vast number of our end users IMHO.  This
is the first such issue, long since the code has existed, so I would
see that as an indicator of it's success.  Also the code in question
was available to you for peer review the entire time it was being
built, so why not raise the concern then instead of criticizing a year
later?  In my own personal opinion, I believe we're an open, helpful
and benevolent community, and criticism should be constructive without
being derisive.

Anyway, the IniShiroFilter was not initially intended to be easily
'overridable' - it was created as a means for web.xml users to set up
Shiro and not necessarily as a framework component intended to be
overridden by end-users or other frameworks (it's JavaDoc makes no
mention of overriding concerns on purpose).  Instead, the
AbstractShiroFilter (the IniShiroFilter's parent class) describes
itself as the extension point.

We can make IniShiroFilter more flexible to accomodate overrides if we
want, but I respectfully submit that this would be considered a
feature request in the spirit of catering to something beyond the
original design concerns and not an effect of 'rotten' code.

> Perhaps we could patch it together one way or another to allow case
> insensitive processing, but we might just as well fix it properly.

Agreed - please open a feature request.

> Considering that a filter would only need to carry around one
> attribute, it's configuration, it'd make it perform far better and
> greatly simplify processing the filter chains if we'd create separate
> instances of the same filter for each filter chain rather than trying
> to use the same filter in different chains. I'll open an issue
> accordingly.

I'm confused here - do you mean multiple instances of the
IniShiroFilter?  Or the filters that it references in its filter
chain?

If it is the latter, I think the appropriate approach for this is to
enable a Factory mechanism that allows users to specify a filter's
configuration and it is expected that the Factory will create new
instances as is necessary.  When not using a Factory, the default
singleton semantics should remain in place IMO (pretty common behavior
in most IoC environments).  In fact, there is an issue very closely
related to this:  https://issues.apache.org/jira/browse/SHIRO-217
that could provide the same consistent solution.

> I'll fix the tapestry-security library separately with an ugly but
> small and efficient hack. I've been toying with the idea of just
> rewriting the request processing part and the filters for simplicity
> and to make it align better with Tapestry, but I'll save that for
> later.

Of course if there are suggestions for making the request processing
smoother, better, etc, we should definitely look in to this if it
makes it better for everyone.  I'm very open to making as many
improvements as we can.  If need be, we can add this to the 2.0
feature list.

It also sounds like there are a few concerns here that are very dev
related, so I've CC'd the dev list so we don't bog down the user list
with implementation/design details.

Best,

Les