I see your point now. Please, go one with your PR or design document I'm sure you'll get tons of reviews as you are touching a quite sensitive area :) Stian can talk more about this, but maybe this is something for Keycloak.Next. Regards. On Mon, Jun 10, 2019 at 3:07 PM Justin Gross <jgross.biz(a)gmail.com&gt; wrote: > Thanks for your feedback Pedro. > > > > I apologize I think I may have misspoke or misrepresented my thoughts. A > design document might prove helpful in explaining. > > > > TLDR; > > > > I didn’t intend to advocate for a single multi-purpose SPI but rather a > “parent” SPI which both User Storage and Client Storage could inherit from > (implementation wise) and potential future storage SPIs. > > > > Looking over User Storage SPI and Client Storage SPI I see a lot of the > code in Client Storage Provider (and related files) as copied from User > Storage and code that is not specific to either and related only to storage > itself. There are more Client Storage Provider related stuff that, when > finished, will probably also just be a copy/duplicate of existing generic > code in User Storage Provider (and related). > > > > Given that most storage SPI’s will probably have similar “storage general” > interfaces and functions it would make sense to me to create a common > ancestor rather than continue to duplicate code in every specific storage > SPI thereby actually simplifying (instead of making more complex) the User > and Client (and other future) storage SPI’s. > > > > My proposition (I’ll PR to community) is that we move code that is not > specific to User Storage (or Client) but that is actually general code > (code currently being duplicated or soon to be if finishing Client Storage > SPI) into “storage” classes and interfaces. This would group together the > storage interfaces and functionality that are general and thereby > simplifies the production of new storage SPI’s as new storage SPI’s can > narrow their focus to the specific requirements you spoke of Pedro. > > > > Thank you, > > Justin Gross > > > > *From: *Pedro Igor Silva <psilva(a)redhat.com&gt; > *Sent: *Monday, June 10, 2019 8:03 AM > *To: *Justin Gross <jgross.biz(a)gmail.com&gt; > *Cc: *keycloak-dev(a)lists.jboss.org > *Subject: *Re: [keycloak-dev] Storage SPI: Users, Clients, and ? > > > > Hi Justin, > > > > Personally, I think that having a single and multi-purpose SPI can lead to > a much more complex and full of hacks to address the different requirements > when integrating with different sources and data. In any case, I would > suggest you to start a design document and send a PR to > https://github.com/keycloak/keycloak-community. So we can understand > better your proposal and discuss it. > > > > Regards. > > Pedro Igor > > > > On Sun, Jun 9, 2019 at 12:10 AM Justin Gross <jgross.biz(a)gmail.com&gt; wrote: > > Born out of necessity it looks like originally (for storage SPI) there was > only user storage SPI and eventually came a partial client storage SPI > motivated by a need for Openshift integration. The more I think about how > it could be finished, and the more I look at the user storage SPI, the more > I am finding code within user storage SPI that can be generalized as simply > storage SPI. > > Seeking feedback on refactoring existing user storage SPI related > interfaces to be agnostic to users and generalized as “storage SPI”. This > should reduce the need for duplicating similar code when implementing other > storage SPI (ie: client storage SPI). > > In the future I could see more storage SPI’s being created and I feel this > would be extremely useful. > > > Thank you, > > Justin Gross > _______________________________________________ > keycloak-dev mailing list > keycloak-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/keycloak-dev > > > _______________________________________________ keycloak-dev mailing list keycloak-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/keycloak-dev

