Copilot commented on code in PR #15550:
URL: https://github.com/apache/grails-core/pull/15550#discussion_r3031609377
##########
grails-datamapping-core/src/main/groovy/grails/gorm/DetachedCriteria.groovy:
##########
@@ -135,7 +135,7 @@ class DetachedCriteria<T> extends
AbstractDetachedCriteria<T> implements GormOpe
*/
List<T> list(Map args = Collections.emptyMap(),
@DelegatesTo(DetachedCriteria) Closure additionalCriteria = null) {
(List)withPopulatedQuery(args, additionalCriteria) { Query query ->
- if (args != null && !args.isEmpty()) {
+ if (args?.max) {
DynamicFinder.populateArgumentsForCriteria(targetClass, query,
args)
return new PagedResultList(query)
}
Review Comment:
`list()` calls `DynamicFinder.populateArgumentsForCriteria(...)` inside the
`args?.max` branch, but `withPopulatedQuery(...)` already populates arguments
unconditionally. This double-populates max/offset/sort and can lead to
duplicated `order(...)` clauses. Consider removing the extra
`populateArgumentsForCriteria` call in the `args?.max` branch and rely on
`withPopulatedQuery` to apply args once.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/CompositeIdBinder.java:
##########
@@ -46,36 +47,33 @@ public CompositeIdBinder(
this.grailsPropertyBinder = grailsPropertyBinder;
}
- public void bindCompositeId(
- @Nonnull HibernatePersistentEntity hibernatePersistentEntity,
- RootClass root,
- CompositeIdentity compositeIdentity) {
- hibernatePersistentEntity.setPersistentClass(root);
- Component id = new Component(metadataBuildingContext, root);
+ public void bindCompositeId(@Nonnull HibernatePersistentEntity
domainClass) {
+ if (domainClass.getIdentityProperty() instanceof
HibernateCompositeIdentityProperty compositeIdentityProperty) {
+ Component id = getComponent(domainClass);
+
+ for (HibernatePersistentProperty property :
compositeIdentityProperty.getParts()) {
+ var value = grailsPropertyBinder.bindProperty(property, null,
"");
+ componentUpdater.updateComponent(id, null, property, value);
+ }
+ return;
+ }
+ throw new MappingException("Invalid simple id binding for entity [" +
domainClass.getName() + "]");
Review Comment:
The thrown MappingException message says "Invalid simple id binding" in
`CompositeIdBinder`, but this binder is for *composite* identity binding
failures. Consider updating the message to reflect composite identity binding
to improve diagnosability.
```suggestion
throw new MappingException("Invalid composite id binding for entity
[" + domainClass.getName() + "]");
```
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/domainbinding/binder/SimpleIdBinder.java:
##########
@@ -28,85 +28,60 @@
import org.hibernate.mapping.RootClass;
import org.hibernate.mapping.Table;
-import org.grails.datastore.mapping.model.DefaultPropertyMapping;
-import org.grails.datastore.mapping.model.config.GormProperties;
-import org.grails.orm.hibernate.cfg.Identity;
-import org.grails.orm.hibernate.cfg.Mapping;
-import org.grails.orm.hibernate.cfg.PropertyConfig;
-import
org.grails.orm.hibernate.cfg.domainbinding.hibernate.HibernateIdentityProperty;
import
org.grails.orm.hibernate.cfg.domainbinding.hibernate.HibernatePersistentEntity;
-import org.grails.orm.hibernate.cfg.domainbinding.util.BasicValueIdCreator;
+import
org.grails.orm.hibernate.cfg.domainbinding.hibernate.HibernateSimpleIdentityProperty;
+import org.grails.orm.hibernate.cfg.domainbinding.util.BasicValueCreator;
import static
org.grails.orm.hibernate.cfg.domainbinding.binder.GrailsDomainBinder.EMPTY_PATH;
+/** The simple id binder class. */
@SuppressWarnings("PMD.DataflowAnomalyAnalysis")
public class SimpleIdBinder {
private final MetadataBuildingContext metadataBuildingContext;
+ private final BasicValueCreator basicValueCreator;
private final SimpleValueBinder simpleValueBinder;
private final PropertyBinder propertyBinder;
- private final BasicValueIdCreator basicValueIdCreator;
public SimpleIdBinder(
MetadataBuildingContext metadataBuildingContext,
- BasicValueIdCreator basicValueIdCreator,
+ BasicValueCreator basicValueCreator,
SimpleValueBinder simpleValueBinder,
PropertyBinder propertyBinder) {
this.metadataBuildingContext = metadataBuildingContext;
+ this.basicValueCreator = basicValueCreator;
this.simpleValueBinder = simpleValueBinder;
this.propertyBinder = propertyBinder;
- this.basicValueIdCreator = basicValueIdCreator;
}
public MetadataBuildingContext getMetadataBuildingContext() {
return metadataBuildingContext;
}
- public void bindSimpleId(
- @Nonnull HibernatePersistentEntity domainClass, RootClass entity,
Identity mappedId, Table table) {
+ public void bindSimpleId(@Nonnull HibernatePersistentEntity domainClass) {
+ if (domainClass.getIdentityProperty() instanceof
HibernateSimpleIdentityProperty simpleIdentityProperty) {
+ RootClass rootClass = domainClass.getRootClass();
+ BasicValue id =
basicValueCreator.bindBasicValue(simpleIdentityProperty);
- Mapping result = domainClass.getHibernateMappedForm();
- boolean useSequence = result != null &&
result.isTablePerConcreteClass();
- // create the id value
+ var identifier =
basicValueCreator.resolveIdentifierProperty(domainClass,
simpleIdentityProperty);
- BasicValue id =
- basicValueIdCreator.getBasicValueId(metadataBuildingContext,
table, mappedId, domainClass, useSequence);
+ Property idProperty = new Property();
+ idProperty.setName(identifier.getName());
+ idProperty.setValue(id);
+ rootClass.setDeclaredIdentifierProperty(idProperty);
+ rootClass.setIdentifier(id);
+ // set type
+ simpleValueBinder.bindSimpleValue(identifier, null, id,
EMPTY_PATH);
- var identifier = domainClass.getIdentity();
- if (identifier == null) {
- var syntheticId = new HibernateIdentityProperty(
- domainClass, domainClass.getMappingContext(),
GormProperties.IDENTITY, Long.class);
- syntheticId.setMapping(new
DefaultPropertyMapping<>(domainClass.getMapping(), new PropertyConfig()));
- identifier = syntheticId;
- }
- if (mappedId != null) {
- String propertyName = mappedId.getName();
- if (propertyName != null &&
!propertyName.equals(domainClass.getName())) {
- var namedIdentityProp =
domainClass.getHibernatePropertyByName(propertyName);
- if (namedIdentityProp == null) {
- throw new MappingException(
- "Mapping specifies an identifier property name
that doesn't exist [" + propertyName + "]");
- }
- if (!namedIdentityProp.equals(identifier)) {
- identifier = namedIdentityProp;
- }
- }
- }
-
- Property idProperty = new Property();
- idProperty.setName(identifier.getName());
- idProperty.setValue(id);
- entity.setDeclaredIdentifierProperty(idProperty);
- entity.setIdentifier(id);
- // set type
- simpleValueBinder.bindSimpleValue(identifier, null, id, EMPTY_PATH);
+ // bind property
+ Property prop = propertyBinder.bindProperty(identifier, id);
+ // set identifier property
+ rootClass.setIdentifierProperty(prop);
- // bind property
- Property prop = propertyBinder.bindProperty(identifier, id);
- // set identifier property
- entity.setIdentifierProperty(prop);
-
- Table pkTable = id.getTable();
- pkTable.setPrimaryKey(new PrimaryKey(pkTable));
+ Table pkTable = id.getTable();
+ pkTable.setPrimaryKey(new PrimaryKey(pkTable));
+ return;
+ }
+ throw new MappingException("Invalid composite id binding for entity ["
+ domainClass.getName() + "]");
Review Comment:
The thrown MappingException message says "Invalid composite id binding" in
`SimpleIdBinder`, but this code path is for *simple* identity binding failures.
Updating the message will make debugging mapping errors clearer and avoid
confusion during triage.
```suggestion
throw new MappingException("Invalid simple id binding for entity ["
+ domainClass.getName() + "]");
```
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/FindByMethodSpec.groovy:
##########
@@ -157,16 +153,14 @@ class FindByMethodSpec extends GrailsDataTckSpec {
when:
highways = Highway.findAllBypassed()
then:
- 2 == highways?.size()
+ 1 == highways?.size()
'Bypassed Highway' == highways[0].name
- 'Bypassed Highway' == highways[1].name
when:
highways = Highway.findAllNotBypassed()
then:
- 2 == highways?.size()
+ 1 == highways?.size()
'Not Bypassed Highway' == highways[0].name
Review Comment:
This test previously created multiple matching `Highway` rows and asserted
`findAll*` dynamic finders returned all matches. By reducing the setup to a
single matching row and changing the expected sizes to `1`, the test no longer
validates multi-row behavior (a key part of `findAll*`). Consider restoring at
least 2 matching rows and asserting both are returned (order-independent if
needed).
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/FindByMethodSpec.groovy:
##########
@@ -220,7 +214,6 @@ class FindByMethodSpec extends GrailsDataTckSpec {
book = TckBook.findPublishedByTitleOrAuthor('Fly Fishing For
Everyone', 'Dierk')
then:
'GINA' == book.title
- TckBook.findPublished() != null
Review Comment:
Removing the `TckBook.findPublished() != null` assertion drops coverage for
the single-result `findPublished()` dynamic finder (the rest of the test only
covers `findAllPublished*`). If the call was flaky, consider asserting
something deterministic about `findPublished()` (for example, that it returns a
published book) rather than removing it entirely.
--
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]
