jdaugherty commented on code in PR #16052:
URL: https://github.com/apache/grails-core/pull/16052#discussion_r3652156240
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateToManyProperty.java:
##########
@@ -226,12 +226,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:
`HibernateToManyPropertySpec` is the direct unit test for this method and it
was not touched. Its enum case currently asserts nothing useful:
```groovy
void "joinTableColumName returns derived column name for enum collection"() {
given:
def property = createTestHibernateToManyProperty(HTMPEntityWithEnum,
"statuses")
def namingStrategy = getGrailsDomainBinder().namingStrategy
expect:
property.joinTableColumName(namingStrategy) != null
}
```
That passed before this change and passes after it, so the behaviour you are
fixing here has no unit-level guard. Since the sibling case two features down
(`"joinTableColumName uses explicit join table column name when present"`)
already asserts an exact string, please pin this one the same way — `==
"htmp_status"` or whatever the strategy resolves for that enum's simple name.
That also documents the Grails 7 parity this restores.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/GrailsPropertyBinder.java:
##########
@@ -75,7 +75,11 @@ public Value bindProperty(
Value value;
- if (currentGrailsProp instanceof HibernateEnumProperty
hibernateEnumProperty) {
+ if (currentGrailsProp instanceof HibernateEnumProperty
hibernateEnumProperty &&
+ !(currentGrailsProp instanceof HibernateToManyProperty)) {
Review Comment:
This guard is correct but states the condition indirectly — it reads as
"enums that are not to-many" when what you mean is "not the new
basic-collection variant". It also silently depends on `HibernateBasicProperty
implements HibernateToManyCollectionProperty`, which is not obvious from here.
`!(currentGrailsProp instanceof HibernateBasicEnumProperty)` says the same
thing directly and is the condition that stays true if another
`HibernateEnumProperty` implementation is added later. Alternatively, put the
decision on the interface (e.g. a `bindsOwnValue()`/`isCollectionElement()`
default) so the binder does not have to enumerate implementations at all — that
is the pattern the rest of this PR follows for table, column name and
nullability.
##########
grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/EnumHasManyDdlSpec.groovy:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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)
+
+ @Issue("https://github.com/apache/grails-core/issues/16051";)
+ void "join table for a hasMany of enum is created with the element
column"() {
+ given:
+ SessionImplementor sessionImplementor = (SessionImplementor)
datastore.sessionFactory.currentSession
+
+ expect: "the join table exists and has an answers column, not just the
owner FK"
+ ResultSet columns = sessionImplementor.doReturningWork {
+ it.prepareStatement(
+ "select column_name from information_schema.columns " +
+ "where table_name = 'SURVEY_RESPONSE_ANSWERS'"
+ ).executeQuery()
+ }
+ Set<String> columnNames = []
+ while (columns.next()) {
+ columnNames << columns.getString('column_name').toLowerCase()
+ }
+ columnNames.contains('survey_response_id')
+ columnNames.any { it.contains('answer') }
Review Comment:
`it.contains('answer')` is too loose to cover the naming half of the fix.
Under the old FQN behaviour the column was `grails_gorm_tests_survey_answer`,
which also satisfies `contains('answer')` — so this feature would have gone
green against the buggy `joinTableColumName` as long as the *table* half of the
fix were in place. Please assert the exact name:
```groovy
columnNames == ['survey_response_id', 'survey_answer'] as Set
```
Two smaller points on the same block:
- The `PreparedStatement`/`ResultSet` are never closed, and the `ResultSet`
is drained after `doReturningWork` has returned. It happens to work because the
session's connection is still open, but the idiomatic form is to do the whole
read inside the callback and return the `Set<String>` — which also lets both
features share one helper instead of duplicating the loop.
- The refactor moved `STRING`/`ORDINAL`/`IDENTITY` handling into
`GrailsEnumType.configure` and nullability into `isEnumColumnNullable`, but
every assertion here exercises the default (STRING) path. A case with `answers
enumType: 'ordinal'` and one with an explicit `joinTable column: [name: '...']`
would cover the branches this PR actually rewrote.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateBasicEnumProperty.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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 org.grails.orm.hibernate.cfg.domainbinding.hibernate;
+
+import java.beans.PropertyDescriptor;
+
+import org.grails.datastore.mapping.model.MappingContext;
+import org.grails.orm.hibernate.cfg.PersistentEntityNamingStrategy;
+import
org.grails.orm.hibernate.cfg.domainbinding.util.ColumnNameForPropertyAndPathFetcher;
+
+/**
+ * Hibernate basic collection element property whose element type is an enum.
Created by {@link
+ * HibernateMappingFactory#createBasicCollection} when the collection's
element type is an enum.
+ */
+public class HibernateBasicEnumProperty extends HibernateBasicProperty
implements HibernateEnumProperty {
Review Comment:
Making this type a `HibernateEnumProperty` silently changes behaviour in
`PropertyBinder`, which is outside this diff:
```java
// PropertyBinder#bindProperty
if (persistentProperty instanceof Association<?> association &&
!(persistentProperty instanceof HibernateEnumProperty)) {
prop.setCascade(cascadeBehaviorFetcher.getCascadeBehaviour(association));
}
```
`HibernateBasicProperty` extends `BasicWithMapping` -> `Basic` -> `ToMany`
-> `Association`, so a `hasMany`-of-enum **is** an `Association`. Before this
PR the `!(... instanceof HibernateEnumProperty)` clause was unreachable (the
only two implementations extended
`HibernateSimpleProperty`/`HibernateCustomProperty`, neither of which is an
`Association`); after this PR it fires, and `prop.setCascade(...)` is no longer
called for enum collections.
Two concrete consequences:
1. `CascadeBehaviorFetcher.getImpliedBehavior()` has an explicit `if
(association instanceof Basic) return ALL;` branch, so `Set<String>` still gets
`cascade="all"` while `Set<SomeEnum>` now gets none — the two basic-collection
shapes have diverged for no stated reason.
2. An explicitly configured `answers cascade: 'all-delete-orphan'` in the
mapping block is read by `getDefinedBehavior()` and is now dropped on the floor
for enum collections only.
For a collection of value types the runtime blast radius is small (there is
no associated entity to cascade to), so this is unlikely to be what CI would
catch — but it is an unintended side effect, and the same class of side effect
applies to every `instanceof HibernateEnumProperty` site once a collection type
joins that hierarchy. Please either narrow the `PropertyBinder` guard to the
scalar enum types it was written for, or add an explicit test asserting the
cascade for `hasMany`-of-enum matches `hasMany`-of-String.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateEnumProperty.java:
##########
@@ -18,18 +18,46 @@
*/
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.
Review Comment:
This opening line is now wrong twice over: the interface is no longer a
marker (it carries three default methods), and `HibernateBasicEnumProperty`'s
Java type is a `Collection`, not an enum. Something like "Contract for
Hibernate persistent properties that bind an enum value — either the property's
own type or a basic collection's element type" would match what the interface
has become, and the list below it already spells out the three implementations.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/hibernate/HibernateBasicEnumProperty.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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 org.grails.orm.hibernate.cfg.domainbinding.hibernate;
+
+import java.beans.PropertyDescriptor;
+
+import org.grails.datastore.mapping.model.MappingContext;
+import org.grails.orm.hibernate.cfg.PersistentEntityNamingStrategy;
+import
org.grails.orm.hibernate.cfg.domainbinding.util.ColumnNameForPropertyAndPathFetcher;
+
+/**
+ * Hibernate basic collection element property whose element type is an enum.
Created by {@link
+ * HibernateMappingFactory#createBasicCollection} when the collection's
element type is an enum.
+ */
+public class HibernateBasicEnumProperty extends HibernateBasicProperty
implements HibernateEnumProperty {
+
+ public HibernateBasicEnumProperty(
+ GrailsHibernatePersistentEntity entity, MappingContext context,
PropertyDescriptor property) {
+ super(entity, context, property);
+ }
+
+ @Override
+ public Class<?> getEnumType() {
+ return getComponentType();
+ }
+
+ @Override
+ public String resolveEnumColumnName(
+ PersistentEntityNamingStrategy namingStrategy,
+ ColumnNameForPropertyAndPathFetcher
columnNameForPropertyAndPathFetcher,
+ String path) {
+ return joinTableColumName(namingStrategy);
+ }
+
+ /** A hasMany element column is always nullable, matching the non-enum
sibling binding path. */
+ @Override
+ public boolean isEnumColumnNullable() {
Review Comment:
This is a behaviour change that the PR description does not call out.
Previously `bindEnumTypeForColumn` reached the shared
`column.setNullable(property.isNullable())`, so a non-nullable enum collection
element got a `NOT NULL` column; now it is unconditionally nullable.
I think the new behaviour is the right one — it matches the `true` that
`BasicCollectionElementBinder` passes to
`simpleValueColumnBinder.bindSimpleValue(...)` on the non-enum branch, and it
matches how Hibernate treats `@ElementCollection` element columns — so this
reads as an intentional alignment rather than an accident. But it does change
generated DDL for anyone who declared `answers nullable: false`, so it deserves
a line in the PR body and a test pinning the resulting column's nullability.
--
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]
