Graylog2 / Graylog2/graylog2-server

PageListResponse.create — total vs grandTotal investigation

Open
#27,024 0 comments 0 reactions 0 assignees View on GitHub
bug triaged
Dominant language
Java
Stars
8.1k
Forks
1.1k
Avg merge
1d 20h
Merged PRs (30d)
217

Description

# `PageListResponse.create` — `total` vs `grandTotal` investigation

## Problem

`PageListResponse` (`graylog2-server/src/main/java/org/graylog2/rest/models/tools/responses/PageListResponse.java`) exposes two related but distinct counts:

- `total` — a top-level `long` field, meant to represent the number of results matching the current search/filter query.
- `paginationInfo` — a `PaginatedList.PaginationInfo`, which *also* carries its own `total()` (an `int`), representing the same thing: the filtered count, ignoring pagination.

The two 8-arg `create(...)` overloads accept `total` as an explicit, independent parameter instead of deriving it from `paginationInfo`. This means a caller can pass any value it wants for `total` — including one that has nothing to do with `paginationInfo.total()`. In practice, several call sites do exactly that, passing `result.grandTotal().orElse(0L)` instead.

`PaginatedList.grandTotal()` is a separate, optional concept: the count of *all* entries ignoring both pagination **and** query filters (e.g. total nodes in a cluster, regardless of any search query the user typed). It's `Optional.empty()` unless a caller explicitly populates it via the 5-arg `PaginatedList` constructor or a `grandTotalFilter(...)` step in `MongoPaginationHelper`.

**The bug:** for every buggy call site examined, `grandTotal()` is never actually populated (`Optional.empty()`), so `result.grandTotal().orElse(0L)` silently evaluates to **`0`** — the response reports `"total": 0` regardless of how many results actually matched the query. This breaks pagination UIs (e.g. "Showing 1-10 of 0").

A third `create(...)` overload takes a `PaginatedList` directly (no explicit `total` param) and *always* derives `total` from `paginatedList.pagination().total()` internally — this overload is immune to the bug by construction.

## Call site audit

25 call sites of `PageListResponse.create(...)` were audited across the codebase.

### ✅ Correct — uses filtered `total` (16 sites)

| File:line | Notes |
|---|---|
| `org/graylog/plugins/views/search/rest/DashboardsResource.java:167` | `result.pagination().total()` |
| `org/graylog/plugins/views/search/rest/SavedSearchesResource.java:126` | `result.pagination().total()` |
| `org/graylog/collectors/rest/FleetResource.java:130` | `result.pagination().total()` |
| `org/graylog/collectors/rest/CollectorInstancesResource.java:256` | `list.pagination().total()` |
| `org/graylog/collectors/rest/SourceResource.java:114` | `result.pagination().total()` |
| `org/graylog/collectors/opamp/rest/EnrollmentTokenResource.java:196` | `list.pagination().total()` |
| `org/graylog/events/rest/EventDefinitionsResource.java:281` | `definitionDtos.pagination().total()` |
| `org/graylog2/rest/resources/opensearch/OpensearchClusterResource.java` | fixed in commit `a34f5f45e6` |
| `org/graylog2/rest/resources/system/inputs/InputsResource.java:414` | `mappedResult.pagination().total()` |
| `org/graylog2/rest/resources/tokenusage/TokenUsageResource.java:128` | `pagination.total()` |
| `org/graylog2/rest/resources/system/indexer/IndexSetTemplateResource.java:230` | passthrough of an already-correct total |
| `org/graylog2/indexer/indexset/IndexSetFieldTypeSummaryService.java:112` | in-memory filtered total |
| `org/graylog2/indexer/indexset/profile/IndexFieldTypeProfileService.java:182` | 6-arg `PaginatedList` overload — always correct |
| `org/graylog2/indexer/indexset/template/IndexSetTemplateService.java:133` | 6-arg overload — always correct |
| `org/graylog2/indexer/fieldtypes/IndexFieldTypesListService.java:90` | in-memory filtered total |
| `org/graylog2/rest/resources/system/NotificationsResource.java:216` | 6-arg overload — always correct |

### ❌ Bug — uses `grandTotal()`, always reports `0` (8 sites)

