jdaugherty commented on code in PR #16720:
URL: https://github.com/apache/grails-core/pull/16720#discussion_r4407956721
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/util/CreateKeyForProps.java:
##########
@@ -59,23 +66,38 @@ public void createKeyForProps(HibernatePersistentProperty
grailsProp, String pat
PropertyConfig mappedForm = grailsProp.getMappedForm();
if (mappedForm.isUnique() && mappedForm.isUniqueWithinGroup()) {
+ if (metadataCollector.isInSecondPass()) {
+ createKeyForGroup(grailsProp, path, table, columnName,
mappedForm);
+ } else {
+ metadataCollector.addSecondPass(persistentClasses ->
Review Comment:
On 8.0.x, `createKeyForColumns` (the key of a composite user type property,
from #16695) resolves its group through `addGroupColumns` inline at bind time,
so `amount unique: 'parent'` with a composite-id `parent` declared after
`amount` and without a `column` mapping takes the same `parent_id` fallback
this pass removes. Please route both entry points through the deferral: a
private method that creates the key at once when `isInSecondPass()` and
otherwise adds the pass, called from `createKeyForProps` and
`createKeyForColumns`.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/util/CreateKeyForProps.java:
##########
@@ -59,23 +66,38 @@ public void createKeyForProps(HibernatePersistentProperty
grailsProp, String pat
PropertyConfig mappedForm = grailsProp.getMappedForm();
if (mappedForm.isUnique() && mappedForm.isUniqueWithinGroup()) {
+ if (metadataCollector.isInSecondPass()) {
+ createKeyForGroup(grailsProp, path, table, columnName,
mappedForm);
+ } else {
+ metadataCollector.addSecondPass(persistentClasses ->
+ createKeyForGroup(grailsProp, path, table, columnName,
mappedForm));
+ }
+ }
+ }
- List<Column> keyList = new ArrayList<>();
- keyList.add(new Column(columnName));
- List<String> propertyNames = mappedForm.getUniquenessGroup();
- GrailsHibernatePersistentEntity owner =
grailsProp.getHibernateOwner();
- for (String propertyName : propertyNames) {
- HibernatePersistentProperty otherProp =
owner.getHibernatePropertyByName(propertyName);
- if (otherProp == null) {
- throw new MappingException(
- owner.getJavaClass().getName() + " references an
unknown property " + propertyName);
- }
+ private void createKeyForGroup(
+ HibernatePersistentProperty grailsProp, String path, Table table,
String columnName, PropertyConfig mappedForm) {
+ List<Column> keyList = new ArrayList<>();
+ keyList.add(new Column(columnName));
+ List<String> propertyNames = mappedForm.getUniquenessGroup();
+ GrailsHibernatePersistentEntity owner = grailsProp.getHibernateOwner();
+ for (String propertyName : propertyNames) {
+ HibernatePersistentProperty otherProp =
owner.getHibernatePropertyByName(propertyName);
+ if (otherProp == null) {
+ throw new MappingException(
+ owner.getJavaClass().getName() + " references an
unknown property " + propertyName);
+ }
+ List<ColumnConfig> columns =
otherProp.getHibernateMappedForm().getColumns();
+ if (columns.isEmpty()) {
+ columns = Collections.singletonList(null);
+ }
+ for (ColumnConfig column : columns) {
String otherColumnName =
-
columnNameForPropertyAndPathFetcher.getColumnNameForPropertyAndPath(otherProp,
path, null);
+
columnNameForPropertyAndPathFetcher.getColumnNameForPropertyAndPath(otherProp,
path, column);
Review Comment:
After the merge with 8.0.x this loop and `addGroupColumns` resolve the same
members two ways. `addGroupColumns` calls `getColumnNamesForPropertyAndPath`,
which expands a composite user type to one name per attribute and otherwise
returns `getColumnNameForPropertyAndPath(otherProp, path, null)`. That single
name goes through `PropertyConfig.getColumn()`, which throws `Cannot treat
multi-column property as a single-column property` for a member with two
configured columns, the failure #16713 reports, and for a member whose columns
are generated later it is the `parent_id` default. Keeping this loop instead
and bypassing `addGroupColumns` loses the composite user type expansion for a
group member mapped with one (`reference unique: 'amount'` in #16695).
Please resolve it with one resolver: move the configured-column expansion
into `getColumnNamesForPropertyAndPath` (a composite user type gives its
attributes, configured columns give one name each, otherwise the single
default) and have `createKeyForGroup` call `addGroupColumns`. The two new unit
cases here then belong in `ColumnNameForPropertyAndPathFetcherSpec`, next to
the composite ones, with `CreateKeyForPropsSpec` stubbing
`getColumnNamesForPropertyAndPath` as it does on 8.0.x.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/ColumnBinder.java:
##########
@@ -65,14 +66,16 @@ public ColumnBinder(
this.indexBinder = indexBinder;
}
- public ColumnBinder(PersistentEntityNamingStrategy namingStrategy, Dialect
dialect) {
+ public ColumnBinder(
+ PersistentEntityNamingStrategy namingStrategy, Dialect dialect,
MetadataBuildingContext metadataBuildingContext) {
Review Comment:
8.0.x already has `ColumnBinder(MetadataBuildingContext,
PersistentEntityNamingStrategy, Dialect)` (`@since 8.0.1`) beside the
two-argument one, so after the merge this is a second three-argument
constructor with the same types in another order. Please drop it, take the
collector from the existing one (a null context gives a null collector), and
leave the two-argument constructor as it is on 8.0.x.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/util/CreateKeyForProps.java:
##########
@@ -35,17 +38,21 @@ public class CreateKeyForProps {
private final ColumnNameForPropertyAndPathFetcher
columnNameForPropertyAndPathFetcher;
private final UniqueKeyForColumnsCreator uniqueKeyForColumnsCreator;
+ private final InFlightMetadataCollector metadataCollector;
- public CreateKeyForProps(ColumnNameForPropertyAndPathFetcher
columnNameForPropertyAndPathFetcher) {
- this.columnNameForPropertyAndPathFetcher =
columnNameForPropertyAndPathFetcher;
- this.uniqueKeyForColumnsCreator = new UniqueKeyForColumnsCreator();
+ public CreateKeyForProps(
Review Comment:
On 8.0.x the one-argument constructor has three callers after #16695:
`ColumnBinder(namingStrategy, dialect)`, which passes a null
`MetadataBuildingContext` on purpose, `CompositeUserTypeBinder`, and
`EnumTypeBinder`. Please keep
`CreateKeyForProps(ColumnNameForPropertyAndPathFetcher)`, with a null collector
meaning the key is created at once, as 8.0.0 did; 3a16c6d939 kept the 8.0.0
constructor of `CollectionSecondPassBinder` the same way.
`CompositeUserTypeBinder` should pass the collector, or the composite path
never defers.
--
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]
