spring-boot
spring-boot copied to clipboard
spring-projects
addresses #33748 validate default table on jdbc session init
Refuses to start when jdbc session is initialized and custom table is specified, but default script is specified. default scripts are not currently customizable for a custom table name, so this configuration will result in a schema being defined for one table name, but sessions being stored in another table. this is almost definitely not the intended the end result. so we will ask the user to specify the schema (or set the initialization to never).
this is the intermediate solution to restore functionality lost in a merge as discussed in #33748
implementation notes:
I'm not sure whether i should have used the "customize" protected method, which can be overridden, and occurs close enough in the general execution workflow, that an exception thrown there or in my overridden runScripts method. But it seems that customization logic should live there, not validation logic? I can override the other method instead of runScripts if that is at all preferable.
it seems there is already an architectural decision in place here - to deal with the configuring the different implementations - all the implementations of DataSourceScriptDatabaseInitializer need to call the super constructor taking DataSource and DatabaseInitializationSettings as the second argument. perhaps it makes sense to augment the database initialization settings to add the necessary information to perform this particular kind of validation... then this solution will more easily translate to other implementations. otherwise it is less than ideal to hook into the constructors and store the implementation-specific ConfigurationProperties object.
I might be a little bit more liberal in what I'd try to implement all use cases (checks for jdbc, r2dbc, skips schema-less (mongo, hazel, etc)) in an application codebase, but tried to submit the smallest patch and start a discussion about the implementation in an established codebase that supports graalvm, all sorts of platforms some I might not be familiar with etc
@alexanderankin Please sign the Contributor License Agreement!
Click here to manually synchronize the status of this Pull Request.
See the FAQ for frequently asked questions.
I don't think that the functionality that was added in #6649 was lost in a merge. As far as I can tell, it was intentionally removed as part of https://github.com/spring-projects/spring-boot/pull/9752.
There's a lengthy discussion about whether or not to fail fast and the conclusion was not to do so. We'll need to think carefully about whether or not failing fast is the right thing to do now. There's also other initialization to consider for Quartz, Spring Batch, etc. Things are quite nicely aligned and consistent at the moment. If we decide that failing fast is the right thing to do, we should then consider if the need to do so is specific to Spring Session JDBC or if it should be applied more broadly.
ah, thanks for that context, I wasn't aware of that and somehow couldn't find it from the commit history.
I agree that where this pr places the checking code is implementation specific and has the drawback of not covering related implementations.
There's also other initialization to consider for Quartz, Spring Batch, etc.
It looks like Quartz is fine as there's no customization related to table names.
Spring Batch does appear to be affected due to the spring.batch.jdbc.table-prefix property. It defaults to BATCH_ and all of the tables in the default schema files are named BATCH_…. Changing the table name prefix without providing a custom schema will, as far as I can tell, result in a failure.
Spring Integration does not appear to be affected as there's no customization related to table names.
I think the above covers all of the DataSourceScriptDatabaseInitializer implementations that we have so the problem's specific to Batch and Spring Session JDBC.
I don't mind going and adding templating using the established pattern (JdbcIndexedSessionRepository queries being parametrized for the table name) for all other related projects, but i would hesitate to go touch large parts of the code without clarification that this is actually desired/acceptable.
I would understand if the decision is to continue to relegate this functionality to only embedded databases.
perhaps in the meantime i can clone and build those other codebases to search for any relevant implementation details over there.
In #6649, the auto-configuration for Spring Session JDBC was changed so that initialization of the database was enabled automatically if the default table name was set or a custom schema was configured. At this time, failing fast was considered but rejected as, among other reasons, there was no way to be certain that the configuration wouldn't work.
In #9752, this changed as part of harmonizing the behavior of all of the initializers (Batch, Integration, Quartz, and Session JDBC). There is no longer any automatic enablement based on other properties with the default behavior being that only an embedded database is initialized.
As was the case when #6649 was discussed, I don't think we can fail fast here without it being a breaking change. A user could have their own org/springframework/session/jdbc/schema-*.sql files in src/main/resources that override the files from the spring-session-jdbc jar and use the custom table name. Failing fast would break such an arrangement. Users would have to rename the files and configure a custom schema to get things working again. I think that cost outweighs the benefit of failing fast. Thanks anyway for the PR.