codeconsole commented on PR #16323: URL: https://github.com/apache/grails-core/pull/16323#issuecomment-5607770489
Thanks — rebased onto current `8.0.x` and took the full-Long option on the pagination, so the paging logic is in scope rather than half-converted. **Pagination is now `Long` end to end.** `paginate` (all five overloads), `getPaginationLinks`, and every offset 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 an offset overflows the moment a total stops fitting in an `Int`. `Parameters` already exposed `long(name, default)`, so the request-read path needed nothing new. You're right that `Number` was the wrong choice; it admits `BigDecimal` and says less than the code knows. One thing I did not change, and want to flag rather than bury. `links(Map, Object, Number total)` on `HalViewHelper` is pre-existing and was already narrowing at `DefaultHalViewHelper:229` via `total.intValue()` — a truncation that predates this PR. Changing that signature to `Long` would reject callers currently passing an `Integer`, so I left it as `Number` and made the call widen (`total?.longValue()`) instead of truncate. That does leave one `Number` in the pagination area, which is the shape you objected to on `paginate`. Happy to take it to `Long` as well if you'd rather have consistency than source compatibility there. **On the other comments:** - §54 → §58 after the rebase, as you called. Separately: `8.0.x` currently has `==== 54.` twice — *Request Processing Behaviour Changes* (line 3256) and *Non-Public Bean Classes Are Marshalled, and Reported Once* (line 3454). I left both alone since neither is mine, but you may want to renumber the second. - `BookGormEntity` fixed — it's now `long total = Book.count();`, which shows the auto-unboxing point directly as you suggested. - The count spec is replaced with your stubbed version pushing `3_000_000_000L`, plus `GormStaticApiThing.count()` / `.count` for the trait path, and the two `getMethod(...).returnType` lines are gone. I checked it isn't vacuous: reverting `longValue()` to `intValue()` makes it fail, restoring it makes it pass. - GraphQL scalar is asserted, but in `SchemaSpec` rather than `ReadOnlyOpSpec` — the only test in the latter is `@Ignore`d, so an assertion there never executes. It checks 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; confirmed at `DefaultGraphQLPaginationResponseHandler:52`, which already builds that field with `typeManager.getType(Long)`. **Verification after the rebase:** `compileGroovy compileTestGroovy` across every module, then `grails-datamapping-core`, `grails-data-graphql-core`, `grails-test-examples-gorm:integrationTest` and `grails-test-examples-views-functional-tests:integrationTest` with `--rerun-tasks` — including `ProductSpec`, which exercises the HAL pagination links over the wire. All green. -- 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]
