code review planning

[Rod Widdowson]

As promised, here are some more noted about tomorrow's review.

The review will be more aimed at learning the code and its quirks than a
formal review.  However feedback is always welcome in particular we are
interested in

-	Javadoc issues.
-	Annotations bugs.
-	The coherence of logging.
-	The use of exceptions.

We’ll discuss the general architecture, but would rather not entertain
sweeping changes – that boat sailed a year ago.

Please do not feel that attending the review is required, attend only if you
want to learn more.  If only Rod and Tom attend then the time will be well
spent (as will have the time spent fixing the documentation and tidying the
code which happened in the run up to the review).  However if you do attend
and do not want to drown in details you would be wise to revise the
attribute filtering language.

It would also help if you understood the dichotomy between PolicyRules and
Permit/Deny Rules and also the similar feeling, but completely orthogonal
dichotomy between context based rules and value based rules.  Nearly all of
complexity in the filtering implementation springs from the need to flatten
these two orthogonal dimensions into a single one.

I have updated the wiki https://wiki.shibboleth.net/confluence/x/dYBX to try
to explain this, but the review will probably need to spend some time on it.

The review itself will start briefly at the –api and touch on 
-	AttributeFilter – the top level factory corresponding to
<afp:AttributeFilterPolicyGroup/>
-	AttributeFilterPolicy – corresponding to <afp:AttributeFilterPolicy>
-	AttributeRule – corresponding to  <afp:AttributeRule/>,
<afp:PermitValueRule/> and <afp:DenyValueRule/>

We will then meet Matcher which corresponds to the MatchFunctorType in the
schema.

Finally we need to discuss MatcherException and AttributeFilterException,
since understanding these (and there may be rework here) will influence the
review of the implementation.

Most of the code in –impl is repetitive; however we need to look at some of
the classes inside net.shibboleth.idp.attribute.filter.impl.matcher.logic
which do carry the burden of the di-dichotomy, and
net.shibboleth.idp.attribute.filter.impl.matcher.AbstractComparisonMatcher
which does most of the work of simplifying the work that the rest of the
rules have to do.  

This last class is probably the most important/contentious class.  Finally
net.shibboleth.idp.attribute.filter.impl.matcher.attributevalue.AbstractAttr
ibuteTargetedStringMatcher demonstrates how to deal with a MatchFunctor
which changes its complete behavior based on its parameterization.

Most of the rest of the rules are a port across from V2, made to fit into
the architecture.  This does not mean that review is valueless, but
reviewing them gets old very quickly.  However a review of (for instance)
net.shibboleth.idp.attribute.filter.impl.saml.AbstractEntityGroupMatcher
might be profitable with a view to understanding tristating and our use of
exceptions.