[keycloak-dev] Refactoring to KeycloakSession and ProviderSession

[Marek Posolda]

On 30.6.2014 18:58, Bill Burke wrote:
>
> On 6/30/2014 12:37 PM, Stian Thorgersen wrote:
>>
>> ----- Original Message -----
>>> From: "Bill Burke" <bburke at redhat.com>
>>> To: keycloak-dev at lists.jboss.org
>>> Sent: Monday, 30 June, 2014 5:25:31 PM
>>> Subject: Re: [keycloak-dev] Refactoring to KeycloakSession and	ProviderSession
>>>
>>> Ugh...Why?  Would have been cool to discuss this before you did this.
>>> There was a clear separation of things prior to this change:
>>>
>>> * ProviderSession was for resolving various session based APIs I thought.
>>> * KeycloakSession was analogous to EntityManager or HibernateSession.
>> I didn't really think it was a big deal, as the separation is still there. What used to be KeycloakSession is now ModelProvider, and the new KeycloakSession is just a wrapper to be able to get to both from the same @Context injection.
>>
> KeycloakSession now mixes up the provider API and the data model.  What
> exactly is the benefit of that?  I thought we were moving to
> *separating* things out not combining them..  Realm meta-model, User
> metamodel, and User Session meta model.
I wonder if we can abstract the fact that there are 3 different 
meta-models from the code, which consumes it? In other words, still have 
same API to invoke the "model" like it currently is (current 
KeycloakSession) but have KeycloakSession implementation to just 
delegate the invocations to proper meta-models (Realm, User, 
UserSession)? This would mean that RealmModelProvider, UserModelProvider 
and UserSessionModelProvider will be invoked just from 
DefaultKeycloakSession.

This would mean that consuming code will be simpler and doesn't need to 
retrieve 3 different providers from KeycloakSession (or 
ProviderSession), but just one.

>
> What I'd like to see is ProviderSession becoming a mini-transaction
> manager where our Filter calls commit/rollback on the ProviderSession
> which in turn, calls commit/rollback on all our model APIs.
+1, I think that it will be in line with the design of KeycloakSession 
of the only externally "consumed" part. So KeycloakSession 
begin/commit/rollback will be called by Filter, and it will delegate 
calls to begin/commit/rollback to all 3 meta-models.

Separation of "ProviderSession" and "KeycloakSession" makes sense to me, 
but IMO stuff like transaction management belongs more to 
KeycloakSession than to ProviderSession.

My vote is for restore previous "ProviderSession" for accessing various 
components and rename "KeycloakSession" to something like "ModelSession" 
or "PersistentSession" . KeycloakSession doesn't mean much and it's not 
so clear that it's component for dealing with model/persistent stuff.

Marek
>
>>> Also, you don't go committing stuff that breaks existing code...i.e. the
>>> caching stuff I did.
>> I commented out the cache stuff as it wasn't working, see https://issues.jboss.org/browse/KEYCLOAK-527. I tried to fix it, but couldn't quite figure out how to go about doing it, so thought it was best to disable it until you got back and could have a look. I was going to send you an email about it, but of course I forgot that :(
>>
> I guess we don't have any tests for adding/removing scope mappings :(
>
> FYI, it is just a matter of using EntityManager.getReference().  Commit
> incoming.
>
>