Skip to content

fix(models): never convert a file onto the file being read - #356

Open
devgtv wants to merge 1 commit into
AlertaDengue:mainfrom
devgtv:fix/parquet-partial-output
Open

devgtv wants to merge 1 commit into
AlertaDengue:mainfrom
devgtv:fix/parquet-partial-output

Conversation

@devgtv

@devgtv devgtv commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Summary

BaseLocalFile.to_parquet streams rows out of self.path and writes them with pq.ParquetWriter(output_path, ...), which opens its destination with "wb". When output_path resolved to self.path, the source was truncated to zero length part-way through the very read serving it, and the Parquet bytes then replaced it:

>>> csv = await ExtensionFactory.instantiate("data.csv")
>>> await csv.to_parquet(output_path="data.csv")   # exits 0
>>> open("data.csv", "rb").read(4)
b'PAR1'

The input file was silently destroyed and replaced by an unreadable one.

to_parquet() with no arguments on a Parquet source hits this as well, since self.path.with_suffix(".parquet") is the source path when the source is already .parquet.

DBF, DBC and Zip already guard against this by short-circuiting when the output exists (pysus/api/extensions.py:538, :687). This adds the same guard to the base implementation, before anything is written.

Fix

  • a Parquet source is returned unchanged — it needs no conversion, and download_to_parquet already anticipates this outcome with its original_path != parquet_file.path check (pysus/api/client.py:596)
  • any other source raises ConversionError naming a usable output_path
  • the check compares against the resolved path, so sub/../data.csv is caught too

Tests

Added 6 tests: CSV in place raises and leaves the file byte-identical, JSON in place likewise, a convoluted-but-equivalent path is caught, a Parquet source is a no-op returning the same object, the default .csv → .parquet conversion still works, and the source survives it.

4 of the 6 fail against the previous code.

  • 1739 passed, 6 skipped
  • black and isort clean

`BaseLocalFile.to_parquet` streams the rows out of `self.path` and
writes them with `pq.ParquetWriter(output_path, ...)`, which opens the
destination with `"wb"`. When `output_path` resolved to `self.path` the
source was truncated to zero length part-way through the very read it
was serving, and the Parquet bytes then replaced it:

    >>> csv = await ExtensionFactory.instantiate("data.csv")
    >>> await csv.to_parquet(output_path="data.csv")   # succeeds!
    >>> open("data.csv", "rb").read(4)
    b'PAR1'

The source file was silently destroyed. `to_parquet()` on a Parquet
source hits this too, because `self.path.with_suffix(".parquet")` is
the source path when the source is already `.parquet`.

`DBF`, `DBC` and `Zip` already guard this by short-circuiting when the
output exists; this is the same guard, checked before any writing:

- a Parquet source is returned unchanged, since it needs no conversion
  and `download_to_parquet` already tolerates `original_path ==
  parquet_file.path`
- any other source raises `ConversionError` naming a usable
  `output_path` instead of destroying the file

The comparison is against the resolved `output_path`, so a path that
only matches after normalisation (`sub/../data.csv`) is caught too.
@devgtv
devgtv force-pushed the fix/parquet-partial-output branch from 20c19af to b1ee260 Compare October 2, 2026 19:18
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@9db5915). Learn more about missing BASE report.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #356   +/-   ##
=======================================
  Coverage        ?   97.19%           
=======================================
  Files           ?      180           
  Lines           ?    23351           
  Branches        ?        0           
=======================================
  Hits            ?    22697           
  Misses          ?      654           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants