Hi Alan and Kalle,
Thanks for chiming in on this.
But I'm not sure that inheritance is used too often. Sure, it is used
in some places where it shouldn't be and that there are definitely
places where we can and should eliminate it. For example, the
CodecSupport class and the HashedCredentialsMatcher hierarchy can be
deprecated now and then removed in a 2.0 release (to retain backwards
compatibility).
But if you look at pretty much all of Shiro's inheritance mechanisms
(SecurityManager and its subclasses, Realm and its subclasses,
SessionManager and its subclasses, etc), almost _all_ of those things
are purely wrapper classes that delegate to internal pluggable
components. Almost all of it is composition.
These class hierarchies for these things exist purely to logically
separate behavior associated with respective state (the internal
delegates). If we didn't do this for example, the SecurityManager
(which mostly uses delegation for 95% of its logic) would be a _huge_
class - probably well over 3000 lines long.
The crypto hierarchy is a *good thing* to me actually, and I often
talk about this as a benefit in presentations and people (at least
from what I'm told afterwards) really appreciate it. There is a very
clear separation of behavior specific to certain types of ciphers, and
this classification most definitely follows a single-inheritance class
hierarchy in real life. Look at BouncyCastle's source code and you'll
see the exact same thing (for good reason). The JDK JCE mechanism,
that represents _all_ Ciphers possible with a single abstract class,
is absolute garbage and is a very large reason why people are so darn
confused by it ('transformation strings' as factory arguments and
procedural methods abound - yuck!).
As far as HashedCredentialsMatcher, I agree that this can be a single
class and all of its subclasses should be deprecated and removed
later. The alternative should be that HashedCredentialsMatcher
instead reflect the Hash algorithm that it will use for its matching,
e.g.:
HashedCredentialsMatcher matcher = new HashedCredentialsMatcher();
matcher.setHashAlgorithm(Sha256Hash.class); //retains type-safety
or
matcher.setHashAlgorithm("SHA-512"); //not type safe and potentially error-prone
the end-user could choose what they want based on their needs.
As for the Hash class hierarchy, I'm not sure if we should do the same
thing. It exists because of type safety, non-forced exceptions, and
to make end-users lives noticeably simpler: Calling
new Sha512Hash(aFile).toHex()
is _much_ nicer for end users than using the JDK's crappy
MessageDigest class. It is also type safe. Using a string to
represent the algorithm (e.g. new SimpleHash("SHA-512", aFile).toHex()
is less type-safe than the first example - you don't know if the
string is validly typed and you can't perform type-safe
algorithm-specific logic (e.g maybe enforcing a certain algorithm is
used). Now if we want to add a SimpleHash that takes in a String
argument as an alternative in the case where we don't represent a
common algorithm already, I think that's a smart idea. In fact, the
existing concrete Hash implementations could probably just subclass
that one for simplicity. Then end-users can choose whatever approach
they want.
Anyway, I'm all for cleaning up things where we can, and making it
even more pluggable - but the level of customization you can do today
due to almost everything relying on delegation is *huge* compared to
most frameworks.
About versions: I think we should do as much of this type of cleanup
as is possible for 1.2 where it is %100 backwards-compatible based on
APR versioning guidelines. We're a TLP project, and a lot more people
are using our software today - I would like to ensure that upgrades
don't cause people API grief (not including the very occasional change
of internal code that is backwards compatible, but wasn't expected to
be used by end-users).
In fact, I think the HashedCredentialsMatcher changes can be done now,
for 1.1. I was in the process of making similar changes when working
on the Salt support, but kept those changes in a separate change list
and didn't commit them. I can probably have that stuff in in an hour
or two for your review.
If there are any other areas where you guys think this stuff can and
should be changed, please bring it up! Better to get this stuff
addressed early and make issues for them.
Best,
Les
