jdaugherty commented on code in PR #16323:
URL: https://github.com/apache/grails-core/pull/16323#discussion_r3970237739
##########
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:
This file conflicts with `8.0.x` on merge, and `8.0.x` now numbers sections
through 57, so after a rebase this becomes §58.
##########
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:
`BookGormEntity` isn't a type. The trait's static methods land on the domain
class itself, so a Java caller writes `Book.count()`, the same as the line
below. `long total = Book.count();` also demonstrates the auto-unboxing point
directly.
##########
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:
The name promises the truncation case, but the body only checks declared
types. Nothing pushes a value past `Integer.MAX_VALUE` through the
`longValue()` branch this PR changed, which is the behaviour that was broken.
Stubbing the session gets at it directly, and this passes on the branch (needs
`org.grails.datastore.mapping.query.Query` imported):
```groovy
void "count() preserves a datastore count above Integer.MAX_VALUE"() {
given:
def ds = Stub(Datastore)
def session = Stub(Session)
def query = Mock(Query)
ds.getMappingContext() >> datastore.mappingContext
ds.connect() >> session
session.getDatastore() >> ds
session.createQuery(GormStaticApiThing) >> query
query.projections() >> Mock(Query.ProjectionList)
query.singleResult() >> 3_000_000_000L
def api = new GormStaticApi(GormStaticApiThing, ds, [])
expect:
api.count() == 3_000_000_000L
}
```
Worth also asserting the trait path, since that is what domain classes
actually expose: `GormStaticApiThing.count() instanceof Long` and
`GormStaticApiThing.count instanceof Long` both hold against the
`SimpleMapDatastore` this spec already has. The two `getMethod(...).returnType
== Long` lines can go once the behaviour is covered; they restate the signature.
##########
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:
Nothing asserts the type the count field is built with; `ReadOnlyOpSpec`
only checks the field exists. A `type == ExtendedScalars.GraphQLLong` assertion
next to that check (or in `SchemaSpec`) would pin this.
Also worth a sentence in the upgrade note: paginated list responses already
type `totalCount` as `Long` through the same type manager
(`DefaultGraphQLPaginationResponseHandler`), so the count field now matches an
existing scalar in the schema rather than introducing a new one.
##########
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:
Why aren't we changing this to Long?
##########
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:
Why are we forcing this to intValue() and not converting it to long too?
##########
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:
The paginate options seem additional to this review, Number allows
BigDecimal, why aren't we making this a Long too?
--
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]
