Skip to content

javax.persistence.schema-generation.database.action is ignored when checking if default DDL auto setting should be applied #25129

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 3 commits into from
Closed

javax.persistence.schema-generation.database.action is ignored when checking if default DDL auto setting should be applied #25129

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21cf833
Checks 'javax.persistence.schema-generation.database.action' before r…
onobc Feb 7, 2021
d9667a3
Considers value of 'javax.persistence.schema-generation.database.acti…
onobc Feb 12, 2021
76186d8
Does not set ddl-auto when 'javax.persistence.schema-generation.datab…
onobc Feb 14, 2021
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
16 changes: 16 additions & 0 deletions ...ure/src/main/java/org/springframework/boot/autoconfigure/orm/jpa/HibernateProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
* Configuration properties for Hibernate.
*
* @author Stephane Nicoll
* @author Chris Bono
* @since 2.1.0
* @see JpaProperties
*/
Expand Down Expand Up @@ -133,6 +134,21 @@ private String determineDdlAuto(Map<String, String> existing, Supplier<String> d
if (ddlAuto != null) {
return ddlAuto;
}
String ddlDatabaseAction = existing.get(AvailableSettings.HBM2DDL_DATABASE_ACTION);
if (ddlDatabaseAction != null) {
Copy link
Contributor Author

@onobc onobc Feb 12, 2021

Choose a reason for hiding this comment

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

Strict Approach

I chose a strict approach that only considers valid JPA database action values (none, create, drop, drop-and-create) - each of which has a well-known counterpart hbm2ddl.auto value. Any other values, including valid HBM specific values are ignored and the defaultDdlAuto provider kicks in.

More Relaxed Approach

However, Hibernate seems to be verify forgiving and relaxed when it comes to these values. It seems to use the JPA/HBM properties interchangeable. It prefers the JPA properties but also allows HBM values to be set on the JPA property.

A more relaxed approach would be to allow whatever values are set on the JPA property to be used as the default ddl auto and simply handle the 2 JPA special cases: create -> create-only and drop-and-create -> create.

Anyways, let's continue the discussion.

Copy link
Contributor Author

@onobc onobc Feb 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Misconception

Ahh ok, I misunderstood the issue. I think my misconception was that the ddl-auto property had to be set to something even if the db-action was set.

Rather, I think we should review this method altogether and set a ddl-auto only if necessary,

I interpreted this to mean set the ddl-auto property if the db-action was set to a value that was a valid ddl-auto

When checking for a user-provided setting before configuring the default setting, I think we need to consider javax.persistence.schema-generation.database.action as well as hibernate.hbm2ddl.auto.

I interpreted "configuring the default setting" as setting the ddl-auto property. Which is true but IFF the user did not already specify a ddl-auto or db-action. If the user set it already under spring.jpa. then there is no need for us to do anything - it will flow down to HB8 and all is well.

Silver lining

I learned quite a bit about the 2 properties and how we use them and how HB8 uses them.

Third time's a charm

Pivoting here shortly and doing the following:

  1. If db-action property is set, do not set a default ddl-auto (we do not require db-auto to be set if db-action is set)
  2. Adjust tests

Question 1)
I feel like it would be helpful to briefly mention this property in the docs. It's not mentioned anywhere but we do support it and looking into the HB8 code they seem to prefer that property over the legacy HB8 specific one. Thoughts?

Question 2)
I noticed that currently we remove the ddl-auto if the default came back as "none" - which could also be from the user setting it to "none". I understand this has no functional impact because the property default is "none". I am curious as to why? Should we do the same for the db-action property?

My mild concern on removing the property when user has explicitly set it to none is this:

  • User sets ddl-auto to "none"
  • We remove it prior to passing through to HB8
  • User steps through HB8 code in debugger
  • User says "I set ddl-auto, why is it not here?"

if (ddlDatabaseAction.equals("none")) {
return "none";
}
if (ddlDatabaseAction.equals("create")) {
return "create-only";
}
if (ddlDatabaseAction.equals("drop")) {
return "drop";
}
if (ddlDatabaseAction.equals("drop-and-create")) {
return "create";
}
}
return (this.ddlAuto != null) ? this.ddlAuto : defaultDdlAuto.get();
}

Expand Down
36 changes: 35 additions & 1 deletion ...rc/test/java/org/springframework/boot/autoconfigure/orm/jpa/HibernatePropertiesTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,15 @@
import java.util.Map;
import java.util.function.Consumer;
import java.util.function.Supplier;
import java.util.stream.Stream;

import org.hibernate.cfg.AvailableSettings;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;

Expand All @@ -44,6 +49,7 @@
*
* @author Stephane Nicoll
* @author Artsiom Yudovin
* @author Chris Bono
*/
class HibernatePropertiesTests {

Expand Down Expand Up @@ -135,9 +141,37 @@ void defaultDdlAutoIsNotInvokedIfHibernateSpecificPropertyIsSet() {
.run(assertDefaultDdlAutoNotInvoked("create"));
}

@ParameterizedTest
@MethodSource("validJpaDatabaseActionsToExpectedHbmValues")
void defaultDdlAutoIsNotInvokedIfJpaDatabaseActionPropertyIsSetToValidJpaValue(String jpaDatabaseAction,
Copy link
Contributor Author

@onobc onobc Feb 12, 2021

Choose a reason for hiding this comment

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

Of course, these tests all reflect the "Strict Approach" outlined above.

String expectedHbmDdlAuto) {
this.contextRunner.withPropertyValues(
"spring.jpa.properties.javax.persistence.schema-generation.database.action=" + jpaDatabaseAction)
.run(assertDefaultDdlAutoNotInvoked(expectedHbmDdlAuto));
}

private static Stream<Arguments> validJpaDatabaseActionsToExpectedHbmValues() {
return Stream.of(Arguments.of("none", null), Arguments.of("create", "create-only"),
Arguments.of("drop", "drop"), Arguments.of("drop-and-create", "create"));
}

@ParameterizedTest
@ValueSource(strings = { "create-only", "create-drop", "validate", "update" })
void defaultDdlAutoIsInvokedIfJpaDatabaseActionPropertyIsSetToNonJpaValue(String nonJpaDatabaseActionValue) {
this.contextRunner
.withPropertyValues("spring.jpa.properties.javax.persistence.schema-generation.database.action="
+ nonJpaDatabaseActionValue)
.run(assertHibernateProperties((hibernateProperties) -> verify(this.ddlAutoSupplier).get()));
}

private ContextConsumer<AssertableApplicationContext> assertDefaultDdlAutoNotInvoked(String expectedDdlAuto) {
return assertHibernateProperties((hibernateProperties) -> {
assertThat(hibernateProperties).containsEntry(AvailableSettings.HBM2DDL_AUTO, expectedDdlAuto);
if (expectedDdlAuto == null) {
assertThat(hibernateProperties).doesNotContainKey(AvailableSettings.HBM2DDL_AUTO);
}
else {
assertThat(hibernateProperties).containsEntry(AvailableSettings.HBM2DDL_AUTO, expectedDdlAuto);
}
verify(this.ddlAutoSupplier, never()).get();
});
}
Expand Down