jdaugherty commented on code in PR #16582:
URL: https://github.com/apache/grails-core/pull/16582#discussion_r4289771551
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/generator/GrailsNativeGenerator.java:
##########
@@ -19,39 +19,59 @@
package org.grails.orm.hibernate.cfg.domainbinding.generator;
import java.io.Serial;
-import java.lang.reflect.Field;
+import java.util.Properties;
import jakarta.persistence.GenerationType;
-import org.hibernate.HibernateException;
+import org.hibernate.boot.model.relational.Database;
+import org.hibernate.boot.model.relational.SqlStringGenerationContext;
+import
org.hibernate.boot.model.relational.internal.SqlStringGenerationContextImpl;
+import org.hibernate.engine.jdbc.env.spi.JdbcEnvironment;
import org.hibernate.engine.spi.SharedSessionContractImplementor;
import org.hibernate.generator.EventType;
import org.hibernate.generator.GeneratorCreationContext;
import org.hibernate.id.NativeGenerator;
-import org.hibernate.id.enhanced.SequenceStyleGenerator;
/**
* A native generator that supports Grails assigned identifiers and fixes
Hibernate 7 ClassCastException.
*
+ * <p>Hibernate drives {@link NativeGenerator} through {@code initialize}
(which picks the delegate for the
+ * dialect), {@code configure} (which configures a sequence or table
delegate), {@code registerExportables} (which
+ * adds the delegate's sequence or table to the schema export) and {@code
initialize(SqlStringGenerationContext)}.
+ * Grails creates this generator itself instead of through Hibernate's
generator binding, so it has to run every one
+ * of those steps. Skipping the middle ones leaves the delegate of a sequence
based dialect without its sequence.
Review Comment:
The fix changes what an upgraded application finds in its database, and the
upgrade guide doesn't cover it yet. On a sequence-based dialect the default id
mapping now gets one `<table>_SEQ` per entity, where Grails 7 on Hibernate 5
drew every default-mapped id from the single `hibernate_sequence`. §71 of
`upgrading80x.adoc` documents that rename only for explicit `id generator:
'sequence'` mappings; the default mapping is the common case, so a PostgreSQL
or Oracle application upgrading against an existing schema meets it on every
domain class. With `dbCreate: validate` startup fails with `Schema validation:
missing sequence [book_SEQ]`, with `none` the first save fails, and with
`update` Hibernate creates `book_SEQ` starting at 1, which collides with the
ids already in the table.
Could you add an upgrade note for the default mapping, next to §71 or folded
into it? Two things worth stating: an existing schema needs the new sequences
(the dbmigration snapshot now sees them, which is what the
`GormIdentifierSnapshot16497Spec` change here shows), and
`hibernate.id.db_structure_naming_strategy: legacy` keeps `hibernate_sequence`
for every unnamed sequence, which is the drop-in choice for a database that
already has it. I checked the latter on PostgreSQL 16 with this branch: the
default mapping creates `probe_book_seq` as-is and `hibernate_sequence` with
the `legacy` setting, and the ids come out as 1, 2 either way.
Anchoring here since the doc file isn't in the diff.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/generator/GrailsNativeGenerator.java:
##########
@@ -19,39 +19,59 @@
package org.grails.orm.hibernate.cfg.domainbinding.generator;
import java.io.Serial;
-import java.lang.reflect.Field;
+import java.util.Properties;
import jakarta.persistence.GenerationType;
-import org.hibernate.HibernateException;
+import org.hibernate.boot.model.relational.Database;
+import org.hibernate.boot.model.relational.SqlStringGenerationContext;
+import
org.hibernate.boot.model.relational.internal.SqlStringGenerationContextImpl;
+import org.hibernate.engine.jdbc.env.spi.JdbcEnvironment;
import org.hibernate.engine.spi.SharedSessionContractImplementor;
import org.hibernate.generator.EventType;
import org.hibernate.generator.GeneratorCreationContext;
import org.hibernate.id.NativeGenerator;
-import org.hibernate.id.enhanced.SequenceStyleGenerator;
/**
* A native generator that supports Grails assigned identifiers and fixes
Hibernate 7 ClassCastException.
*
+ * <p>Hibernate drives {@link NativeGenerator} through {@code initialize}
(which picks the delegate for the
+ * dialect), {@code configure} (which configures a sequence or table
delegate), {@code registerExportables} (which
+ * adds the delegate's sequence or table to the schema export) and {@code
initialize(SqlStringGenerationContext)}.
+ * Grails creates this generator itself instead of through Hibernate's
generator binding, so it has to run every one
+ * of those steps. Skipping the middle ones leaves the delegate of a sequence
based dialect without its sequence.
+ *
* @since 8.0
*/
public class GrailsNativeGenerator extends NativeGenerator {
@Serial
private static final long serialVersionUID = 1L;
- public GrailsNativeGenerator(GeneratorCreationContext context) {
- // This triggers the internal switch logic in NativeGenerator,
- // which calls setIdentity(true) on the column for H2.
- try {
- this.initialize(null, null, context);
- } catch (Exception ignored) {
- // ignore for now, helps with testing robustness where context
might be incomplete
- }
+ /** Carries the default {@code @NativeGenerator} the way Hibernate reads
it from an unannotated identifier. */
+ @org.hibernate.annotations.NativeGenerator
+ private static final class DefaultNativeGenerator {}
+
+ private static final org.hibernate.annotations.NativeGenerator
DEFAULT_ANNOTATION =
+
DefaultNativeGenerator.class.getAnnotation(org.hibernate.annotations.NativeGenerator.class);
+
+ public GrailsNativeGenerator(GeneratorCreationContext context,
JdbcEnvironment jdbcEnvironment) {
+ // Picks the delegate for the dialect; for IDENTITY it also calls
setIdentity(true) on the column.
+ this.initialize(DEFAULT_ANNOTATION, null, context);
+ this.configure(context, new Properties());
+
+ Database database = context.getDatabase();
+ this.registerExportables(database);
+
+ var physicalName = database.getDefaultNamespace().getPhysicalName();
+ String catalog = (physicalName.catalog() != null) ?
physicalName.catalog().getCanonicalName() : null;
+ String schema = (physicalName.schema() != null) ?
physicalName.schema().getCanonicalName() : null;
+ SqlStringGenerationContext sqlContext =
+ SqlStringGenerationContextImpl.fromExplicit(jdbcEnvironment,
database, catalog, schema);
Review Comment:
Optional, and the two sibling generators do the same, so it needn't change
in this PR. `database.getDefaultNamespace()` is `locateNamespace(null, null)`,
so its physical name carries no catalog or schema under the standard physical
naming: the two branches above are dead and this is always
`fromExplicit(jdbcEnvironment, database, null, null)`, which means
`hibernate.default_schema` and `hibernate.default_catalog` never reach the
generator's rendered `next value` SQL even though the schema export qualifies
the `create sequence` with them. Hibernate's own binder passes
`creationContext.getSqlStringGenerationContext()`
(`GeneratorBinder.callConfigure`), which carries those defaults, and
`GeneratorCreationContextWrapper` already forwards it, so
`this.initialize(context.getSqlStringGenerationContext())` would do the same
job without the `JdbcEnvironment` parameter. Happy for that to be a follow-up
covering all three generators together.
--
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]
