borinquenkid commented on code in PR #15568: URL: https://github.com/apache/grails-core/pull/15568#discussion_r3439400228
########## grails-data-hibernate7/AGENTS.md: ########## @@ -0,0 +1,232 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +--> +<!-- +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +--> + +# HIBERNATE7-UPGRADE-PROGRESS.md + +## 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. + +**Status: COMPLETED** + +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. + +**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 + +**Objective:** Document the core components and dependencies used by `GrailsDomainBinder` during the Hibernate 7 mapping process. + +`GrailsDomainBinder` is the central entry point for binding Grails domain classes to the Hibernate meta-model. It coordinates various specialized binder classes to handle entities, properties, identifiers, and collections. + +### Core Dependencies (org.grails.orm.hibernate.cfg.*) + +#### Logical Domain Mapping (cfg package) +* **GrailsHibernatePersistentEntity**: Core interface extending `PersistentEntity` with Hibernate-specific mapping capabilities (discriminators, data sources, etc.). +* **HibernatePersistentEntity**: Default implementation of `GrailsHibernatePersistentEntity`. +* **Mapping**: Groovy DSL representation of GORM mapping configurations. +* **HibernateMappingContext**: Specialized `MappingContext` for Hibernate. +* **HibernateMappingContextConfiguration**: Coordinates the creation of Hibernate `Metadata` and `SessionFactory` using GORM entities. +* **PersistentEntityNamingStrategy**: Strategy interface for resolving physical names (tables, columns). +* **NamingStrategyWrapper**: Wraps Hibernate's `PhysicalNamingStrategy` for GORM usage. +* **MappingCacheHolder**: Singleton used to cache `Mapping` instances for entities to avoid repeated DSL evaluation. + +#### Property & Value Binding (cfg.domainbinding package) +* **GrailsPropertyBinder**: Main coordinator for binding individual persistent properties to Hibernate `Value` objects. +* **PropertyBinder**: Binds Hibernate `Property` objects, handling updateable/insertable flags. +* **SimpleValueBinder**: Binds simple types (String, Integer, etc.) to Hibernate `BasicValue`. +* **SimpleValueColumnBinder**: Handles the binding of columns to `SimpleValue` instances. +* **ComponentPropertyBinder**: Specialized binder for GORM embedded components. +* **ComponentBinder**: Binds Hibernate `Component` instances. +* **EnumTypeBinder**: Handles the binding of Java Enums using `GrailsEnumType`. +* **OneToOneBinder / ManyToOneBinder**: Handle GORM associations and their corresponding Hibernate mappings. +* **ManyToOneValuesBinder**: Specifically handles the `Value` binding for many-to-one associations. +* **CollectionBinder**: Handles GORM collections (Set, List, Map) and their Hibernate `Collection` mappings. +* **PropertyFromValueCreator**: Utility to create Hibernate `Property` instances from a `Value`. + +#### Identifier & Version Binding (cfg.domainbinding package) +* **IdentityBinder**: Main coordinator for binding entity identifiers (simple or composite). +* **SimpleIdBinder**: Binds simple primary keys. +* **CompositeIdBinder**: Binds composite primary keys. +* **BasicValueCreator**: Factory for creating identifier `Value` objects and their generators. +* **VersionBinder**: Binds the version property used for optimistic locking. +* **NaturalIdentifierBinder**: Binds properties marked as `naturalId`. + +#### ID Generators (cfg.domainbinding.generator package) +* **GrailsSequenceWrapper**: Wraps Hibernate 7 generator creation. +* **GrailsSequenceGeneratorEnum**: Enum mapping Grails generator names to Hibernate 7 `Generator` implementations. +* **GrailsIdentityGenerator / GrailsIncrementGenerator / GrailsNativeGenerator / GrailsSequenceStyleGenerator / GrailsTableGenerator**: GORM-specific extensions of Hibernate 7 generators. + +#### Sub-mapping & Collection Types (cfg.domainbinding.collectionType package) +* **CollectionHolder**: Context object passed through binders to maintain collection state. +* **ListCollectionType / SetCollectionType / MapCollectionType / BagCollectionType**: Metadata classes defining how different GORM collections are mapped. + +#### Second Pass Binding (cfg.domainbinding.secondpass package) +* **GrailsSecondPass**: Base interface for binding operations that must occur after all entities are initially processed. +* **CollectionSecondPassBinder / ListSecondPassBinder / MapSecondPassBinder**: Implementations handling the final binding of associations and collection elements. + +#### Miscellaneous Utilities +* **NamespaceNameExtractor**: Extracts schema and catalog information from Hibernate metadata. +* **TableNameFetcher**: Resolves the table name for a given entity using the naming strategy. +* **DefaultColumnNameFetcher**: Resolves default column names for properties. +* **ColumnNameForPropertyAndPathFetcher**: Resolves column names considering embedded paths. +* **BackticksRemover**: Utility for handling database identifiers with quotes. Replaced redundant `BackTigsTrimmer`. +* **ConfigureDerivedPropertiesConsumer**: Applies `derived` flag to properties based on mapping. +* **GrailsHibernateUtil**: General utility methods for Hibernate integration. + +### Migration Status Breakdown + +#### Main Classes + +| Class | Package | Status | Notes | +| :--- | :--- | :--- | :--- | +| `GrailsDomainBinder` | `org.grails.orm.hibernate.cfg` | Migrated | Main entry point for domain binding. Implements `AdditionalMappingContributor`, `TypeContributor`. | +| `HibernateMappingContext` | `org.grails.orm.hibernate.cfg` | Migrated | | +| `GrailsHibernatePersistentEntity` | `org.grails.orm.hibernate.cfg` | Migrated | | +| `GrailsHibernatePersistentProperty` | `org.grails.orm.hibernate.cfg` | Migrated | | +| `GrailsHibernateUtil` | `org.grails.orm.hibernate.cfg` | Migrated | | +| `MappingCacheHolder` | `org.grails.orm.hibernate.cfg` | Migrated | | +| `PersistentEntityNamingStrategy` | `org.grails.orm.hibernate.cfg` | Migrated | | +| `NamingStrategyWrapper` | `org.grails.orm.hibernate.cfg.domainbinding` | Migrated | | + +#### Binders (`org.grails.orm.hibernate.cfg.domainbinding`) + +| Class | Status | Notes | +| :--- | :--- | :--- | +| `ClassBinder` | Migrated | Binds `PersistentClass` basic info. | +| `EnumTypeBinder` | Migrated | | +| `PropertyFromValueCreator` | Migrated | | +| `ComponentPropertyBinder` | Migrated | | +| `GrailsPropertyBinder` | Migrated | Simplified and consolidated. | +| `CollectionBinder` | Migrated | | +| `CompositeIdBinder` | Migrated | | +| `IdentityBinder` | Migrated | | +| `VersionBinder` | Migrated | | +| `SimpleValueBinder` | Migrated | | +| `OneToOneBinder` | Migrated | | +| `ManyToOneBinder` | Migrated | | +| `ColumnBinder` | Migrated | | +| `ColumnConfigToColumnBinder` | Migrated | | +| `SimpleValueColumnBinder` | Migrated | | +| `NaturalIdentifierBinder` | Migrated | | +| `IndexBinder` | Migrated | | +| `ComponentBinder` | Migrated | | +| `SimpleIdBinder` | Migrated | | +| `SimpleValueBinder` | Migrated | | + +#### Collection Types (`org.grails.orm.hibernate.cfg.domainbinding.collectionType`) + +| Class | Status | Notes | +| :--- | :--- | :--- | +| `CollectionHolder` | Migrated | | +| `BagCollectionType` | Migrated | | +| `ListCollectionType` | Migrated | | +| `MapCollectionType` | Migrated | | +| `SetCollectionType` | Migrated | | +| `SortedSetCollectionType` | Migrated | | + +#### Second Pass Binders (`org.grails.orm.hibernate.cfg.domainbinding.secondpass`) + +| Class | Status | Notes | +| :--- | :--- | :--- | +| `CollectionSecondPassBinder` | Migrated | Unidirectional many-to-many support implemented. | +| `GrailsSecondPass` | Migrated | | +| `ListSecondPass` | Migrated | | +| `ListSecondPassBinder` | Migrated | | +| `MapSecondPass` | Migrated | | +| `MapSecondPassBinder` | Migrated | | +| `SetSecondPass` | Migrated | | + +#### Generators (`org.grails.orm.hibernate.cfg.domainbinding` and `generator` subpackage) + +| Class | Status | Notes | +| :--- | :--- | :--- | +| `GrailsIdentityGenerator` | Migrated | | +| `GrailsIncrementGenerator` | Migrated | Contains reflection hacks for Hibernate 7, to be removed in Hibernate 8. | +| `GrailsNativeGenerator` | Migrated | | +| `GrailsSequenceStyleGenerator` | Migrated | | +| `GrailsTableGenerator` | Migrated | | +| `GrailsSequenceGeneratorEnum` | Migrated | In `generator` subpackage. | +| `GrailsSequenceWrapper` | Migrated | In `generator` subpackage. | + +#### Fetchers and Utilities (`org.grails.orm.hibernate.cfg.domainbinding`) + +| Class | Status | Notes | +| :--- | :--- | :--- | +| `ColumnNameForPropertyAndPathFetcher` | Migrated | | +| `TableNameFetcher` | Migrated | | +| `DefaultColumnNameFetcher` | Migrated | | +| `SimpleValueColumnFetcher` | Migrated | | +| `CascadeBehaviorFetcher` | Migrated | | +| `NamespaceNameExtractor` | Migrated | | +| `ForeignKeyColumnCountCalculator` | Migrated | | +| `TableForManyCalculator` | Migrated | | +| `UniqueNameGenerator` | Migrated | | +| `BackticksRemover` | Migrated | | +| `BasicValueCreator` | Migrated | | + +## Utility Class Refactoring & Mock Compatibility + +**Objective:** Modernize utility classes in `domainbinding.util` to use Hibernate-specific GORM types while maintaining compatibility with Spock mocks. + +**Summary of Changes:** +- **Refactored Utility Classes:** Updated `CreateKeyForProps`, `TableForManyCalculator`, `DefaultColumnNameFetcher`, `ConfigureDerivedPropertiesConsumer`, and `NamingStrategyWrapper` to use `GrailsHibernatePersistentProperty` and `GrailsHibernatePersistentEntity` where possible. +- **Mock Compatibility Fixes:** Addressed `ClassCastException` in Spock specs by: + - Reverting public method signatures to use base interfaces (`PersistentProperty`, `PersistentEntity`) where required by mocks. + - Implementing internal safe casting using `instanceof` pattern matching. + - Updating test stubs to include `additionalInterfaces: [GrailsHibernatePersistentProperty]`. +- **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. +- **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 + +- `GrailsIncrementGenerator`: Reflection hacks for Hibernate 7 (scheduled for removal in Hibernate 8). Review Comment: Removed in a prior cleanup commit — no longer exists in the branch. ########## grails-data-hibernate7/ISSUES.md: ########## @@ -0,0 +1,146 @@ +<!-- Review Comment: Good point. Both `grails-data-hibernate7/ISSUES.md` and `grails-data-hibernate7/AGENTS.md` have been removed from the branch — there is nothing left to relocate. ########## plans/aggregate-style-violations.md: ########## @@ -0,0 +1,65 @@ +<!-- +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +--> +# Implementation Plan: Aggregate Style Violations Review Comment: Removed — `plans/aggregate-style-violations.md` no longer exists in the branch. -- 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]
