Conversation
|
I tested this locally and it works as expected |
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to emit failed tuples to stderr in the same format as the input, and updates related fixtures, documentation, and the changelog.
- Writes failed tuples to stderr when any write operations fail
- Introduces
failed_output.gowith CSV/YAML/JSON formatting helpers - Updates test fixtures, README usage examples, and CHANGELOG
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/fixtures/basic-tuples.yaml | Added basic YAML fixture for tuple inputs |
| tests/fixtures/basic-tuples.csv | Added basic CSV fixture for tuple inputs |
| cmd/tuple/write.go | Prints failed tuples to stderr after formatting |
| cmd/tuple/failed_output.go | New utilities for formatting tuples in CSV/YAML/JSON |
| README.md | Documents stderr output of failed tuples |
| CHANGELOG.md | Notes the new stderr feature under “Added” |
| a.fga.yml | Sample config for tests, including tuple definitions |
Comments suppressed due to low confidence (1)
cmd/tuple/write.go:208
- Add a unit test to capture stderr output and verify failed tuples are emitted correctly in each supported format.
fmt.Fprint(os.Stderr, out)
a.fga.yml
Outdated
| define owner: [user] | ||
| define parent: [folder] | ||
| define can_view: owner or can_view from parent | ||
| define can_write: owner or can_write from parentdi |
There was a problem hiding this comment.
Typo in 'parentdi'; it should be 'parent'.
| define can_write: owner or can_write from parentdi | |
| define can_write: owner or can_write from parent |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes introduce functionality to output failed tuples to Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant FileSystem
User->>CLI: Run 'fga tuple write --file tuples.csv 2> failed_tuples.csv > result.json'
CLI->>FileSystem: Read tuples.csv
CLI->>CLI: Attempt to write tuples
CLI->>CLI: Collect failed tuples (if any)
CLI->>CLI: Format failed tuples in input file format
CLI->>FileSystem: Write failed tuples to stderr (failed_tuples.csv)
CLI->>FileSystem: Write results to stdout (result.json)
Assessment against linked issues
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
tests/fixtures/basic-tuples.yaml (1)
1-3: YAML style + missing newline
- Keys do not need to be quoted and quoting them reduces readability – the parser accepts it but it is unconventional.
- The file lacks a trailing newline, flagged by yamllint.
-- "user": "user:anne" - "relation": "owner" - "object": "group:foo" +- user: user:anne + relation: owner + object: group:foo + # ← newline (editor will usually add)🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 3-3: no new line character at the end of file
(new-line-at-end-of-file)
cmd/tuple/write.go (1)
195-210: Print failed-tuple block with a newline and cleaner error variable
fmt.Fprint(os.Stderr, out)may merge with subsequent shell prompts or logs.errFmt2shadows the outererrand the trailing numeral is noisy.- out, errFmt2 := formatTuples(failedTuples, failedFormat) - if errFmt2 != nil { - return fmt.Errorf("failed to marshal failed tuples: %w", errFmt2) - } - - fmt.Fprint(os.Stderr, out) + out, ferr := formatTuples(failedTuples, failedFormat) + if ferr != nil { + return fmt.Errorf("failed to marshal failed tuples: %w", ferr) + } + + // always end with a newline so the shell prompt starts on a new line + if !strings.HasSuffix(out, "\n") { + out += "\n" + } + fmt.Fprint(os.Stderr, out)cmd/tuple/failed_output.go (1)
99-118: CSV output may lack trailing newlineMany tools expect the file to end with a newline;
gocsv.MarshalBytesomits it when there are zero tuples.
Add one to be safe:b, err := gocsv.MarshalBytes(dto) if len(b) > 0 && b[len(b)-1] != '\n' { b = append(b, '\n') }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/fixtures/basic-tuples.csvis excluded by!**/*.csv
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)README.md(1 hunks)cmd/tuple/failed_output.go(1 hunks)cmd/tuple/write.go(2 hunks)tests/fixtures/basic-tuples.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
tests/fixtures/basic-tuples.yaml
[error] 3-3: no new line character at the end of file
(new-line-at-end-of-file)
🪛 LanguageTool
README.md
[typographical] ~788-~788: It appears that a comma is missing.
Context: ...": 1, "total_count": 2 } ``` In some cases you may want to retry failed tuples (fo...
(DURING_THAT_TIME_COMMA)
🔇 Additional comments (3)
cmd/tuple/failed_output.go (1)
88-97:formatFromExtensionmisses.ymlin default branchSmall: the switch already lower-cases the extension. Returning
jsonfor an unrecognised extension is fine, but consider an explicit.ymlcase to avoid surprises.- case ".yaml", ".yml": + case ".yaml", ".yml": return yamlFormatCHANGELOG.md (1)
5-7: Changelog entry LGTMClear and concise.
README.md (1)
788-795: Great usability improvementThe updated example clearly shows stderr redirection; docs align with the new behaviour.
🧰 Tools
🪛 LanguageTool
[typographical] ~788-~788: It appears that a comma is missing.
Context: ...": 1, "total_count": 2 } ``` In some cases you may want to retry failed tuples (fo...(DURING_THAT_TIME_COMMA)
| for _, tupleKey := range tuples { | ||
| userParts := strings.SplitN(tupleKey.User, ":", 2) | ||
| if len(userParts) != 2 { | ||
| continue | ||
| } | ||
|
|
||
| userType := userParts[0] | ||
| userIDRel := userParts[1] | ||
| userID := userIDRel | ||
| userRel := "" | ||
|
|
||
| if strings.Contains(userIDRel, "#") { | ||
| parts := strings.SplitN(userIDRel, "#", 2) | ||
| userID = parts[0] | ||
| userRel = parts[1] | ||
| } | ||
|
|
||
| objParts := strings.SplitN(tupleKey.Object, ":", 2) | ||
| if len(objParts) != 2 { | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Dropping malformed tuples silently can hide the very tuples a user needs to retry
If user or object don’t follow type:id[#relation] the write failed, yet we skip the tuple completely when preparing the CSV.
A user then loses visibility of why the write failed.
Instead, include the raw tuple in the CSV with blank split fields or fall back to JSON formatting and return an explicit error to the caller.
- if len(userParts) != 2 {
- continue
- }
+ if len(userParts) != 2 {
+ // keep the tuple – populate only the fields we can parse
+ result = append(result, tupleCSVDTO{
+ UserType: "UNKNOWN",
+ UserID: tupleKey.User,
+ Relation: tupleKey.Relation,
+ ObjectType: "UNKNOWN",
+ ObjectID: tupleKey.Object,
+ })
+ continue
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/tuple/failed_output.go around lines 35 to 56, the code currently skips
tuples with malformed user or object fields without any indication, causing loss
of visibility into why writes failed. Modify the logic to include these
malformed tuples in the CSV output by either leaving the split fields blank or
falling back to JSON formatting for those entries. Additionally, ensure an
explicit error is returned to the caller to indicate the presence of malformed
tuples, so users can understand and retry appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
18-19: Inconsistent list marker style in CHANGELOG
The new bullet uses-while earlier sections mix*and-. To satisfy markdownlint (MD004) and maintain consistency, switch to a single marker—preferably*for all unordered lists.Proposed diff:
- Added: - - Failed tuples are now written to `stderr` in the format of the input file (#449) + Added: + * Failed tuples are now written to `stderr` in the format of the input file (#449)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
19-19: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Release Process
- GitHub Check: Tests
Siddhant-K-code
left a comment
Siddhant-K-code
left a comment
There was a problem hiding this comment.
Looks ok, but i have couple of thoughts:
- I am unsure whether we should have this failed tuples behind a configuration option, i.e., there should be an option to disable to by using configuration option in CLI.
- a following comment regarding continuing the process even if storing to stderr failes.
|
TLDR: I will approve this as it's good for now. Normally STDERR is be reserved for application errors (crash, OOM, etc..) not service errors. But I will create an issue for us to ideate on some improvements. What I would like to see:
|
Siddhant-K-code
left a comment
Siddhant-K-code
left a comment
There was a problem hiding this comment.
LGTM for now, will do the followups!!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
789-796: Minor wording tweak for readabilityAdd commas (or an em-dash) around the “for example” parenthetical to avoid the run-on feel and satisfy the linter.
-In some cases you may want to retry failed tuples (for example, due to network issues). Failed tuples are now written to `stderr` in the same format as the input file and can be redirected to another file: +In some cases, you may want to retry failed tuples—for example, due to network issues. Failed tuples are now written to `stderr` in the same format as the input file and can be redirected to another file:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)README.md(1 hunks)cmd/tuple/write.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/tuple/write.go
- CHANGELOG.md
🧰 Additional context used
🪛 LanguageTool
README.md
[typographical] ~789-~789: It appears that a comma is missing.
Context: ...": 1, "total_count": 2 } ``` In some cases you may want to retry failed tuples (fo...
(DURING_THAT_TIME_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Release Process
|
Let's hold this one from merging, we have another idea |
4 participants
Summary
Testing
make formatmake lintmake test(fails:dockernot found)#Fixes #449
https://chatgpt.com/codex/tasks/task_e_6842099b1c9c832282621dce3b0fd738
Summary by CodeRabbit
New Features
Documentation
Tests