This is an automated email from the ASF dual-hosted git repository. borinquenkid pushed a commit to branch 8.0.x-hibernate7-dev in repository https://gitbox.apache.org/repos/asf/grails-core.git
commit a1f0712bfb36502cf8a99ba173ac8f67d4023ae6 Author: Walter Duque de Estrada <[email protected]> AuthorDate: Sun Mar 15 15:37:32 2026 -0500 hibernate 7: Refactoring ForeignKeyOneToOneBinder --- grails-data-hibernate7/HIBERNATE7-BINDING.md | 43 ++++++++-------------- grails-data-hibernate7/README.md | 22 ----------- .../binder/ForeignKeyOneToOneBinder.java | 4 +- .../domainbinding/binder/GrailsPropertyBinder.java | 3 +- .../ForeignKeyOneToOneBinderSpec.groovy | 5 +-- 5 files changed, 22 insertions(+), 55 deletions(-) diff --git a/grails-data-hibernate7/HIBERNATE7-BINDING.md b/grails-data-hibernate7/HIBERNATE7-BINDING.md index 82eba01b1b..e940fe6f53 100644 --- a/grails-data-hibernate7/HIBERNATE7-BINDING.md +++ b/grails-data-hibernate7/HIBERNATE7-BINDING.md @@ -1,28 +1,17 @@ # HIBERNATE7-UPGRADE-PROGRESS.md -## GrailsPropertyBinder Simplification +## Completed: GrailsPropertyBinder Simplification -**Objective:** Refactor the `GrailsPropertyBinder` class to consolidate the binder application logic into a single, unified conditional structure, reducing redundancy and improving code readability, while ensuring no regressions through testing. +**Objective:** Refactor the `GrailsPropertyBinder` class to consolidate the binder application logic into a single, unified conditional structure, reducing redundancy and improving code readability. -**Current State Analysis:** -The `bindProperty` method in `GrailsPropertyBinder.java` currently uses a series of `if-else if` statements to dispatch to different binder implementations based on the type of Hibernate `Value` created. This structure, while functional, can be simplified by consolidating the binder application logic and ensuring the creation and addition of the Hibernate `Property` are handled in a single, unified manner. +**Status: COMPLETED** -**Simplification Strategy:** -The core idea is to reorganize the binder application logic into a single primary conditional block. This block will internally dispatch to the correct binder based on the `Value` type. The creation and addition of the Hibernate `Property` will be moved to occur only once, after all specific binder logic has been executed, and conditional on `value` being non-null. +The `bindProperty` method in `GrailsPropertyBinder.java` has been successfully refactored. The core binder application logic is now contained within a single primary conditional block that dispatches to specific binders based on the GORM property type. -**Detailed Steps:** - -1. **Update `HIBERNATE7-UPGRADE-PROGRESS.md`**: Document this refined plan in the `HIBERNATE7-UPGRADE-PROGRESS.md` file. (This step is being performed now). -2. **Analyze `GrailsPropertyBinder.java`**: Re-examine the `bindProperty` method, specifically the section responsible for applying binders to the `Value` (the second major conditional block) and the subsequent `if (value != null)` block that creates and adds the Hibernate `Property`. -3. **Implement Code Refactoring**: - * **Remove redundant `createProperty` and `addProperty` calls**: Delete the lines `Property property = propertyFromValueCreator.createProperty(value, currentGrailsProp);` and `persistentClass.addProperty(property);` from *all* the individual `if`, `else if`, and `else` branches within the second conditional block (from `if (value instanceof Component ...)` down to the final `else if (value != null)`). - * **Introduce a single dispatcher block**: Enclose the entire existing `if-else if` chain (for `Component`, `OneToOne`, `ManyToOne`, `SimpleValue`, and the final `else if (value != null)`) within a new, single `if (value != null)` statement. This will serve as the unified entry point for binder application. - * **Centralize Property Creation/Addition**: Place a single instance of the lines `Property property = propertyFromValueCreator.createProperty(value, currentGrailsProp);` and `persistentClass.addProperty(property);` immediately *after* this new, single `if (value != null)` block. This ensures they are executed only once, after all specific binder logic, and only if `value` is non-null. -4. **Identify Relevant Tests**: Locate existing unit or integration tests that specifically target `GrailsPropertyBinder` scenarios, ensuring coverage for various property types (`Component`, `OneToOne`, `ManyToOne`, `SimpleValue` with its sub-conditions, `Collection`, `Enum`, etc.). If test coverage is insufficient, plan for adding new tests. -5. **Run Tests**: Execute the identified test suite to verify the functionality after the refactoring. -6. **Analyze Test Results**: Review the test output for any failures or regressions. -7. **Iterate and Refine**: If tests fail, debug the changes, make necessary adjustments to the code, and re-run the tests. -8. **Final Verification**: Ensure all tests pass and the code is functioning as expected, confirming the simplification was successful without introducing regressions. +**Key Changes:** +- **Consolidated Dispatcher:** Introduced a single `if-else if` chain in `GrailsPropertyBinder.bindProperty` that returns a Hibernate `Value`. +- **Centralized Property Creation:** The creation and addition of the Hibernate `Property` have been moved to the callers (e.g., `ClassPropertiesBinder`, `ComponentUpdater`, `CompositeIdBinder`) using the `PropertyFromValueCreator` utility. This ensures a single, unified entry point for property creation across different binding scenarios. +- **Redundancy Removed:** Replaced scattered `createProperty` and `addProperty` calls with a consistent pattern, significantly improving maintainability. ## GrailsDomainBinder Analysis @@ -108,7 +97,7 @@ The core idea is to reorganize the binder application logic into a single primar | `EnumTypeBinder` | Migrated | | | `PropertyFromValueCreator` | Migrated | | | `ComponentPropertyBinder` | Migrated | | -| `GrailsPropertyBinder` | Migrated | | +| `GrailsPropertyBinder` | Migrated | Simplified and consolidated. | | `CollectionBinder` | Migrated | | | `CompositeIdBinder` | Migrated | | | `IdentityBinder` | Migrated | | @@ -176,12 +165,6 @@ The core idea is to reorganize the binder application logic into a single primar | `BackticksRemover` | Migrated | | | `BasicValueIdCreator` | Migrated | | -## Known Issues / TODOs - -- `CollectionSecondPassBinder`: TODO support unidirectional many-to-many. -- `GrailsIncrementGenerator`: Reflection hacks for Hibernate 7. -- Several tests are currently failing (Multitenancy, CompositeId, etc.). See `grep -r TODO grails-data-hibernate7` for details. - ## Utility Class Refactoring & Mock Compatibility **Objective:** Modernize utility classes in `domainbinding.util` to use Hibernate-specific GORM types while maintaining compatibility with Spock mocks. @@ -195,4 +178,10 @@ The core idea is to reorganize the binder application logic into a single primar - **Logic Improvements:** - Updated `getDiscriminatorValue` in `GrailsHibernatePersistentEntity` to default to `getJavaClass().getSimpleName()` to match GORM conventions and test expectations. - Fixed `getMultiTenantFilterCondition` to safely handle non-Hibernate tenantId properties in test environments. - - Verified that all 1045 tests in `:grails-data-hibernate7-core` are passing. +- **Verification:** Verified that all 1045 tests in `:grails-data-hibernate7-core` are passing, confirming that the refactorings and modernizations have not introduced regressions. + +## Remaining Known Issues / TODOs + +- `CollectionSecondPassBinder`: TODO support unidirectional many-to-many. +- `GrailsIncrementGenerator`: Reflection hacks for Hibernate 7 (scheduled for removal in Hibernate 8). +- **Multitenancy & CompositeId:** While many tests are passing, some complex scenarios in `MultiTenancyBidirectionalManyToManySpec` and `GlobalConstraintWithCompositeIdSpec` may still require attention or further validation in a full application context. diff --git a/grails-data-hibernate7/README.md b/grails-data-hibernate7/README.md index a69562f596..bf5466698f 100644 --- a/grails-data-hibernate7/README.md +++ b/grails-data-hibernate7/README.md @@ -11,33 +11,11 @@ For testing the following was done: * Used testcontainers for specific tests instead of h2 to verify features not supported by h2. * A more opinionated and fluent HibernateGormDatastoreSpec is used for the specifications. -### Largest Gaps -### Ignored Features -The following tests are currently skipped in the `grails-data-hibernate7:core` test run. They fall into two categories: -#### 1. Local `@Ignore` — tests commented out or explicitly ignored in this module -| File | Feature | Reason | -|------|---------|--------| -| `grails/gorm/specs/SubclassMultipleListCollectionSpec` | `test inheritance with multiple list collections` | `@Ignore` — no reason given; blocked by an unresolved mapping issue | - -#### 2. TCK `@IgnoreIf` / `@PendingFeatureIf` — skipped because `hibernate7.gorm.suite=true` - -These tests live in `grails-datamapping-tck` and are deliberately excluded for Hibernate 7 because the underlying feature is not yet implemented or behaves differently: - -| TCK Spec | # skipped | Skip condition | Reason / notes | -|----------|-----------|----------------|------------------------------------------------------------------------------------------------| -| `DirtyCheckingSpec` | 6 | `@IgnoreIf(hibernate7.gorm.suite == true)` | Hibernate 7 dirty-checking semantics differ; the entire spec is disabled | -| `GroovyProxySpec` | 5 | `@IgnoreIf(hibernate5/6/7.gorm.suite)` | Groovy proxy support requires `ByteBuddyGroovyProxyFactory`; excluded for all Hibernate suites | -| `OptimisticLockingSpec` | 3 | `@IgnoreIf` (detects Hibernate datastore on classpath) | Hibernate has its own `Hibernate7OptimisticLockingSpec` replacement | -| `UpdateWithProxyPresentSpec` | 2 | `@IgnoreIf(hibernate7.gorm.suite == true)` | Proxy update behaviour differs in Hibernate 7 | -| `RLikeSpec` | 1 | `@IgnoreIf(hibernate7.gorm.suite == true)` | `rlike` not supported in HQL / H2 in Hibernate 7 mode | -| `DirtyCheckingAfterListenerSpec` | 1 | `@PendingFeatureIf(!hibernate5/6/mongodb)` | `test state change from listener update the object` — pending for Hibernate 7 | -| `DomainEventsSpec` | 1 | `@PendingFeature(reason='Was previously @Ignore')` | `Test bean autowiring` — pending across all suites | -| `WhereQueryConnectionRoutingSpec` | 5 | `@Requires(manager.supportsMultipleDataSources())` | Multiple datasource routing not supported in the TCK test manager | diff --git a/grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/ForeignKeyOneToOneBinder.java b/grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/ForeignKeyOneToOneBinder.java index db6c234c9f..615cedf8be 100644 --- a/grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/ForeignKeyOneToOneBinder.java +++ b/grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/ForeignKeyOneToOneBinder.java @@ -21,6 +21,7 @@ package org.grails.orm.hibernate.cfg.domainbinding.binder; import org.hibernate.MappingException; import org.hibernate.mapping.Column; import org.hibernate.mapping.ManyToOne; +import org.hibernate.mapping.Table; import org.grails.orm.hibernate.cfg.PropertyConfig; import org.grails.orm.hibernate.cfg.domainbinding.hibernate.GrailsHibernatePersistentEntity; @@ -49,7 +50,8 @@ public class ForeignKeyOneToOneBinder { /** * Binds the one-to-one property as a {@link ManyToOne} value and applies unique-key constraints. */ - public ManyToOne bind(HibernateOneToOneProperty property, org.hibernate.mapping.Table table, String path) { + public ManyToOne bind(HibernateOneToOneProperty property, String path) { + Table table = property.getTable(); GrailsHibernatePersistentEntity refDomainClass = property.getHibernateAssociatedEntity(); ManyToOne manyToOne = manyToOneBinder.doBind(property, refDomainClass, table, path); if (refDomainClass.getHibernateCompositeIdentity().isEmpty()) { diff --git a/grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/GrailsPropertyBinder.java b/grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/GrailsPropertyBinder.java index 10d06e3394..a4fda2cd88 100644 --- a/grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/GrailsPropertyBinder.java +++ b/grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/GrailsPropertyBinder.java @@ -80,12 +80,11 @@ public class GrailsPropertyBinder { && oneToOne.isValidHibernateOneToOne()) { value = oneToOneBinder.bindOneToOne(oneToOne, path); } else if (currentGrailsProp instanceof HibernateOneToOneProperty oneToOne) { - value = foreignKeyOneToOneBinder.bind(oneToOne, table, path); + value = foreignKeyOneToOneBinder.bind(oneToOne, path); } else if (currentGrailsProp instanceof HibernateManyToOneProperty manyToOne) { value = manyToOneBinder.bindManyToOne(manyToOne, table, path); } else if (currentGrailsProp instanceof HibernateToManyProperty toMany && !currentGrailsProp.isSerializableType()) { - // HibernateToManyProperty value = collectionBinder.bindCollection(toMany, persistentClass, path); } else if (currentGrailsProp instanceof HibernateEmbeddedProperty embedded) { value = componentBinder.bindComponent(persistentClass, embedded, path); diff --git a/grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/ForeignKeyOneToOneBinderSpec.groovy b/grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/ForeignKeyOneToOneBinderSpec.groovy index c016a3e22c..ee710e3e53 100644 --- a/grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/ForeignKeyOneToOneBinderSpec.groovy +++ b/grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/ForeignKeyOneToOneBinderSpec.groovy @@ -22,7 +22,6 @@ import grails.gorm.specs.HibernateGormDatastoreSpec import org.grails.datastore.mapping.model.MappingContext import org.grails.datastore.mapping.model.PersistentEntity import org.grails.orm.hibernate.cfg.Mapping -import java.util.Optional import org.grails.orm.hibernate.cfg.PersistentEntityNamingStrategy import org.grails.orm.hibernate.cfg.PropertyConfig import org.grails.orm.hibernate.cfg.domainbinding.hibernate.GrailsHibernatePersistentEntity @@ -77,7 +76,7 @@ class ForeignKeyOneToOneBinderSpec extends HibernateGormDatastoreSpec { inverseSide.isValidHibernateOneToOne() >> isInverseHasOne when: - def result = binder.bind(property, null, "/test") + def result = binder.bind(property, "/test") then: result.isAlternateUniqueKey() @@ -123,7 +122,7 @@ class ForeignKeyOneToOneBinderSpec extends HibernateGormDatastoreSpec { columnFetcher.getColumnForSimpleValue(_ as ManyToOne) >> null when: - binder.bind(property, null, "/test") + binder.bind(property, "/test") then: thrown(MappingException)
