On Sun, Jun 26, 2011 at 5:54 PM, Les Hazlewood <[email protected]> wrote:
> In working to resolve SHIRO-266 [1], I've finished my initial
> implementation attempt and committed it as rev. 1139968. As this is a
> significant contribution wrt how Shiro saves Subject state, so please
> review if you get a chance - feedback/comments are welcome.
> Implementation strategy:
> Using sessions as a Subject storage strategy for Shiro's own
> implementation needs was spread out in a few places but it has now
> been consolidated and abstracted into a new SubjectDAO concept (now a
> JavaBeans-compatible property on the DefaultSecurityManager
> implementation (get/setSubjectDAO).
Superb job on the javadoc once again, Les! I do have a few things to note.
1) There's no interface for accessing subjectDAO? It's a recurring
problem in Shiro that you have to know the implementation class to
access its operations. Another example from the javadoc:
((DefaultSessionStorageEvaluator)sessionDAO.getSessionStorageEvaluator()).setSessionStorageEnabled(false);
- note that you also need to know the implementation type of
sessionDAO for this to work.
2) I don't like the DAO as a name even in general (personally I feel
that something like persistenceService conveys the purpose much
better) but for this feature, it's even worse since it's not a DAO
(the interface has only save() and delete() operations). Perhaps
something like SubjectStorageService instead?
3) On the interface design, does it make sense that we have a very
generic SubjectDAO interface with save(), delete() operations, or,
would it be better to have more specific saveSubject(),
deleteSubject() on the interface that could then be directly
implemented by the securityManager. Especially given that we now have
somewhat meaningless pass-through operations for them in the
DefaultSecurityManager.
4) On the implementation, what's the reason for using this for example in:
protected void save(Subject subject) {
this.subjectDAO.save(subject);
}
Personally, I'd avoid writing anything extra.
Kalle
