Skip to content

Fixing FilePad.update_file()#574

Open
yeyuan98 wants to merge 2 commits into
materialsproject:mainfrom
yeyuan98:agent/coder/fix-FilePadUpdateFile
Open

Fixing FilePad.update_file()#574
yeyuan98 wants to merge 2 commits into
materialsproject:mainfrom
yeyuan98:agent/coder/fix-FilePadUpdateFile

Conversation

@yeyuan98
Copy link
Copy Markdown

Note: This bug fix was created with help of coding agents. Would greatly appreciate any feedback & happy to make further changes.

Summary

FilePad.update_file() and FilePad.update_file_by_id() are broken on main, causing the following problems:

  • updates are silently lost
  • GridFS storage leaks

This PR includes fixes and documentation updates to correct bugs in the file update feature of FilePad.

Issue list

# Bug Impact
1 _update_file_contents() never calls update_one() to persist the new gfs_id to MongoDB File updates are silently lost — get_file() always returns old content
2 from_db_file() passes database= to __init__(), but the parameter is named name Instant TypeErrorfrom_db_file() is unusable
3 _update_file_contents() deletes old GridFS entry before writing the new one If the new file write fails, the old data is permanently lost
4 gridfs.delete() called with string gfs_id instead of ObjectId Silent no-op — GridFS entries are never cleaned up, causing storage leaks
5 zlib.compress(contents, compress) passes boolean True as compression level Compresses at level 1 (weakest) instead of default level 6 — storage inefficiency
6 open(path, read_mode).read() in _update_file_contents never closes the file handle File handle leak on every update

Fix list

fireworks/utilities/filepad.py (lines 257, 304, 334–342, 375):

  • Line 257 (delete_file_by_id): Wrap gfs_id with ObjectId() for proper GridFS deletion.
  • Line 304 (_insert_to_gridfs): Remove boolean compress argument from zlib.compress() call, using the default level 6.
  • Lines 334–342 (_update_file_contents): Three fixes applied together:
    • Use with open() context manager to prevent file handle leak.
    • Reorder: insert new GridFS entry first, then delete old one (prevents data loss on failure).
    • Add missing self.filepad.update_one() call to persist the new gfs_id and compressed flag to MongoDB.
  • Line 375 (from_db_file): Rename database= keyword argument to name= to match __init__ signature.

fireworks/utilities/tests/test_filepad.py:

  • Import ObjectId for proper GridFS existence checks.
  • test_update_file: Add round-trip assertion — after update, verify get_file() returns updated content and gfs_id matches.
  • test_update_file_by_id: Add round-trip assertion — after update, verify get_file_by_id() returns correct content and metadata.

This fix also contains docstring and package doc changes relevant to FilePad's file update functionality.

yeyuan98 added 2 commits May 19, 2026 11:11
- Add missing self.filepad.update_one() call in _update_file_contents
  to persist gfs_id and compressed changes to MongoDB after update
- Reorder _update_file_contents to insert new file before deleting
  old GridFS entry, preventing data loss on write failure
- Fix file handle leak by using `with open()` context manager
- Wrap gfs_id with ObjectId() in delete_file_by_id for proper
  GridFS deletion (string vs ObjectId mismatch)
- Fix from_db_file() passing `database=` instead of `name=` to
  __init__, which caused TypeError on every call
- Fix zlib.compress() receiving boolean as compression level instead
  of using the default level (6)
- Update tests with round-trip assertions to verify data persistence
  after update_file and update_file_by_id
- Add summary line to _update_file_contents and delete_file_by_id
  docstrings (previously had only Args blocks, no description)
- Add "Updating files" section to FilePad tutorial covering
  update_file() and update_file_by_id()
- Fix tutorial intro to mention "update" alongside add/delete,
  consistent with the module-level docstring
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.

1 participant