feat: add support for configurable write conflict options #595
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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. WalkthroughThis PR introduces configurable conflict-handling options for tuple write and delete operations via new CLI flags ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Layer
participant Parser as Options Parser
participant SDK as SDK Client
User->>CLI: fga tuple write --on-duplicate ignore
CLI->>Parser: Parse --on-duplicate flag
Parser->>Parser: Validate "ignore" value
Parser->>SDK: Create ClientWriteOptions<br/>with Conflict.OnDuplicateWrites=IGNORE
SDK->>SDK: Process write request<br/>with conflict options
SDK-->>CLI: Response
CLI-->>User: Success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
This PR adds configurable write conflict options for tuple operations, allowing users to control behavior when writing duplicate tuples or deleting missing tuples.
Key changes:
- Adds
--on-duplicateflag totuple writecommand withignoreorerroroptions - Adds
--on-missingflag totuple deletecommand withignoreorerroroptions - Replaces
interface{}withanythroughout the codebase for better readability
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tuple/conflictoptions.go | New file defining custom types for conflict handling options with validation |
| cmd/tuple/write.go | Adds --on-duplicate flag and passes conflict options through import functions |
| cmd/tuple/delete.go | Adds --on-missing flag and passes conflict options through import functions |
| internal/tuple/import.go | Updates import functions to accept and use ClientWriteOptions parameter |
| cmd/store/import.go | Updates import call to pass default conflict options |
| internal/storetest/testresult.go | Refactors string concatenation to use strings.Builder for performance |
| internal/slices/contains.go | Refactors to use standard library slices.Contains |
| internal/tuplefile/csv.go | Changes map[string]interface{} to map[string]any |
| internal/storetest/storedata.go | Changes map[string]interface{} to map[string]any |
| internal/cmdutils/get-query-context.go | Changes map[string]interface{} to map[string]any |
| cmd/query/*.go | Updates function signatures to use map[string]any |
| cmd/tuple/*_test.go | Updates test fixtures to use map[string]any |
| README.md | Documents the new flags and parameters |
| CHANGELOG.md | Documents the added features |
💡 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: 3
🧹 Nitpick comments (1)
internal/slices/contains.go (1)
22-23: Update the comment to reflect standard library usage.The comment references
golang.org/x/exp/slices, but the implementation now uses the standard libraryslicespackage (available since Go 1.21). Consider updating the comment to reflect this change or removing it entirely since the function now directly delegates to a standard library implementation.Apply this diff to update the comment:
// Contains returns whether a string is in a list of strings -// Similar to slices.Contains from golang.org/x/exp/slices -// https://cs.opensource.google/go/x/exp/+/515e97eb:slices/slices.go;l=116 func Contains(itemArray []string, lookingFor string) bool { return slices.Contains(itemArray, lookingFor) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
CHANGELOG.md(1 hunks)README.md(3 hunks)cmd/query/check.go(1 hunks)cmd/query/list-objects.go(1 hunks)cmd/query/list-relations.go(1 hunks)cmd/query/list-users.go(1 hunks)cmd/store/import.go(1 hunks)cmd/tuple/delete.go(4 hunks)cmd/tuple/read_test.go(1 hunks)cmd/tuple/write.go(6 hunks)cmd/tuple/write_test.go(7 hunks)internal/cmdutils/get-query-context.go(2 hunks)internal/slices/contains.go(1 hunks)internal/storetest/storedata.go(1 hunks)internal/storetest/testresult.go(6 hunks)internal/tuple/conflictoptions.go(1 hunks)internal/tuple/import.go(3 hunks)internal/tuplefile/csv.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/tuple/conflictoptions.go (1)
internal/clierrors/clierrors.go (1)
ErrInvalidFormat(27-27)
cmd/store/import.go (2)
internal/tuple/conflictoptions.go (1)
CLIENT_WRITE_REQUEST_ON_DUPLICATE_WRITES_IGNORE(15-15)internal/tuple/import.go (1)
ImportTuplesWithoutRampUp(85-90)
cmd/tuple/write.go (2)
internal/tuple/conflictoptions.go (3)
CLIENT_WRITE_REQUEST_ON_DUPLICATE_WRITES_ERROR(14-14)CLIENT_WRITE_REQUEST_ON_DUPLICATE_WRITES_IGNORE(15-15)ClientWriteRequestOnDuplicateWrites(11-11)internal/tuple/import.go (3)
RPSToParallelRequestsDivisor(34-34)ImportTuples(97-134)DefaultMinRPS(28-28)
cmd/tuple/delete.go (2)
internal/tuple/conflictoptions.go (3)
CLIENT_WRITE_REQUEST_ON_MISSING_DELETES_IGNORE(50-50)CLIENT_WRITE_REQUEST_ON_MISSING_DELETES_ERROR(49-49)ClientWriteRequestOnMissingDeletes(46-46)internal/tuple/import.go (3)
ImportTuplesWithoutRampUp(85-90)MaxTuplesPerWrite(22-22)MaxParallelRequests(25-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: CodeQL analysis (go)
- GitHub Check: Test Release Process
- GitHub Check: Tests
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
internal/slices/contains.go (1)
24-25: LGTM! Good refactor to use standard library.Replacing the custom loop implementation with
slices.Containsfrom the standard library improves maintainability and leverages well-tested code.internal/tuplefile/csv.go (1)
67-67: LGTM! Type alias migration aligns with modern Go conventions.The change from
map[string]interface{}tomap[string]anyis functionally equivalent and aligns with Go 1.18+ conventions. This improves readability without affecting behavior.internal/cmdutils/get-query-context.go (2)
26-38: LGTM! Type alias migration improves readability.The migration from
map[string]interface{}tomap[string]anyin both the return type and map initialization is functionally equivalent and improves code readability. This aligns with the broader codebase migration to use theanyalias.
41-44: LGTM! Consistent type alias usage.The return type update maintains consistency with the inner function changes.
cmd/query/list-users.go (1)
69-77: LGTM! Type alias migration maintains consistency.The parameter type change from
*map[string]interface{}to*map[string]anyis functionally equivalent and consistent with similar updates across other query command files.CHANGELOG.md (1)
10-14: LGTM! Clear documentation of new features.The CHANGELOG entry clearly documents the new configurable write conflict options with:
- The new flags (
--on-duplicateand--on-missing)- Available choices (
errororignore)- Default behaviors (ignore for file imports/deletes, error for single tuple operations)
This provides users with the necessary information to understand and use the new features.
internal/storetest/testresult.go (3)
126-155: LGTM! Excellent performance optimization using strings.Builder.Replacing repeated string concatenation with
strings.Buildersignificantly reduces memory allocations and improves performance, especially when processing many test results. The refactor maintains identical output while being more efficient.
165-194: LGTM! Consistent application of strings.Builder pattern.The same efficient pattern is applied to list objects results, maintaining consistency with the check results refactor.
204-236: LGTM! Complete strings.Builder migration.The list users results follow the same efficient pattern, completing the performance optimization across all three result types.
cmd/query/check.go (1)
38-46: LGTM! Type alias migration aligns with broader refactor.The parameter type change from
*map[string]interface{}to*map[string]anyis part of the consistent type alias migration across all query-related functions. The change is functionally equivalent and improves code readability.internal/storetest/storedata.go (3)
44-51: LGTM! Context field type updated to use modern alias.The migration of the
Contextfield from*map[string]interface{}to*map[string]anyaligns with the broader type alias migration across test data structures. The struct tag formatting adjustments maintain consistency.
53-58: LGTM! Consistent Context field type migration.The same type alias update applied to
ModelTestListObjects.Contextmaintains consistency across test structures.
60-65: LGTM! Complete Context field migration.The
ModelTestListUsers.Contextfield completes the consistent type alias migration across all test data structures.cmd/tuple/write.go (1)
186-186: Go version 1.24.0 supports themaxbuiltin—no changes required.The module declares Go 1.24.0 in go.mod, which is well above the Go 1.21 threshold where
maxbecame a predeclared identifier. The code at line 186 will compile without issues.
Description
Add support for configuring the write conflict options
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
Release Notes
New Features
--on-duplicateflag for tuple write operations to control duplicate handling (ignore or error).--on-missingflag for tuple delete operations to control missing tuple handling (ignore or error).Bug Fixes
Documentation