-
-
Notifications
You must be signed in to change notification settings - Fork 973
Fix DDL failure for hasMany collections of enums #16052
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 8.0.x
Are you sure you want to change the base?
Changes from all commits
98a1acf
5e05828
607ae4e
c7f4f4e
70f8751
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| /* | ||
| * 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() { | ||
|
jdaugherty marked this conversation as resolved.
|
||
| return true; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isCollectionElement() { | ||
| return true; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ | |
| import java.beans.PropertyDescriptor; | ||
|
|
||
| import org.hibernate.mapping.Collection; | ||
| import org.hibernate.mapping.Table; | ||
|
|
||
| import org.grails.datastore.mapping.model.MappingContext; | ||
| import org.grails.datastore.mapping.model.types.mapping.BasicWithMapping; | ||
|
|
@@ -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(); | ||
| } | ||
|
Comment on lines
+49
to
+60
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two things about placing this override on Only the enum path needs it. It changes what 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
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 Either scope the override to the enum subclass, or have
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(); | ||
| } | ||
|
|
||
|
Comment on lines
+57
to
+64
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The debug log that used to accompany the table-per-hierarchy case in LOG.debug("[GrailsDomainBinder] Sub class property [{}] for column name [{}] forced to nullable", ...)That message is the only signal a user gets that their |
||
| /** | ||
| * Whether this property is a {@code hasMany} basic-collection element rather than a scalar | ||
| * enum-typed property. {@link org.grails.orm.hibernate.cfg.domainbinding.binder.GrailsPropertyBinder} | ||
| * uses this to decide whether to bind it directly here, or let it fall through to the normal | ||
| * to-many collection path (whose element is bound later, from within the collection binder). | ||
| */ | ||
| default boolean isCollectionElement() { | ||
| return false; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Comment on lines
+170
to
+173
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The three features are:
The latter two build entities whose collections are enums, and all three assert The latter two should assert |
||
| basic.setMapping(createPropertyMapping(basic, entity)) | ||
| CustomTypeMarshaller customTypeMarshaller = findCustomType(context, property.propertyType) | ||
| if (collectionType != null && collectionType.isEnum()) { | ||
| if (isEnumCollection) { | ||
| customTypeMarshaller = findCustomType(context, collectionType) | ||
| if (customTypeMarshaller == null) { | ||
| customTypeMarshaller = findCustomType(context, Enum) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ | |
|
|
||
| import org.grails.datastore.mapping.model.types.Association; | ||
| import org.grails.datastore.mapping.model.types.Basic; | ||
| import org.grails.datastore.mapping.model.types.EmbeddedCollection; | ||
| import org.grails.datastore.mapping.model.types.mapping.PropertyWithMapping; | ||
| import org.grails.orm.hibernate.cfg.CacheConfig; | ||
| import org.grails.orm.hibernate.cfg.ColumnConfig; | ||
|
|
@@ -39,11 +40,15 @@ | |
| import org.grails.orm.hibernate.cfg.PropertyConfig; | ||
| import org.grails.orm.hibernate.cfg.domainbinding.binder.GrailsDomainBinder; | ||
| import org.grails.orm.hibernate.cfg.domainbinding.util.BackticksRemover; | ||
| import org.grails.orm.hibernate.cfg.domainbinding.util.CascadeBehavior; | ||
|
|
||
| import static java.util.Optional.ofNullable; | ||
| import static org.grails.orm.hibernate.cfg.GrailsHibernateUtil.qualify; | ||
| import static org.grails.orm.hibernate.cfg.domainbinding.binder.GrailsDomainBinder.UNDERSCORE; | ||
| import static org.grails.orm.hibernate.cfg.domainbinding.util.CascadeBehavior.ALL; | ||
| import static org.grails.orm.hibernate.cfg.domainbinding.util.CascadeBehavior.ALL_DELETE_ORPHAN; | ||
| import static org.grails.orm.hibernate.cfg.domainbinding.util.CascadeBehavior.NONE; | ||
| import static org.grails.orm.hibernate.cfg.domainbinding.util.CascadeBehavior.SAVE_UPDATE; | ||
|
|
||
| /** Marker interface for Hibernate to-many associations */ | ||
| public interface HibernateToManyProperty extends PropertyWithMapping<PropertyConfig>, HibernateAssociation { | ||
|
|
@@ -92,6 +97,38 @@ default boolean isOneToMany() { | |
| return this instanceof HibernateOneToManyProperty; | ||
| } | ||
|
|
||
| /** | ||
| * The cascade behavior implied by this to-many property's shape, absent an explicit {@code | ||
| * cascade} mapping. Self-contained: every fact this needs (basic-ness, Map-typedness, embedded | ||
| * collection-ness, ownership, circularity) is already exposed by this interface or inherited | ||
| * from the GORM {@code Association} hierarchy, so no external dispatch is required. | ||
| */ | ||
| default CascadeBehavior getImpliedCascadeBehavior() { | ||
| if (!(this instanceof Association<?> association)) { | ||
| throw new MappingException("Unrecognized to-many association type " + getType()); | ||
| } | ||
| if (isBasic()) { | ||
| return ALL; | ||
| } | ||
| if (Map.class.isAssignableFrom(getType())) { | ||
| return association.isCorrectlyOwned() ? ALL : SAVE_UPDATE; | ||
| } | ||
| if (this instanceof EmbeddedCollection) { | ||
| return ALL; | ||
| } | ||
| // Fail-fast only for entity relationships that are truly missing an association | ||
| if (getAssociatedEntity() == null) { | ||
| throw new MappingException("Relationship " + this + " has no associated entity"); | ||
| } | ||
| if (isOneToMany()) { | ||
| return association.isCorrectlyOwned() ? ALL : SAVE_UPDATE; | ||
| } | ||
| if (isManyToMany()) { | ||
| return association.isCorrectlyOwned() || isCircular() ? SAVE_UPDATE : NONE; | ||
| } | ||
| throw new MappingException("Unrecognized to-many association type " + getType()); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the component type for this to-many collection, or {@code null} if it cannot be | ||
| * determined. | ||
|
|
@@ -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()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 (
Comment on lines
+266
to
+269
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Repeating this from the last round because it is unchanged: 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 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, |
||
| } else { | ||
| var clazz = namingStrategy.resolveColumnName(referencedType.getName()); | ||
| var prop = namingStrategy.resolveTableName(getName()); | ||
| columnName = referencedType.isEnum() ? | ||
| clazz : | ||
| new BackticksRemover().apply(prop) + UNDERSCORE + new BackticksRemover().apply(clazz); | ||
| columnName = new BackticksRemover().apply(prop) + UNDERSCORE + new BackticksRemover().apply(clazz); | ||
| } | ||
| return columnName; | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.