RefactoringPersonDirNameFinder
- Andrew Petro
This page exists for two purposes. 1) is to propose a concrete commit for changes to PersonDirNameFinder, PersonDirNameFinderFactory, and an associated testcase. 2) is to provide an example of refactoring uPortal 2 code to be more testable and less tightly coupled to static services.
The players
PersonDirNameFinder is the IEntityNameFinder implementation which knows how to lookup displayName attributes for entities representing uPortal users.
/**
* PersonDirectory implementation of <code>IEntityNameFinder</code> for <code>IPersons</code>.
* @version $Revision: 1.12 $
*/
public class PersonDirNameFinder
implements IEntityNameFinder {
// Singleton instance:
private static IEntityNameFinder singleton;
private static IPersonAttributeDao pa;
// Our cache of entity names:
private SoftHashMap names;
/**
* ReferenceIPersonNameFinder constructor comment.
*/
private PersonDirNameFinder () throws SQLException
{
super();
pa = PersonDirectory.getPersonAttributeDao();
names = new SoftHashMap();
}
/**
* Given the key, returns the entity's name.
* @param key java.lang.String
*/
public String getName (String key) throws Exception {
if (primGetNames().get(key) == null) {
primGetNames().put(key, primGetName(key));
}
return (String)primGetNames().get(key);
}
/**
* Given an array of keys, returns the names of the entities. If a key
* is not found, its name will be null.
* @param keys java.lang.String[]
*/
public java.util.Map getNames (java.lang.String[] keys) throws Exception {
Map selectedNames = new HashMap();
for (int i = 0; i < keys.length; i++) {
String name = getName(keys[i]);
selectedNames.put(keys[i], name);
}
return selectedNames;
}
/**
* Returns the entity type for this <code>IEntityFinder</code>.
* @return java.lang.Class
*/
public Class getType () {
return org.jasig.portal.security.IPerson.class;
}
/**
* put your documentation comment here
* @param key
* @return
* @exception java.sql.SQLException
*/
private String primGetName (String key) throws java.sql.SQLException {
String name = key;
Map userInfo = pa.getUserAttributes(name);
Object displayName = userInfo.get("displayName");
String displayNameStr = "";
if (displayName != null)
{
if (displayName instanceof java.util.List)
{
List displayNameList = (List) displayName;
if (! displayNameList.isEmpty() )
{ displayNameStr = (String)displayNameList.get(0); }
}
else displayNameStr = (String)displayName;
if (! displayNameStr.trim().equals(""))
{ name = displayNameStr; }
}
return name;
}
/**
* @return java.util.Map
*/
private Map primGetNames () {
return names;
}
/**
* @return IEntityNameFinder
*/
public static synchronized IEntityNameFinder singleton () throws SQLException {
if (singleton == null) {
singleton = new PersonDirNameFinder();
}
return singleton;
}
}
PersonDirNameFinder is a static singleton. It is implemented by delegation to the PersonDirectory static service to lookup user attributes. Its constructor is private and it provides a public static synchronized singleton() method whereby code wishing to use PersonDirNameFinder can get one.
PersonDirNameFinderFactory is the IEntityNameFinderFactory implementation which produces PersonDirNameFinder instances.
/**
* Factory for creating <code>PersonDirNameFinders</code>.
* @author Alex Vigdor
* @version $Revision: 1.5 $
*/
public class PersonDirNameFinderFactory implements IEntityNameFinderFactory {
private static final Log log = LogFactory.getLog(PersonDirNameFinderFactory.class);
/**
* ReferencePersonNameFinderFactory constructor comment.
*/
public PersonDirNameFinderFactory() {
super();
}
/**
* Return a finder instance.
* @return org.jasig.portal.groups.IEntityNameFinder
* @exception org.jasig.portal.groups.GroupsException
*/
public IEntityNameFinder newFinder() throws GroupsException
{
try
{ return PersonDirNameFinder.singleton(); }
catch ( SQLException sqle )
{
log.error( "ReferencePersonNameFinderFactory.newFinder(): " + sqle);
throw new GroupsException(sqle);
}
}
}
Its "newFinder()" method doesn't really create a new instance of PersonDirNameFinder – it retrieves the static instance of the class.
A bug
Dan Ellentuck noticed the following bug: IPersonAttributeDao returns null in the case of an unknown user, and the PersonDirNameFinder implementation above doesn't check for the null return value from IPersonAttributeDao.
And a fix for the bug
Here's the changed primGetName() implementation to fix this bug:
private String primGetName (String key) throws java.sql.SQLException {
String name = key;
Map userInfo = pa.getUserAttributes(name);
if (userInfo != null)
{
Object displayName = userInfo.get("displayName");
String displayNameStr = "";
if (displayName != null)
{
if (displayName instanceof java.util.List)
{
List displayNameList = (List) displayName;
if (! displayNameList.isEmpty() )
{ displayNameStr = (String)displayNameList.get(0); }
}
else displayNameStr = (String)displayName;
if (! displayNameStr.trim().equals(""))
{ name = displayNameStr; }
}
}
return name;
}
Refactored PersonDirNameFinder
Here's a refactored version of PersonDirNameFinder:
Error rendering macro 'code': Invalid value specified for parameter 'lang'
/**
* Implementation of <code>IEntityNameFinder</code> for <code>IPersons</code> by
* looking up displayName from an <code>IPersonAttributeDao</code>.
* @version Revision: 1.13++
*/
public class PersonDirNameFinder
implements IEntityNameFinder {
/**
* Data Access Object backing this name finder.
*/
private IPersonAttributeDao paDao;
/** Our cache of entity names: */
private Map names = Collections.synchronizedMap(new SoftHashMap());
/**
* Instantiate a PersonDirNameFinder backed by the given
* IPersonAttributeDao.
* @param pa DAO to back this PersonDirNameFinder
*/
PersonDirNameFinder (IPersonAttributeDao pa) {
this.paDao = pa;
}
public String getName (String key) {
String name = (String) this.names.get(key);
if (name == null) {
// cached name not found, get name from underlying DAO.
name = primGetName(key);
// cache the name
this.names.put(key, name);
}
return name;
}
public java.util.Map getNames (java.lang.String[] keys) {
Map selectedNames = new HashMap();
for (int i = 0; i < keys.length; i++) {
String name = getName(keys[i]);
selectedNames.put(keys[i], name);
}
return selectedNames;
}
public Class getType () {
return org.jasig.portal.security.IPerson.class;
}
/**
* Actually lookup a user name using the underlying IPersonAttributeDao.
* @param key - entity key which in this case is a unique identifier for a user
* @return the display name for the identified user
*/
private String primGetName (String key) {
String name = key;
Map userInfo = this.paDao.getUserAttributes(name);
if (userInfo != null)
{
Object displayName = userInfo.get("displayName");
String displayNameStr = "";
if (displayName != null)
{
if (displayName instanceof java.util.List)
{
List displayNameList = (List) displayName;
if (! displayNameList.isEmpty() )
{ displayNameStr = (String)displayNameList.get(0); }
}
else displayNameStr = (String)displayName;
if (! displayNameStr.trim().equals(""))
{ name = displayNameStr; }
}
}
return name;
}
/**
* Get a static singleton instance of this class backed by PersonDirectory.
* @return singleton PersonDirNameFinder backed by PersonDirectory
* @deprecated as of uP 2.5 instead use PersonDirNameFinderFactory
*/
public static IEntityNameFinder singleton () {
return new PersonDirNameFinderFactory().newFinder();
}
/**
* Returns a String that represents the value of this object.
* @return a string representation of the receiver
*/
public String toString () {
return "PersonDirNameFinder backed by " + this.paDao;
}
}
Dependency on PersonDirectory
Changed to receive the IPersonAttributeDao we're going to use to look up user display named by means of Dependency Injection (a constructor argument) rather than by Lookup (static invocation of PersonDirectory). PersonDirectory is still the place that the PersonDirNameFinderFactory gets the IPersonAttributeDao that it uses to construct a PersonDirNameFinder, but this dependency is moved to the factory and out of PersonDirNameFinder itself. PersonDirNameFinder will be happy to use any IPersonAttributeDao someone wants to give it.
On Singletons
Static singleton is a place to store, a way to use, PersonDirNameFinder. It is not inherent to the object itself. So we move this static singleton out of PersonDirNameFinder, which is about implementing an IEntityNameFinder by use of an IPersonAttributeDao, and into PersonDirNameFinderFactory, which is all about getting access to a properly configured PersonDirNameFinder. The static singleton() method continues to be supported but is deprecated so that in a future release of this code the dependency of PersonDirNameFinder upon its factory can be removed.
So we add visibility to our constructor to make it default (package) scoped. That way our factory can call it. Code outside our package will have to use the factory to get an instance of PersonDirNameFinder.
Comments
JavaDoc comments for private members make comments available in JavaDoc generated from this code and in IDEs that use JavaDoc to provide context, e.g. Eclipse.
JavaDoc comments are not included for methods declared by the IEntityNameFinder interface. We are not re-specifying these methods, we're just implementing them, so appropriate comments in PersonDirNameFinder are comments about the implementation. By eliminating duplication of the method specifications in both IEntityNameFinder and in this class we eliminate the possibility of these comments getting out of sync.
Taking the advice present in the code to "put your documentation comment here", the private method primGetName() now has a JavaDoc comment.
primGetNames()
Dropped method primGetNames() because all this private method did was give us access to the private field private SoftHashMap names. The method was private and so was not providing an opportunity for anyone to extend this class and override the method to have us use some other Map.
Returning null Thread safety
This implementation of getName() could return null: JIRA issue for fix for uP 2.4.3 JIRA issue for fix for uP 2.5
/**
* Given the key, returns the entity's name.
* @param key java.lang.String
*/
public String getName (String key) throws Exception {
if (primGetNames().get(key) == null) {
primGetNames().put(key, primGetName(key));
}
return (String)primGetNames().get(key);
}
With suitably pathological thread switching and garbage collection, the thread could be interrupted after the if statement but before the return statement. If during that interruption the GC runs and harvests the entry we verified, then when control returns and we get the mapped value for the key, we may get and return null.
By keeping a local reference to the displayName String, we make the implementation threadsafe:
public String getName (String key) {
String name = (String) this.names.get(key);
if (name == null) {
// cached name not found, get name from underlying DAO.
name = primGetName(key);
// cache the name
this.names.put(key, name);
}
return name;
}
However, we're still not threadsafe, because multiple threads can be accessing the instance SoftHashMap "name" while one or more threads are changing that map. So, when we construct our SoftHashMap, we wrap it in a Collections.synchronizedMap(). JIRA issue for fix in uPortal 2.4.3 JIRA issue for fix in uPortal 2.5
Exceptions
While the IEntityNameFinder interface entitles us to throw Exception, we don't actually need to throw any checked Exceptions. So the local methods here are not declared to throw exceptions. Anyone accessing us as a PersonDirNameFinder, rather than as an IEntityNameFinder won't have to deal with these exceptions.
Refactored factory
And here's a refactored version of the factory:
**
* Factory for creating and obtaining reference to
* a static singleton <code>PersonDirNameFinder</code>.
* @version Revision: 1.5++
*/
public class PersonDirNameFinderFactory implements IEntityNameFinderFactory {
/**
* Lazily initialized static singleton PersonDirNameFinder backed
* by the IPersonAttributeDao provided by the PersonDirectory static service.
*/
private static PersonDirNameFinder PERSON_DIR_NAME_FINDER_INSTANCE;
/**
* Do-nothing constructor.
*/
public PersonDirNameFinderFactory() {
super();
}
/**
* Get the static singleton PersonDirNameFinder instance backed by
* PersonDirectory.
*
* @return the static singleton PersonDirNameFinder backed by PersonDirectory
*/
public IEntityNameFinder newFinder() {
if (PERSON_DIR_NAME_FINDER_INSTANCE == null) {
PersonDirNameFinderFactory.storeSingleton();
}
return PERSON_DIR_NAME_FINDER_INSTANCE;
}
/**
* Intantiates the static singleton field PERSON_DIR_NAME_FINDER_INSTANCE.
* Synchronized to guarantee singletonness of the field.
*/
private synchronized static void storeSingleton() {
// recheck that we need to run because field could have been
// set since we decided to invoke this method but before we acquired
// the lock on this object
if (PERSON_DIR_NAME_FINDER_INSTANCE == null) {
IPersonAttributeDao personAttributeDao = PersonDirectory
.getPersonAttributeDao();
PERSON_DIR_NAME_FINDER_INSTANCE = new PersonDirNameFinder(
personAttributeDao);
}
}
}
It is now the Factory that is responsible for instantiating and storing the static singleton instance of PersonDirNameFinder. It lazily, safely instantiates this instance.
Payoff: a testcase
Now that we can inject a mock IPersonAttributeDao which we know will return null for some users, and test that PersonDirNameFinder is handling these cases as we expect.
public class PersonDirNameFinderTest extends TestCase {
PersonDirNameFinder finder;
protected void setUp() throws Exception {
super.setUp();
Map userWithDisplayNameAttributes = new HashMap();
userWithDisplayNameAttributes.put("phone", "777-7777");
userWithDisplayNameAttributes.put("displayName", "Display Name");
Map userWithoutDisplayNameAttributes = new HashMap();
userWithoutDisplayNameAttributes.put("phone", "666-6666");
userWithoutDisplayNameAttributes.put("givenName", "Howard");
Map daoBackingMap = new HashMap();
daoBackingMap.put("userWithDisplayName", userWithDisplayNameAttributes);
daoBackingMap.put("userWithoutDisplayName", userWithoutDisplayNameAttributes);
IPersonAttributeDao paDao = new ComplexStubPersonAttributeDao(daoBackingMap);
this.finder = new PersonDirNameFinder(paDao);
}
protected void tearDown() throws Exception {
super.tearDown();
}
/**
* Test getting the display name for a user.
*/
public void testGetName() {
assertEquals("Display Name", this.finder.getName("userWithDisplayName"));
}
/**
* Test that getting the name for a user without a display name returns the
* uid.
*/
public void testGetNameWhereNoDisplayName() {
assertEquals("userWithoutDisplayName", this.finder.getName("userWithoutDisplayName"));
}
/**
* Test that getting the name for an unknown user returns the uid.
*/
public void testGetNameUnknownUser() {
assertEquals("unknownUser", this.finder.getName("unknownUser"));
}
/**
* Test that PersonDirNameFinders report their type as being IPerson.
*/
public void testGetType() {
assertEquals(IPerson.class, this.finder.getType());
}
}
Conclusion
Chagrined at having introduced this NullPointerException-yielding bug, I got interested in producing a JUnit testcase to test this code. With this testcase in place, the next time I touch this code I won't as easily introduce this particular bug.
I'd like to commit the refactored PersonDirNameFinder, PersonDirNameFinderFactory, and associated testcase and supporting stub IPersonAttributeDao implementation.