jdaugherty commented on code in PR #16497:
URL: https://github.com/apache/grails-core/pull/16497#discussion_r4175611327
##########
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) {
Review Comment:
This crashes the snapshot for any GORM domain class that keeps the default
`native` id on PostgreSQL or Oracle. Both dialects report `SEQUENCE` as their
native strategy.
GORM doesn't create this generator through `@NativeGenerator`.
`GrailsSequenceGeneratorEnum` returns a `GrailsNativeGenerator`, which calls
`initialize(null, null, context)` but never `configure(...)`. Its
`SequenceStyleGenerator` delegate therefore has no database structure, and
`resolveSequenceName` dereferences it:
```
java.lang.NullPointerException: Cannot invoke
"org.hibernate.id.enhanced.DatabaseStructure.getPhysicalName()" because
"structure" is null
at
liquibase.ext.hibernate.snapshot.HibernateColumnSnapshotGenerator.resolveSequenceName(HibernateColumnSnapshotGenerator.java:319)
at
liquibase.ext.hibernate.snapshot.HibernateColumnSnapshotGenerator.handleSequenceGenerator(HibernateColumnSnapshotGenerator.java:279)
at
liquibase.ext.hibernate.snapshot.HibernateColumnSnapshotGenerator.handleNativeGenerator(HibernateColumnSnapshotGenerator.java:298)
```
I reproduced it with a `HibernateSnapshotIntegrationSpec` subclass
(PostgreSQL container, `GormDatabase`) that snapshots GORM entities with
`native`, `sequence` and `identity` ids. The id column comes out as:
| id generator | 8.0.x | this PR |
|---|---|---|
| `native` (default) | autoIncrement, no default | NPE |
| `sequence` | `nextval('..._SEQ'::regclass)` default | unchanged |
| `identity` | NPE (`context.getProperty()` is null) | autoIncrement, no
default |
So passing the identifier property fixes `identity`, but the default
`native` mapping now fails. Skipping the sequence path when the delegate has no
database structure gives back the 8.0.x result for `native` and keeps the
`identity` fix. I checked that with the same spec:
```suggestion
if (delegate instanceof
org.hibernate.id.enhanced.SequenceStyleGenerator seqGen
&& seqGen.getDatabaseStructure() != null) {
```
The ported tests all use JPA-annotated fixtures in `dbmigration-core`, so
none of them reaches the generators GORM creates, and both module test suites
pass with this NPE in place. Could you add a spec in `dbmigration` next to
`HibernateSnapshotIntegrationSpec` that snapshots GORM entities with `native`,
`sequence` and `identity` ids through `GormDatabase`, so each branch here is
covered with the generators Grails actually builds?
##########
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:
Confirmed with an entity mapped as `@SequenceGenerator(name = "qs_gen",
sequenceName = "qs_seq", schema = "app")` on `PostgreSQLDialect`. The snapshot
now contains both `qs_seq` and `app.qs_seq`, where 8.0.x contains only `qs_seq`.
##########
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:
Confirmed. Snapshotting `com.example.ejb3.auction` with `MySQLDialect` now
produces 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 all five are the generators' backing tables.
##########
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:
Confirmed. Removing the `@GeneratedValue` line from `NativeGenEntity` makes
`NativeGeneratorAutoIncrementTest` fail. `NativeGeneratorSequenceTest` still
passes, but only because Hibernate already registers `native_gen_seq` in the
namespace, not because the new generator path finds it.
--
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]
