jdaugherty commented on code in PR #16052:
URL: https://github.com/apache/grails-core/pull/16052#discussion_r3670148120
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateMappingFactory.groovy:
##########
@@ -167,10 +167,13 @@ class HibernateMappingFactory extends
AbstractGormMappingFactory<Mapping, Proper
PersistentEntity entity, MappingContext context,
PropertyDescriptor property, Class collectionType) {
if (entity instanceof GrailsHibernatePersistentEntity) {
GrailsHibernatePersistentEntity ghpEntity =
(GrailsHibernatePersistentEntity) entity
- HibernateBasicProperty basic = new
HibernateBasicProperty(ghpEntity, context, property)
+ boolean isEnumCollection = collectionType != null &&
collectionType.isEnum()
+ HibernateBasicProperty basic = isEnumCollection
+ ? new HibernateBasicEnumProperty(ghpEntity, context,
property)
+ : new HibernateBasicProperty(ghpEntity, context, property)
Review Comment:
`HibernateMappingFactorySpec` has three features covering exactly this
method, and none of them can tell whether this branch is present. Reducing it
back to `new HibernateBasicProperty(ghpEntity, context, property)` leaves the
spec fully green:
```
Results: SUCCESS (29 tests, 29 successes, 0 failures, 0 skipped)
```
The three features are:
- `"createBasicCollection produces HibernateBasicProperty for a basic
element collection"`
- `"createBasicCollection sets custom marshaller for enum hasMany"`
- `"createBasicCollection uses Enum base marshaller when no specific
marshaller for enum collection type"`
The latter two build entities whose collections *are* enums, and all three
assert `instanceof HibernateBasicProperty` — the supertype, which stays true
either way.
The latter two should assert `HibernateBasicEnumProperty`; the first should
assert the negative (`!(sectionsProp instanceof HibernateBasicEnumProperty)`)
so the split is pinned from both sides. `EnumHasManyDdlSpec` does catch the
regression at boot, so this is a weak-assertion problem rather than uncovered
behaviour — but it is a one-word fix in each case.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateBasicProperty.java:
##########
@@ -45,4 +46,16 @@ public Collection getHibernateCollection() {
public void setHibernateCollection(Collection collection) {
this.collection = collection;
}
+
+ /**
+ * For a basic (scalar or enum) collection element, the property's table
is the
+ * collection's join table rather than the owning entity's table. Before
the collection
+ * table has been assigned (e.g. while it is itself being computed), falls
back to the
+ * owning entity's table, matching the pre-collection-binding default.
+ */
+ @Override
+ public Table getTable() {
+ Table collectionTable = collection != null ?
collection.getCollectionTable() : null;
+ return collectionTable != null ? collectionTable :
getPersistentClass().getTable();
+ }
Review Comment:
Two things about placing this override on `HibernateBasicProperty` rather
than on `HibernateBasicEnumProperty`.
**Only the enum path needs it.** `BasicCollectionElementBinder`'s non-enum
branch reads `collection.getCollectionTable()` directly and never calls
`property.getTable()`, so scalar basic collections gain nothing here and only
inherit the risk. Scoping it to the enum subclass would match where
`resolveEnumColumnName` and `isEnumColumnNullable` were placed.
**It changes what `TableForManyCalculator.getJoinTableSchema()` reads.**
```java
String owningTableSchema = property.getTable().getSchema();
```
For a basic collection that expression no longer means what the variable is
named. It still returns the right value, but only because of an ordering
coincidence in `CollectionBinder.bindCollection()`:
1. `collectionHolder.create(property)` -> `CollectionType.create()` does
`coll.setCollectionTable(owner.getTable())`
2. `property.setCollection(collection, path)` -> the field here becomes
non-null, so `getTable()` starts returning the collection table
3. `bindCollectionTable()` calls `getJoinTableSchema()`, which now reads the
collection table — still the owner's table from step 1
4. `collection.setCollectionTable(<real join table>)`
The correct schema survives only because of the seed in step 1. The javadoc
says the fallback covers "before the collection table has been assigned", but
by the time `getJoinTableSchema()` runs the collection table *is* assigned — to
the owner's table. Anything that reorders steps 1 and 3 silently changes the
schema of every basic join table.
Either scope the override to the enum subclass, or have
`getJoinTableSchema()` ask the owner directly
(`property.getPersistentClass().getTable().getSchema()`) so it stops depending
on this ordering.
`EnumHasManyDdlSpec` covers the override end-to-end, so there is no coverage
hole — but a feature in `HibernateBasicPropertySpec` pinning both arms of the
ternary would be worth adding alongside whichever fix you pick.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateToManyProperty.java:
##########
@@ -226,12 +263,14 @@ default String
joinTableColumName(PersistentEntityNamingStrategy namingStrategy)
String columnName;
if (present) {
columnName = joinColumnMappingOptional.get().getName();
+ } else if (referencedType.isEnum()) {
+ // Use the enum's simple name, not its fully-qualified name, so
the column
+ // isn't named after the enum's package.
+ columnName =
namingStrategy.resolveColumnName(referencedType.getSimpleName());
Review Comment:
Repeating this from the last round because it is unchanged:
`HibernateToManyPropertySpec` is the direct unit test for this method and it
still asserts nothing about the value.
```groovy
void "joinTableColumName returns derived column name for enum collection"() {
...
expect:
property.joinTableColumName(namingStrategy) != null
}
```
I confirmed the gap is real. Reverting just this line to
`referencedType.getName()` and running both specs:
```
EnumHasManyDdlSpec > join table for a hasMany of enum is created with the
element column FAILED
EnumHasManyDdlSpec > a hasMany of enum with enumType ordinal ... FAILED
EnumHasManyDdlSpec > the hasMany enum element column stays nullable ...
FAILED
HibernateToManyPropertySpec > joinTableColumName returns derived column name
for enum collection PASSED
```
So the integration spec now guards the fix (good — that is an improvement
over last round), but the unit spec for the changed method still passes against
the bug. The sibling feature two down, `"joinTableColumName uses explicit join
table column name when present"`, already asserts `== "tag_val"`; please pin
this one the same way.
##########
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/GrailsPropertyBinderSpec.groovy:
##########
@@ -250,6 +256,37 @@ class GrailsPropertyBinderSpec extends
HibernateGormDatastoreSpec {
def dataProp = persistentEntity.getPropertyByName("data") as
HibernatePersistentProperty
Value value = propertyBinder.bindProperty(dataProp, null, EMPTY_PATH)
+ then: "the type: mapping DSL routes this through
isUserButNotCollectionType(), as a plain HibernateSimpleProperty"
+ !(dataProp instanceof HibernateCustomProperty)
+ dataProp.isUserButNotCollectionType()
+ value instanceof BasicValue
+ }
+
+ void "Test bind a genuine HibernateCustomProperty (GORM-detected custom
type marshaller, no type: mapping)"() {
+ given: "a HibernateCustomProperty built the way
HibernateMappingFactory#createCustom does: no type: " +
+ "mapping is set, so isUserButNotCollectionType() is false and
the instanceof HibernateCustomProperty " +
+ "branch is the only one that can match"
+ def binder = getGrailsDomainBinder()
+ def propertyBinder = getBinders(binder).propertyBinder
+ def persistentEntity =
getPersistentEntity(PropertyBinderSpecSimpleBook) as
GrailsHibernatePersistentEntity
+ def rootClass = new RootClass(binder.getMetadataBuildingContext())
+ rootClass.setTable(new Table("SIMPLE_BOOK"))
+ persistentEntity.setPersistentClass(rootClass)
+
+ def propertyDescriptor = new PropertyDescriptor("title",
PropertyBinderSpecSimpleBook)
+ def marshaller = Mock(CustomTypeMarshaller)
+ def customProp = new HibernateCustomProperty(persistentEntity,
getMappingContext(), propertyDescriptor, marshaller)
+ customProp.setMapping(new PropertyMapping<PropertyConfig>() {
+ ClassMapping getClassMapping() { null }
+ PropertyConfig getMappedForm() { new PropertyConfig() }
+ })
+
+ expect: "no type: mapping means the isUserButNotCollectionType()
branch cannot intercept it"
+ !customProp.isUserButNotCollectionType()
+
+ when:
+ Value value = propertyBinder.bindProperty(customProp, null, EMPTY_PATH)
+
Review Comment:
The new branch in `GrailsPropertyBinder` — `&&
!hibernateEnumProperty.isCollectionElement()` — has no test in this spec. The
only thing currently standing behind it is `EnumHasManyDdlSpec` booting
successfully, which will not tell you which side of the condition broke.
Two features in the same style as the ones already here would pin it at the
level it was written:
- a scalar enum property yields the `BasicValue` from `enumTypeBinder`
- a `hasMany`-of-enum property falls through to `collectionBinder` and
yields a `Collection`
The second is the one that regresses silently if `isCollectionElement()`
ever returns the wrong thing.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateEnumProperty.java:
##########
@@ -18,18 +18,57 @@
*/
package org.grails.orm.hibernate.cfg.domainbinding.hibernate;
+import org.grails.orm.hibernate.cfg.PersistentEntityNamingStrategy;
+import
org.grails.orm.hibernate.cfg.domainbinding.util.ColumnNameForPropertyAndPathFetcher;
+
/**
- * Marker interface for Hibernate persistent properties whose Java type is an
enum.
+ * Contract for Hibernate persistent properties that bind an enum value —
either the property's
+ * own type or a basic collection's element type.
*
- * <p>Two concrete subtypes exist, corresponding to the two creation paths in
{@link
+ * <p>Three concrete subtypes exist, corresponding to the three creation paths
in {@link
* HibernateMappingFactory}:
*
* <ul>
* <li>{@link HibernateSimpleEnumProperty} — plain enum with no custom type
marshaller
* <li>{@link HibernateCustomEnumProperty} — enum backed by a custom type
marshaller
+ * <li>{@link HibernateBasicEnumProperty} — enum element of a {@code
hasMany} basic collection
* </ul>
*
* <p>Use {@code instanceof HibernateEnumProperty} instead of {@code
isEnumType()} to branch on
- * enum properties at binding time.
+ * enum properties at binding time. Each implementation resolves its own enum
class and column
+ * name so {@link
org.grails.orm.hibernate.cfg.domainbinding.binder.EnumTypeBinder} can bind any
+ * of them through a single code path.
*/
-public interface HibernateEnumProperty extends HibernatePersistentProperty {}
+public interface HibernateEnumProperty extends HibernatePersistentProperty {
+
+ /** The enum class to bind: the property's own type, or a basic
collection's element type. */
+ default Class<?> getEnumType() {
+ return getType();
+ }
+
+ /** Resolves the column name to bind the enum value under. */
+ default String resolveEnumColumnName(
+ PersistentEntityNamingStrategy namingStrategy,
+ ColumnNameForPropertyAndPathFetcher
columnNameForPropertyAndPathFetcher,
+ String path) {
+ return
columnNameForPropertyAndPathFetcher.getColumnNameForPropertyAndPath(this, path,
null);
+ }
+
+ /**
+ * Whether the enum column should allow NULL. Subclass properties in a
table-per-hierarchy
+ * strategy must be nullable; otherwise this follows the property's own
nullable constraint.
+ */
+ default boolean isEnumColumnNullable() {
+ return getHibernateOwner().isTablePerHierarchySubclass() ||
isNullable();
+ }
+
Review Comment:
The debug log that used to accompany the table-per-hierarchy case in
`EnumTypeBinder` is gone:
```java
LOG.debug("[GrailsDomainBinder] Sub class property [{}] for column name [{}]
forced to nullable", ...)
```
That message is the only signal a user gets that their `nullable: false` on
a `table-per-hierarchy` subclass was deliberately overridden.
`isEnumColumnNullable()` returns a boolean so it has nowhere natural to log;
keeping the message at the `EnumTypeBinder` call site would preserve it.
##########
grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/EnumHasManyDdlSpec.groovy:
##########
@@ -0,0 +1,207 @@
+/*
+ * 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.
+ */
+package grails.gorm.tests
+
+import grails.gorm.annotation.Entity
+import grails.gorm.transactions.Rollback
+import org.grails.orm.hibernate.HibernateDatastore
+import org.hibernate.engine.spi.SessionImplementor
+import spock.lang.AutoCleanup
+import spock.lang.Issue
+import spock.lang.Shared
+import spock.lang.Specification
+
+import java.sql.ResultSet
+
+/**
+ * Reproduces https://github.com/apache/grails-core/issues/16051
+ *
+ * A domain with a `hasMany` collection whose related type is an enum (a
+ * Set of a basic/enum type, not an entity) produces broken join table DDL:
+ * the element column is bound against the owning entity's table instead of
+ * the join table, and its name is derived from the enum's fully-qualified
+ * class name instead of its simple name.
+ */
+@Rollback
+class EnumHasManyDdlSpec extends Specification {
+
+ @Shared @AutoCleanup HibernateDatastore datastore =
+ new HibernateDatastore(SurveyResponse, OrdinalSurveyResponse,
NamedColumnSurveyResponse, NonNullableSurveyResponse)
+
+ @Issue("https://github.com/apache/grails-core/issues/16051";)
+ void "join table for a hasMany of enum is created with the element
column"() {
+ expect: "the join table has exactly the owner FK column and the
element column, named from the enum's simple name"
+ columnNamesFor('SURVEY_RESPONSE_ANSWERS') == ['survey_response_id',
'survey_answer'] as Set
+ }
+
+ @Issue("https://github.com/apache/grails-core/issues/16051";)
+ void "the owner table does not get a spurious column for the hasMany enum
element"() {
+ expect: "no answer-related column leaked onto survey_response itself"
+ !columnNamesFor('SURVEY_RESPONSE').any { it.contains('answer') }
+ }
+
+ @Issue("https://github.com/apache/grails-core/issues/16051";)
+ void "a hasMany of enum can actually be saved and reloaded"() {
+ given:
+ def response = new SurveyResponse(respondent: "Alice")
+ response.addToAnswers(SurveyAnswer.MAYBE)
+ response.addToAnswers(SurveyAnswer.DONT_KNOW)
+
+ when:
+ response.save(flush: true)
+ response.discard()
+ def reloaded = SurveyResponse.get(response.id)
+
+ then:
+ reloaded.answers.sort() == [SurveyAnswer.MAYBE,
SurveyAnswer.DONT_KNOW].sort()
+ }
+
+ @Issue("https://github.com/apache/grails-core/issues/16051";)
+ void "a hasMany of enum with enumType ordinal stores the ordinal, not the
name"() {
+ expect: "the element column is a numeric ordinal column, not a string
one"
+ columnNamesFor('ORDINAL_SURVEY_RESPONSE_ANSWERS') ==
['ordinal_survey_response_id', 'survey_answer'] as Set
+
+ when:
+ def response = new OrdinalSurveyResponse(respondent: "Bob")
+ response.addToAnswers(SurveyAnswer.FOR_SURE)
+ response.save(flush: true)
+ response.discard()
+ def reloaded = OrdinalSurveyResponse.get(response.id)
+
+ then:
+ reloaded.answers == [SurveyAnswer.FOR_SURE] as Set
Review Comment:
This feature does not test what its name claims. I ran it against the PR
with the only change being `enumType: 'ordinal'` -> `enumType: 'string'` on
`OrdinalSurveyResponse`, and it still passes:
```
EnumHasManyDdlSpec > a hasMany of enum with enumType ordinal stores the
ordinal, not the name PASSED
Results: SUCCESS (6 tests, 6 successes, 0 failures, 0 skipped)
```
That is expected from the two assertions:
- `columnNamesFor(...)` compares column *names*, which are identical for
`STRING` and `ORDINAL`. The `expect:` label even says "the element column is a
numeric ordinal column, not a string one", but nothing in the expression looks
at the type.
- The round trip reads through the same mapping it wrote through, so it
succeeds under either style.
This is the same shape as the `contains('answer')` issue from the last round
— the feature would go green against a build where
`GrailsEnumType.ORDINAL.configure()` was never reached, which is exactly the
branch this PR rewrote.
`IdentityEnumTypeSpec` in this same package is the established pattern for
pinning storage representation — it selects the raw column and asserts the
stored value:
```groovy
ResultSet resultSet = ds.getConnection().prepareStatement('select status
from enum_entity_domain').executeQuery()
```
Something equivalent here would actually discriminate, e.g. asserting the
raw `survey_answer` value is `0` for `FOR_SURE`, or reading `data_type` from
`information_schema.columns` and asserting it is numeric rather than character.
As written the `ORDINAL` and `IDENTITY` arms of the new
`GrailsEnumType.configure()` have no coverage on the collection path at all.
Minor, same block: `expect:` followed by `when:`/`then:` puts two unrelated
assertions in one feature. Splitting the DDL check from the round trip would
let each fail with its own message.
--
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]
