Describe the bug JdbcOAuth2AuthorizedClientService doesn't have a way to specify the table name.

Projects often need to customize the table name to include a schema prefix.

Other Spring classes that access the database do provide a way to customize the table name (which can be used to include the schema), for example, spring.session.jdbc.table-name in Spring Session JDBC.

To Reproduce Use JdbcOAuth2AuthorizedClientService with an oauth2_authorized_client table in a schema different than the default. It cannot be done.

Expected behavior There should be a way to customize the table name used for the queries.

I suggest that JdbcOAuth2AuthorizedClientService get a new setTableName(String) method like Spring Session JDBC has: https://github.com/spring-projects/spring-session/blob/3.1.3/spring-session-jdbc/src/main/java/org/springframework/session/jdbc/JdbcIndexedSessionRepository.java#L295

Comment From: sjohnr

Thanks for the suggestion, @candrews. Related to this request, do you feel like there are other implementations that would need to be made similarly configurable? Or is this the only one you currently have (or foresee) a need for customizing?

Comment From: candrews

This is the only one I currently need to customize. I don't see anything else right now that warrants customizability (for example, customizing column names wouldn't make sense).

Comment From: sjohnr

Thanks @candrews. I'm wondering about that though. If it makes sense to customize a table name, it might also make sense to customize column names. I feel like it would be more meaningful to add both of these customization options, and if that's the case it could take a different form than your suggested PR. I think I'd prefer to think on this and discuss it a bit more before proceeding.

Comment From: candrews

If it makes sense to customize a table name, it might also make sense to customize column names.

Customizing the table name makes sense because it oftentimes needs a schema prefix, or perhaps a unique name not to conflict with another table when multiple applications share the same schema. I believe that's why Spring Session permits customization of the table name in the same way I propose here.

Customizing the column names goes too far. There's no way column names can conflict. And if the table form really does need such modification, it probably would also mean more changes than just the table name (such as to logic), which would mean an extension of this class (or creating a new implementation from scratch) would make more sense.

Comment From: sjohnr

@candrews I generally agree with your thought process, however it's harder to convince a user of the logic you're outlining when arguing that something should or should not be customizable. Users are not always in control of the schema, and are sometimes asked to work with what already is or something being prescribed. Someone may ask for another customization and have good reason to do so. I'm proposing that we consider whether more flexibility could be foreseen and make sure that we choose a path forward that either allows for that right away (when delivering this enhancement) or doesn't prevent it from being possible in the future. Being able to customize only the table name may make it more difficult to customize an entire SQL statement later, for example.

Comment From: sjohnr

@candrews thanks for being patient. I have spoken with the team about this issue and the general topic of configuring JDBC implementations in certain ways. While it would be nice to be consistent with Spring Session, one thing that is more important is that we're consistent within the Spring Security project. We do have precedent for being able to configure entire SQL statements for the JdbcUserDetailsManager, but I don't see precedent in Spring Security for setting just the table name.

Further, the JdbcOAuth2AuthorizedClientService implementation is intended to support basic persistence needs, but advanced customization is best served through the use of technologies like Spring Data JDBC and others. We want to strike a balance that supports the most common use cases out of the box but does not make the framework more complicated for users. Having multiple ways to configure this class (which I feel is possible or even likely following this request) would not strike that balance well. Lastly, we don't want to make too many updates to these implementations, because Spring Security is not a persistence framework. The most basic needs are addressed in the framework without requiring additional dependencies, but this is for convenience, and not intended to address every persistence requirement.

With that in mind, I would prefer to update the title of this issue to support customizing SQL statements in JdbcOAuth2AuthorizedClientService and see if that's of interest to the community (by you or someone else submitting a PR, or otherwise just waiting for community upvotes). This would allow maximum flexibility with the simplest possible configuration touch point. If that's not your preference, then I think we should close this issue as the team does not feel like customizing just the table name is a step forward for this (or any Spring Security) implementation.

Comment From: candrews

Configuring entire SQL statements has the disadvantage of making it more difficult for users to take advantage of potential future enhancements that use improved SQL.

Consider if JdbcOAuth2AuthorizedClientService goes with the path of requiring the user to set the entire SQL in order to customize the table name. A user takes advantage of that capability. Then, a new version of Spring Security is released that improves the SQL to be faster (perhaps using MERGE INTO instead of INSERT INTO or something similar). The user must notice that this change was made and then update their custom SQL to make the same change. In contrast, if the setTableName approach is taken, the user's update experience is seamless - they don't need to do anything.

I think if a user wants to do something as invasive as changing the entire SQL statement, then they should extend the class and override methods as appropriate. Changing the table name, however, seems far less invasive and much simpler, and therefore should be done via a straightforward method as exemplified in Spring Session JDBC.

Comment From: sjohnr

Thanks for the feedback, @candrews.

As I mentioned, I generally agree with the thought process you're outlining here, and you make a good point regarding the potential benefits of a seamless upgrade for users. However, there are counter-points as well. For every scenario where a user gets a seamless upgrade, there could be a scenario that requires work on their part to upgrade no matter what.

With that in mind, I feel disinclined to set the precedent of configuring repository/service implementations with only a table name. As I mentioned, following precedents set by spring-session is not a high priority. Spring Data makes it quite easy to provide a custom implementation for persistence, and I'd prefer to push users toward that option than making changes in the core implementation(s) that only benefit a few users.

I'm going to close this issue for now with the above explanation. I'm happy to re-evaluate and re-open if the community disagrees with this decision, as my goal is not to block progress on community requests but to navigate the best path forward considering the bigger picture.

Comment From: elreydetodo

I agree with @candrews that the table name really should be configurable. It's annoying to have to create a subclass just for something that seams that simple and obviously needed.

It makes less sense to me to allow totally changing the table structure though. Normally changing the name would be to make it align better with local naming practices. It doesn't hurt to let the column a names be configurable, but I feel it will see little practice use. The table name matters somewhat more than the column names imo.

I do not think allowing the sql to be free-formed is a good idea. This is a framework, not a free-for-all. Some amount of structure should be enforced. I also really agree with that point about automatic uptake of sql changes and improvements.

Comment From: madorb

Another +1 to the pragmatic solution of allowing table name to be configurable