jamesfredley commented on code in PR #16497:
URL: https://github.com/apache/grails-core/pull/16497#discussion_r4177925799
##########
grails-data-hibernate7/dbmigration-core/src/main/java/liquibase/ext/hibernate/snapshot/IdentifierGeneratorSupport.java:
##########
@@ -0,0 +1,68 @@
+package liquibase.ext.hibernate.snapshot;
+
+import liquibase.Scope;
+import org.hibernate.boot.model.relational.SqlStringGenerationContext;
+import
org.hibernate.boot.model.relational.internal.SqlStringGenerationContextImpl;
+import org.hibernate.generator.Generator;
+import org.hibernate.id.NativeGenerator;
+import org.hibernate.mapping.GeneratorSettings;
+import org.hibernate.mapping.SimpleValue;
+
+/**
+ * Shared helpers for the snapshot generators that inspect identifier
generators.
+ */
+final class IdentifierGeneratorSupport {
+
+ private IdentifierGeneratorSupport() {
+ }
+
+ /**
+ * For annotation-based entities an identifier without {@code
@GeneratedValue} is application-assigned, so no
+ * generator applies. XML-mapped entities have no member details and
declare their generator in the hbm.xml
+ * mapping, so they always count as having generation intent.
+ */
+ static boolean hasGenerationIntent(SimpleValue simpleValue) {
+ var memberDetails = simpleValue.getMemberDetails();
+ return memberDetails == null ||
+
memberDetails.hasDirectAnnotationUsage(jakarta.persistence.GeneratedValue.class);
Review Comment:
This treats any annotation mapping without `@GeneratedValue` as
application-assigned. Hibernate 7 `@IdGeneratorType` annotations, including
`@NativeGenerator`, are generation intent on their own. `NativeGenEntity` hides
the gap by also adding `@GeneratedValue`; dropping that annotation makes
`NativeGeneratorAutoIncrementTest` fail, and `NativeGeneratorSequenceTest` only
still passes because the namespace pass already registered the sequence.
Recognize generator meta-annotations, or Hibernate's
assigned/generator-creator status, and test `@Id @NativeGenerator` without
`@GeneratedValue` for both identity and sequence dialects.
##########
grails-data-hibernate7/dbmigration-core/src/main/java/liquibase/ext/hibernate/snapshot/HibernateColumnSnapshotGenerator.java:
##########
@@ -267,6 +268,41 @@ public Class<? extends SnapshotGenerator>[] replaces() {
return new Class[]
{liquibase.snapshot.jvm.ColumnSnapshotGenerator.class};
}
+ private boolean handleSequenceGenerator(
+ org.hibernate.id.enhanced.SequenceStyleGenerator seqGen,
+ Dialect dialect,
+ HibernateDatabase database,
+ Column column,
+ org.hibernate.mapping.Table hibernateTable,
+ org.hibernate.mapping.Column hibernateColumn) {
+ if (PostgreSQLDialect.class.isAssignableFrom(dialect.getClass())) {
+ String sequenceName = resolveSequenceName(seqGen, hibernateTable,
hibernateColumn);
Review Comment:
This calls `resolveSequenceName`, which dereferences
`structure.getPhysicalName()` with no null check (line 319, unchanged, so not
commentable on its own). GORM's default id is a `GrailsNativeGenerator` that
calls `initialize(null, null, context)` and never `configure`, so the
`SequenceStyleGenerator` delegate has a null `DatabaseStructure`. On PostgreSQL
the new native `SEQUENCE` branch reaches this and the snapshot NPEs.
`GormColumnSnapshotGenerator` delegates here first, so its postprocessing
cannot catch it.
Guard the unconfigured delegate (do not invent a sequence name from a null
structure) and add a public GORM default-id snapshot regression on PostgreSQL.
##########
grails-data-hibernate7/dbmigration-core/src/main/java/liquibase/ext/hibernate/snapshot/HibernateSequenceSnapshotGenerator.java:
##########
@@ -36,15 +48,69 @@ protected void addTo(DatabaseObject foundObject,
DatabaseSnapshot snapshot)
if (foundObject instanceof Schema schema) {
HibernateDatabase database = (HibernateDatabase)
snapshot.getDatabase();
+ Set<String> addedSequences = new HashSet<>();
+
for (org.hibernate.boot.model.relational.Namespace namespace :
database.getMetadata().getDatabase().getNamespaces()) {
for (org.hibernate.boot.model.relational.Sequence sequence :
namespace.getSequences()) {
+ String name =
sequence.getName().getSequenceName().getText();
schema.addDatabaseObject(new Sequence()
-
.setName(sequence.getName().getSequenceName().getText())
+ .setName(name)
.setSchema(schema)
.setStartValue(BigInteger.valueOf(sequence.getInitialValue()))
.setIncrementBy(BigInteger.valueOf(sequence.getIncrementSize())));
+ addedSequences.add(name.toLowerCase(Locale.ROOT));
+ }
+ }
+
+ addGeneratorSequences(database, schema, addedSequences);
+ }
+ }
+
+ private void addGeneratorSequences(HibernateDatabase database, Schema
schema, Set<String> addedSequences) {
+ MetadataImplementor metadata = (MetadataImplementor)
database.getMetadata();
+ var dialect = database.getDialect();
+
+ for (PersistentClass entityBinding : metadata.getEntityBindings()) {
+ if (!(entityBinding instanceof RootClass rootClass) ||
+ !(rootClass.getIdentifier() instanceof SimpleValue
simpleValue) ||
+
!IdentifierGeneratorSupport.hasGenerationIntent(simpleValue)) {
+ continue;
+ }
+
+ try {
+ var generator = simpleValue.createGenerator(
+ dialect,
+ rootClass,
+ rootClass.getIdentifierProperty(),
+
IdentifierGeneratorSupport.createGeneratorSettings(simpleValue));
+
+ SequenceStyleGenerator seqGen = null;
+ // NativeGenerator may wrap a SequenceStyleGenerator delegate
depending on the dialect.
+ if (generator instanceof NativeGenerator nativeGen) {
+ if (IdentifierGeneratorSupport.nativeDelegate(nativeGen)
instanceof SequenceStyleGenerator s) {
+ seqGen = s;
+ }
+ } else if (generator instanceof SequenceStyleGenerator s) {
+ seqGen = s;
+ }
+
+ if (seqGen != null) {
+ var structure = seqGen.getDatabaseStructure();
+ if (structure != null && structure.getPhysicalName() !=
null) {
+ String name = structure.getPhysicalName().render();
Review Comment:
`render()` includes catalog/schema qualification and quoting, but the
namespace pass above stores only `getSequenceName().getText()`. A sequence
already recorded as `qs_seq` is added again as `app.qs_seq` (confirmed with
`@SequenceGenerator(sequenceName = "qs_seq", schema = "app")` on
`PostgreSQLDialect`; 8.0.x contains only `qs_seq`). `toLowerCase` on the dedup
key also collapses distinct quoted identifiers such as `"Foo"` and `"foo"`.
Use one canonical catalog/schema/object identity for both passes, store the
bare name separately from its schema, preserve case for quoted names, and
assert exactly one snapshot object and one emitted sequence change.
##########
grails-data-hibernate7/dbmigration-core/src/main/java/liquibase/ext/hibernate/snapshot/HibernateSequenceSnapshotGenerator.java:
##########
@@ -36,15 +48,69 @@ protected void addTo(DatabaseObject foundObject,
DatabaseSnapshot snapshot)
if (foundObject instanceof Schema schema) {
HibernateDatabase database = (HibernateDatabase)
snapshot.getDatabase();
+ Set<String> addedSequences = new HashSet<>();
+
for (org.hibernate.boot.model.relational.Namespace namespace :
database.getMetadata().getDatabase().getNamespaces()) {
for (org.hibernate.boot.model.relational.Sequence sequence :
namespace.getSequences()) {
+ String name =
sequence.getName().getSequenceName().getText();
schema.addDatabaseObject(new Sequence()
-
.setName(sequence.getName().getSequenceName().getText())
+ .setName(name)
.setSchema(schema)
.setStartValue(BigInteger.valueOf(sequence.getInitialValue()))
.setIncrementBy(BigInteger.valueOf(sequence.getIncrementSize())));
+ addedSequences.add(name.toLowerCase(Locale.ROOT));
+ }
+ }
+
+ addGeneratorSequences(database, schema, addedSequences);
+ }
+ }
+
+ private void addGeneratorSequences(HibernateDatabase database, Schema
schema, Set<String> addedSequences) {
+ MetadataImplementor metadata = (MetadataImplementor)
database.getMetadata();
+ var dialect = database.getDialect();
+
+ for (PersistentClass entityBinding : metadata.getEntityBindings()) {
+ if (!(entityBinding instanceof RootClass rootClass) ||
+ !(rootClass.getIdentifier() instanceof SimpleValue
simpleValue) ||
+
!IdentifierGeneratorSupport.hasGenerationIntent(simpleValue)) {
+ continue;
+ }
+
+ try {
+ var generator = simpleValue.createGenerator(
+ dialect,
+ rootClass,
+ rootClass.getIdentifierProperty(),
+
IdentifierGeneratorSupport.createGeneratorSettings(simpleValue));
+
+ SequenceStyleGenerator seqGen = null;
+ // NativeGenerator may wrap a SequenceStyleGenerator delegate
depending on the dialect.
+ if (generator instanceof NativeGenerator nativeGen) {
+ if (IdentifierGeneratorSupport.nativeDelegate(nativeGen)
instanceof SequenceStyleGenerator s) {
+ seqGen = s;
+ }
+ } else if (generator instanceof SequenceStyleGenerator s) {
+ seqGen = s;
+ }
+
+ if (seqGen != null) {
+ var structure = seqGen.getDatabaseStructure();
+ if (structure != null && structure.getPhysicalName() !=
null) {
Review Comment:
A Liquibase `Sequence` is recorded whenever `getPhysicalName()` is non-null.
`SequenceStyleGenerator` can be table-backed (`TableStructure`), including
MySQL sequence emulation and `force_table_use`. Snapshotting
`com.example.ejb3.auction` with `MySQLDialect` now emits five `Sequence`
objects (`AUDITED_ITEM_SEQ`, `AuctionItem_SEQ`, `Bid_SEQ`, `ITEM_SEQ`,
`User_SEQ`); on 8.0.x the same snapshot has none. MySQL has no sequences, so
those are the generators' backing tables. A forced-table mapping on a
sequence-capable database can emit a fictitious sequence next to the generator
table.
Require `structure.isPhysicalSequence()` before adding a sequence, and cover
MySQL emulation plus a PostgreSQL forced-table generator as a negative test.
##########
grails-data-hibernate7/dbmigration-core/src/main/java/liquibase/ext/hibernate/snapshot/HibernateColumnSnapshotGenerator.java:
##########
@@ -267,6 +268,41 @@ public Class<? extends SnapshotGenerator>[] replaces() {
return new Class[]
{liquibase.snapshot.jvm.ColumnSnapshotGenerator.class};
}
+ private boolean handleSequenceGenerator(
+ org.hibernate.id.enhanced.SequenceStyleGenerator seqGen,
+ Dialect dialect,
+ HibernateDatabase database,
+ Column column,
+ org.hibernate.mapping.Table hibernateTable,
+ org.hibernate.mapping.Column hibernateColumn) {
+ if (PostgreSQLDialect.class.isAssignableFrom(dialect.getClass())) {
+ String sequenceName = resolveSequenceName(seqGen, hibernateTable,
hibernateColumn);
+ column.setDefaultValue(new DatabaseFunction("nextval('" +
sequenceName + "'::regclass)"));
+ return false;
+ }
+ return database.supportsAutoIncrement();
Review Comment:
For a native `SEQUENCE` strategy this returns
`database.supportsAutoIncrement()`. `HibernateDatabase.supportsAutoIncrement()`
is hard-coded `true`, so Oracle (and any non-PostgreSQL dialect whose native
strategy is `SEQUENCE`) gets `AutoIncrementInformation` on a sequence-generated
column. The unresolved-delegate branch at line 300 has the same problem.
Return false for sequence and table generators. Mark only identity-backed
columns auto-increment, and add a non-PostgreSQL sequence-strategy regression.
##########
grails-data-hibernate7/dbmigration-core/src/main/java/liquibase/ext/hibernate/diff/HibernateChangedIndexChangeGenerator.java:
##########
@@ -0,0 +1,41 @@
+package liquibase.ext.hibernate.diff;
+
+import liquibase.change.Change;
+import liquibase.database.Database;
+import liquibase.diff.ObjectDifferences;
+import liquibase.diff.output.DiffOutputControl;
+import liquibase.diff.output.changelog.ChangeGeneratorChain;
+import liquibase.ext.hibernate.database.HibernateDatabase;
+import liquibase.structure.DatabaseObject;
+import liquibase.structure.core.Index;
+
+/**
+ * Hibernate does not know every index attribute ({@code unique}, {@code
using}), so those differences are
+ * suppressed to prevent needless drop and recreate changes on every diff.
+ */
+public class HibernateChangedIndexChangeGenerator
+ extends
liquibase.diff.output.changelog.core.ChangedIndexChangeGenerator {
+
+ @Override
+ public int getPriority(Class<? extends DatabaseObject> objectType,
Database database) {
+ return Index.class.isAssignableFrom(objectType) ? PRIORITY_ADDITIONAL
: PRIORITY_NONE;
+ }
+
+ @Override
+ public Change[] fixChanged(
+ DatabaseObject changedObject,
+ ObjectDifferences differences,
+ DiffOutputControl control,
+ Database referenceDatabase,
+ Database comparisonDatabase,
+ ChangeGeneratorChain chain) {
+ if (referenceDatabase instanceof HibernateDatabase ||
comparisonDatabase instanceof HibernateDatabase) {
+ differences.removeDifference("unique");
Review Comment:
`unique` is removed unconditionally, including a real `false` versus `true`
mismatch. `HibernateIndexSnapshotGenerator` does populate uniqueness, so this
is not always unknown metadata. An otherwise matching unique database index
versus a non-unique Hibernate index now yields no corrective change, on every
dialect.
Suppress only missing or unknown metadata. Keep a concrete Boolean mismatch,
and update the suppression test: it currently manufactures string differences,
so it cannot tell metadata noise from real uniqueness drift.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
