Copilot commented on code in PR #16497:
URL: https://github.com/apache/grails-core/pull/16497#discussion_r4175510220
##########
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:
`HibernateIndexSnapshotGenerator` already sets index uniqueness
(`setUnique()` at line 71). Unconditionally removing this difference also hides
genuine `true`/`false` mismatches, such as a mapped non-unique index compared
with an existing unique index, leaving an unwanted uniqueness restriction in
place. Suppress only missing metadata. Update the suppression test to use
realistic null/Boolean values, and add a Hibernate-involved test that preserves
a concrete Boolean mismatch.
##########
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:
`SequenceStyleGenerator` can use a table-backed structure, for example with
`force_table_use=true` or a dialect without sequence support. This branch
records that table as a `Sequence`, introducing fictitious sequence changes
into the migration diff. Require `isPhysicalSequence()` before adding it, and
add a table-backed generator as a negative test.
##########
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();
+ if (addedSequences.add(name.toLowerCase(Locale.ROOT)))
{
Review Comment:
Lowercasing the deduplication key collapses distinct quoted identifiers such
as `"Foo"` and `"foo"`. If both sequences are discovered through generators,
the second is skipped and is missing from the snapshot. Use the same
quote-aware identifier key in both discovery paths, preserving case for quoted
names, and cover two generator-owned sequences that differ only in quoted case.
##########
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:
`@NativeGenerator` declares generation through `@IdGeneratorType` and does
not require `@GeneratedValue`. This guard rejects a valid `@Id
@NativeGenerator` mapping without `@GeneratedValue`, so column snapshots omit
identity metadata and generator sequence discovery skips it. Recognize the
native annotation as generation intent too, and test both identity and sequence
snapshots without the redundant `@GeneratedValue` in `NativeGenEntity`.
##########
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();
+ }
+
+ private boolean handleNativeGenerator(
+ org.hibernate.id.NativeGenerator nativeGen,
+ Dialect dialect,
+ HibernateDatabase database,
+ Column column,
+ org.hibernate.mapping.Table hibernateTable,
+ org.hibernate.mapping.Column hibernateColumn) {
+ return switch (nativeGen.getGenerationType()) {
+ case IDENTITY -> true;
+ case SEQUENCE -> {
+ var delegate =
IdentifierGeneratorSupport.nativeDelegate(nativeGen);
+ if (delegate instanceof
org.hibernate.id.enhanced.SequenceStyleGenerator seqGen) {
+ yield handleSequenceGenerator(seqGen, dialect, database,
column, hibernateTable, hibernateColumn);
+ }
+ yield database.supportsAutoIncrement();
+ }
Review Comment:
When a non-PostgreSQL dialect selects `SEQUENCE` for a native generator,
`handleSequenceGenerator()` returns `database.supportsAutoIncrement()`, which
is always true in `HibernateDatabase`. The caller then marks a
sequence-generated column as an identity column, producing incorrect
auto-increment migration metadata. Return false for the native `SEQUENCE`
strategy, including when its delegate cannot be resolved, while preserving
PostgreSQL default handling. Add a non-PostgreSQL sequence-strategy regression
test.
##########
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 loop uses the unqualified identifier text. A sequence already
recorded as `item_seq` can therefore be added again as `app.item_seq`, with the
qualifier incorrectly embedded in Liquibase's sequence name. Use the object
identifier text consistently with the existing namespace snapshot, and test
qualified and quoted sequence names.
--
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]
