codeconsole commented on code in PR #16323:
URL: https://github.com/apache/grails-core/pull/16323#discussion_r3973686824
##########
grails-doc/src/en/guide/upgrading/upgrading80x.adoc:
##########
@@ -3097,3 +3097,92 @@ grails {
imported whether or not they are present — a star import of an absent package
contributes no classes and is
not an error in Groovy, so the probe changed nothing it could observe. An
application that relied on the
import being *omitted* when the package was absent sees no difference in
compiled output.
+
+==== 54. `count()` Returns `Long`
Review Comment:
Rebased onto current `8.0.x` — it's §58 now.
Separately: `8.0.x` already has `==== 54.` twice, *Request Processing
Behaviour Changes* (3256) and *Non-Public Bean Classes Are Marshalled, and
Reported Once* (3454). Left both alone since neither is mine, but you may want
to renumber the second.
##########
grails-doc/src/en/guide/upgrading/upgrading80x.adoc:
##########
@@ -3097,3 +3097,92 @@ grails {
imported whether or not they are present — a star import of an absent package
contributes no classes and is
not an error in Groovy, so the probe changed nothing it could observe. An
application that relied on the
import being *omitted* when the package was absent sees no difference in
compiled output.
+
+==== 54. `count()` Returns `Long`
+
+`count()` and the `count` property return `Long` instead of `Integer`:
+
+[source,groovy]
+----
+// Grails 7
+Integer total = Book.count()
+
+// Grails 8
+Long total = Book.count()
+----
+
+How wide the underlying value is depends on the datastore. Hibernate counts
with SQL `COUNT(*)` and Neo4j
+with Cypher `count(*)`, both 64-bit. MongoDB aggregates with `{$sum: 1}`,
which returns Int32 and promotes
+to Int64 once the total exceeds it, so the width depends on how many documents
the collection holds. Only
+the in-memory datastore is fixed at 32 bits, and it returns a list size. GORM
normalises whatever it gets
+with `longValue()`, then the declared return type narrowed it again with
`intValue()`. That second step truncates silently, so
+a table with more than `Integer.MAX_VALUE` rows reported a wrong and possibly
negative count with no
+error. `Long` is the only type that holds every value a datastore can produce
here, so it is what the API
+now declares.
+
+The change affects `GormStaticOperations.count()` and `getCount()`, the
`count()` and `count` members
+generated on every domain class, and `RestfulController.countResources()`.
Dynamic Groovy is unaffected:
+`Book.count() == 5`, `int n = Book.count()` and arithmetic on the result all
continue to work, because
+Groovy converts between numeric types on assignment and compares them by value.
+
+Code that is statically compiled, or that overrides one of these methods,
needs updating:
+
+[source,groovy]
+----
+@CompileStatic
+class ReportService {
+ // Grails 7
+ // Integer bookCount() { Book.count() }
+
+ // Grails 8
+ Long bookCount() { Book.count() }
+}
+
+class BookController extends RestfulController<Book> {
+ // Grails 7
+ // protected Integer countResources() { ... }
+
+ // Grails 8
+ protected Long countResources() { ... }
+}
+----
+
+Java callers assigning the result to an `Integer` or `int` need an explicit
conversion, since `Long` does
+not unbox to `int`:
+
+[source,java]
+----
+long total = BookGormEntity.count(); // preferred
Review Comment:
Fixed — it's `long total = Book.count();` now, which shows the auto-unboxing
point directly as you suggested.
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/GormStaticApiSpec.groovy:
##########
@@ -105,10 +106,20 @@ class GormStaticApiSpec extends Specification {
def api = new GormStaticApi(GormStaticApiThing, datastore, [])
when:
- Integer n = api.count()
+ Long n = api.count()
then:
- n == 0
+ n == 0L
+ }
+
+ void "count() returns a Long so a large table is not truncated"() {
Review Comment:
Taken as written, plus `GormStaticApiThing.count()` / `.count` for the trait
path, and the two `getMethod(...).returnType` lines are gone.
Checked it isn't vacuous the way the old one was: reverting `longValue()` to
`intValue()` makes it fail, restoring it makes it pass.
##########
grails-data-graphql/core/src/main/groovy/org/grails/gorm/graphql/Schema.groovy:
##########
@@ -381,7 +381,7 @@ class Schema {
DataFetcher countFetcher =
dataFetcherManager.getReadingFetcher(entity, COUNT).orElse(new
CountEntityDataFetcher(entity))
final String countFieldName =
namingConvention.getCount(entity)
- final GraphQLOutputType countOutputType =
(GraphQLOutputType) typeManager.getType(Integer)
+ final GraphQLOutputType countOutputType =
(GraphQLOutputType) typeManager.getType(Long)
Review Comment:
Added — in `SchemaSpec` rather than `ReadOnlyOpSpec`, because the only test
in the latter is `@Ignore`d, so an assertion there would never execute. It
asserts every `*Count` query field is `ExtendedScalars.GraphQLLong`, with a
non-empty guard so it can't pass by matching nothing.
Added the `totalCount` sentence to the upgrade note too — confirmed at
`DefaultGraphQLPaginationResponseHandler:52`, which already builds that field
with `typeManager.getType(Long)`.
##########
grails-views-gson/src/main/groovy/grails/plugin/json/view/api/HalViewHelper.groovy:
##########
@@ -113,7 +113,7 @@ interface HalViewHelper {
* @param object The object to create links for
* @param total The total number of objects to be paginated
*/
- void paginate(Object object, Integer total)
+ void paginate(Object object, Number total)
Review Comment:
Agreed — `Number` admits `BigDecimal` and says less than the code knows. All
five `paginate` overloads take `Long` now, and so does `links(Map, Object, Long
total)`, which was pre-existing but truncating for the same reason.
##########
grails-views-gson/src/main/groovy/grails/plugin/json/view/api/internal/DefaultHalViewHelper.groovy:
##########
@@ -258,7 +258,7 @@ class DefaultHalViewHelper extends DefaultJsonViewHelper
implements HalViewHelpe
* @param order The order in which the results are to be sorted eg: DESC
or ASC
*/
//TODO: Once GROOVY-9662 is fixed, remove explicit delegate call and
typecast to StreamingJsonDelegate
- void paginate(Object object, Integer total, Integer offset = null, Integer
max = null, String sort = null, String order = null) {
+ void paginate(Object object, Number total, Integer offset = null, Integer
max = null, String sort = null, String order = null) {
Review Comment:
Done — `Long total`.
##########
grails-views-gson/src/main/groovy/grails/plugin/json/view/api/internal/DefaultHalViewHelper.groovy:
##########
@@ -277,7 +277,7 @@ class DefaultHalViewHelper extends DefaultJsonViewHelper
implements HalViewHelpe
((StreamingJsonBuilder.StreamingJsonDelegate)
delegate).call(HREFLANG_ATTRIBUTE, locale.toString())
((StreamingJsonBuilder.StreamingJsonDelegate)
delegate).call(TYPE_ATTRIBUTE, contentTypeMimeType ?: contentType)
}
- List<Link> links = getPaginationLinks(object, total, max, offset,
sort, order) as List<Link>
+ List<Link> links = getPaginationLinks(object, total?.intValue(),
max, offset, sort, order) as List<Link>
Review Comment:
Gone. `getPaginationLinks` takes `Long`, and so do the offsets derived from
a total — `getNextOffset`, `getLastOffset`, `getPrevOffset`,
`paramsWithOffset`, `buildPaginateParams`.
The offsets mattered as much as the total: `getLastOffset` computes
`(ceil(total / max) - 1) * max`, so the offset overflows the moment the total
stops fitting in an `Int`. `Parameters` already exposed `long(name, default)`
for the request path.
--
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]
