Some comments on code: * Why have a special flag in invalidateUser? Just invalidate the links if they exist. Aggressive eviction isn't a bad thing, is it?

On a side note, cache needs more refactoring, we're reusing the same pattern over and over. Wonder if functions can help. On 3/22/2016 5:42 AM, Marek Posolda wrote: > Until now, we don't have support for caching of identityProvider > (social) links. So every social login or every going to "federated > identities" tab in account management needs to send DB queries. I was > looking at fixing it and I've send PR > https://github.com/keycloak/keycloak/pull/2404 . It turned to be a bit > tricky because of: > > 1) Caching needs to be done on both directions. For social login, you > need to lookup user by social link. But on the other hand, you also need > to look all social/identityProvider links of particular user when you go > to account management etc. > > 2) Because of "store token" option, the link may need to be updated in > DB quite often (in theory even during each social login). > > I was thinking that storing links directly on CachedUser doesn't work > very well, because during each update of social link (which may be often > because of "store token") the user would need to be fully invalidated > from cache. Having separate cache entry for each social link also has > some downsides (many items in cache, need to have separate entry for > store the all links of user anyway). So I ended up with having the cache > entry, which contains list of all links of particular user. It needs to > be updated when any social link is added,removed or updated. This seemed > to me like good compromise. WDYT? > > There are also entries for lookup user by federated identity, so you > don't need to query DB during social login. > > So ATM there are not DB queries during social login or during go to > "federated identities" . I hope I handled all corner cases and > invalidations correctly, but if someone want to take look it will be > good. I don't want to add new regressions atm :-) > > Marek > > > > > > > _______________________________________________ > keycloak-dev mailing list > keycloak-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/keycloak-dev

Is this really critical for 1.9.x? I'm worried about us intruding more complexity so late and I'd rather postpone to 2.x. On 22 Mar 2016 15:36, "Marek Posolda" <mposolda(a)redhat.com <mailto:mposolda@redhat.com>> wrote: On 22/03/16 15:18, Bill Burke wrote: > Some comments on code: > > * Why have a special flag in invalidateUser? Just invalidate the links > if they exist. Aggressive eviction isn't a bad thing, is it? The only reason was performance. The flag "false" is used when you're adding new user. But this new user is not yet in cache, so calling "getFederatedIdentities" will always need to send DB query to get all links of user. But when you're adding new user, you know that this user doesn't yet have any social links. So in the end, without the flag there will be one more DB query, which will always return empty set. Currently we don't support store/load of social links from federation provider. But maybe in the future, if we support it and we chain cache on top of federation storage, it could theoretically happen that new user have social links at the moment when he is registered. So I can do what you proposed as it seems to be a bit safer for the future. Marek > > On a side note, cache needs more refactoring, we're reusing the same > pattern over and over. Wonder if functions can help. > > > On 3/22/2016 5:42 AM, Marek Posolda wrote: >> Until now, we don't have support for caching of identityProvider >> (social) links. So every social login or every going to "federated >> identities" tab in account management needs to send DB queries. I was >> looking at fixing it and I've send PR >> https://github.com/keycloak/keycloak/pull/2404 . It turned to be a bit >> tricky because of: >> >> 1) Caching needs to be done on both directions. For social login, you >> need to lookup user by social link. But on the other hand, you also need >> to look all social/identityProvider links of particular user when you go >> to account management etc. >> >> 2) Because of "store token" option, the link may need to be updated in >> DB quite often (in theory even during each social login). >> >> I was thinking that storing links directly on CachedUser doesn't work >> very well, because during each update of social link (which may be often >> because of "store token") the user would need to be fully invalidated >> from cache. Having separate cache entry for each social link also has >> some downsides (many items in cache, need to have separate entry for >> store the all links of user anyway). So I ended up with having the cache >> entry, which contains list of all links of particular user. It needs to >> be updated when any social link is added,removed or updated. This seemed >> to me like good compromise. WDYT? >> >> There are also entries for lookup user by federated identity, so you >> don't need to query DB during social login. >> >> So ATM there are not DB queries during social login or during go to >> "federated identities" . I hope I handled all corner cases and >> invalidations correctly, but if someone want to take look it will be >> good. I don't want to add new regressions atm :-) >> >> Marek >> >> >> >> >> >> >> _______________________________________________ >> keycloak-dev mailing list >> keycloak-dev(a)lists.jboss.org <mailto:keycloak-dev@lists.jboss.org> >> https://lists.jboss.org/mailman/listinfo/keycloak-dev _______________________________________________ keycloak-dev mailing list keycloak-dev(a)lists.jboss.org <mailto:keycloak-dev@lists.jboss.org> https://lists.jboss.org/mailman/listinfo/keycloak-dev

