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

onobc · 2021-02-07T22:08:11Z

Checks 'javax.persistence.schema-generation.database.action' before returning default ddlAuto in HibernateProperties.

…eturning default ddlAuto in HibernateProperties Fixes gh-25054

...figure/src/main/java/org/springframework/boot/autoconfigure/orm/jpa/HibernateProperties.java

snicoll

Thanks for the PR. I don't think handling the spec value as a ddl auto value is legit. See further comments for details.

snicoll · 2021-02-09T14:42:56Z

...figure/src/main/java/org/springframework/boot/autoconfigure/orm/jpa/HibernateProperties.java

 		}
+		String ddlDatabaseAction = existing.get(AvailableSettings.HBM2DDL_DATABASE_ACTION);
+		if (ddlDatabaseAction != null) {
+			return ddlDatabaseAction;


The problem here is that ddlDatabaseAction is not, strictly speaking, a ddlAuto. The values are close but not the same. Considering that, I don't think that change is correct.

Rather, I think we should review this method altogether and set a ddl-auto only if necessary, calling the supplier if need to be. This could prove tricky as we've had a few regressions in this area.

@snicoll I found the mapping from JPA (javax.persistence.schema-generation.database.action) to HBM (hibernate.hbm2ddl.auto) here.

There is a direct mapping from each possible value of the JPA property to its corresponding HBM property.

JPA HBM Description

none none No action will be performed

create create-only Database creation will be generated

drop drop Database dropping will be generated

drop-and-create create Database dropping will be generated followed by database creation

The code in CREATE_ONLY and CREATE confirms this as well.

Since this ticket is about setting the HBM default value from the JPA value, I think the above grid gives us what we want to use. What do you think?

Since this ticket is about setting the HBM default value from the JPA value, I think the above grid gives us what we want to use. What do you think?

Unfortunately, I don't think this statement is accurate. IMO, this issue is about not setting a default ddlAuto value if the JPA spec property is set. I think translating one to the other is also a completely different feature that I am not convinced we want to provide.

snicoll · 2021-02-09T14:45:18Z

...e/src/test/java/org/springframework/boot/autoconfigure/orm/jpa/HibernatePropertiesTests.java

+	void defaultDdlAutoIsNotInvokedIfJpaDatabaseActionPropertyIsSet() {
+		this.contextRunner
+				.withPropertyValues("spring.jpa.properties.javax.persistence.schema-generation.database.action=create")
+				.run(assertDefaultDdlAutoNotInvoked("create"));


I don't think this test is correct. The spec states that drop-and-create is a valid value. If you'd rewrite this test with this, you'd have to assert that ddl auto was set with it, which is not a valid ddlAuto.

Rather, we should check the ddl auto isn't set (but the spec property is).

onobc · 2021-02-09T14:58:05Z

Thanks for that info @snicoll . I will make the adjustments sometime w/in the next 24 hrs.

…on' before using it as hbm2ddl default

onobc · 2021-02-12T22:20:58Z

...figure/src/main/java/org/springframework/boot/autoconfigure/orm/jpa/HibernateProperties.java

 			return ddlAuto;
 		}
+		String ddlDatabaseAction = existing.get(AvailableSettings.HBM2DDL_DATABASE_ACTION);
+		if (ddlDatabaseAction != null) {


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.

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:

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)

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?"

onobc · 2021-02-12T22:22:15Z

...e/src/test/java/org/springframework/boot/autoconfigure/orm/jpa/HibernatePropertiesTests.java


+	@ParameterizedTest
+	@MethodSource("validJpaDatabaseActionsToExpectedHbmValues")
+	void defaultDdlAutoIsNotInvokedIfJpaDatabaseActionPropertyIsSetToValidJpaValue(String jpaDatabaseAction,


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

onobc · 2021-02-14T04:54:53Z

...figure/src/main/java/org/springframework/boot/autoconfigure/orm/jpa/HibernateProperties.java


 	private String determineDdlAuto(Map<String, String> existing, Supplier<String> defaultDdlAuto) {
+        if (existing.get(AvailableSettings.HBM2DDL_DATABASE_ACTION) != null) {
+            return null;


If the user specified db-action, respect that, do not set any ddl-auto default.

onobc · 2021-02-14T04:55:43Z

...figure/src/main/java/org/springframework/boot/autoconfigure/orm/jpa/HibernateProperties.java

 	}

 	private String determineDdlAuto(Map<String, String> existing, Supplier<String> defaultDdlAuto) {
+        if (existing.get(AvailableSettings.HBM2DDL_DATABASE_ACTION) != null) {


I made the db-action property take precedence over the hbm property to be consistent w/ Hibernate.

onobc · 2021-02-14T04:58:27Z

...main/java/org/springframework/boot/autoconfigure/orm/jpa/DataSourceInitializedPublisher.java

 				this.jpaProperties.getProperties(), new HibernateSettings().ddlAuto(defaultDdlAuto));
-		return hibernate.containsKey("hibernate.hbm2ddl.auto");
+		return hibernate.containsKey("hibernate.hbm2ddl.auto") || !hibernate
+				.getOrDefault("javax.persistence.schema-generation.database.action", "none").equals("none");


I am curious as to why? Should we do the same for the db-action property?

I found out what relies on the removed key. I have added the db-action property to this check to ensure we have consistent behavior when the user is setting only the db-action property to drive db schema creation. I also added tests to verify the cases.

…ase.action' is specified

See gh-25129

snicoll · 2021-03-22T13:48:42Z

Thanks @Bono007. I've polished the PR in c2f452a. One thing to note is that the Hibernate property should take precedence over the JPA one (as noted by @wilkinsona in the original issue) and the proposal did the opposite. I've also reworked the tests to focus on the vendor properties as it felt more focused and the event doesn't exist on master anymore.

Checks 'javax.persistence.schema-generation.database.action' before r…

21cf833

…eturning default ddlAuto in HibernateProperties Fixes gh-25054

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 7, 2021

onobc commented Feb 7, 2021

View reviewed changes

...figure/src/main/java/org/springframework/boot/autoconfigure/orm/jpa/HibernateProperties.java Outdated Show resolved Hide resolved

snicoll mentioned this pull request Feb 9, 2021

javax.persistence.schema-generation.database.action may be ignored #25054

Closed

snicoll added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 9, 2021

snicoll added this to the 2.3.9 milestone Feb 9, 2021

snicoll self-assigned this Feb 9, 2021

snicoll requested changes Feb 9, 2021

View reviewed changes

snicoll added the status: waiting-for-feedback We need additional information before we can continue label Feb 9, 2021

snicoll modified the milestones: 2.3.9, 2.3.x Feb 9, 2021

spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Feb 9, 2021

snicoll added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Feb 9, 2021

Considers value of 'javax.persistence.schema-generation.database.acti…

d9667a3

…on' before using it as hbm2ddl default

onobc commented Feb 12, 2021

View reviewed changes

onobc commented Feb 14, 2021

View reviewed changes

Does not set ddl-auto when 'javax.persistence.schema-generation.datab…

76186d8

…ase.action' is specified

onobc requested a review from snicoll February 19, 2021 16:58

snicoll removed the status: waiting-for-feedback We need additional information before we can continue label Mar 7, 2021

snicoll mentioned this pull request Mar 22, 2021

Checks 'javax.persistence.schema-generation.database.action' when determining DDL auto default #25754

Closed

snicoll modified the milestones: 2.3.x, 2.3.10 Mar 22, 2021

snicoll pushed a commit that referenced this pull request Mar 22, 2021

Take JPA database action into account when setting ddlAuto

674b01c

See gh-25129

snicoll added a commit that referenced this pull request Mar 22, 2021

Polish "Take JPA database action into account when setting ddlAuto"

c2f452a

See gh-25129

snicoll closed this in 2b29b99 Mar 22, 2021

wilkinsona changed the title ~~Checks 'javax.persistence.schema-generation.database.action' when determining DDL auto default~~ javax.persistence.schema-generation.database.action is ignored when checking if default DDL auto setting should be applied Apr 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

onobc commented Feb 7, 2021

Uh oh!

Uh oh!

snicoll left a comment

Uh oh!

snicoll Feb 9, 2021

Uh oh!

onobc Feb 12, 2021

Uh oh!

snicoll Feb 13, 2021

Uh oh!

snicoll Feb 9, 2021

Uh oh!

onobc commented Feb 9, 2021

Uh oh!

onobc Feb 12, 2021

Uh oh!

onobc Feb 13, 2021 •

edited

Loading

Uh oh!

onobc Feb 12, 2021

Uh oh!

onobc Feb 14, 2021

Uh oh!

onobc Feb 14, 2021

Uh oh!

onobc Feb 14, 2021

Uh oh!

snicoll commented Mar 22, 2021

Uh oh!

Uh oh!

JPA	HBM	Description
none	none	No action will be performed
create	create-only	Database creation will be generated
drop	drop	Database dropping will be generated
drop-and-create	create	Database dropping will be generated followed by database creation

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

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

Uh oh!

Conversation

onobc commented Feb 7, 2021

Uh oh!

Uh oh!

snicoll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onobc commented Feb 9, 2021

Uh oh!

Choose a reason for hiding this comment

Strict Approach

More Relaxed Approach

Uh oh!

onobc Feb 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Misconception

Silver lining

Third time's a charm

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snicoll commented Mar 22, 2021

Uh oh!

Uh oh!

onobc Feb 13, 2021 •

edited

Loading