borinquenkid commented on code in PR #15568:
URL: https://github.com/apache/grails-core/pull/15568#discussion_r3440352931
##########
grails-data-hibernate5/core/src/test/groovy/grails/gorm/tests/WhereQueryOldIssueVerificationSpec.groovy:
##########
@@ -360,7 +359,7 @@ class WqBiBook implements HibernateEntity<WqBiBook> {
String title
static hasMany = [authors: WqBiAuthor]
- static belongsTo = WqBiAuthor
+ static belongsTo = [WqBiAuthor]
Review Comment:
The bare-class form `static belongsTo = WqBiAuthor` is ambiguous in GORM —
it can be misinterpreted as a single-entry map literal. The list form `static
belongsTo = [WqBiAuthor]` is explicit: it declares set-based ownership without
a named back-reference property. This matches the intended semantics and
eliminates a Hibernate 7 mapping warning about ambiguous `belongsTo`
declarations.
##########
grails-data-hibernate5/core/src/test/groovy/grails/gorm/tests/WhereQueryOldIssueVerificationSpec.groovy:
##########
@@ -235,13 +236,11 @@ class WhereQueryOldIssueVerificationSpec extends
Specification {
@Issue('https://github.com/apache/grails-core/issues/14600')
def "findAllBy works with bidirectional hasMany relation"() {
given: "authors with books in a bidirectional hasMany"
- def author1 = new WqBiAuthor(name: "Stephen King").save(flush: true)
- def book1 = new WqBiBook(title: "IT").save(flush: true)
- def book2 = new WqBiBook(title: "The Shining").save(flush: true)
+ def author1 = new WqBiAuthor(name: "Stephen King")
+ def book1 = new WqBiBook(title: "IT")
+ def book2 = new WqBiBook(title: "The Shining")
author1.addToBooks(book1)
author1.addToBooks(book2)
- book1.addToAuthors(author1)
- book2.addToAuthors(author1)
author1.save(flush: true)
Review Comment:
Same change as the previous comment — `static belongsTo = [WqBiAuthor]`
(list form) is explicit about set-based ownership with no back-reference
property, replacing the ambiguous bare-class form that was triggering a
Hibernate 7 mapping warning.
##########
grails-datamapping-core/src/main/groovy/grails/gorm/DetachedCriteria.groovy:
##########
@@ -136,14 +136,18 @@ class DetachedCriteria<T> extends
AbstractDetachedCriteria<T> implements GormOpe
* @return A list of matching instances
*/
List<T> list(Map args = Collections.emptyMap(),
@DelegatesTo(DetachedCriteria) Closure additionalCriteria = null) {
- (List) withPopulatedQuery(args, additionalCriteria) { Query query ->
+ (List)withPopulatedQuery(args, additionalCriteria) { Query query ->
if (args?.max) {
- return new PagedResultList(query)
+ return newPagedResultList(query)
}
return query.list()
}
}
+ protected PagedResultList<T> newPagedResultList(Query query) {
+ new PagedResultList<T>(query)
+ }
+
Review Comment:
The extracted method was reverted — the current revision removes the complex
projection guard from `DetachedCriteria.count()` and returns it to the simpler
`8.0.x` form. The `PagedResultList<T>` generic type parameter was added as a
minor type-safety improvement. The projection logic was moved down to
`AbstractHibernateQuery.countResults()` in the H5/H7 adapter layer where it
belongs.
##########
grails-datamapping-core/src/main/groovy/grails/gorm/DetachedCriteria.groovy:
##########
@@ -514,24 +518,8 @@ class DetachedCriteria<T> extends
AbstractDetachedCriteria<T> implements GormOpe
* @return The count
*/
Number count(Map args = Collections.emptyMap(),
@DelegatesTo(DetachedCriteria) Closure additionalCriteria = null) {
- if (!projections.isEmpty()) {
- // When user-defined projections exist (e.g. groupProperty +
count),
- // a simple count() projection returns incorrect results because it
- // appends to the existing projections rather than replacing them.
- // Fall back to counting the grouped result rows.
- // This will be resolved properly in Grails 8 with Hibernate 7's
- // JpaSelectCriteria.from(Subquery) support for derived tables.
- log.warn('DetachedCriteria.count() with user-defined projections
cannot use a SQL count query ' +
Review Comment:
Partially, yes — the limitation still exists in H5, but the workaround was
moved to `AbstractHibernateQuery.countResults()` (the correct layer) rather
than left in core `DetachedCriteria`. H7 overrides `countResults()` with the
optimised derived-table subquery. H5's override retains the fallback (load all
grouped rows and count them in memory) but it's now confined to the H5 adapter
and no longer pollutes the core class.
--
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]