Ok, if Bill reviews and is happy to include in 1.9.x let's do it. On 22 Mar 2016 16:29, "Marek Posolda" <mposolda(a)redhat.com <mailto:mposolda@redhat.com>> wrote: Yeah, there might be some risk of regression. On the other hand, there are DB queries during each social login or every time when you click on "federated identities" in account management. If you think missing caching is acceptable from performance perspective etc, I can merge just to master and avoid 1.9.x. Marek On 22/03/16 17:18, Stian Thorgersen wrote: > > Is this really critical for 1.9.x? I'm worried about us intruding > more complexity so late and I'd rather postpone to 2.x. > > On 22 Mar 2016 15:36, "Marek Posolda" <mposolda(a)redhat.com > <mailto:mposolda@redhat.com>> wrote: > > On 22/03/16 15:18, Bill Burke wrote: > > Some comments on code: > > > > * Why have a special flag in invalidateUser? Just > invalidate the links > > if they exist. Aggressive eviction isn't a bad thing, is it? > The only reason was performance. The flag "false" is used > when you're > adding new user. But this new user is not yet in cache, so > calling > "getFederatedIdentities" will always need to send DB query to > get all > links of user. But when you're adding new user, you know that > this user > doesn't yet have any social links. So in the end, without the > flag there > will be one more DB query, which will always return empty set. > > Currently we don't support store/load of social links from > federation > provider. But maybe in the future, if we support it and we > chain cache > on top of federation storage, it could theoretically happen > that new > user have social links at the moment when he is registered. > So I can do > what you proposed as it seems to be a bit safer for the future. > > Marek > > > > On a side note, cache needs more refactoring, we're reusing > the same > > pattern over and over. Wonder if functions can help. > > > > > > On 3/22/2016 5:42 AM, Marek Posolda wrote: > >> Until now, we don't have support for caching of > identityProvider > >> (social) links. So every social login or every going to > "federated > >> identities" tab in account management needs to send DB > queries. I was > >> looking at fixing it and I've send PR > >> https://github.com/keycloak/keycloak/pull/2404 . It turned > to be a bit > >> tricky because of: > >> > >> 1) Caching needs to be done on both directions. For social > login, you > >> need to lookup user by social link. But on the other hand, > you also need > >> to look all social/identityProvider links of particular > user when you go > >> to account management etc. > >> > >> 2) Because of "store token" option, the link may need to > be updated in > >> DB quite often (in theory even during each social login). > >> > >> I was thinking that storing links directly on CachedUser > doesn't work > >> very well, because during each update of social link > (which may be often > >> because of "store token") the user would need to be fully > invalidated > >> from cache. Having separate cache entry for each social > link also has > >> some downsides (many items in cache, need to have separate > entry for > >> store the all links of user anyway). So I ended up with > having the cache > >> entry, which contains list of all links of particular > user. It needs to > >> be updated when any social link is added,removed or > updated. This seemed > >> to me like good compromise. WDYT? > >> > >> There are also entries for lookup user by federated > identity, so you > >> don't need to query DB during social login. > >> > >> So ATM there are not DB queries during social login or > during go to > >> "federated identities" . I hope I handled all corner cases and > >> invalidations correctly, but if someone want to take look > it will be > >> good. I don't want to add new regressions atm :-) > >> > >> Marek > >> > >> > >> > >> > >> > >> > >> _______________________________________________ > >> keycloak-dev mailing list > >> keycloak-dev(a)lists.jboss.org > <mailto:keycloak-dev@lists.jboss.org> > >> https://lists.jboss.org/mailman/listinfo/keycloak-dev > > _______________________________________________ > keycloak-dev mailing list > keycloak-dev(a)lists.jboss.org > <mailto:keycloak-dev@lists.jboss.org> > https://lists.jboss.org/mailman/listinfo/keycloak-dev >

