Shibboleth Developers

[Chad La Joie <>]

[As part of the IdP v3 work, since we have more than just me working
on the IdP now, I'll be doing code reviews on committed code.  These
will be sent to this list.]

General Comments
----
All resolver plugins must be thread safe and need to be marked with 
@ThreadSafe

Where possible, class fields should be declared final.  The Logger
should always be declared final.

All method param should be marked as final

@return should always indicate whether the return could be null and,
if a collection, whether null elements/keys may be in the collection

Within methods, instead of declaring all your variables at the top of
the method, declare, and initialize them, close to where they are
used.  This cuts down on the chances that objects are constructed and
then, because of particular code paths, thrown away without being used

Within methods, local variables should be marked as final unless they
really are expected to be reassigned during method execution

In you logging statements, include the ID of the plugin so it is
absolutely clear which

MappedAttributeDefinition
----
The class comment needs to be significantly more informative.

In the constructor, the passed in 'map' should be set via #setValueMaps

#setDefaultValue(String) should use StringSupport#trimOfNull(String)
on provided input

#setValueMaps(Collection) should defensively copy its parameter in to
a new collection, remove null values, made unmodifable, and assigned
to #valueMaps.  Use CollectionSupport#XXXXXXXXXXXXXX

#doAttributeResolution() the variable names 'result' and
'mappedResult' are too similar, I'd change 'result' to 'attribute',
'resultantAttribute', etc.

#doAttributeResolution() has the same comment twice with lots of
whitespace around it.  Just have it once and have it be one line

#doAttributeResolution(), line 126, by using StringSupport#trimOrNull
on the setter for the default value you no longer need to check if the
string is null or empty here, just use 'null == defaultValue' check

#mapValue(), line 166-168, this can be simplified to "valueMapMatch =
!mappedValues.isEmpty()"

#validate() instead of creating the same error message string twice
just create it once and then past it to the logger and exception

TemplateAttributeDefinition
----
The class comment needs to be cleaned up.  It should note exactly what
gets added to the velocity context.

Now that the dependency definitions carry the attribute ID you no
longer need to pass that in as a config parameter.  This will clean up
the constructor and #countAndSetupSourceValues() can probably go away
all together.

In the constructor, for the template name I'd used the full class name
(this.getClass().getName()) + id as the template ID.  You can even
throw the package name in there if you want.

In the constructor, run the template through StringSupport#trimOrNull
then use Assert.isNotNull(String, String) to throw an error if it is
null.  See the constructor for AbstractComponent which does this with
the passed in component ID

In the constructor, check to ensure that there is no resource with the
given ID before you add it to the StringResourceRepository, if there
is, thrown an IllegalArgumentExeption that another template is already
registered with that ID.

There are no getters for any of the set properties.  Even if you don't
have setters (thus not allowing things to be changed after
instantiation) you should still have getters so the bean could be
inspected (via JMX for example)

#doAttributeResolution it would probably be helpful to have DEBUG
statement that logs, at least, the ID of each attribute being added to
the velocity context.  Logging the value via #toString is probably
okay too though, in theory, some of those objects may not necessarily
have a reasonable #toString() impl.  I suspect that's a rarity though.

#doAttributeResolution, lines 184-185, can be merged in to one line

That's it for now.

-- 
Chad La Joie
www.itumi.biz
trusted identities, delivered