matrei commented on PR #16323:
URL: https://github.com/apache/grails-core/pull/16323#issuecomment-5623514371
# AI Review
Thanks, the direction is right and the core change is complete: every
`GormStaticOperations.count()` implementation in the repo is updated (Mongo and
Neo4j go through `GormStaticApi.count()` and need nothing), and the tests for
`grails-datamapping-core`, `grails-views-gson` and `grails-data-graphql-core`
pass locally. The GSP template, the `RestfulServiceController` and the
section-58 renumbering all look correct.
A few things to address before merge.
### 1. The JSON API pagination path still truncates
`DefaultJsonApiViewHelper` (around line 478) was not touched:
```groovy
Integer total = (Integer) paginationArgs.get(PAGINATION_TOTAL)
...
List<Link> links = getPaginationLinks(resource, total, params)
```
Under `@CompileStatic` that cast is Groovy's numeric cast, not a checkcast,
so a `Long` coming from `Book.count()` is silently wrapped (I verified
`(Integer) 3_000_000_000L` gives `-1294967296`). That is the exact bug this PR
is fixing, still present in `jsonapi.render(list, [pagination: [total:
Book.count(), resource: Book]])`. Suggest:
```groovy
Long total = ((Number) paginationArgs.get(PAGINATION_TOTAL)).longValue()
```
and a row in `IterableRenderSpec` with a total above `Integer.MAX_VALUE`
that asserts the `last` link offset.
### 2. `PaginationSpec` was not updated for the changed signatures
`getPaginationLinks`, `getPrevOffset`, `getNextOffset` and `getLastOffset`
all changed from `Integer` to `Long`, but `PaginationSpec` still declares the
results as `Integer` and only exercises int-range values. It passes because
dynamic Groovy converts on assignment, so it no longer pins the type it is
testing. Please switch the declarations to `Long` and add at least one row
above `Integer.MAX_VALUE`, for example `getLastOffset(3_000_000_000L, 10) ==
2_999_999_990L`. That row is the behaviour the change exists to enable, and it
also covers the `Math.round(Math.ceil(...))` to `(long) Math.ceil(...)` rewrite.
### 3. `offset` in `HalViewHelper.paginate` stayed `Integer`
`total` is now `Long`, and the protected helpers all take `Long offset`, but
the public `paginate(Object, Long total, Integer offset, ...)` overloads still
take `Integer offset`. A page past row 2^31 can be described by `total` but not
requested through `paginate`. Low priority, but widening `offset` in the same
PR keeps the surface consistent instead of doing it in a second breaking change
later.
### 4. Upgrade note
- "GORM normalises whatever it gets with `longValue()`, then the declared
return type narrowed it again with `intValue()`" describes the new code as if
it were the old pipeline. The pre-change `GormStaticApi.count()` called
`intValue()` directly. Suggest: "`GormStaticApi.count()` narrowed the datastore
result with `intValue()`, which truncates silently, so ...".
- The PR had to change `Integer productCount` to `Long productCount` in two
`.gson` `model {}` blocks, but the note only mentions the GSP `@{ model=... }`
directive. Please add a sentence for JSON views: a `model { Integer fooCount }`
declaration should become `Long` (or `Number`).
- Worth a line that `HalViewHelper.paginate()` and `links()` now declare
`total` as `Long`. Callers are unaffected because Groovy widens `Integer` to
`Long` in both dynamic and statically compiled code (verified), but anyone
implementing `HalViewHelper` has to update.
### Minor / no action needed
- `GormStaticApiSpec`'s new large-count test drives everything through
`Datastore`, `Session` and `Query`, so it stays on the public surface. Good.
- `SchemaSpec` pinning the `GraphQLLong` scalar is a good regression guard
for the schema-level change.
- Custom GraphQL count fetchers registered through the data fetcher manager
that still return `Integer` keep working, since the `Long` scalar coerces
integers, so the type change is only breaking for generated clients, which the
note already says.
--
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]
