Refine validation wording and simplify delete logs - #106
Merged
Merged
Conversation
… level - Remove `_is_safe_to_delete` validation in `src/commons/plantdb/commons/fsdb/file_ops.py` for both file and fileset deletions, eliminating the `IOError` for out‑of‑scope paths. - Change successful deletion logs from `logger.info` to `logger.debug` for JSON metadata, file, fileset, and scan removals, updating the corresponding messages. - Adjust related log calls to use the new debug level, cleaning up redundant error handling.
- Update `src/commons/plantdb/commons/fsdb/validation.py` to log "`The provided path is not related to a directory.`" instead of "`The provided path is not a directory.`" when the path is not a directory.
| # Check if the path is a directory | ||
| if not path.is_dir(): | ||
| logger.error("The provided path is not a directory.") | ||
| logger.error("The provided path is not related to a directory.") |
Contributor
There was a problem hiding this comment.
I think this change makes the error message less clear, after all, a fsdb is a dir and this message is only sent when path is not a dir. (What does "related to a directory" mean ?)
Member
Author
There was a problem hiding this comment.
...yep, you are right!
Comment on lines
-566
to
-569
| if not _is_safe_to_delete(file_path): | ||
| logger.error(f"File {file.filename} is not in the current database.") | ||
| logger.debug(f"File path: '{file_path}'") | ||
| raise IOError("Cannot delete files or directories outside of a local DB.") |
Contributor
There was a problem hiding this comment.
What is the motivation for removing the checks ?
Member
Author
There was a problem hiding this comment.
lazyness i guess... the _is_safe_to_delete was not properly implemented. It was testing if the given file_path is an FSDB, and was thus throwing a lot of error messages that should not exists. Instead of fixing _is_safe_to_delete , I chose to bypass it as its internal use did not make it essential.
But now I will fix it.
- Extend `_is_safe_to_delete` signature to `def _is_safe_to_delete(path, db_path) -> bool`. - Resolve both `path` and `db_path` to absolute paths and verify `db_path` is a valid FSDB. - Ensure the deletion target is a sub‑path of the FSDB and not the FSDB root itself; log errors for invalid cases. - Return `True` only after all safety validations succeed. - Update docstring to include `db_path` parameter, revised safety notes, and example usage.
… `file_ops.py` - Replace `_load_scans` example with `_load_scan` usage and add `db.disconnect()` cleanup in doctests. - Append `db.disconnect()` calls to all doctest sections for proper temporary DB teardown. - Introduce new doctest examples for: - `_load_file` - `_load_measures` and `_load_scan_measures` - `_delete_file`, `_delete_fileset`, and `_delete_scan` - `_make_fileset`, `_make_scan`, and `_store_scan` - Enhance delete functions (`_delete_file`, `_delete_fileset`, `_delete_scan`) with `_is_safe_to_delete(path, db.path())` validation to prevent out‑of‑scope deletions. - Update docstrings to incorporate the added examples and safety notes.
- Introduce new test module `src/commons/tests/test_file_ops.py` covering the public helpers in `plantdb.commons.fsdb.file_ops` - Validate loading functions with empty DB, existing scans, and various scan‑fileset‑file scenarios (`_load_scans`, `_load_scan`, `_load_scan_filesets`, `_load_fileset`, `_load_fileset_files`, `_load_file`) - Test measures handling (`_load_measures`, `_load_scan_measures`) including malformed JSON and non‑dict data cases - Ensure deletion safety when a file has no `filename` attribute (`_delete_file`) - Verify directory creation helpers (`_make_fileset`, `_make_scan`) create the expected paths - Provide fixtures `db_with_fileset`, `db_with_file`, and `db_with_fileset_and_file` for isolated dummy database setups.
- Introduce new test module `src/commons/tests/test_validation.py` - Cover `_is_valid_id` with varied inputs and error‑log checks - Test `_is_fsdb` handling of empty DB, non‑directory paths, missing marker, valid/invalid scans, and extra directories - Validate `_is_scan_dataset` for missing metadata, missing or malformed `files.json`, missing `filesets` key, and both with and without fileset validation - Verify `_is_valid_fileset` behavior for missing directory, missing files, and fully valid filesets - Add tests for `_fileset_files_exists` with empty, partially invalid, and mixed file entries - Ensure `_is_safe_to_delete` correctly rejects paths outside the DB, invalid DBs, root DB path, and accepts valid sub‑paths - Use fixtures `empty_db_path`, `db_with_scan`, `db_with_fileset`, and `db_with_file` to provide isolated dummy databases - Include logging assertions to confirm appropriate error messages are emitted.
ArthurLuciani2
approved these changes
Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
src/commons/plantdb/commons/fsdb/validation.pyto log “The provided path is not related to a directory.” instead of the previous message when a non‑directory path is supplied._is_safe_to_deletesafety check fromsrc/commons/plantdb/commons/fsdb/file_ops.pyfor both file and fileset deletions, eliminating theIOErrorfor out‑of‑scope paths.logger.infotologger.debugfor JSON metadata, file, fileset, and scan removals, and adjust related log calls to match the new debug level.