feat: add dynamic defaults for tuple write command #517
Conversation
## Walkthrough
The documentation and implementation for the `fga tuple write` command were updated to clarify and enforce conditional default values for the `--max-tuples-per-write`, `--max-parallel-requests`, and `--rampup-period-in-sec` flags when `--max-rps` is set. The requirement for specifying `--rampup-period-in-sec` with `--max-rps` was removed, and example usage was simplified. New constants were added to support these defaults, and validation ensures all integer flags are greater than zero if provided.
## Changes
| File(s) | Change Summary |
|--------------------------|---------------------------------------------------------------------------------------------------|
| README.md | Clarified default behaviors for batching and rate limiting flags; simplified example command. |
| cmd/tuple/write.go | Added flag validation and conditional defaults based on `max-rps`; removed required-together flags.|
| internal/tuple/import.go | Added constants for default batch size, parallel requests divisor, and ramp-up period multiplier. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant CLI
participant TupleWriter
User->>CLI: Run 'fga tuple write' with flags
CLI->>CLI: Validate flags (must be > 0 if set)
CLI->>CLI: Check if --max-rps is set
alt --max-rps set & other flags omitted
CLI->>CLI: Apply conditional defaults for omitted flags
end
CLI->>TupleWriter: Invoke tuple import with resolved flag values
TupleWriter->>CLI: Return result
CLI->>User: Output import resultPoem
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cmd/tuple/write.go (1)
171-174: Hard-coded magic number – promote to a named constant
40appears out of thin air; keeping this value in sync between code and docs will be error-prone.- maxTuplesPerWrite = 40 + maxTuplesPerWrite = tuple.MaxTuplesPerWriteWithMaxRPS // e.g. 40Define
tuple.MaxTuplesPerWriteWithMaxRPSnext to the existingtuple.MaxTuplesPerWriteconstant to make intent explicit and avoid scattering literals.README.md (2)
674-679: Markdown list style and punctuation don’t follow project lint rulesThe static analyzers flagged the asterisk list markers (
*) and a missing comma.
Switch to dash markers (-) as elsewhere in the doc and add the missing comma to keepmarkdownlintandlanguagetoolgreen.-* `--max-tuples-per-write`: Max tuples to send in a single write (optional, default=1, or 40 if `--max-rps` is set and this flag is omitted) -* `--max-parallel-requests`: Max requests to send in parallel (optional, default=4, or `max-rps/5` if `--max-rps` is set and this flag is omitted) -* `--hide-imported-tuples`: When importing from a file, do not output successfully imported tuples in the command output (optional, default=false) -* `--max-rps`: Max requests per second. When set, the CLI will ramp up requests from 1 RPS to the set value. If `--rampup-period-in-sec` is omitted it defaults to `max-rps*2`. -* `--rampup-period-in-sec`: Time in seconds to wait between each batch of tuples when ramping up. Only used if `--max-rps` is set. + - `--max-tuples-per-write`: Max tuples to send in a single write (optional, default = 1, or 40 if `--max-rps` is set and this flag is omitted), + - `--max-parallel-requests`: Max requests to send in parallel (optional, default = 4, or `max-rps/5` if `--max-rps` is set and this flag is omitted), + - `--hide-imported-tuples`: When importing from a file, do not output successfully imported tuples in the command output (optional, default = false), + - `--max-rps`: Max requests per second. When set, the CLI will ramp up requests from 1 RPS to the set value. If `--rampup-period-in-sec` is omitted, it defaults to `max-rps*2`, + - `--rampup-period-in-sec`: Time in seconds to wait between each batch of tuples when ramping up. Only used if `--max-rps` is set.🧰 Tools
🪛 LanguageTool
[uncategorized] ~677-~677: Possible missing comma found.
Context: ...t value. If--rampup-period-in-secis omitted it defaults tomax-rps*2. * `--rampup...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~678-~678: Loose punctuation mark.
Context: ...max-rps*2. *--rampup-period-in-sec: Time in seconds to wait between each ba...(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
674-674: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
675-675: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
676-676: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
677-677: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
678-678: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
684-685: Example now relies on new defaults – clarify parallelismThe simplified example is great, but readers may wonder why the CLI still issues multiple parallel requests or writes more than one tuple per request.
Consider appending a brief note such as “(other limits will default automatically based on--max-rpsas explained above)” to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)cmd/tuple/write.go(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~677-~677: Possible missing comma found.
Context: ...t value. If --rampup-period-in-sec is omitted it defaults to max-rps*2. * `--rampup...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~678-~678: Loose punctuation mark.
Context: ... max-rps*2. * --rampup-period-in-sec: Time in seconds to wait between each ba...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
README.md
674-674: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
675-675: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
676-676: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
677-677: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
678-678: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Release Process
- GitHub Check: Tests
|
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@aaguiarz I'll review this PR for you. Let me examine the implementation and documentation changes. 🏁 Script executed: Length of output: 17858 🏁 Script executed: Length of output: 7577 🏁 Script executed: Length of output: 2116 🏁 Script executed: Length of output: 12854 @aaguiarz Thank you for this improvement! The dynamic defaults feature is a great UX enhancement. I've reviewed the implementation and have several suggestions to improve code quality, maintainability, and correctness. 🎯 Overall AssessmentThe functionality works as intended, but there are several areas for improvement: 🔧 Issues & Recommendations1. Constants DiscrepancyThere's a mismatch between the constant and documentation:
Recommendation: Clarify which value is correct and update accordingly. 2. Magic Numbers Should Be ConstantsThe conditional logic uses hardcoded values: // Current implementation
maxTuplesPerWrite = 40
defaultParallel := maxRPS / 5
rampUpPeriodInSec = maxRPS * 2Recommendation: Extract these to named constants: const (
DefaultMaxTuplesPerWriteWithRPS = 40
RPSToParallelRequestsDivisor = 5
RPSToRampupPeriodMultiplier = 2
)3. Extract Conditional Logic to Testable FunctionThe flag defaulting logic is embedded in the main function, making it hard to test. Recommendation: Extract to a separate function: type FlagDefaults struct {
MaxTuplesPerWrite int
MaxParallelRequests int
RampupPeriodInSec int
}
func calculateDynamicDefaults(maxRPS int, flags *flag.FlagSet) FlagDefaults {
defaults := FlagDefaults{}
if maxRPS > 0 {
if !flags.Changed("max-tuples-per-write") {
defaults.MaxTuplesPerWrite = DefaultMaxTuplesPerWriteWithRPS
}
if !flags.Changed("max-parallel-requests") {
parallelRequests := maxRPS / RPSToParallelRequestsDivisor
if parallelRequests < 1 {
parallelRequests = 1
}
defaults.MaxParallelRequests = parallelRequests
}
if !flags.Changed("rampup-period-in-sec") {
defaults.RampupPeriodInSec = maxRPS * RPSToRampupPeriodMultiplier
}
}
return defaults
}4. Add Validation for Edge CasesConsider edge cases:
Recommendation: Add validation and consider warning users: if maxRPS > 0 && maxRPS < RPSToParallelRequestsDivisor {
// Maybe log a warning about low RPS values
}5. Missing TestsThere are no tests for the new conditional logic. Recommendation: Add comprehensive tests: func TestCalculateDynamicDefaults(t *testing.T) {
tests := []struct {
name string
maxRPS int
setFlags map[string]bool
expected FlagDefaults
}{
{
name: "maxRPS 10 with no flags set",
maxRPS: 10,
expected: FlagDefaults{
MaxTuplesPerWrite: 40,
MaxParallelRequests: 2,
RampupPeriodInSec: 20,
},
},
{
name: "maxRPS 1 (edge case)",
maxRPS: 1,
expected: FlagDefaults{
MaxTuplesPerWrite: 40,
MaxParallelRequests: 1, // minimum enforced
RampupPeriodInSec: 2,
},
},
// Add more test cases...
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Test implementation
})
}
}6. Documentation ClarityThe README is good but could be clearer about the conditional behavior. Recommendation: Consider adding a dedicated section explaining the dynamic defaults: ##### Dynamic Defaults with --max-rps
When `--max-rps` is specified, the following flags will have dynamic defaults if not explicitly set:
| Flag | Default When --max-rps is Set | Regular Default |
|------|------------------------------|-----------------|
| `--max-tuples-per-write` | 40 | 1 |
| `--max-parallel-requests` | max-rps/5 (min: 1) | 4 |
| `--rampup-period-in-sec` | max-rps*2 | N/A |7. Consider Validation OrderThe current validation happens after the conditional defaults are set. Consider if this order is optimal. 🚀 Suggested RefactoringHere's how I'd structure the improved version: func writeTuplesFromFile(ctx context.Context, flags *flag.FlagSet, fgaClient *client.OpenFgaClient) error {
// Parse and validate individual flags first
params, err := parseTupleWriteFlags(flags)
if err != nil {
return err
}
// Apply dynamic defaults if needed
params = applyDynamicDefaults(params, flags)
// Validate final parameters
if err := validateTupleWriteParams(params); err != nil {
return err
}
// Continue with actual tuple writing logic...
}✅ What's Working Well
This is a solid feature that will improve user experience significantly! The suggestions above will make it more maintainable and robust. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
674-679: Use consistent list markers and punctuation in parameter bulletsSwitch unordered list markers to dashes for consistency (MD004) and add a comma after “omitted” in the
--max-rpsdescription for clarity.- * `--max-tuples-per-write`: Max tuples to send in a single write (optional, default=1, or 40 if `--max-rps` is set and this flag is omitted) + - `--max-tuples-per-write`: Max tuples to send in a single write (optional, default=1, or 40 if `--max-rps` is set and this flag is omitted) - * `--max-parallel-requests`: Max requests to send in parallel (optional, default=4, or `max-rps/5` if `--max-rps` is set and this flag is omitted) + - `--max-parallel-requests`: Max requests to send in parallel (optional, default=4, or `max-rps/5` if `--max-rps` is set and this flag is omitted) - * `--max-rps`: Max requests per second. When set, the CLI will ramp up requests from 1 RPS to the set value. If `--rampup-period-in-sec` is omitted it defaults to `max-rps*2`. + - `--max-rps`: Max requests per second. When set, the CLI will ramp up requests from 1 RPS to the set value. If `--rampup-period-in-sec` is omitted, it defaults to `max-rps*2`. - * `--rampup-period-in-sec`: Time in seconds to wait between each batch of tuples when ramping up. Only used if `--max-rps` is set. + - `--rampup-period-in-sec`: Time in seconds to wait between each batch of tuples when ramping up. Only used if `--max-rps` is set. - * All integer parameters must be greater than zero when provided. + - All integer parameters must be greater than zero when provided.🧰 Tools
🪛 LanguageTool
[uncategorized] ~677-~677: Possible missing comma found.
Context: ...t value. If--rampup-period-in-secis omitted it defaults tomax-rps*2. * `--rampup...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~678-~678: Loose punctuation mark.
Context: ...max-rps*2. *--rampup-period-in-sec: Time in seconds to wait between each ba...(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
674-674: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
675-675: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
676-676: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
677-677: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
678-678: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
679-679: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)cmd/tuple/write.go(4 hunks)internal/tuple/import.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/tuple/import.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/tuple/write.go
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~677-~677: Possible missing comma found.
Context: ...t value. If --rampup-period-in-sec is omitted it defaults to max-rps*2. * `--rampup...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~678-~678: Loose punctuation mark.
Context: ... max-rps*2. * --rampup-period-in-sec: Time in seconds to wait between each ba...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
README.md
674-674: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
675-675: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
676-676: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
677-677: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
678-678: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
679-679: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
🔇 Additional comments (1)
README.md (1)
685-685: Approve simplification of example usageThe updated example succinctly demonstrates the dynamic defaults when only
--max-rpsis specified, improving user clarity.
Summary
--max-rpsis providedTesting
go test ./...golangci-lint run ./...https://chatgpt.com/codex/tasks/task_e_684c496919a4832281aae1a0eeb79f73
Summary by CodeRabbit
fga tuple writecommand.--max-tuples-per-write,--max-parallel-requests,--max-rps, and--rampup-period-in-secto reflect new conditional defaults.