-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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.
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
Configuration property to work with database schemas generated for Hibernate ORM 5.6 #31540
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
af1abc7
Add quarkus.hibernate-orm.database.orm-compatibility.version
yrodiere 58ebe5c
Integration tests for quarkus.hibernate-orm.database.orm-compatibilit…
yrodiere 8dc4c1d
Warn on startup when setting a database/orm compatibility version
yrodiere 1e6b33b
Test explicitly defined generators when testing ORM 5.6 compatibility
yrodiere 47f418b
Make ORM 5 compatibility test work on machines with different timezones
yrodiere File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
113 changes: 113 additions & 0 deletions
113
...us/hibernate/orm/config/databaseormcompatibility/DatabaseOrmCompatibilityVersionTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
package io.quarkus.hibernate.orm.config.databaseormcompatibility; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
|
||
import java.util.Map; | ||
import java.util.logging.Formatter; | ||
import java.util.logging.Level; | ||
|
||
import jakarta.inject.Inject; | ||
import jakarta.persistence.Entity; | ||
import jakarta.persistence.EntityManager; | ||
import jakarta.persistence.EntityManagerFactory; | ||
import jakarta.persistence.GeneratedValue; | ||
import jakarta.persistence.Id; | ||
|
||
import org.hibernate.annotations.GenericGenerator; | ||
import org.hibernate.cfg.AvailableSettings; | ||
import org.jboss.logmanager.formatters.PatternFormatter; | ||
import org.junit.jupiter.api.Test; | ||
import org.junit.jupiter.api.extension.RegisterExtension; | ||
|
||
import io.quarkus.hibernate.orm.config.SettingsSpyingIdentifierGenerator; | ||
import io.quarkus.hibernate.orm.runtime.FastBootHibernatePersistenceProvider; | ||
import io.quarkus.test.QuarkusUnitTest; | ||
|
||
public class DatabaseOrmCompatibilityVersionTest { | ||
|
||
private static final Formatter LOG_FORMATTER = new PatternFormatter("%s"); | ||
|
||
@RegisterExtension | ||
static QuarkusUnitTest runner = new QuarkusUnitTest() | ||
.withApplicationRoot((jar) -> jar | ||
.addClass(SpyingIdentifierGeneratorEntity.class) | ||
.addClass(SettingsSpyingIdentifierGenerator.class)) | ||
.withConfigurationResource("application.properties") | ||
.overrideConfigKey("quarkus.hibernate-orm.database.orm-compatibility.version", "5.6") | ||
// We allow overriding database/orm compatibility settings with .unsupported-properties, | ||
// to enable step-by-step migration | ||
.overrideConfigKey( | ||
"quarkus.hibernate-orm.unsupported-properties.\"" + AvailableSettings.PREFERRED_INSTANT_JDBC_TYPE + "\"", | ||
"TIMESTAMP_UTC") | ||
// Expect warnings on startup | ||
.setLogRecordPredicate(record -> FastBootHibernatePersistenceProvider.class.getName().equals(record.getLoggerName()) | ||
&& record.getLevel().intValue() >= Level.WARNING.intValue()) | ||
.assertLogRecords(records -> { | ||
var assertion = assertThat(records) | ||
.as("Warnings on startup") | ||
.hasSizeGreaterThanOrEqualTo(3); | ||
assertion.element(0).satisfies(record -> assertThat(LOG_FORMATTER.formatMessage(record)) | ||
.contains("Persistence-unit [<default>] sets unsupported properties") | ||
// We should not log property values, that could be a security breach for some properties. | ||
.doesNotContain("some-value")); | ||
assertion.element(1).satisfies(record -> assertThat(LOG_FORMATTER.formatMessage(record)) | ||
.contains("Persistence-unit [<default>]:" | ||
+ " enabling best-effort backwards compatibility with 'quarkus.hibernate-orm.database.orm-compatibility.version=5.6'.", | ||
"Quarkus will attempt to change the behavior and expected schema of Hibernate ORM" | ||
+ " to match those of Hibernate ORM 5.6.", | ||
"This is an inherently best-effort feature", | ||
"may stop working in future versions of Quarkus", | ||
"Consider migrating your application", | ||
"https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.0:-Hibernate-ORM-5-to-6-migration")); | ||
assertion.anySatisfy(record -> assertThat(LOG_FORMATTER.formatMessage(record)) | ||
.contains( | ||
"Persistence-unit [<default>] - 5.6 compatibility: setting 'hibernate.timezone.default_storage=NORMALIZE'.", | ||
"affects Hibernate ORM's behavior and schema compatibility", | ||
"may stop working in future versions of Quarkus")); | ||
}); | ||
|
||
@Inject | ||
EntityManagerFactory emf; | ||
|
||
@Inject | ||
EntityManager em; | ||
|
||
@Test | ||
public void testPropertiesPropagatedToStaticInit() { | ||
assertThat(SettingsSpyingIdentifierGenerator.collectedSettings).hasSize(1); | ||
Map<String, Object> settings = SettingsSpyingIdentifierGenerator.collectedSettings.get(0); | ||
assertThat(settings).containsAllEntriesOf(Map.of( | ||
AvailableSettings.TIMEZONE_DEFAULT_STORAGE, "NORMALIZE", | ||
// We allow overriding database/orm compatibility settings with .unsupported-properties, | ||
// to enable step-by-step migration | ||
AvailableSettings.PREFERRED_INSTANT_JDBC_TYPE, "TIMESTAMP_UTC")); | ||
} | ||
|
||
@Test | ||
public void testPropertiesPropagatedToRuntimeInit() { | ||
assertThat(emf.getProperties()).containsAllEntriesOf(Map.of( | ||
AvailableSettings.TIMEZONE_DEFAULT_STORAGE, "NORMALIZE", | ||
// We allow overriding database/orm compatibility settings with .unsupported-properties, | ||
// to enable step-by-step migration | ||
AvailableSettings.PREFERRED_INSTANT_JDBC_TYPE, "TIMESTAMP_UTC")); | ||
} | ||
|
||
@Entity | ||
public static class SpyingIdentifierGeneratorEntity { | ||
@Id | ||
@GeneratedValue(generator = "spying-generator") | ||
@GenericGenerator(name = "spying-generator", strategy = "io.quarkus.hibernate.orm.config.SettingsSpyingIdentifierGenerator") | ||
private Long id; | ||
|
||
public SpyingIdentifierGeneratorEntity() { | ||
} | ||
|
||
public Long getId() { | ||
return id; | ||
} | ||
|
||
public void setId(Long id) { | ||
this.id = id; | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
45 changes: 33 additions & 12 deletions
45
...bernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/BuildTimeSettings.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,36 +1,57 @@ | ||
package io.quarkus.hibernate.orm.runtime; | ||
|
||
import java.util.Collections; | ||
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
public class BuildTimeSettings { | ||
import io.quarkus.hibernate.orm.runtime.config.DatabaseOrmCompatibilityVersion; | ||
|
||
private Map<String, Object> settings; | ||
public class BuildTimeSettings { | ||
|
||
public BuildTimeSettings(Map<String, Object> settings) { | ||
this.settings = Collections.unmodifiableMap(new HashMap<>(settings)); | ||
private Map<String, Object> quarkusConfigSettings; | ||
private DatabaseOrmCompatibilityVersion databaseOrmCompatibilityVersion; | ||
private Map<String, String> databaseOrmCompatibilitySettings; | ||
private Map<String, Object> allSettings; | ||
|
||
public BuildTimeSettings(Map<String, Object> quarkusConfigSettings, | ||
DatabaseOrmCompatibilityVersion databaseOrmCompatibilityVersion, | ||
Map<String, String> databaseOrmCompatibilitySettings, | ||
Map<String, Object> allSettings) { | ||
this.quarkusConfigSettings = Map.copyOf(quarkusConfigSettings); | ||
this.databaseOrmCompatibilityVersion = databaseOrmCompatibilityVersion; | ||
this.databaseOrmCompatibilitySettings = Map.copyOf(databaseOrmCompatibilitySettings); | ||
this.allSettings = Map.copyOf(allSettings); | ||
} | ||
|
||
public Object get(String key) { | ||
return settings.get(key); | ||
return allSettings.get(key); | ||
} | ||
|
||
public boolean getBoolean(String key) { | ||
Object propertyValue = settings.get(key); | ||
Object propertyValue = allSettings.get(key); | ||
return propertyValue != null && Boolean.parseBoolean(propertyValue.toString()); | ||
} | ||
|
||
public boolean isConfigured(String key) { | ||
return settings.containsKey(key); | ||
return allSettings.containsKey(key); | ||
} | ||
|
||
public Map<String, Object> getQuarkusConfigSettings() { | ||
return quarkusConfigSettings; | ||
} | ||
|
||
public DatabaseOrmCompatibilityVersion getDatabaseOrmCompatibilityVersion() { | ||
return databaseOrmCompatibilityVersion; | ||
} | ||
|
||
public Map<String, String> getDatabaseOrmCompatibilitySettings() { | ||
return databaseOrmCompatibilitySettings; | ||
} | ||
|
||
public Map<String, Object> getSettings() { | ||
return settings; | ||
public Map<String, Object> getAllSettings() { | ||
return allSettings; | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return this.getClass().getSimpleName() + " {" + settings.toString() + "}"; | ||
return this.getClass().getSimpleName() + " {" + allSettings.toString() + "}"; | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This confuses me a bit - why the first one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular reason,
findAny
(if that exists) would work. I copy pasted that code from the method that handles non-persistence.xml config.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But don't we attempt to match the right datasource configuration for this PU ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code for
persistence.xml
handling has always assumed we were using the default datasource for these PUs:https://github.com/yrodiere/quarkus/blob/8f171cdbcfa4c810f5ec10377240a5f3278607c0/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java#L396-L399
This new code is in line with that assumption.
If you think it's wrong and we should extract the datasource name from the
persistence.xml
(I guess it can be configured there?), I can open another ticket?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, existing code - no rush then. But yes it surprised me a bit - I guess I forgot we ignore the datasource section from a persistence.xml.
I guess ignoring it is fine, but to make things better we could try to spot if the user defined a datasource section and warn about it being ignored / or even fail.