Repository navigation
Add CSV column formatting and atomic output publication - #115
Conversation
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (26)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds validated per-column CSV formatting and staged publication for CSV, Parquet, and MMDB outputs. It also adds platform-specific publication handling, error propagation, City CSV parity tests, and reproducible fixtures. ChangesCSV formatting and output publication
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Run
participant RowWriter
participant PendingOutput
Run->>PendingOutput: create staged output
Run->>RowWriter: convert records
RowWriter->>PendingOutput: write formatted data
Run->>RowWriter: flush
Run->>PendingOutput: commit output
Merge Risk: ⚪ Minimal · up to The output publication behavior meets its documented guarantees, with no outstanding actionable merge risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 17.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 14 files. (11 skipped: 11 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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. A rabbit reads each line, Comment |
bed4d29 to
2af3365
Compare
2ed86bc to
32a851e
Compare
Preserve existing CSV, Parquet, and MMDB outputs until conversion and all writer flushes succeed. Use renameio for atomic per-file replacement on Unix and staged replacement on Windows. Split files publish separately. Require regular-file destinations or new paths, rejecting symlinks and special files. Preserve existing Unix rwx permissions, and keep handles to staged files so setup failures can clean them up. Report sync, close, publication, and cleanup errors. Change the internal NewMMDBWriter constructor to accept an io.Writer instead of a path, so conversion controls staging for every output format. Cover publication, failure preservation, split ordering, permissions, symlinks, and cleanup errors in tests.
1f63377 to
56b0721
Compare
Support fixed decimal precision for both MMDB float types and explicit true/false labels, including empty labels. Validate formatter options at configuration loading and reject them for Parquet and MMDB output. Check data-column names before other configuration validation so format errors can identify their columns. Apply formatting during CSV serialization after filtering and merging. Preserve existing defaults and missing-value behavior, and report columns whose non-null values have incompatible types. Test prefix, range, and bucket writers, rounding, labels, and escaping. Verify end to end that distinct values rendering identically do not merge and a present false value with an empty label survives filtering.
Pair the public City CSV sample with its historical MMDB input for offline regression tests. Record provenance, checksums, and reproduction commands alongside the reference CSVs and expected converter output. Assert exact output bytes and coordinate parity on matching networks, and constrain the known GeoNames fallback, missing-boolean, and omitted-row differences.
Reject identical configured IPv4 and IPv6 paths during configuration validation. Before staging outputs, compare directory and file identities to catch aliases and reject filenames differing only in case within the same directory. Check again before publishing the second file for aliases revealed by publishing the first. Preserve configured paths in diagnostics and resolve symlink/.. traversal without requiring Windows path canonicalization.
Use upstream revision 8f49d9bce747 to satisfy the dependency-freshness check. Remove the redundant inserter.Replace option; the default inserter provides the same replacement behavior.
56b0721 to
83488a2
Compare
horgh
left a comment
horgh
left a comment
There was a problem hiding this comment.
Looks good. Some Claude comments!!!!
| } | ||
|
|
||
| func (f *pendingFile) Commit() error { | ||
| if err := f.CloseAtomicallyReplace(); err != nil { |
There was a problem hiding this comment.
Replacing the output by rename fails when the output file is a bind mount. An example is a single file mounted into a Docker or Kubernetes container.
With docker run -v /host/out.csv:/data/out.csv and file = "/data/out.csv", the merge completes, then rename(temp, /data/out.csv) returns EBUSY. The error comes only after the full conversion. Before this change, os.Create truncated the file and wrote it in place.
A check at prepare time could fail early or fall back to an in-place write. For example, compare the st_dev of the file with the st_dev of its parent directory.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
I am leaving this as a limitation for now. An in-place fallback would give up preservation of the previous output, and mount-specific detection adds complexity for an uncommon setup. Mounting the output directory allows replacement.
🤖 Comment by Codex on behalf of Greg.
| secondPath string | ||
| } | ||
|
|
||
| func (f *splitPendingOutput) Commit() error { |
There was a problem hiding this comment.
When the alias between the split outputs shows only after the first file is published, Run returns an error but leaves the new IPv4 file in place.
Example on macOS: ipv4_file = "café.csv" (NFC) and ipv6_file = "café.csv" (NFD), and neither file exists. The checks before staging pass. The IPv4 file is published, then the check on the second file reports "refer to the same file". The user gets a failed run and a new file.
This case can occur only when both paths were absent. Thus, removing the first file that was just published restores the earlier state.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
I am keeping the published IPv4 file. Split outputs are replaced separately, and the docs already allow one file to be updated when the other fails. The late-alias tests cover this behavior; I do not think this case warrants separate rollback logic.
🤖 Comment by Codex on behalf of Greg.
| } | ||
| } | ||
| mode := os.FileMode(0o666) | ||
| preserveMode := info != nil && info.Mode().IsRegular() |
There was a problem hiding this comment.
An existing write-protected output (for example, mode 0444) is now replaced without an error. Before this change, the run failed with "permission denied" from os.Create.
Now the temp file is created and renamed over the output, and the new content keeps mode 0444. I confirmed this with a probe test. The CHANGELOG says that replacement needs write access to the directory. It does not say that write protection on the file no longer applies. Is this change intended? If yes, I suggest that the CHANGELOG says so.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Yes, this is intended with replacement. b60f6a7 adds the changelog clarification and a test that replaces a read-only file while preserving its permission bits.
🤖 Comment by Codex on behalf of Greg.
| // Restore permissions ourselves: WithExistingPermissions can leak the | ||
| // temporary file if chmod fails before it returns a handle. | ||
| file, err := renameio.NewPendingFile(path, | ||
| renameio.WithTempDir(dir), |
There was a problem hiding this comment.
The temp file name is . + basename + up to 19 random digits. Thus, basenames of about 236 to 255 bytes now fail with ENAMETOOLONG. These names worked before.
With file = "<250 chars>.csv", prepare fails with creating pending output ...: file name too long. I confirmed this with a probe test. On Windows, the pattern "."+base+"-*" has the same problem. A shorter temp name (for example, a truncated basename or a fixed prefix) prevents this.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Fixed in 3a73ec5. Temporary files now use a short random name independent of the destination filename. Added a regression test with a long destination filename.
🤖 Comment by Codex on behalf of Greg.
|
|
||
| // Restore permissions ourselves: WithExistingPermissions can leak the | ||
| // temporary file if chmod fails before it returns a handle. | ||
| file, err := renameio.NewPendingFile(path, |
There was a problem hiding this comment.
An interrupted run (Ctrl-C or SIGTERM) now leaves a hidden temp file of full size in the output directory. The CLI does not handle signals, so the deferred cleanup does not run.
If a cron job or a user stops a large Parquet build partway several times, each run leaves another .out.parquet<random> file. These files can be gigabytes, and nothing removes them. Before this change, the next run overwrote the same file.
The docs mention only "abrupt termination", but SIGINT is a normal way to stop a run. Consider signal.NotifyContext in main so that cleanup runs, or document this behavior.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
| return fmt.Errorf("decoding format options: %w", err) | ||
| } | ||
| for _, col := range raw.Columns { | ||
| for key := range col.Format { |
There was a problem hiding this comment.
validateFormatKeys iterates over a Go map. Thus, when there is more than one unknown key, the key named in the error changes from run to run.
format = { width = 8, type = "x" } reports unknown format option "width" on one run and "type" on the next. The message is not stable for users or for exact-match tests. Sort the keys, or report all of them.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Fixed in 3b4c4a6. Format keys are checked alphabetically. A regression test repeatedly loads a configuration with multiple unknown options and checks that the error stays the same.
🤖 Comment by Codex on behalf of Greg.
|
|
||
| require ( | ||
| github.com/maxmind/mmdbwriter/v2 v2.0.0-20260922152428-3aebce28ead6 | ||
| github.com/google/renameio/v2 v2.0.0 |
There was a problem hiding this comment.
The new renameio dependency is pinned at v2.0.0 (2021). v2.0.2 (January 2026) exists. This branch also updates mmdbwriter only to keep dependencies current, so the new dependency will probably be flagged soon after merge.
v2.0.2 also adds WithRoot (os.Root), which could replace the manual .. and EvalSymlinks handling.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Removed the renameio dependency in 3a73ec5 as part of sharing the staging implementation across platforms.
🤖 Comment by Codex on behalf of Greg.
| // containing "..", where cleaning before following symlinks changes meaning. | ||
| if slices.Contains(strings.Split(dir, string(filepath.Separator)), "..") { | ||
| var err error | ||
| dir, err = filepath.EvalSymlinks(dir) |
There was a problem hiding this comment.
The Unix code works around renameio: it calls EvalSymlinks when the directory contains .., and it has an injected hook to restore permissions. output_windows.go already implements the same pending-file state machine with os.CreateTemp, which does not clean the path.
Thus, there are two implementations of the same commit and cleanup logic, one for each platform. There are also special cases that exist only because filepath.Join in renameio cleans the path.
One shared implementation (os.CreateTemp, then Chmod on Unix, then Sync, Close, and Rename) removes the .. special case, the restorePermissions test seam, and the new dependency.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Done in 3a73ec5. Both platforms now share staging, commit, and cleanup. Temporary files use a short random name with exclusive os.OpenFile creation to preserve Unix umask behavior. This removes renameio, the path-resolution workaround, and the permission-test hook.
🤖 Comment by Codex on behalf of Greg.
| // Check names first so format errors can identify their columns. | ||
| for _, col := range config.Columns { | ||
| if col.Name == "" { | ||
| return nil, errors.New("invalid configuration: column name is required") |
There was a problem hiding this comment.
The column-name check moved out of validate() into LoadConfig, with a hand-copied invalid configuration: prefix. It seems the only reason is so that the validateFormatKeys messages can name columns.
Now the prefix text exists in two places, and validate() does not enforce column names by itself. A future direct caller of validate() will skip the check, and a change to the prefix can make the messages inconsistent. If the unknown-key check runs inside validate(), there is one validation path.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Moved both checks into validate in b3c8342. Column names are still checked before format keys, and LoadConfig adds the invalid configuration: prefix in one place. Updated the tests to cover both the validation entry point and error wording.
🤖 Comment by Codex on behalf of Greg.
| } | ||
| } | ||
| mode := os.FileMode(0o666) | ||
| preserveMode := info != nil && info.Mode().IsRegular() |
There was a problem hiding this comment.
Nit: info.Mode().IsRegular() is redundant, because inspectOutput returns a non-nil FileInfo only for regular files. The extra condition suggests that a non-regular info can get to this point. preserveMode := info != nil does the same job.
🤖 Comment by Claude (Claude Code) on behalf of Will.
There was a problem hiding this comment.
Removed in 3a73ec5. Permission preservation now checks only whether info is non-nil.
🤖 Comment by Codex on behalf of Greg.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Cross-platform filesystem replacement and cleanup semantics warrant final human review despite the comprehensive tests.
Review effort: Balanced
Findings: None
What changed in this PR
Adds configurable CSV formatting and staged output publication across CSV, Parquet, and MMDB exports.
Changes:
- Adds fixed-precision float and custom boolean CSV formatting.
- Preserves existing outputs until conversion and flushing succeed.
- Adds extensive cross-platform publication and City CSV regression coverage.
| File | Description |
|---|---|
CHANGELOG.md |
Records formatting and publication changes. |
docs/config.md |
Documents formats and replacement behavior. |
format_integration_test.go |
Tests formatting through full conversions. |
geoip_csv_test.go |
Verifies City CSV fixture parity. |
go.mod |
Adds renameio and updates mmdbwriter. |
go.sum |
Updates dependency checksums. |
internal/config/config.go |
Integrates format and split-path validation. |
internal/config/config_test.go |
Tests split output paths. |
internal/config/format.go |
Defines and validates column formats. |
internal/config/format_test.go |
Covers format configuration validation. |
internal/writer/csv.go |
Applies per-column CSV formatting. |
internal/writer/format.go |
Implements float and boolean formatting. |
internal/writer/format_test.go |
Tests formatting and type errors. |
internal/writer/integration_test.go |
Adapts MMDB integration tests to writers. |
internal/writer/mmdb.go |
Serializes MMDB output to an io.Writer. |
internal/writer/mmdb_test.go |
Tests in-memory MMDB serialization. |
mmdbconvert.go |
Coordinates conversion, commit, and cleanup. |
output.go |
Implements shared staged-output handling. |
output_test.go |
Covers publication, failures, and aliases. |
output_unix.go |
Implements atomic Unix replacement. |
output_unix_test.go |
Tests Unix permissions and path handling. |
output_windows.go |
Implements staged Windows replacement. |
output_windows_test.go |
Tests Windows path aliases. |
testdata/geoip-csv/.gitattributes |
Preserves fixture bytes. |
testdata/geoip-csv/GeoIP2-City-Test.mmdb |
Pins the City MMDB fixture. |
testdata/geoip-csv/README.md |
Documents fixture provenance and reproduction. |
testdata/geoip-csv/city.toml |
Configures City CSV regression output. |
testdata/geoip-csv/expected/GeoIP2-City-Blocks-IPv4.csv |
Pins expected IPv4 output. |
testdata/geoip-csv/expected/GeoIP2-City-Blocks-IPv6.csv |
Pins expected IPv6 output. |
testdata/geoip-csv/reference/GeoIP2-City-Blocks-IPv4.csv |
Adds upstream IPv4 reference data. |
testdata/geoip-csv/reference/GeoIP2-City-Blocks-IPv6.csv |
Adds upstream IPv6 reference data. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Use one exclusive-create, sync, close, rename, and cleanup implementation across platforms. Preserve Unix permissions and umask behavior while removing renameio and its path-cleaning workaround. Keep temporary names independent of the destination basename so long output filenames remain usable. Retain existing publication and cleanup tests and add a long-filename regression.
Treat a bare drive prefix as its current directory before appending the temporary filename. Cover staging and publication through a drive-relative output path.
Validate column names before format options within the validation entry point. Wrap configuration errors once in LoadConfig while retaining permissive parsing outside column formats.
Check format keys alphabetically so repeated loads of the same invalid configuration report the same option.
Document that directory write access permits replacement of a read-only file. Verify that replacement writes the new contents and preserves its permission bits.
CSV exports currently drop trailing zeroes from coordinates, and output files are truncated before conversion succeeds. Add per-column formatting so
37.751can be written as37.7510, and replace existing outputs only after conversion succeeds.format = { precision = 4 }orformat = { true = "1", false = "" }. Reject mixed options and incompatible values, including maps and arrays. Defaults and missing values are unchanged; formats are rejected for Parquet/MMDB.Validation:
go test -race -count=1 ./...andprecious lint --allon both PR heads. Regression tests cover formatting, output preservation, publication and cleanup failures, permissions, and conflicting output paths.Update
mmdbwriter/v2to8f49d9bce747for the dependency-freshness check. Cancellation and CLI signal handling are covered by #116 (STF-1809).Summary by CodeRabbit