I had already reviewed it. Adding another thing doesn't make the cache more complex as the same pattern/algorithm is pretty much repeated for each cached piece of data. The risk is that we missed an edge case. On 3/22/2016 12:50 PM, Stian Thorgersen wrote: > > Ok, if Bill reviews and is happy to include in 1.9.x let's do it. > > On 22 Mar 2016 16:29, "Marek Posolda" <mposolda(a)redhat.com > <mailto:mposolda@redhat.com>> wrote: > > Yeah, there might be some risk of regression. On the other hand, > there are DB queries during each social login or every time when > you click on "federated identities" in account management. > > If you think missing caching is acceptable from performance > perspective etc, I can merge just to master and avoid 1.9.x. > > Marek > > On 22/03/16 17:18, Stian Thorgersen wrote: >> >> Is this really critical for 1.9.x? I'm worried about us >> intruding more complexity so late and I'd rather postpone to 2.x. >> >> On 22 Mar 2016 15:36, "Marek Posolda" <mposolda(a)redhat.com >> <mailto:mposolda@redhat.com>> wrote: >> >> On 22/03/16 15:18, Bill Burke wrote: >> > Some comments on code: >> > >> > * Why have a special flag in invalidateUser? Just >> invalidate the links >> > if they exist. Aggressive eviction isn't a bad thing, is it? >> The only reason was performance. The flag "false" is used >> when you're >> adding new user. But this new user is not yet in cache, so >> calling >> "getFederatedIdentities" will always need to send DB query >> to get all >> links of user. But when you're adding new user, you know >> that this user >> doesn't yet have any social links. So in the end, without >> the flag there >> will be one more DB query, which will always return empty set. >> >> Currently we don't support store/load of social links from >> federation >> provider. But maybe in the future, if we support it and we >> chain cache >> on top of federation storage, it could theoretically happen >> that new >> user have social links at the moment when he is registered. >> So I can do >> what you proposed as it seems to be a bit safer for the future. >> >> Marek >> > >> > On a side note, cache needs more refactoring, we're >> reusing the same >> > pattern over and over. Wonder if functions can help. >> > >> > >> > On 3/22/2016 5:42 AM, Marek Posolda wrote: >> >> Until now, we don't have support for caching of >> identityProvider >> >> (social) links. So every social login or every going to >> "federated >> >> identities" tab in account management needs to send DB >> queries. I was >> >> looking at fixing it and I've send PR >> >> https://github.com/keycloak/keycloak/pull/2404 . It >> turned to be a bit >> >> tricky because of: >> >> >> >> 1) Caching needs to be done on both directions. For >> social login, you >> >> need to lookup user by social link. But on the other >> hand, you also need >> >> to look all social/identityProvider links of particular >> user when you go >> >> to account management etc. >> >> >> >> 2) Because of "store token" option, the link may need to >> be updated in >> >> DB quite often (in theory even during each social login). >> >> >> >> I was thinking that storing links directly on CachedUser >> doesn't work >> >> very well, because during each update of social link >> (which may be often >> >> because of "store token") the user would need to be fully >> invalidated >> >> from cache. Having separate cache entry for each social >> link also has >> >> some downsides (many items in cache, need to have >> separate entry for >> >> store the all links of user anyway). So I ended up with >> having the cache >> >> entry, which contains list of all links of particular >> user. It needs to >> >> be updated when any social link is added,removed or >> updated. This seemed >> >> to me like good compromise. WDYT? >> >> >> >> There are also entries for lookup user by federated >> identity, so you >> >> don't need to query DB during social login. >> >> >> >> So ATM there are not DB queries during social login or >> during go to >> >> "federated identities" . I hope I handled all corner >> cases and >> >> invalidations correctly, but if someone want to take look >> it will be >> >> good. I don't want to add new regressions atm :-) >> >> >> >> Marek >> >> >> >> >> >> >> >> >> >> >> >> >> >> _______________________________________________ >> >> keycloak-dev mailing list >> >> keycloak-dev(a)lists.jboss.org >> <mailto:keycloak-dev@lists.jboss.org> >> >> https://lists.jboss.org/mailman/listinfo/keycloak-dev >> >> _______________________________________________ >> keycloak-dev mailing list >> keycloak-dev(a)lists.jboss.org >> <mailto:keycloak-dev@lists.jboss.org> >> https://lists.jboss.org/mailman/listinfo/keycloak-dev >> > -- Bill Burke JBoss, a division of Red Hat http://bill.burkecentral.com