Repository navigation
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
| Layer / File(s) | Summary |
|---|---|
Format rendering internal/output/marshal.go, internal/output/marshal_test.go |
Render returns YAML or indented JSON bytes. Tests check the expected output for both formats. |
Tuple read file output cmd/tuple/read.go, cmd/tuple/read_test.go, README.md |
The command routes output to stdout or a file. File output handles CSV conversion or renders other formats, writes with 0600 permissions, and returns errors. Tests cover content, permissions, tuple keys, and write failures. The README documents the flag. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant TupleRead
participant DisplayReadResponse
participant FileWriter
participant OutputRender
participant OutputFile
TupleRead->>DisplayReadResponse: pass response, format, and output path
DisplayReadResponse->>FileWriter: write response to the selected path
FileWriter->>OutputRender: render non-CSV output
OutputRender-->>FileWriter: return formatted bytes
FileWriter->>OutputFile: write bytes with 0600 permissions
Suggested reviewers: soulpancake
Merge Risk: 
1042d
-
This change adds an optional --output-file flag to tuple read and leaves stdout behavior unchanged when the flag is absent. No concrete merge-blocking risk remains.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 4 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: adding the --output-file option to tuple read. Linked Issues check Issue #569asks for an --output-file option on fga tuple read that writes tuples to a file instead of stdout. At head, cmd/tuple/read.go registers the flag (readCmd.Flags().String("output-file", "", .…Out of Scope Changes check All changed areas support the --output-file feature. cmd/tuple/read.go adds the flag and the file-write path, and extracts displayReadResponse for the shared stdout and file logic. internal/output/mar… Full details: Linked Issues check
-
Explanation
Issue
#569asks for an --output-file option on fga tuple read that writes tuples to a file instead of stdout. At head, cmd/tuple/read.go registers the flag (readCmd.Flags().String("output-file", "", ...)). The read command passes outputFile to displayReadResponse. When outputFile is non-empty, displayReadResponse calls writeReadResponseToFile. That function builds the content in the selected format (CSV via MarshalCSV, other formats via output.Render) and writes it with os.WriteFile(..., 0o600) only after rendering succeeds. When the flag is empty, the stdout path keeps the previous CSV and UniPrinter behavior. internal/output/marshal.go (read at head) defines Render, which returns JSON or YAML bytes without colors and ends with a newline. The issue sets no further acceptance criteria. The PR summary reports tests for file output in all four formats, permissions, and write errors. I did not read the test files directly. The marshal.go diff was unavailable, so I relied on the file read at head for Render. The conclusion rests on the read.go diff and the head file content.
Full details: Out of Scope Changes check
-
Explanation
All changed areas support the --output-file feature. cmd/tuple/read.go adds the flag and the file-write path, and extracts displayReadResponse for the shared stdout and file logic. internal/output/marshal.go adds Render, which the file path uses for JSON and YAML output. README.md documents the new flag. The test files cover the new file-output behavior and Render. The stdout path keeps its previous behavior. The diff shows no unrelated refactoring or behavior change. The PR description notes an unrelated Windows failure in TestParseTuplesFileData. That test does not appear in the summarized changes, so it does not count as an out-of-scope change here.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 4 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
ℹ️ Recent review info
-
⚙️ Run configuration
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
b82ba097-ae9b-4753-b832-782876d64db6
📒 Files selected for processing: 5
-
README.mdcmd/tuple/read.gocmd/tuple/read_test.gointernal/output/marshal.gointernal/output/marshal_test.go
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
ℹ️ Recent review info
-
⚙️ Run configuration
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
b82ba097-ae9b-4753-b832-782876d64db6
📒 Files selected for processing: 5
-
README.mdcmd/tuple/read.gocmd/tuple/read_test.gointernal/output/marshal.gointernal/output/marshal_test.go
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
-
Fixes openfga#569 Signed-off-by: Srinivas Chippagiri <cvas22@gmail.com>
1042d02 to
e02b91d
Compare
Siddhant-K-code
left a comment
Siddhant-K-code
left a comment
There was a problem hiding this comment.
Checked against a local OpenFGA: stdout is unchanged from main for all four formats (TTY colors included), the file matches NO_COLOR=1 stdout (YAML without the extra trailing blank line), new files get 0600, and a failed read leaves an existing file untouched. Suggestions below are non-blocking.
This conflicts with #791 in cmd/tuple/read.go and bypasses its --jq filter, so whichever lands second should reject --jq with --output-file up front.
| // displayReadResponse prints the response to stdout, or writes it to outputFile when that is set. | ||
| func displayReadResponse(response *readResponse, outputFormat string, simpleOutput bool, outputFile string) error { | ||
| if outputFile != "" { | ||
| return writeReadResponseToFile(response, outputFormat, simpleOutput, outputFile) | ||
| } | ||
|
|
||
| if outputFormat == "csv" { | ||
| records, err := response.toCsvDTO() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to convert response to csv: %w", err) | ||
| } | ||
|
|
||
| writer := bufio.NewWriter(os.Stdout) | ||
| if err := output.MarshalCSV(records, writer, readResponseCSVHeaders...); err != nil { | ||
| return fmt.Errorf("failed to marshal csv: %w", err) | ||
| } | ||
|
|
||
| if err := writer.Flush(); err != nil { | ||
| return fmt.Errorf("failed to display csv: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| dataPrinter := output.NewUniPrinter(outputFormat) | ||
|
|
||
| var data any | ||
|
|
||
| data = *response.complete | ||
|
|
||
| if simpleOutput || outputFormat == "simple-json" { | ||
| data = response.simple | ||
| } | ||
|
|
||
| return dataPrinter.Display(data) | ||
| } | ||
|
|
||
| // writeReadResponseToFile writes the response, in the selected output format, only to the given file. | ||
| // The file is created with 0600 permissions and is only touched once the output has been produced. | ||
| func writeReadResponseToFile(response *readResponse, outputFormat string, simpleOutput bool, outputFile string) error { | ||
| var content bytes.Buffer | ||
|
|
||
| if outputFormat == "csv" { | ||
| records, err := response.toCsvDTO() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to convert response to csv: %w", err) | ||
| } | ||
|
|
||
| if err := output.MarshalCSV(records, &content, readResponseCSVHeaders...); err != nil { | ||
| return fmt.Errorf("failed to marshal csv: %w", err) | ||
| } | ||
| } else { | ||
| var data any | ||
|
|
||
| data = *response.complete | ||
|
|
||
| if simpleOutput || outputFormat == "simple-json" { | ||
| data = response.simple | ||
| } | ||
|
|
||
| rendered, err := output.Render(outputFormat, data) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to render output: %w", err) | ||
| } | ||
|
|
||
| content.Write(rendered) | ||
| } | ||
|
|
||
| if err := os.WriteFile(outputFile, content.Bytes(), 0o600); err != nil { | ||
| return fmt.Errorf("failed to write output file %q: %w", outputFile, err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
writeReadResponseToFile repeats the CSV conversion and the simple/complete selection. Only colored JSON/YAML to stdout needs UniPrinter; everything else can go through one buffer. This also gives #791 a single place to hook in. Drop the bufio import with it. Verified: stdout and file bytes match this PR for all formats, tests and lint pass.
| // displayReadResponse prints the response to stdout, or writes it to outputFile when that is set. | |
| func displayReadResponse(response *readResponse, outputFormat string, simpleOutput bool, outputFile string) error { | |
| if outputFile != "" { | |
| return writeReadResponseToFile(response, outputFormat, simpleOutput, outputFile) | |
| } | |
| if outputFormat == "csv" { | |
| records, err := response.toCsvDTO() | |
| if err != nil { | |
| return fmt.Errorf("failed to convert response to csv: %w", err) | |
| } | |
| writer := bufio.NewWriter(os.Stdout) | |
| if err := output.MarshalCSV(records, writer, readResponseCSVHeaders...); err != nil { | |
| return fmt.Errorf("failed to marshal csv: %w", err) | |
| } | |
| if err := writer.Flush(); err != nil { | |
| return fmt.Errorf("failed to display csv: %w", err) | |
| } | |
| return nil | |
| } | |
| dataPrinter := output.NewUniPrinter(outputFormat) | |
| var data any | |
| data = *response.complete | |
| if simpleOutput || outputFormat == "simple-json" { | |
| data = response.simple | |
| } | |
| return dataPrinter.Display(data) | |
| } | |
| // writeReadResponseToFile writes the response, in the selected output format, only to the given file. | |
| // The file is created with 0600 permissions and is only touched once the output has been produced. | |
| func writeReadResponseToFile(response *readResponse, outputFormat string, simpleOutput bool, outputFile string) error { | |
| var content bytes.Buffer | |
| if outputFormat == "csv" { | |
| records, err := response.toCsvDTO() | |
| if err != nil { | |
| return fmt.Errorf("failed to convert response to csv: %w", err) | |
| } | |
| if err := output.MarshalCSV(records, &content, readResponseCSVHeaders...); err != nil { | |
| return fmt.Errorf("failed to marshal csv: %w", err) | |
| } | |
| } else { | |
| var data any | |
| data = *response.complete | |
| if simpleOutput || outputFormat == "simple-json" { | |
| data = response.simple | |
| } | |
| rendered, err := output.Render(outputFormat, data) | |
| if err != nil { | |
| return fmt.Errorf("failed to render output: %w", err) | |
| } | |
| content.Write(rendered) | |
| } | |
| if err := os.WriteFile(outputFile, content.Bytes(), 0o600); err != nil { | |
| return fmt.Errorf("failed to write output file %q: %w", outputFile, err) | |
| } | |
| return nil | |
| } | |
| // displayReadResponse prints the response to stdout, or writes it to outputFile when that is set. | |
| func displayReadResponse(response *readResponse, outputFormat string, simpleOutput bool, outputFile string) error { | |
| var data any = *response.complete | |
| if simpleOutput || outputFormat == "simple-json" { | |
| data = response.simple | |
| } | |
| if outputFile == "" && outputFormat != "csv" { | |
| return output.NewUniPrinter(outputFormat).Display(data) | |
| } | |
| var content bytes.Buffer | |
| if outputFormat == "csv" { | |
| records, err := response.toCsvDTO() | |
| if err != nil { | |
| return fmt.Errorf("failed to convert response to csv: %w", err) | |
| } | |
| if err := output.MarshalCSV(records, &content, readResponseCSVHeaders...); err != nil { | |
| return fmt.Errorf("failed to marshal csv: %w", err) | |
| } | |
| } else { | |
| rendered, err := output.Render(outputFormat, data) | |
| if err != nil { | |
| return fmt.Errorf("failed to render output: %w", err) | |
| } | |
| content.Write(rendered) | |
| } | |
| if outputFile == "" { | |
| if _, err := os.Stdout.Write(content.Bytes()); err != nil { | |
| return fmt.Errorf("failed to display csv: %w", err) | |
| } | |
| return nil | |
| } | |
| if err := os.WriteFile(outputFile, content.Bytes(), 0o600); err != nil { | |
| return fmt.Errorf("failed to write output file %q: %w", outputFile, err) | |
| } | |
| return nil | |
| } |
| readCmd.Flags().String("output-file", "", "Write the output to this file instead of stdout. "+ | ||
| "The file is created with 0600 permissions. The selected output format is used.") |
There was a problem hiding this comment.
0600 only applies to new files. An existing file is truncated and keeps its mode (a 0644 file stays 0644), with no overwrite prompt unlike store export. Worth saying so here and in README.md:991.
| readCmd.Flags().String("output-file", "", "Write the output to this file instead of stdout. "+ | |
| "The file is created with 0600 permissions. The selected output format is used.") | |
| readCmd.Flags().String("output-file", "", "Write the output to this file instead of stdout. "+ | |
| "Uses --output-format. Overwrites an existing file; new files are created with 0600 permissions.") |
| var ( | ||
| result []byte | ||
| err error | ||
| ) | ||
|
|
||
| if outputFormat == "yaml" { | ||
| result, err = yaml.Marshal(data) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to marshal yaml with error %w", err) | ||
| } | ||
|
|
||
| // yaml.Marshal already ends with a newline. | ||
| return result, nil | ||
| } else { | ||
| result, err = json.MarshalIndent(data, "", " ") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to marshal json with error %w", err) | ||
| } | ||
| } | ||
|
|
||
| return append(result, '\n'), nil |
There was a problem hiding this comment.
Drop the else after return and the pre-declared vars.
| var ( | |
| result []byte | |
| err error | |
| ) | |
| if outputFormat == "yaml" { | |
| result, err = yaml.Marshal(data) | |
| if err != nil { | |
| return nil, fmt.Errorf("unable to marshal yaml with error %w", err) | |
| } | |
| // yaml.Marshal already ends with a newline. | |
| return result, nil | |
| } else { | |
| result, err = json.MarshalIndent(data, "", " ") | |
| if err != nil { | |
| return nil, fmt.Errorf("unable to marshal json with error %w", err) | |
| } | |
| } | |
| return append(result, '\n'), nil | |
| if outputFormat == "yaml" { | |
| result, err := yaml.Marshal(data) | |
| if err != nil { | |
| return nil, fmt.Errorf("unable to marshal yaml with error %w", err) | |
| } | |
| // yaml.Marshal already ends with a newline. | |
| return result, nil | |
| } | |
| result, err := json.MarshalIndent(data, "", " ") | |
| if err != nil { | |
| return nil, fmt.Errorf("unable to marshal json with error %w", err) | |
| } | |
| return append(result, '\n'), nil |
Fixes #569
Adds an optional
--output-fileflag tofga tuple read, following the proposal in the issue.--output-format(json,simple-json,yamlorcsv).0600permissions, and only after the output has been produced, so a conversion error does not leave a partial file. An error is returned if the file cannot be written.This adds
output.Render(JSON/YAML without colors, returning bytes) and splits the display code incmd/tuple/read.gointodisplayReadResponseandwriteReadResponseToFile. The README documents the new flag.Tests: file output for json, simple-json, yaml and csv (content and permissions), that the simple-json file parses back into tuple keys, the write error case, and
Render. I rango test ./cmd/tuple -run "DisplayReadResponse|TestRead"andgo test ./internal/output. A different test incmd/tuple(TestParseTuplesFileData) fails on Windows because it compares an OS error message; it is unrelated to this change.Summary by CodeRabbit
--output-fileoption totuple readto save results in the selected format instead of printing them to standard output. Output files are created with restricted permissions.