feat: configurable max page size for ReadChanges#2887
Merged
Conversation
Contributor
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a configurable server-side maximum page size for ReadChanges (default 100); exposed in config and JSON schema, wired to CLI flag/env var, surfaced as a server option, and validated in the ReadChanges handler to reject out-of-range PageSize values early. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI/Flags
participant Config as Config
participant Server as gRPC Server
participant Handler as ReadChanges Handler
participant Datastore as Datastore
User->>CLI: provide --readChanges-max-page-size
CLI->>Config: set ReadChangesMaxPageSize
Config->>Server: initialize with WithReadChangesMaxPageSize()
Server->>Server: store readChangesMaxPageSize
User->>Handler: ReadChangesRequest(PageSize=N)
Handler->>Handler: if PageSize set, validate 1 <= N <= readChangesMaxPageSize
alt PageSize > max
Handler-->>User: return InvalidArgument error
else
Handler->>Datastore: query changes (PageSize=N)
Datastore-->>Handler: return changes
Handler-->>User: return ReadChangesResponse
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jpadilla
force-pushed
the
read-changes-max-page-size
branch
from
8e5a268 to
94726b3
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
jpadilla
force-pushed
the
read-changes-max-page-size
branch
from
94726b3 to
33fa8e7
Compare
|
Related Documentation Checked 5 published document(s) in 0 knowledge base(s). No updates required. How did I do? Any feedback? |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a configurable server-side maximum for ReadChanges page size, exposing it via config/CLI/env and enforcing it at request time.
Changes:
- Introduces
ReadChangesMaxPageSizeconfiguration with default 100 and wires it into server construction. - Adds server-side validation in
ReadChangesto reject requests exceeding the configured max. - Adds CLI/env flag wiring and new unit tests covering default/custom max behavior.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/server/server.go | Adds server field + option WithReadChangesMaxPageSize and sets default. |
| pkg/server/read_changes.go | Enforces configured max page size during ReadChanges request handling. |
| pkg/server/read_changes_test.go | Adds tests for default and custom max page size behavior. |
| pkg/server/config/config.go | Adds config field + default for ReadChangesMaxPageSize. |
| cmd/run/run.go | Adds --readChanges-max-page-size flag and applies it to server options. |
| cmd/run/flags.go | Binds flag/env vars to config key readChangesMaxPageSize. |
| go.mod / go.sum | Bumps github.com/openfga/api/proto dependency to pick up validation changes. |
| CHANGELOG.md | Documents the new configurable server-side validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@pkg/server/config/config.go`:
- Around line 339-342: ReadChangesMaxPageSize can be zero which will effectively
block all ReadChanges calls; update VerifyServerSettings (or equivalent config
validation path) to detect when ReadChangesMaxPageSize == 0 and either return a
validation error rejecting the config or normalize the value (e.g., treat 0 as
"unlimited" by setting it to math.MaxUint32 or another sentinel) so runtime
requests are not accidentally locked out; reference ReadChangesMaxPageSize and
VerifyServerSettings when making the change so the check lives in the server
configuration verification logic.
In `@pkg/server/read_changes_test.go`:
- Around line 17-63: The test reuses a single memory datastore (ds :=
memory.New()) across subtests but each subtest calls openfga.Close(), which
closes that shared datastore and leads to flakes; fix by creating and using a
fresh datastore and server per subtest (e.g., call memory.New() and pass it to
MustNewServerWithOpts / WithDatastore inside each t.Run or use the existing
newServer(t, ...) helper), ensure you call t.Cleanup on both the datastore.Close
and openfga.Close within the subtest so each subtest has an isolated, live
datastore/server and does not operate on a closed datastore.
In `@pkg/server/read_changes.go`:
- Around line 32-39: The PageSize validation incorrectly always reads
req.GetPageSize().GetValue() and only checks the upper bound; update the logic
in ReadChangesRequest handling so you first check presence (req.GetPageSize() !=
nil or inspect PageSize proto field) before calling GetValue(), and when present
enforce both lower and upper bounds (pageSize >= 1 && pageSize <=
s.readChangesMaxPageSize) using the same descriptive error via status.Errorf;
keep the variable name pageSize and s.readChangesMaxPageSize to locate and
update the checks and error message so the semantics and message [1, max] align.
In `@pkg/server/server.go`:
- Around line 340-344: Add a validation in NewServerWithOpts after applying all
OpenFGAServiceV1Option funcs to ensure s.readChangesMaxPageSize is a positive,
non-zero int32: check s.readChangesMaxPageSize > 0 and if not return an error
(use fmt.Errorf with a clear message like "readChangesMaxPageSize must be > 0,
got %d"). This guards the WithReadChangesMaxPageSize path which currently
assigns a uint32 into int32 (function WithReadChangesMaxPageSize) and prevents
zero or overflowed negative values from silently disabling or bypassing the
ReadChanges page-size limit.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2887 +/- ##
==========================================
+ Coverage 90.47% 90.53% +0.06%
==========================================
Files 154 154
Lines 17516 17535 +19
==========================================
+ Hits 15846 15873 +27
+ Misses 1088 1083 -5
+ Partials 582 579 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jpadilla
force-pushed
the
read-changes-max-page-size
branch
from
33fa8e7 to
142bf66
Compare
Signed-off-by: José Padilla <jose.padilla@okta.com>
jpadilla
force-pushed
the
read-changes-max-page-size
branch
from
142bf66 to
42915ef
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
willvedd
reviewed
Jan 22, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.config-schema.json:
- Around line 105-111: The schema allows readChangesMaxPageSize values down to 1
but NewServerWithOpts enforces a minimum of storage.DefaultPageSize (50),
causing config validation mismatch; update the "readChangesMaxPageSize" entry in
.config-schema.json to set "minimum" to 50 (or the value of
storage.DefaultPageSize) and optionally adjust the "description" to note the
server-enforced minimum so config validation aligns with NewServerWithOpts and
storage.DefaultPageSize.
In `@pkg/server/server.go`:
- Around line 931-933: The error message in the check comparing
s.readChangesMaxPageSize and storage.DefaultPageSize has a grammatical mistake
("must be a greater than"); update the fmt.Errorf call (the string produced
where s.readChangesMaxPageSize < storage.DefaultPageSize) to remove the
extraneous "a" and read clearly—e.g. "ReadChanges max page size must be greater
than or equal to %d"—so the message is grammatically correct while still
including storage.DefaultPageSize for context.
♻️ Duplicate comments (2)
pkg/server/read_changes.go (1)
32-41: Enforce the max against the effective page size (omitted PageSize can bypass the limit).
WhenPageSizeis absent/0, downstream pagination defaults (e.g., 50). IfreadChangesMaxPageSizeis configured below that, the limit is bypassed.🛠️ Suggested fix
@@ import ( "context" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" openfgav1 "github.com/openfga/api/proto/openfga/v1" "github.com/openfga/openfga/internal/utils/apimethod" "github.com/openfga/openfga/pkg/middleware/validator" "github.com/openfga/openfga/pkg/server/commands" + "github.com/openfga/openfga/pkg/storage" "github.com/openfga/openfga/pkg/telemetry" ) @@ - if p := req.GetPageSize(); p != nil { - pageSize := p.GetValue() - if pageSize < 1 || pageSize > s.readChangesMaxPageSize { + pageSize := int32(0) + if p := req.GetPageSize(); p != nil { + pageSize = p.GetValue() + } + effectivePageSize := pageSize + if effectivePageSize == 0 { + effectivePageSize = int32(storage.DefaultPageSize) + } + if effectivePageSize < 1 || effectivePageSize > s.readChangesMaxPageSize { return nil, status.Errorf( codes.InvalidArgument, "invalid ReadChangesRequest.PageSize: value must be inside range [1, %d]", s.readChangesMaxPageSize, ) - } }pkg/server/read_changes_test.go (1)
17-23: Shared datastore across subtests may cause flakiness.Each subtest's
openfga.Close()ultimately closes resources that may affect subsequent subtests. Consider creating a fresh datastore per subtest using a helper function.Suggested refactor
func TestReadChangesPageSizeValidation(t *testing.T) { t.Cleanup(func() { goleak.VerifyNone(t) }) - ds := memory.New() - t.Cleanup(ds.Close) + + newServer := func(t *testing.T, opts ...OpenFGAServiceV1Option) *Server { + ds := memory.New() + t.Cleanup(ds.Close) + s := MustNewServerWithOpts(append([]OpenFGAServiceV1Option{WithDatastore(ds)}, opts...)...) + t.Cleanup(s.Close) + return s + } t.Run("page_size_not_provided", func(t *testing.T) { - openfga := MustNewServerWithOpts( - WithDatastore(ds), - ) - t.Cleanup(openfga.Close) + openfga := newServer(t)
🧹 Nitpick comments (1)
pkg/server/server.go (1)
341-350: Option allows values that startup validation rejects.
WithReadChangesMaxPageSizefalls back to the default formax == 0ormax > math.MaxInt32, but values between 1 and 49 are accepted here and only rejected later inNewServerWithOpts(lines 931-933). This inconsistency could confuse users—consider also falling back to the default for values belowstorage.DefaultPageSize:Suggested fix
func WithReadChangesMaxPageSize(max uint32) OpenFGAServiceV1Option { return func(s *Server) { - if max <= 0 || max > math.MaxInt32 { + if max < storage.DefaultPageSize || max > math.MaxInt32 { max = serverconfig.DefaultReadChangesMaxPageSize } s.readChangesMaxPageSize = int32(max) } }Alternatively, remove the startup validation (lines 931-933) and rely solely on the option guard—though keeping both provides defense-in-depth.
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
adriantam
previously approved these changes
Jan 22, 2026
willvedd
previously approved these changes
Jan 22, 2026
jpadilla
changed the title
adriantam
approved these changes
Jan 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
What problem is being solved?
The ReadChanges API had a hardcoded max page size of 100 defined at the proto level, making it impossible to customize this limit for different deployment environments. Different deployments may need different max page sizes based on their infrastructure capacity, performance requirements, or specific use cases.
https://github.com/openfga/api/blob/main/openfga/v1/openfga_service.proto#L1682-L1709
How is it being solved?
By moving the max page size validation from the proto definition to server-side validation with configurable limits. The proto-level max validation has been removed (in the openfga/api repository), and server-side validation has been implemented with configuration options via CLI flags and environment variables. The default max remains 100 to maintain backward compatibility with existing deployments.
What changes are made to solve it?
Configuration (pkg/server/config/config.go)
DefaultReadChangesMaxPageSize = 100constantReadChangesMaxPageSizefield to Config structDefaultConfig()with the default valueServer (pkg/server/server.go)
readChangesMaxPageSizefield to Server structWithReadChangesMaxPageSize()option function to configure the limitNewServerWithOpts()with the default valuereadChangesMaxPageSize > 0during server initializationValidation (pkg/server/read_changes.go)
"invalid ReadChangesRequest.PageSize: value must be inside range [1, %d]"page_size_invaliderror code by existing error mapping logicCLI Integration (cmd/run/run.go)
--readChanges-max-page-sizeCLI flag with default value of100Environment Variables (cmd/run/flags.go)
OPENFGA_READ_CHANGES_MAX_PAGE_SIZEenvironment variableOPENFGA_READCHANGESMAXPAGESIZEenvironment variable (alternate format)Backward Compatibility
References
openfga/api#241
Review Checklist
mainSummary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.