Copilot commented on code in PR #15567: URL: https://github.com/apache/grails-core/pull/15567#discussion_r3061524494
########## grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateDetachedCriteria.groovy: ########## @@ -0,0 +1,54 @@ +/* + * 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 + +import groovy.transform.CompileDynamic + +import grails.gorm.DetachedCriteria +import org.grails.datastore.mapping.model.PersistentProperty +import org.grails.orm.hibernate.query.PropertyReference + +/** + * Hibernate-specific subclass of {@link DetachedCriteria} that overrides + * {@code propertyMissing} to return a {@link PropertyReference} for numeric + * persistent properties. This enables cross-property arithmetic in where-DSL + * expressions such as {@code pageCount > price * 10} without touching shared + * modules (and therefore without affecting H5 or MongoDB backends). + */ +@CompileDynamic +class HibernateDetachedCriteria<T> extends DetachedCriteria<T> { + + HibernateDetachedCriteria(Class<T> targetClass, String alias = null) { + super(targetClass, alias) + } + + @Override + protected HibernateDetachedCriteria<T> newInstance() { + new HibernateDetachedCriteria<T>(targetClass, alias) + } + + @Override + def propertyMissing(String name) { + PersistentProperty prop = getPersistentEntity()?.getPropertyByName(name) + if (prop != null && Number.isAssignableFrom(prop.type)) { Review Comment: propertyMissing only treats a property as numeric when `Number.isAssignableFrom(prop.type)` is true. This excludes primitive numeric types (int/long/double/etc.), so arithmetic expressions in where-DSL (e.g., `pageCount > price * 10`) will still break for domains that declare numeric properties using primitives. Consider normalizing primitive types to their boxed equivalents (or explicitly whitelisting primitive numeric classes) before the Number check. ```suggestion private static boolean isNumericPropertyType(Class<?> type) { if (type == null) { return false } if (type.isPrimitive()) { if (type == Byte.TYPE) { type = Byte } else if (type == Short.TYPE) { type = Short } else if (type == Integer.TYPE) { type = Integer } else if (type == Long.TYPE) { type = Long } else if (type == Float.TYPE) { type = Float } else if (type == Double.TYPE) { type = Double } else { return false } } Number.isAssignableFrom(type) } @Override def propertyMissing(String name) { PersistentProperty prop = getPersistentEntity()?.getPropertyByName(name) if (prop != null && isNumericPropertyType(prop.type)) { ``` ########## grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/ToManyEntityMultiTenantFilterBinder.java: ########## @@ -0,0 +1,64 @@ +/* + * 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.secondpass; + +import java.util.Collections; + +import org.hibernate.mapping.Collection; + +import org.grails.datastore.mapping.model.config.GormProperties; +import org.grails.orm.hibernate.cfg.domainbinding.hibernate.HibernateToManyEntityProperty; +import org.grails.orm.hibernate.cfg.domainbinding.util.DefaultColumnNameFetcher; + +/** Applies multi-tenant filters to a collection based on the associated entity's tenancy. */ +public class ToManyEntityMultiTenantFilterBinder { + + private final DefaultColumnNameFetcher defaultColumnNameFetcher; + + /** Creates a new {@link ToManyEntityMultiTenantFilterBinder} instance. */ + public ToManyEntityMultiTenantFilterBinder(DefaultColumnNameFetcher defaultColumnNameFetcher) { + this.defaultColumnNameFetcher = defaultColumnNameFetcher; + } + + /** Applies the multi-tenant filter to the collection if the associated entity is multi-tenant. */ + public void bind(HibernateToManyEntityProperty entityProperty) { + var referenced = entityProperty.getHibernateAssociatedEntity(); + if (entityProperty.isOneToMany() && referenced.isMultiTenant()) { + String filterCondition = referenced.getMultiTenantFilterCondition(defaultColumnNameFetcher); Review Comment: ToManyEntityMultiTenantFilterBinder.bind dereferences `referenced.isMultiTenant()` without checking whether `entityProperty.getHibernateAssociatedEntity()` returned null. Other parts of the binding pipeline explicitly handle null associated entities (e.g., getComponentType()/TableForManyCalculator), so this can produce an avoidable NullPointerException during metadata binding for partially-resolved associations. Add a null guard (return early) before calling isMultiTenant()/getMultiTenantFilterCondition(). ########## grails-data-hibernate7/README.md: ########## @@ -42,11 +42,69 @@ With the removal of Criterion API in Hibernate 7, we wanted to continue to suppo * Only the grails-datastore-gorm-hibernate7 module is being developed at the time. For testing the following was done: -* Used testcontainers for specific tests instead of h2 to verify features not supported by h2. +* Used testcontainers for specific tests instead of h2 to verify features not supported by h2. * A more opinionated and fluent HibernateGormDatastoreSpec is used for the specifications. +## Module Structure +| Module | Description | +|---|---| +| `grails-data-hibernate7-core` | Domain binding pipeline, GORM/Hibernate mapping, `HibernateDatastore` | Review Comment: The "Module Structure" table doesn’t mention the newly added `grails-data-hibernate7-spring-orm` subproject (now included in settings.gradle and consumed by core/boot-plugin/grails-plugin). Consider adding it to this table (and/or clarifying that the table only lists a subset of modules) to keep the README aligned with the actual multi-module layout. ```suggestion | `grails-data-hibernate7-core` | Domain binding pipeline, GORM/Hibernate mapping, `HibernateDatastore` | | `grails-data-hibernate7-spring-orm` | Shared Spring ORM / Hibernate integration support used by the core, boot-plugin, and Grails plugin modules | ``` -- 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]
