feat(model/test): add --max-types-per-authorization-model flag#641
Conversation
|
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds a new Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as fga model test
participant Config as LocalServerConfig
participant LocalStore as getLocalServerModelAndTuples
participant Memory as memory.New()
User->>CLI: Run with --max-types-per-authorization-model 10
CLI->>CLI: Parse flag value (10)
CLI->>Config: Create LocalServerConfig{MaxTypesPerAuthorizationModel: 10}
CLI->>LocalStore: Call with serverConfig parameter
LocalStore->>LocalStore: Check if MaxTypesPerAuthorizationModel > 0
alt Limit configured
LocalStore->>Memory: New(WithMaxTypesPerAuthorizationModel(10))
else No limit
LocalStore->>Memory: New()
end
Memory->>LocalStore: Return configured datastore
LocalStore->>CLI: Return server and authz model
CLI->>CLI: Run tests with configured server
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Related Documentation Checked 8 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
There was a problem hiding this comment.
Pull request overview
Adds configurability for the embedded OpenFGA server used by fga model test so local model tests can run against authorization models with more than 100 type definitions.
Changes:
- Add
--max-types-per-authorization-modeltofga model testand thread it through to the embedded server configuration. - Apply the configured max-types limit to the embedded memory datastore used during local tests.
- Add unit + integration fixtures/tests and update README documentation for the new flag.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
cmd/model/test.go |
Registers and reads the new CLI flag; passes config into storetest.RunTests. |
internal/storetest/tests.go |
Introduces LocalServerConfig and extends RunTests to accept it. |
internal/storetest/localstore.go |
Plumbs the config into memory.New() via memory.WithMaxTypesPerAuthorizationModel(...). |
internal/storetest/localstore_test.go |
New unit test covering config-to-memory option behavior. |
tests/model-test-cases.yaml |
Adds integration cases verifying failure/success when adjusting the limit. |
tests/fixtures/many-types.fga.yaml |
New store fixture referencing a model with multiple types. |
tests/fixtures/many-types-model.fga |
New FGA model fixture with 6 type definitions. |
README.md |
Documents the new flag in the command table and model test parameters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/model/test.go`:
- Around line 55-62: The flag value retrieved via cmd.Flags().GetInt into
maxTypes must be validated as a positive integer before constructing
storetest.LocalServerConfig; update the code after retrieving maxTypes (from
cmd.Flags().GetInt) to check if maxTypes <= 0 and return a clear fmt.Errorf
describing the invalid --max-types-per-authorization-model value, otherwise
proceed to set MaxTypesPerAuthorizationModel on storetest.LocalServerConfig;
this ensures invalid non-positive inputs fail fast instead of propagating
downstream.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5553b6ce-d0ef-4f50-8b94-9eda9e6ca462
📒 Files selected for processing (8)
README.mdcmd/model/test.gointernal/storetest/localstore.gointernal/storetest/localstore_test.gointernal/storetest/tests.gotests/fixtures/many-types-model.fgatests/fixtures/many-types.fga.yamltests/model-test-cases.yaml
The embedded server used by `fga model test` inherits the default MaxTypesPerAuthorizationModel limit of 100 from the in-memory datastore. This causes local tests to fail for authorization models that exceed 100 type definitions, even though production OpenFGA servers can be configured with a higher limit. Add a --max-types-per-authorization-model flag to `fga model test` (default: 100, matching the current behavior) so users can raise the limit when testing larger models locally. This introduces a LocalServerConfig struct that can be extended with additional server options in the future (see openfga#564). Closes openfga#640
nverbos-godaddy
force-pushed
the
fix/raise-max-types-for-embedded-test-server
branch
from
63f3a0e to
51b05b7
Compare
|
Addressed the review feedback from @copilot and @coderabbitai — non-positive values for |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Description
What problem is being solved?
The embedded server used by
fga model testcreates an in-memory datastore viamemory.New()without overridingMaxTypesPerAuthorizationModel, which defaults to 100. This causes local model tests to fail withCode(2053): The number of type definitions in an authorization model exceeds the allowed limit of 100for any authorization model with more than 100 type definitions — even though production OpenFGA servers support configuring this limit via--max-types-per-authorization-modelorOPENFGA_MAX_TYPES_PER_AUTHORIZATION_MODEL.How is it being solved?
Add a
--max-types-per-authorization-modelflag tofga model testthat defaults to 100 (preserving current behavior). When set, the value is passed through to the embedded server's memory datastore viamemory.WithMaxTypesPerAuthorizationModel().Thanks to the existing Viper integration in the CLI, this flag can be set in three ways without any additional code:
This introduces a
LocalServerConfigstruct ininternal/storetestas an extensibility point for threading additional server options through to the embedded test server in the future. Additional flags added to the struct will automatically get config file and env var support via Viper, following the same pattern as all other CLI flags. For the broader question of exposing all server configuration to the embedded test server, we defer to the maintainers' preferred approach — see #564.What changes are made to solve it?
cmd/model/test.go: Register the--max-types-per-authorization-modelflag, read it, and pass it toRunTestsviaLocalServerConfiginternal/storetest/tests.go: AddLocalServerConfigstruct; extendRunTestssignature to accept itinternal/storetest/localstore.go: ApplyMaxTypesPerAuthorizationModelfrom config tomemory.New()internal/storetest/localstore_test.go(new): Unit tests for the config-to-memory-option plumbingREADME.md: Document the new flag in the model test parameters sectiontests/: Add a 6-type model fixture and integration tests verifying the flag controls the limitReferences
Review Checklist
mainSummary by CodeRabbit
New Features
Tests