feat: report serialized model size in fga model get and validate#712
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:
WalkthroughAdds model size reporting to the ChangesModel size reporting for get and validate
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 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 |
There was a problem hiding this comment.
Pull request overview
Adds reporting of protobuf-serialized authorization model size (in KB) to CLI model inspection/validation flows, helping users gauge proximity to server-side max model size limits.
Changes:
- Add
sizeas a selectable--fieldforfga model get, exposingsize_kbin JSON and# Size: … KBin FGA/DSL output. - Include
size_kbinfga model validateJSON output when parsing succeeds. - Add/update tests and linter allowlists for the new protobuf usage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents new size field behavior and shows example outputs. |
| internal/authorizationmodel/model.go | Adds size_kb field support and size computation + DSL metadata rendering. |
| internal/authorizationmodel/model_test.go | Adds unit tests for size computation and output formatting. |
| cmd/model/validate.go | Extends validate JSON output to include size_kb. |
| cmd/model/validate_test.go | Updates validate tests and adds a size reporting test. |
| cmd/model/get.go | Updates --field help text to include size. |
| .golangci.yaml | Allows google.golang.org/protobuf/proto imports under depguard rules. |
💡 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/model/validate.go (1)
58-69:⚠️ Potential issue | 🟠 MajorUse
DiscardUnknown: trueoption in protojson.Unmarshal at line 58 to prevent rejection of valid models withid.When
inputModelcontains a valid ULIDid, thesetCreatedAt()method populatesCreatedAt. The subsequentGetAsJSONString()call includes this field in the JSON output. The strictprotojson.Unmarshal()at line 58 fails on the unknowncreated_atfield (which doesn't exist inpb.AuthorizationModel), preventing validation from completing andsize_kbfrom being set.This pattern is already used correctly elsewhere in
GetProtoModel()(internal/authorizationmodel/model.go:118). Apply the same solution here:Suggested fix
- err = protojson.Unmarshal([]byte(*modelJSONString), model) + err = (protojson.UnmarshalOptions{DiscardUnknown: true}).Unmarshal([]byte(*modelJSONString), model) if err != nil { output.IsValid = false errorString := "unable to parse json input" output.Error = &errorString return output }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/model/validate.go` around lines 58 - 69, The protojson.Unmarshal call in the validate function is failing because it strictly rejects unknown fields, specifically the created_at field that gets added to the JSON when a model with an id is processed. Modify the protojson.Unmarshal call to include the UnmarshalOptions parameter with DiscardUnknown set to true, similar to how it is already implemented in GetProtoModel function in internal/authorizationmodel/model.go. This will allow the unmarshaling to succeed by ignoring unknown fields and allow the validation to complete so that SizeKB can be properly set in the output.
🧹 Nitpick comments (1)
cmd/model/validate_test.go (1)
103-119: ⚡ Quick winAdd a regression case for a valid ULID
idpath.Current additions validate
size_kb, but there’s still no test that passes a valid ULIDidand asserts successful validation plussize_kbemission.🧪 Suggested test addition
func TestValidateReportsSize(t *testing.T) { t.Parallel() model := authorizationmodel.AuthzModel{} if err := model.ReadFromJSONString(`{"schema_version":"1.1"}`); err != nil { t.Fatalf("unexpected parse error: %v", err) } result := validate(model) if result.SizeKB == nil { t.Fatalf("expected SizeKB to be set") } if *result.SizeKB != model.GetSizeInKB() { t.Errorf("expected %v to equal %v", *result.SizeKB, model.GetSizeInKB()) } } + +func TestValidateWithValidIDAndSize(t *testing.T) { + t.Parallel() + + model := authorizationmodel.AuthzModel{} + if err := model.ReadFromJSONString(`{"id":"01GVKXGDCV2SMG6TRE9NMBQ2VG","schema_version":"1.1"}`); err != nil { + t.Fatalf("unexpected parse error: %v", err) + } + + result := validate(model) + if !result.IsValid { + t.Fatalf("expected valid model, got error: %v", *result.Error) + } + if result.SizeKB == nil { + t.Fatalf("expected SizeKB to be set") + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/model/validate_test.go` around lines 103 - 119, Add a new test case or modify the TestValidateReportsSize test to include a valid ULID value in the `id` field of the JSON input string passed to ReadFromJSONString. The test should assert that when a valid ULID id is present, the validate function still successfully processes the model without errors, and correctly reports the SizeKB value. This ensures the validation logic properly handles the id field alongside the size_kb emission.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 502-503: The created_at timestamp example in the README.md file at
lines 502-503 uses ISO 8601 format with a T separator (2023-04-11T23:26:34.759
+0000 UTC), but the actual DSL output from internal/authorizationmodel/model.go
uses %v formatting which produces a space-separated timestamp instead of
T-separated. Update the example in README.md to use the correct format that
matches the actual CLI output by replacing the T separator with a space between
the date and time components.
---
Outside diff comments:
In `@cmd/model/validate.go`:
- Around line 58-69: The protojson.Unmarshal call in the validate function is
failing because it strictly rejects unknown fields, specifically the created_at
field that gets added to the JSON when a model with an id is processed. Modify
the protojson.Unmarshal call to include the UnmarshalOptions parameter with
DiscardUnknown set to true, similar to how it is already implemented in
GetProtoModel function in internal/authorizationmodel/model.go. This will allow
the unmarshaling to succeed by ignoring unknown fields and allow the validation
to complete so that SizeKB can be properly set in the output.
---
Nitpick comments:
In `@cmd/model/validate_test.go`:
- Around line 103-119: Add a new test case or modify the TestValidateReportsSize
test to include a valid ULID value in the `id` field of the JSON input string
passed to ReadFromJSONString. The test should assert that when a valid ULID id
is present, the validate function still successfully processes the model without
errors, and correctly reports the SizeKB value. This ensures the validation
logic properly handles the id field alongside the size_kb emission.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 316e37e9-9284-4866-8cf0-a6504a7ecdcd
📒 Files selected for processing (7)
.golangci.yamlREADME.mdcmd/model/get.gocmd/model/validate.gocmd/model/validate_test.gointernal/authorizationmodel/model.gointernal/authorizationmodel/model_test.go
rhamzeh
force-pushed
the
feat/model-size-field
branch
from
ec126d4 to
d1f0e13
Compare
Description
What problem is being solved?
How is it being solved?
What changes are made to solve it?
We added
size_kbto the output offga model getandfga model validateReferences
closes #711
Review Checklist
mainSummary by CodeRabbit
New Features
sizefield option to thefga model getcommand for retrieving model size in kilobytes.Documentation
fga model getcommand to show the newsizefield option and sample responses.fga model validatecommand to include size information in response examples.