Skip to content

Commit 939e5ea

Browse files
borinquenkidCopilot
andcommitted
fix: address Copilot PR review comments
- HibernateDetachedCriteria.isNumericPropertyType: box primitive types before the Number.isAssignableFrom check so that domains declaring numeric properties as primitives (int/long/double/float/short/byte) work correctly in where-DSL arithmetic expressions. Method is protected to allow subclass overrides. - ToManyEntityMultiTenantFilterBinder.bind: add null guard for getHibernateAssociatedEntity() return value to prevent NullPointerException on partially-resolved associations. - grails-data-hibernate7/README.md: add grails-data-hibernate7-spring-orm to the Module Structure table. - Tests: new HibernateDetachedCriteriaSpec covering boxed and all 6 primitive numeric types, non-numeric delegation, and unknown property. Added null-associated-entity test to ToManyEntityMultiTenantFilterBinderSpec. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 34805d2 commit 939e5ea

File tree

6 files changed

+156
-3
lines changed

6 files changed

+156
-3
lines changed

grails-data-hibernate7/AGENTS.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -220,8 +220,10 @@ The `bindProperty` method in `GrailsPropertyBinder.java` has been successfully r
220220

221221
## Testing Guidelines
222222

223-
* **HibernateGormDatastoreSpec**: Use `HibernateGormDatastoreSpec` for all Hibernate 7 integration and domain binding specifications. It provides a more opinionated and fluent API for configuring the Hibernate environment and managing entities during tests.
224-
* **Unique Class Names**: When defining domain classes within a spec, ensure they are uniquely named to avoid package-level conflicts.
223+
* **HibernateGormDatastoreSpec**: Use `HibernateGormDatastoreSpec` for all Hibernate 7 integration and domain binding specifications. Unit tests in Hibernate 7 cannot effectively mock Hibernate internal classes; this spec provides the necessary environment.
224+
* **Entity Creation**: While `GrailsHibernatePersistentEntity` can be created manually using `createHibernatePersistentEntity`, it is more convenient to use `manager.addAllDomainClasses([...])` within `setupSpec()` to register entities for the test.
225+
* **Top-Level Entities**: Entity classes used in tests must be defined as top-level classes within the same Groovy file as the specification.
226+
* **Unique Class Names**: Ensure all domain classes defined within a spec have globally unique names within the package to avoid collisions with other test classes during parallel execution.
225227
* **Real Entities**: Prefer using real entities defined as top-level classes in the spec file over heavy mocking when testing binder logic.
226228

227229
## Resolved Issues

grails-data-hibernate7/README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ For testing the following was done:
5050
| Module | Description |
5151
|---|---|
5252
| `grails-data-hibernate7-core` | Domain binding pipeline, GORM/Hibernate mapping, `HibernateDatastore` |
53+
| `grails-data-hibernate7-spring-orm` | Shared Spring ORM / Hibernate integration support used by the core, boot-plugin, and Grails plugin modules |
5354
| `grails-data-hibernate7-boot-plugin` | Spring Boot autoconfiguration (`HibernateGormAutoConfiguration`) and Grails CLI SPI (`GormCompilerAutoConfiguration`) |
5455

5556
## Autoconfiguration

grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateDetachedCriteria.groovy

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,39 @@ class HibernateDetachedCriteria<T> extends DetachedCriteria<T> {
4646
@Override
4747
def propertyMissing(String name) {
4848
PersistentProperty prop = getPersistentEntity()?.getPropertyByName(name)
49-
if (prop != null && Number.isAssignableFrom(prop.type)) {
49+
if (prop != null && isNumericPropertyType(prop.type)) {
5050
return new PropertyReference(name)
5151
}
5252
super.propertyMissing(name)
5353
}
54+
55+
protected static boolean isNumericPropertyType(Class<?> type) {
56+
if (type == null) {
57+
return false
58+
}
59+
if (type.isPrimitive()) {
60+
if (type == Byte.TYPE) {
61+
type = Byte
62+
}
63+
else if (type == Short.TYPE) {
64+
type = Short
65+
}
66+
else if (type == Integer.TYPE) {
67+
type = Integer
68+
}
69+
else if (type == Long.TYPE) {
70+
type = Long
71+
}
72+
else if (type == Float.TYPE) {
73+
type = Float
74+
}
75+
else if (type == Double.TYPE) {
76+
type = Double
77+
}
78+
else {
79+
return false
80+
}
81+
}
82+
Number.isAssignableFrom(type)
83+
}
5484
}

grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/ToManyEntityMultiTenantFilterBinder.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,9 @@ public ToManyEntityMultiTenantFilterBinder(DefaultColumnNameFetcher defaultColum
3939
/** Applies the multi-tenant filter to the collection if the associated entity is multi-tenant. */
4040
public void bind(HibernateToManyEntityProperty entityProperty) {
4141
var referenced = entityProperty.getHibernateAssociatedEntity();
42+
if (referenced == null) {
43+
return;
44+
}
4245
if (entityProperty.isOneToMany() && referenced.isMultiTenant()) {
4346
String filterCondition = referenced.getMultiTenantFilterCondition(defaultColumnNameFetcher);
4447
if (filterCondition != null) {
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* https://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.grails.orm.hibernate
20+
21+
import grails.gorm.annotation.Entity
22+
import grails.gorm.specs.HibernateGormDatastoreSpec
23+
import org.grails.orm.hibernate.query.PropertyReference
24+
import spock.lang.Unroll
25+
26+
class HibernateDetachedCriteriaSpec extends HibernateGormDatastoreSpec {
27+
28+
void setupSpec() {
29+
manager.addAllDomainClasses([HDCProduct])
30+
}
31+
32+
@Unroll
33+
def "propertyMissing returns PropertyReference for boxed numeric property #propertyName"() {
34+
when:
35+
def criteria = new HibernateDetachedCriteria(HDCProduct)
36+
def result = criteria.propertyMissing(propertyName)
37+
38+
then:
39+
result instanceof PropertyReference
40+
result.propertyName == propertyName
41+
42+
where:
43+
propertyName << ['price', 'quantity', 'rating', 'score', 'stock', 'discount']
44+
}
45+
46+
@Unroll
47+
def "propertyMissing returns PropertyReference for primitive numeric property #propertyName"() {
48+
when:
49+
def criteria = new HibernateDetachedCriteria(HDCProduct)
50+
def result = criteria.propertyMissing(propertyName)
51+
52+
then:
53+
result instanceof PropertyReference
54+
result.propertyName == propertyName
55+
56+
where:
57+
propertyName << ['primitiveInt', 'primitiveLong', 'primitiveDouble', 'primitiveFloat', 'primitiveShort', 'primitiveByte']
58+
}
59+
60+
def "propertyMissing delegates to super for non-numeric property (returns property criterion)"() {
61+
when:
62+
def criteria = new HibernateDetachedCriteria(HDCProduct)
63+
def result = criteria.propertyMissing("name")
64+
65+
then:
66+
noExceptionThrown()
67+
!(result instanceof PropertyReference)
68+
}
69+
70+
def "propertyMissing delegates to super for unknown property (throws MissingPropertyException)"() {
71+
when:
72+
def criteria = new HibernateDetachedCriteria(HDCProduct)
73+
criteria.propertyMissing("nonExistent")
74+
75+
then:
76+
thrown(MissingPropertyException)
77+
}
78+
}
79+
80+
@Entity
81+
class HDCProduct {
82+
Long id
83+
84+
// Boxed numeric types
85+
BigDecimal price
86+
Integer quantity
87+
Double rating
88+
Float score
89+
Long stock
90+
Short discount
91+
92+
// Primitive numeric types (these were broken before the fix)
93+
int primitiveInt
94+
long primitiveLong
95+
double primitiveDouble
96+
float primitiveFloat
97+
short primitiveShort
98+
byte primitiveByte
99+
100+
// Non-numeric
101+
String name
102+
}

grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/cfg/domainbinding/secondpass/ToManyEntityMultiTenantFilterBinderSpec.groovy

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import org.grails.datastore.mapping.model.config.GormProperties
2525
import org.grails.datastore.mapping.multitenancy.MultiTenancySettings
2626
import org.grails.datastore.mapping.multitenancy.resolvers.SystemPropertyTenantResolver
2727
import org.grails.orm.hibernate.cfg.domainbinding.hibernate.GrailsHibernatePersistentEntity
28+
import org.grails.orm.hibernate.cfg.domainbinding.hibernate.HibernateToManyEntityProperty
2829
import org.grails.orm.hibernate.cfg.domainbinding.hibernate.HibernateToManyProperty
2930
import org.grails.orm.hibernate.cfg.domainbinding.util.BackticksRemover
3031
import org.grails.orm.hibernate.cfg.domainbinding.util.DefaultColumnNameFetcher
@@ -121,6 +122,20 @@ class ToManyEntityMultiTenantFilterBinderSpec extends HibernateGormDatastoreSpec
121122
collection.getFilters().isEmpty()
122123
collection.getManyToManyFilters().isEmpty()
123124
}
125+
126+
def "bind does nothing when associated entity is null (partially-resolved association)"() {
127+
given:
128+
def property = Stub(HibernateToManyEntityProperty) {
129+
getHibernateAssociatedEntity() >> null
130+
isOneToMany() >> true
131+
}
132+
133+
when:
134+
binder.bind(property)
135+
136+
then:
137+
noExceptionThrown()
138+
}
124139
}
125140
126141
@Entity

0 commit comments

Comments
 (0)