Mailing-List: contact dev-help@shiro.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@shiro.apache.org
Received-SPF: neutral (nike.apache.org: local policy)
MIME-Version: 1.0
Sender: les.hazlewood@anjinllc.com
In-Reply-To: <BANLkTinVZ30Obk1wN6LNhyfdt-jgZFrTRA@mail.gmail.com>
References: <BANLkTikEih3i91sh=egsSrSaG-XdWZ7wHw@mail.gmail.com>
	<BANLkTinVZ30Obk1wN6LNhyfdt-jgZFrTRA@mail.gmail.com>
Date: Sun, 26 Jun 2011 21:02:16 -0700
Message-ID: <BANLkTi=omk7f7xUjVZBSo1iT1yWddT3vJQ@mail.gmail.com>
Subject: Re: Shiro and Stateless Applications
From: Les Hazlewood <lhazlewood@apache.org>
To: dev@shiro.apache.org
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Sun, Jun 26, 2011 at 7:49 PM, Kalle Korhonen
<kalle.o.korhonen@gmail.com> wrote:

Hi Kalle - I really appreciate all the feedback in this email - thanks
for taking the time to evaluate.  I'll address each comment below.

> 1) There's no interface for accessing subjectDAO? It's a recurring
> problem in Shiro that you have to know the implementation class to

Yes, I don't have a good idea how to solve this - suggestions are welcome!

The whole idea with this is that the object graph navigation to set
these things is typically expected to be done with INI config or some
other object-graph capable configuration (IoC container flavor, etc).
That is, when you see this:

securityManager.subjectDAO.sessionStorageEvaluator =3D $myEvaluator

There is no casting or knowledge of implementations - you just set it
as if it were a simple configuration property.

Granted, I know you know that, but that's the reason for not having an
interface at the moment.  But there's backstory to this:  Shiro's
codebase used to be riddled with convenience methods and even more
*Aware interfaces that allowed you to set this stuff as simple direct
properties.  It made the codebase really nasty and felt like a red
flag that things were being done incorrectly.  So, we got rid of most
of that stuff, with the expectation that INI or Groovy or
insert-your-sexy-configuration-mechanism would be used so you didn't
have to worry about the not-so-nice things the Java language forces
upon you.

Now, that being said, I'm fine adding interfaces where it makes sense
- I just didn't because 1) the SubjectDAO concept is an
implementation-specific detail of how the DefaultSecurityManager is
implemented (i.e. you have to know you're working with a
DefaultSecurityManager instance) and 2) My expectation is that most
people would not configure one in Java code directly.

Even if we had an interface, you'd have to do nasty things like:

if (securityManager instanceof SubjectDAOAware) {
    ((SubjectDAOAware)securityManager).setSubjectDAO(subjectDAO);
} else {
  //warn that a SubjectDAO was provided but isn't supported?
  //throw an exception?
}

My only solution off of the top of my head would be to have a
SecurityManagerBuilder that accepted a ton of direct configuration
properties and we would apply them as necessary.  I'm not sure how
much value this would provide over INI or Groovy or other config
mechanisms though.

Anyway, I'd *love* for recommendations, suggestions, on how to make
all of this stuff nicer for end-users - this is one thing that I
struggle with quite a bit w/ the codebase!

> 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

Yeah, I chose that name because I couldn't think of anything better
and I thought it was a decent parallel to the existing SessionDAO in
the SessionManager.  The current implementation performs CUD but no R.
 Although, now that you bring it up, would it make sense to support
the Read case?  Could we leverage it such that when a Subject is being
acquired at the beginning of a request?  I wonder if it make sense for
us to leverage the DAO =3D CRUD thing to full effect.  Maybe not - gotta
think about it.  Hrm....

Anyway, I'm definitely open to changing the name.  I personally don't
like anything named *Service though, as at least for me, that has
strong connotations with things that are coarse grained and typically
perform logic of some sort beyond CRUD (transactions, manipulation,
etc).  But maybe that's just me.  If others feel differently I'm ok
forgoing my predisposition.

SubjectStateAccessor? I dunno - maybe that feels contrived.  I'm
definitely open to continued suggestions though!  I've gotta think
about the Read case and see if we might be able to fully realize the
CRUD case (in which case, I'm assuming you'd be ok calling it a proper
DAO?)

> 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.

I added those passthroughs as a template hook in case custom
implementations (subclasses) might wish to override the method to
perform pre/post logic before/after calling the SubjectDAO.  I
typically like to do this as a convenience because without them, if
any of that pre/post logic is desired by subclasses, they must
completely override the calling methods, which is pretty nasty.

For example, if a subclass implementor wanted to do pre/post
SubjectDAO interaction and the template methods did not exist, they'd
have to completely override (and potentially re-implement) the
createSubject(SubjectContext) method and the logout(Subject) methods -
pretty nasty.

> 4) On the implementation, what's the reason for using this for example in=
:
> =C2=A0 =C2=A0protected void save(Subject subject) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0this.subjectDAO.save(subject);
> =C2=A0 =C2=A0}

Yep, I hope that my answer to 3) explains the reasoning.

Thoughts?

Best,

Les