Hibernate 7#15568
Open
jdaugherty wants to merge 1166 commits into8.0.xfrom
Open
Conversation
Move multi-line @requires closure statements to new line after opening brace in FindByMethodSpec. Remove unused GrailsTransactionTemplate import from OptimisticLockingSpec. Assisted-by: OpenCode <opencode@opencode.ai>
…rage Add 15 new test methods (from 5 to 20 total) covering all ProxyHandler and ProxyFactory contract methods: isInitialized (null, non-proxied, persistent collection, Hibernate proxy, Groovy proxy), unwrap (Hibernate proxy, Groovy proxy, non-proxy), getIdentifier (Hibernate proxy, non-proxied, null), createProxy, and isProxy (non-Hibernate object). The Groovy proxy isInitialized test is excluded because it depends on a source fix to HibernateProxyHandler.isInitialized() that will be submitted in a separate PR. Assisted-by: OpenCode <opencode@opencode.ai>
The Hibernate 7 HibernateProxyHandler.isInitialized() was missing a check for Groovy proxies (ProxyInstanceMetaClass), causing uninitialized Groovy proxies to be incorrectly reported as initialized. This is because the fallback Hibernate.isInitialized() returns true for any non-Hibernate object. Add GroovyProxyInterceptorLogic.isInitialized() helper that checks ProxyInstanceMetaClass.isProxyInitiated(), and call it from HibernateProxyHandler.isInitialized() before the Hibernate fallback. The Hibernate 5 implementation already had this check. This restores parity and adds the corresponding test. Assisted-by: OpenCode <opencode@opencode.ai>
- Rename groovyProxyInit to groovyProxyInitialized for clearer state semantics - Add ProxyInstanceMetaClass precondition assertion in Groovy proxy test Assisted-by: OpenCode <opencode@opencode.ai>
Hibernate 7's SqmCriteriaNodeBuilder.valueParameter() throws HibernateException when encountering Groovy's GStringImpl, unlike Hibernate 5 which silently coerced it. Add normalizeValue() to PredicateGenerator that converts CharSequence (including GString) to String before passing values to JPA Criteria predicates. Applied to Equals, NotEquals, IdEquals, SizeEquals, SizeNotEquals, and Between handlers. Like/ILike already called toString() and were unaffected. Removes @PendingFeature from MultipleDataSourceConnectionsSpec which now passes all 4 tests. Assisted-by: OpenCode <opencode@opencode.ai>
…query fix: normalize Groovy GString to String in Hibernate 7 criteria queries
…in H7 binding Introduce HibernateSimpleIdentityProperty and HibernateCompositeIdentityProperty as proper GORM persistent property types, replacing direct use of the DSL config objects (HibernateSimpleIdentity / HibernateCompositeIdentity) in the binding pipeline. - Add HibernateSimpleIdentityProperty and HibernateCompositeIdentityProperty (both extend HibernateIdentityProperty) - Add HibernatePersistentEntity.getIdentityProperty() returning the typed identity property (simple or composite) - HibernateMappingFactory.createIdentity() now returns HibernateSimpleIdentityProperty - Merge two BasicValueCreator.bindBasicValue() overloads into a single bindBasicValue(HibernatePersistentProperty) — the property supplies its own table and owner, removing redundant parameters - Remove Table parameter from SimpleIdBinder.bindSimpleId() and its callers (IdentityBinder, SimpleValueBinder) - Update all H7 unit specs (IdentityBinderSpec, SimpleIdBinderSpec, SimpleValueBinderSpec, BasicValueCreatorSpec) to match the new API Fix pre-existing TCK failures on Hibernate 7 (never passing in isolation): - EnumSpec: removed duplicate V1 EnumThing records in 3 feature methods that caused findByEn() to throw NonUniqueResultException - FindByMethodSpec.testBooleanPropertyQuery: reduced Highway fixture to 1 bypassed + 1 not-bypassed record; adjusted findAll* count assertions; removed TckBook.findPublished() unique-finder assertion against 3 records These TCK tests were broken since commit 20131c0 ('fix: restore non-hibernate7 test coverage') due to Hibernate 7 strict getSingleResult() enforcement. The full top-level build did not surface them because the grails-datamapping-tck jar was built before those feature methods existed in the compiled artifact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…erty Add getGeneratorName() as a default method on HibernatePersistentProperty that reads from the property's mapped form. HibernateSimpleIdentityProperty overrides it to delegate to the owning entity's getIdentityGeneratorName(), replacing the instanceof check that lived in BasicValueCreator. - HibernatePersistentProperty.getGeneratorName(): default reads PropertyConfig.getGenerator() - HibernateSimpleIdentityProperty.getGeneratorName(): delegates to entity identity generator name - BasicValueCreator.bindBasicValue(): simplified to property.getGeneratorName() with Optional.ofNullable().ifPresent() fluent style; no longer needs instanceof check - BasicValueCreator.createGenerator(): drop unused HibernateSimpleIdentityProperty parameter - BasicValueCreatorSpec: stub identityProperty.getGeneratorName() directly; remove dead domainClass.getIdentityGeneratorName() and getRootClass() stubs; remove unused RootClass entity field Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Verify the new getGeneratorName() default method and its override: - Regular property with no generator configured → returns null (default impl reads PropertyConfig.getGenerator()) - HibernateSimpleIdentityProperty with no explicit generator → delegates correctly to entity.getIdentityGeneratorName() and is non-null - HibernateSimpleIdentityProperty with id generator: 'uuid2' → returns 'uuid2' Adds GeneratorDefaultEntity and GeneratorUuid2Entity test fixtures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…returning null HibernatePersistentEntity wraps a Hibernate RootClass which always has an identifier. Returning null was misleading — callers would silently get NPEs downstream. Now getIdentityProperty() throws MappingException with a clear message if it is called on an entity that has no HibernateSimpleIdentityProperty (a programming error; only embedded entities have no identity, and they are not HibernatePersistentEntity instances). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- SimpleIdBinder.bindSimpleId: remove RootClass and HibernateSimpleIdentityProperty params; derive from domainClass internally; fix always-throw bug (add return) - CompositeIdBinder.bindCompositeId: remove RootClass and HibernateCompositeIdentity params; derive from domainClass internally; fix always-throw bug (add return) - IdentityBinder: call single-arg binders; set persistentClass from root before dispatch - HibernateCompositeIdentityProperty: change parts type from HibernateSimpleIdentityProperty[] to HibernatePersistentProperty[] so regular composite key fields (HibernateSimpleProperty) are not filtered out - HibernatePersistentEntity.getIdentityProperty: pass all composite parts directly without filtering by type - Update all specs: CompositeIdBinderSpec, SimpleIdBinderSpec, IdentityBinderSpec, HibernatePersistentPropertySpec (add composite key regression test) - AGENTS.md: add rule 11 — every code touch must update all tests for the changed class Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
setPersistentClass is already called in ClassBinder.bindClass before bindIdentity is invoked, making the param and the call redundant. Update RootPersistentClassCommonValuesBinder and IdentityBinderSpec accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Extract performUpsert() from save() in HibernateGormInstanceApi Routes to performPersist (id==null) or performMerge (id set) - Override GormStaticApi.merge(D d) in HibernateGormStaticApi to delegate to instanceApi.merge(d) instead of session.persist(d) which throws on detached entities in Hibernate 7 - Fix performMerge to sync id (before flush) and version (after flush) back to the caller's instance, matching Hibernate 5 mutation semantics - Add two tests: merge on new instance gets id+version=0; merge on detached instance keeps id, version increments to 1 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Existing test: list(max:N) and list(offset:N, max:M) -> PagedResultList - New test: list() and list(offset:N) without max -> plain List, not PagedResultList Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ria.list withPopulatedQuery already calls populateArgumentsForCriteria unconditionally. The extra call inside the args?.max branch caused duplicate ORDER BY clauses since Query.order() is additive (orderBy.add). Remove the redundant call. Add test to verify sort+max returns results in correct order exactly once. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ate7 # Conflicts: # NOTICE # gradle/publish-root-config.gradle
- Add Query.clearOrders() to core — clears orderBy list; subclasses override - Override HibernateQuery.clearOrders() to also clear JPA detachedCriteria orders, replacing the TODO HACK that used order(null) to piggyback order clearing - HibernateQuery.order() now unconditionally delegates to detachedCriteria (no null check) - PagedResultList.initialize() calls clearOrders() on cloned query before COUNT, replacing the direct orderBy.clear() which missed HibernateQuery's JPA orders - DetachedCriteria.list() uses protected newPagedResultList() factory method - Tests: HibernateQuerySpec (clearOrders), PagedResultListSpec (DetachedCriteria sort+max totalCount), DetachedCriteriaSpec TCK (sort+max+totalCount) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a second bypassed and not-bypassed Highway row so findAllBypassed() and findAllNotBypassed() actually validate multi-row behavior. Replace findNotBypassed()/findBypassed() singular calls (which now throw NonUniqueResultException with 2 rows) with findBypassedByName/ findNotBypassedByName to keep singular finders unambiguous. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When actionName is null and no id is present, append the controller token so that <g:form controller="foo"> (no action) generates /foo instead of an empty string. Preserve existing behaviour when an id is present and no action is given (id-only URL patterns such as /$id? still resolve correctly without prepending the context controller name). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 'many-to-many queries with sorting do not throw exception' feature (issue #14636) now passes on Hibernate 5 as well. Remove the @PendingFeatureIf guard and the unused IgnoreIf/PendingFeatureIf imports. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…& remove properties that are no longer used
Contributor
Author
|
FYI: I think I found out why testcontainers kept getting added to the boms - grails-forge is a micronaut app that still needs the older version. I've updated the micronaut dependencies to pull from the forge bom instead. |
Member
|
I am not sure if you’ll be able to make the older jar work with the newer DockerWalterSent from my iPhoneOn May 3, 2026, at 3:52 PM, James Daugherty ***@***.***> wrote:jdaugherty left a comment (apache/grails-core#15568)
FYI: I think I found out why testcontainers kept getting added to the boms - grails-forge is a micronaut app that still needs the older version. I've updated the micronaut dependencies to pull from the forge bom instead.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Contributor
Author
|
@borinquenkid FYI: the forge tests are passing with the changes. All of the grails projects are using the newer version from the spring bom. |
Member
|
Cool. |
# Conflicts: # grails-forge/grails-forge-core/src/test/groovy/org/grails/forge/build/dependencies/GradleDependencyComparatorSpec.groovy
Contributor
Author
|
I split the micronaut bom into 2 - 1 for hibernate 5 & 1 for hibernate 7. Also, as part of the merge I lowered the memory back down and the tests are passing. At this point, I need to review the licensing for the liquibase-hibernate import & then we can review this PR. |
Contributor
Author
|
@borinquenkid I need your help - do you have a base branch of the liquibase library that you imported? I need to clearly know which files were brought into the project (we can't put our own headers on them). I'm assuming it's the files under: grails-data-hibernate7/dbmigration/src/main/java ? |
Contributor
Author
|
Per discussion with @borinquenkid 4.27 liquibase-hibernate is what was imported. This means: https://github.com/liquibase/liquibase-hibernate/tree/v4.27.0-hibernate5 is the branch |
Import the original source from https://github.com/liquibase/liquibase-hibernate branch v4.27.0-hibernate5 into grails-data-hibernate7/dbmigration-core without modification. This establishes the upstream baseline for subsequent Hibernate 7 adaptation changes. The build.gradle and gradle.properties are new files to integrate this as a Gradle subproject within the grails-core build.
…ject Separate the liquibase-hibernate extension code from the dbmigration plugin into its own subproject (grails-data-hibernate7-dbmigration-core). This makes the dependency boundary clear: dbmigration-core contains the upstream forked library, while dbmigration contains the Grails-specific plugin code. - Remove liquibase/ext source and tests from dbmigration - Update dbmigration to depend on dbmigration-core - Update service files to keep only Grails-specific entries in dbmigration
- Rename snapshot/diff generators with 'Hibernate' prefix for clarity - Extract NoOpConnectionProvider and NoOpMultiTenantConnectionProvider from inner classes to standalone classes - Remove deprecated Hibernate 5 APIs: ClassLoaderDelegate, MetadataBuilderImpl, USE_NEW_ID_GENERATOR_MAPPINGS - Replace Class.newInstance() with Constructor.newInstance() reflection - Update Hibernate 7 API calls (MetadataBuilder, BootstrapServiceRegistry) - Update META-INF service files to match renamed classes - Update test fixtures for Hibernate 7 compatibility
Contributor
Author
|
I've reimported the code under dbmigration-core (dropping the liquibase name since it's trademarked). I've followed the guidelines here: https://www.apache.org/legal/src-headers.html#3party Which explicitly say do not add source headers. I've made 3 commits to illustrate the original forked code vs what Walter updated. I believe the licensing side of this is "right" now, but @jamesfredley do you think we should have someone in the ASF review this part of the contribution? |
…s that are forked from third party
…ng invoked (prevents .classpath, etc folders from being created)
✅ All tests passed ✅🏷️ Commit: 23649db Learn more about TestLens at testlens.app. |
jdaugherty
commented
May 6, 2026
Contributor
Author
|
I did some more minor cleanup & removed spotless since it's not being used right now. #15555 will add automated formatting |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Reopening. This PR replaces #15530