Your feedback is much appreciated! Especially the note on UserStorage SPI being officially supported and therefore guaranteed to be backward compatible. I hadn’t planned on changing that interface but I could see how by “transplanting” large pieces of it to an ancestor that someone could change the ancestor and then negatively impact compatibility in UserStorage SPI accidentally or by happenstance. Definitely good to keep in mind and I’ll make sure my proposal doesn’t effect User Storage SPI directly or make it any easier to disturb it. Thanks again! Justin Gross From: Marek Posolda Sent: Monday, June 10, 2019 3:02 PM To: Pedro Igor Silva; Justin Gross Cc: keycloak-dev(a)lists.jboss.org Subject: Re: [keycloak-dev] Storage SPI: Users, Clients, and ? Hi, I remember we already discuss this some time ago. I agree that have some "generic" storage SPI could be beneficial for various use-cases. For example Role Storage SPI or Group Storage SPI could be beneficial for LDAP integration - Keycloak currently supports syncing LDAP roles and groups to KC database and use role (group) mappings based on the role (group) mappings from LDAP, but there are some corner cases, which causes that data are not always 100% accurate. For example when some LDAP group is changed, Keycloak is not notified about this change etc. But agree with Pedro, that complexity of this probably won't justify the added value. We can maybe think about it for Keycloak.next if something like this can work, but ATM I have quite doubts... But maybe I am wrong and you come with some cool design proposal :) BTV. Just a warning, that if you think about refactoring of current User + Client Storage SPI (and not adding any new SPI), then the potential challenge is, that UserStorage SPI is officially supported and is guaranteed to be backwards compatible. In other words, existing interfaces can't be easily changed in backwards incompatible way. Marek On 10/06/2019 20:25, Pedro Igor Silva wrote: > I see your point now. Please, go one with your PR or design document I'm > sure you'll get tons of reviews as you are touching a quite sensitive area > :) > > Stian can talk more about this, but maybe this is something for > Keycloak.Next. > > Regards. > > On Mon, Jun 10, 2019 at 3:07 PM Justin Gross <jgross.biz(a)gmail.com&gt; wrote: > >> Thanks for your feedback Pedro. >> >> >> >> I apologize I think I may have misspoke or misrepresented my thoughts. A >> design document might prove helpful in explaining. >> >> >> >> TLDR; >> >> >> >> I didn’t intend to advocate for a single multi-purpose SPI but rather a >> “parent” SPI which both User Storage and Client Storage could inherit from >> (implementation wise) and potential future storage SPIs. >> >> >> >> Looking over User Storage SPI and Client Storage SPI I see a lot of the >> code in Client Storage Provider (and related files) as copied from User >> Storage and code that is not specific to either and related only to storage >> itself. There are more Client Storage Provider related stuff that, when >> finished, will probably also just be a copy/duplicate of existing generic >> code in User Storage Provider (and related). >> >> >> >> Given that most storage SPI’s will probably have similar “storage general” >> interfaces and functions it would make sense to me to create a common >> ancestor rather than continue to duplicate code in every specific storage >> SPI thereby actually simplifying (instead of making more complex) the User >> and Client (and other future) storage SPI’s. >> >> >> >> My proposition (I’ll PR to community) is that we move code that is not >> specific to User Storage (or Client) but that is actually general code >> (code currently being duplicated or soon to be if finishing Client Storage >> SPI) into “storage” classes and interfaces. This would group together the >> storage interfaces and functionality that are general and thereby >> simplifies the production of new storage SPI’s as new storage SPI’s can >> narrow their focus to the specific requirements you spoke of Pedro. >> >> >> >> Thank you, >> >> Justin Gross >> >> >> >> *From: *Pedro Igor Silva <psilva(a)redhat.com&gt; >> *Sent: *Monday, June 10, 2019 8:03 AM >> *To: *Justin Gross <jgross.biz(a)gmail.com&gt; >> *Cc: *keycloak-dev(a)lists.jboss.org >> *Subject: *Re: [keycloak-dev] Storage SPI: Users, Clients, and ? >> >> >> >> Hi Justin, >> >> >> >> Personally, I think that having a single and multi-purpose SPI can lead to >> a much more complex and full of hacks to address the different requirements >> when integrating with different sources and data. In any case, I would >> suggest you to start a design document and send a PR to >> https://github.com/keycloak/keycloak-community. So we can understand >> better your proposal and discuss it. >> >> >> >> Regards. >> >> Pedro Igor >> >> >> >> On Sun, Jun 9, 2019 at 12:10 AM Justin Gross <jgross.biz(a)gmail.com&gt; wrote: >> >> Born out of necessity it looks like originally (for storage SPI) there was >> only user storage SPI and eventually came a partial client storage SPI >> motivated by a need for Openshift integration. The more I think about how >> it could be finished, and the more I look at the user storage SPI, the more >> I am finding code within user storage SPI that can be generalized as simply >> storage SPI. >> >> Seeking feedback on refactoring existing user storage SPI related >> interfaces to be agnostic to users and generalized as “storage SPI”. This >> should reduce the need for duplicating similar code when implementing other >> storage SPI (ie: client storage SPI). >> >> In the future I could see more storage SPI’s being created and I feel this >> would be extremely useful. >> >> >> Thank you, >> >> Justin Gross >> _______________________________________________ >> keycloak-dev mailing list >> keycloak-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/keycloak-dev >> >> >> > _______________________________________________ > keycloak-dev mailing list > keycloak-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/keycloak-dev _______________________________________________ keycloak-dev mailing list keycloak-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/keycloak-dev