Skip to content

Validate Spring Session database initializer configuration #6649

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Validate Spring Session database initializer configuration #6649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions ...igure/src/main/java/org/springframework/boot/autoconfigure/session/SessionProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ public static class Jdbc {
private static final String DEFAULT_SCHEMA_LOCATION = "classpath:org/springframework/"
+ "session/jdbc/schema-@@platform@@.sql";

private static final String DEFAULT_TABLE_NAME = "SPRING_SESSION";

/**
* Path to the SQL file to use to initialize the database schema.
*/
Expand All @@ -115,7 +117,7 @@ public static class Jdbc {
/**
* Name of database table used to store sessions.
*/
private String tableName = "SPRING_SESSION";
private String tableName = DEFAULT_TABLE_NAME;

private final Initializer initializer = new Initializer();

Expand All @@ -139,15 +141,19 @@ public Initializer getInitializer() {
return this.initializer;
}

public static class Initializer {
public class Initializer {

/**
* Create the required session tables on startup if necessary.
*/
private boolean enabled = true;

public boolean isEnabled() {
return this.enabled;
boolean isDefaultTableName = DEFAULT_TABLE_NAME.equals(
Jdbc.this.getTableName());
boolean isDefaultSchema = DEFAULT_SCHEMA_LOCATION.equals(
Jdbc.this.getSchema());
return this.enabled && (isDefaultTableName || !isDefaultSchema);
}

public void setEnabled(boolean enabled) {
Expand Down
23 changes: 23 additions & 0 deletions ...ava/org/springframework/boot/autoconfigure/session/SessionAutoConfigurationJdbcTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ public void defaultConfig() {
JdbcOperationsSessionRepository.class);
assertThat(new DirectFieldAccessor(repository).getPropertyValue("tableName"))
.isEqualTo("SPRING_SESSION");
assertThat(this.context.getBean(SessionProperties.class)
.getJdbc().getInitializer().isEnabled()).isTrue();
assertThat(this.context.getBean(JdbcOperations.class)
.queryForList("select * from SPRING_SESSION")).isEmpty();
}
Expand All @@ -66,6 +68,8 @@ public void disableDatabaseInitializer() {
JdbcOperationsSessionRepository.class);
assertThat(new DirectFieldAccessor(repository).getPropertyValue("tableName"))
.isEqualTo("SPRING_SESSION");
assertThat(this.context.getBean(SessionProperties.class)
.getJdbc().getInitializer().isEnabled()).isFalse();
this.thrown.expect(BadSqlGrammarException.class);
assertThat(this.context.getBean(JdbcOperations.class)
.queryForList("select * from SPRING_SESSION")).isEmpty();
Expand All @@ -82,8 +86,27 @@ public void customTableName() {
JdbcOperationsSessionRepository.class);
assertThat(new DirectFieldAccessor(repository).getPropertyValue("tableName"))
.isEqualTo("FOO_BAR");
assertThat(this.context.getBean(SessionProperties.class)
.getJdbc().getInitializer().isEnabled()).isTrue();
assertThat(this.context.getBean(JdbcOperations.class)
.queryForList("select * from FOO_BAR")).isEmpty();
}

@Test
public void customTableNameWithDefaultSchemaDisablesInitializer() {
load(Arrays.asList(EmbeddedDataSourceConfiguration.class,
DataSourceTransactionManagerAutoConfiguration.class),
"spring.session.store-type=jdbc",
"spring.session.jdbc.table-name=FOO_BAR");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't feel right at all. So basically you're saying you want JDBC and a custom table name and you'll get an exception? I would rather move the initialization of the table conditional on whether a custom table name has been specified or not. That's consistent with other places where we determine a flag based on the environment.

You provided a custom table name? Then you're on your own to create the schema (instead of throwing an exception as this PR does).

You provided the path to a script? We'll of course use that to initialize the database

You didn't provide anything? We're going to assume you want us to create the schema with the default table name.

That seems a much better solution to the problem. If you agree, can you rework the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback.

So basically you're saying you want JDBC and a custom table name and you'll get an exception?

No, it's an exception when you configure custom table name together with default schema script location, those two together form an invalid configuration.

I would rather move the initialization of the table conditional on whether a custom table name has been specified or not.

Solely on whether a custom table name has been specified or not? Configuring custom table name together with appropriate schema script is a valid configuration with existing test in SessionAutoConfigurationJdbcTests#customTableName to support it. Spring Batch also has the similar test.

You provided a custom table name? Then you're on your own to create the schema (instead of throwing an exception as this PR does).

Well, not unless you also customize the schema script location, as explained above.

You provided the path to a script? We'll of course use that to initialize the database

👍

You didn't provide anything? We're going to assume you want us to create the schema with the default table name.

👍

To address your first comment here, I've initially considered placing the logic in the SessionProperties itself, but wasn't sure if that was the right place to do it.

I've also considered simply disabling the initializer when configuring custom table name and leaving the default schema script location, but ultimately I've felt that it would be like a silent failure that can easily blow up in runtime so it looked better to me to fail fast.

I'm all for reworking the PR but I don't think the support for custom table name combined with custom schema should be dropped.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's an exception when you configure custom table name together with default schema script location, those two together form an invalid configuration.

Correct me if I am wrong but you're not configuring the "default schema script location" so your sentence essentially can be rewritten as "it's an exception when you only configure a custom table name". That's also what your test shows. IMO that's wrong. Failing fast for something you haven't configured seems a bit harsh to me.

If you specify a custom table name, I wouldn't expect Spring Boot to auto-magically adapt the script (thought it could). We have other cases where we flip a switch based on other properties.

Maybe @wilkinsona has an opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I am wrong but you're not configuring the "default schema script location" so your sentence essentially can be rewritten as "it's an exception when you only configure a custom table name".

That's true, it's not being explicitly configured.

Question is would you want to change the existing behavior that supports use of initializer with custom table name and custom schema?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't. If you specify both, then we're just going to initialize the schema as before. I already wrote that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion but from your previous comments it wasn't obvious as you didn't provide the logical correlation between your You provided a custom table name? Then you're on your own to create the schema and You provided the path to a script? We'll of course use that to initialize the database statements.

I'll update the PR to place the validation logic into the SessionProperties. Do you agree that if we don't fail fast, there should be some kind of warning log indicating that initializer was disabled due to particular combination of configuration?

Copy link
Member

@snicoll snicoll Aug 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you agree that if we don't fail fast, there should be some kind of warning log indicating that initializer was disabled due to particular combination of configuration?

I don't: If you have expressed explicitly that you want your table to be named XYZ, I wouldn't complain if boot will crash down the road because XYZ doesn't exist. I am not a huge fan of warning. What does a warning buy you with an exception a bit after anyway?

I'd like @wilkinsona to chime in to confirm because I am not 100% sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we know it's not going to work then I'd throw an exception as early as possible rather than logging the warning. If we can't be sure if it'll work or not then the warning seems potentially misleading and we should just continue and let it throw an exception later on if it turns out that it doesn't work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't know if it's going to fail or not. I might have that custom table in my database init script. There is no way to know.

What @vpavic suggested initially is that a user would have to explicitly disable the initializer flag to prevent the exception. Doesn't feel right to me.

It will fail down the road if you configure a custom table name and you have no script to create said custom table name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will fail down the road if you configure a custom table name and you have no script to create said custom table name.

Letting it fail down the road seems reasonable to me then. If we want to improve the diagnostics we should consider a FailureAnalyzer.

JdbcOperationsSessionRepository repository = validateSessionRepository(
JdbcOperationsSessionRepository.class);
assertThat(new DirectFieldAccessor(repository).getPropertyValue("tableName"))
.isEqualTo("FOO_BAR");
assertThat(this.context.getBean(SessionProperties.class)
.getJdbc().getInitializer().isEnabled()).isFalse();
this.thrown.expect(BadSqlGrammarException.class);
assertThat(this.context.getBean(JdbcOperations.class)
.queryForList("select * from SPRING_SESSION")).isEmpty();
}

}