| File:line | Expression | `grandTotal` actually populated upstream? |
|---|---|---|
| `org/graylog2/rest/resources/mongodb/MongodbClusterResource.java:154` | `result.grandTotal().orElse(0L)` | No |
| `org/graylog2/rest/resources/system/indexer/OutdatedIndexResource.java:165` | `result.grandTotal().orElse(0L)` | No |
| `org/graylog/events/rest/EventNotificationsResource.java:161` | `result.grandTotal().orElse(0L)` | No |
| `org/graylog2/rest/resources/streams/StreamPipelineRulesResource.java:129` | `result.grandTotal().orElse(0L)` | No |
| `org/graylog2/rest/resources/system/ClusterResource.java:171` | `result.grandTotal().orElse(0L)` | No |
| `org/graylog2/rest/resources/datanodes/DatanodeResource.java:114` | `result.grandTotal().orElse(0L)` | No |
| `org/graylog2/inputs/diagnosis/InputRoutingRulesService.java:183` (`getPipelineRulesPage`) | `paginatedList.grandTotal().orElse(0L)` | No — discards an already-correctly-computed count |
| `org/graylog2/inputs/diagnosis/InputRoutingRulesService.java:239` (`getStreamRulesPage`) | `paginatedList.grandTotal().orElse(0L)` | No — same pattern |

Note: `InputRoutingRulesService.java:116` also calls `.grandTotal().orElse(0L)`, but on an explicit `PaginatedList.emptyList(...)` where `grandTotal` is deliberately `0L` and the list is genuinely empty — not a real bug, just stylistically inconsistent with the fix below.

`org/graylog2/rest/resources/opensearch/OpensearchClusterResource.java` had the identical bug and was already fixed in commit `a34f5f45e6` ("fixed total count of results") by swapping `result.grandTotal().orElse(0L)` for `result.pagination().total()` — the same pattern applies to all 8 sites above.

### ⚠️ Different root cause, same symptom (1 site)

| File:line | Issue |
|---|---|
| `org/graylog2/rest/resources/streams/StreamResource.java:291` | Passes `total = paginatedStreamService.count()` — a separate, unfiltered, unpermissioned count query — instead of `result.pagination().total()`. Same visible bug (wrong total shown to the user), different code path; not a `grandTotal()` call, so it isn't fixed by the general rule below on its own and needs a one-off look. |

## Suggested fix

Rather than fixing the 8 broken call sites one at a time (which only fixes today's bugs and leaves the footgun in place for the next caller), remove the possibility of the bug entirely:

**Drop the explicit `total` parameter from the two 8-arg `create(...)` overloads and derive it internally from `paginationInfo.total()`**, the same way the existing 6-arg `PaginatedList`-based overload already does.

```java
// Before
public static PageListResponse create(
@Nullable String query,
PaginatedList.PaginationInfo paginationInfo,
long total,
@Nullable String sort,
@Nullable SortOrder order,
List elements,
List attributes,
EntityDefaults defaults) {
return new AutoValue_PageListResponse<>(query, paginationInfo, total, sort, order, elements, attributes, defaults);
}

// After
public static PageListResponse create(
@Nullable String query,
PaginatedList.PaginationInfo paginationInfo,
@Nullable String sort,
@Nullable SortOrder order,
List elements,
List attributes,
EntityDefaults defaults) {
return new AutoValue_PageListResponse<>(query, paginationInfo, paginationInfo.total(), sort, order, elements, attributes, defaults);
}
```

(Mirror the change for the `String order` overload.)

### Why this is the better fix

- It makes the invalid state unrepresentable: there is no longer any parameter through which a caller can supply a `total` that disagrees with `paginationInfo.total()`.
- Every one of the 16 "correct" call sites already passes `paginationInfo.total()` (or an equivalent) as `total` today — so no legitimate use case is lost by removing the parameter.
- It fixes all 8 `grandTotal()`-based bugs at once, without touching each call site's logic individually.
- The public JSON contract is unaffected — `total` remains a serialized field on `PageListResponse` (via the AutoValue accessor), it's just no longer an input.

### What still needs separate attention

- **All ~20 call sites of the two 8-arg overloads** need their invocations updated to drop the now-removed `total` argument (mechanical, but touches many files).
- **`StreamResource.java:291`** is not a `grandTotal()` bug — it needs its own fix (stop calling `paginatedStreamService.count()` and pass `result.pagination().total()` / switch to the 6-arg overload), independent of the signature change above.
- **`@JsonCreator`-annotated overload**: confirm nothing actually round-trips a `PageListResponse` through JSON deserialization relying on an explicit `"total"` field being fed back into the constructor — if so, verify dropping it as a constructor arg (while keeping it as a serialized accessor) doesn't break that path.
- **`PageListResponseTest.java`**: existing tests should be revisited once the signature changes, since some currently call the 8-arg overload directly with an explicit `total`.

Contributor guide

Open the contributing guide

Research direction

Start in graylog2-server/src/main/java/org/graylog2/rest/models/tools/responses/PageListResponse.java and inspect both 8-arg create overloads and the existing PaginatedList-based overload. Audit the listed create call sites, including StreamResource.java, then revisit PageListResponseTest.java and the @JsonCreator path. Done means filtered totals are used consistently, signatures and callers compile, tests pass, and JSON behavior remains valid.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.