----- Original Message -----
From: "Bill Burke" <bburke(a)redhat.com>
To: "Stian Thorgersen" <stian(a)redhat.com>
Cc: keycloak-dev(a)lists.jboss.org
Sent: Thursday, 24 July, 2014 4:25:59 PM
Subject: Re: [keycloak-dev] federation commited need feedback
On 7/24/2014 11:02 AM, Stian Thorgersen wrote:
>
>
> ----- Original Message -----
>> From: "Bill Burke" <bburke(a)redhat.com>
>> To: "Stian Thorgersen" <stian(a)redhat.com>
>> Cc: keycloak-dev(a)lists.jboss.org
>> Sent: Thursday, 24 July, 2014 3:56:35 PM
>> Subject: Re: [keycloak-dev] federation commited need feedback
>>
>>
>>
>> On 7/24/2014 10:32 AM, Stian Thorgersen wrote:
>>>
>>>
>>> ----- Original Message -----
>>>> From: "Bill Burke" <bburke(a)redhat.com>
>>>> To: "Stian Thorgersen" <stian(a)redhat.com>
>>>> Cc: keycloak-dev(a)lists.jboss.org
>>>> Sent: Thursday, 24 July, 2014 2:47:57 PM
>>>> Subject: Re: [keycloak-dev] federation commited need feedback
>>>>
>>>>
>>>>
>>>> On 7/24/2014 9:25 AM, Stian Thorgersen wrote:
>>>>> Looks good. Only two comments from me:
>>>>>
>>>>> 1. FederationManager.getFederationProvider uses factories directly
>>>>>
>>>>> Why is this? This will cause the provider instance not to be
registered
>>>>> with the session, so won't be closed automatically when the
session is
>>>>> closed, nor will it be able to attach to the transaction.
>>>>>
>>>>
>>>> FederationProviders are not singletons within KeycloakSession like
>>>> RealmProvider, UserProvider, and UserSessionProvider are. You can have
>>>> multiple FederationProvider instances per realm.
>>>>
>>>>
>>>> public interface FederationProviderFactory extends
>>>> ProviderFactory<FederationProvider> {
>>>> FederationProvider getInstance(KeycloakSession session,
>>>> FederationProviderModel model);
>>>> }
>>>>
>>>>
>>>> FederationProviders implementations have the option to enlist
themselves
>>>> with the KeycloakSession transaction manager. LDAP (really Picketlink)
>>>> doesn't have the concept of a session or transaction, so it
doesn't
>>>> enlist itself.
>>>>
>>>> I guess we do need a enlistForClose() method on KeycloakSession though.
>>>
>>> As long as the lifespan of a FederationProvider is the same as the
>>> KeycloakSession they should be created through the KeycloakSession IMO.
>>> We
>>> just need to add an additional createProvider that can create a instance
>>> tied to a specific realm.
>>>
>>
>> I disagree. Its really up to the implementation on whether the provider
>> is managed by KeycloakSession or not or whether its lifespan is tied to
>> KeycloakSession or not.
>>
>
> Why would a provider not be tied to a session? IMO there should always be a
> session.
>
It might be expensive to create the provider and/or the provider may
have no concept of a session. For example, Picketlink PartitionManagers
are not tied to a KeycloakSession
That's exactly why a ProviderFactory has application scope. Anything that is expensive
to create should be stored in the ProviderFactory and reused in Provider instances.
For example JpaConnectionProviderFactory creates one EntityManagerFactory (which is
expensive) and reuses it to create a EntityManager for each JpaConnectionProvider.
>> We already manage transactions in this manner. For example
>> JpaUserProvider does not enlist its own KeycloakTransaction and its
>> close() method is a no-op. CacheUserProvider and JpaConnectionProvider
>> enlist their own transaction objects manually. JpaConnectionProvider's
>> close() method actually does something.
>
> Commit transactions and closing providers are tied to the session, that's
> the whole point. With your code you're creating providers that are never
> closed.
>
Whether or not a transaction is enlisted is up to the implementation
though. Also different provider factories may have differing create()
methods with different parameters, so it doesn't make sense for the
KeycloakSession to manage Provider creation all the time.
FYI I added an "enlistForClose()" method to KeycloakSession.
Exactly, but that's what we already do with the transaction. The factory is
responsible for enlisting it if it wants to.
See above - Provider instances should be created and closed per session, while factories
can if they want reuse resources for multiple providers and then manage closing these
itself.
>>
>> --
>> Bill Burke
>> JBoss, a division of Red Hat
>>
http://bill.burkecentral.com
>>
--
Bill Burke
JBoss, a division of Red Hat
http://bill.burkecentral.com