Allow relative paths in FCPath #40

rfrenchseti · 2025-01-06T21:23:42Z

Fixed Issues

Fixes Enable support for local file paths that are not absolute #38

Summary of Changes

When the operations "retrieve", "exists", "get_local_path", "upload", and "unlink" are performed on an FCPath, allow the use of relative paths which are converted to absolute paths using expanduser().resolve().

Known Problems

None

Summary by Bito

This PR enhances FCPath by implementing relative path support and simplifying path handling logic. It introduces as_absolute() and _make_paths_absolute() methods for automatic conversion of relative paths to absolute using expanduser() and resolve(). The changes include debug printing capabilities for path splitting operations and consolidate code paths for different path types. A comprehensive test suite validates file operations including retrieve, exists, get_local_path, upload, and unlink functionalities.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 3

pds-admin · 2025-01-06T21:23:54Z

Code Review Agent Run #87dbbf

Actionable Suggestions - 3

filecache/file_cache_path.py - 3
- Consider validating optional sub_path parameter · Line 1088-1088
- Consider adding path validation checks · Line 630-633
- Consider consolidating duplicate unlink parameters · Line 1099-1105

Review Details

Files reviewed - 3 · Commit Range: 64be6e6..64be6e6
- filecache/file_cache_path.py
- tests/test_file_cache.py
- tests/test_file_cache_path.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

pds-admin · 2025-01-06T21:28:19Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Enhanced Path Handling in FCPath	- `file_cache_path.py` - Added relative path support and simplified path handling logic with new as_absolute() and _make_paths_absolute() methods - `test_file_cache.py` - Removed redundant relative path error test cases - `test_file_cache_path.py` - Added comprehensive test coverage for relative path handling functionality

pds-admin · 2025-01-06T21:28:21Z

filecache/file_cache_path.py

-            if not FCPath._is_absolute(str(new_sub_path2)):
-                raise ValueError(
-                    f'Derived path must be absolute, got {new_sub_path2}')
+        new_sub_path = self._make_paths_absolute(sub_path)


Consider validating optional sub_path parameter

Consider validating sub_path before calling _make_paths_absolute() to ensure it's not None. While the type hint suggests Optional[StrOrPathOrSeqType], there's no explicit check.

Code suggestion

Check the AI-generated fix before applying

Suggested change

new_sub_path = self._make_paths_absolute(sub_path)

if sub_path is None:

sub_path = ''

new_sub_path = self._make_paths_absolute(sub_path)

Code Review Run #87dbbf

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

pds-admin · 2025-01-06T21:28:22Z

filecache/file_cache_path.py

+                if not FCPath._is_absolute(new_sub_path):
+                    new_sub_path = (FCPath(Path(new_sub_path).expanduser().resolve())
+                                    .as_posix())
+                new_sub_paths.append(new_sub_path)


Consider adding path validation checks

Consider adding validation for the resolved absolute path to ensure it stays within allowed directories for security. The current implementation could potentially allow access to sensitive files outside intended directories.

Code suggestion

Check the AI-generated fix before applying

Suggested change

if not FCPath._is_absolute(new_sub_path):

new_sub_path = (FCPath(Path(new_sub_path).expanduser().resolve())

.as_posix())

new_sub_paths.append(new_sub_path)

if not FCPath._is_absolute(new_sub_path):

new_sub_path = (FCPath(Path(new_sub_path).expanduser().resolve())

.as_posix())

base_dir = Path(self._path).parent.resolve()

resolved_path = Path(new_sub_path).resolve()

if not str(resolved_path).startswith(str(base_dir)):

raise ValueError(f'Path {new_sub_path} resolves outside allowed directory {base_dir}')

new_sub_paths.append(new_sub_path)

Code Review Run #87dbbf

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

pds-admin · 2025-01-06T21:28:22Z

filecache/file_cache_path.py

+        return self._filecache_to_use.unlink(cast(StrOrPathOrSeqType,
+                                                  new_sub_path),
+                                             missing_ok=missing_ok,
+                                             anonymous=self._anonymous,
+                                             nthreads=nthreads,
+                                             exception_on_fail=exception_on_fail,
+                                             url_to_path=url_to_path)


Consider consolidating duplicate unlink parameters

Consider consolidating the duplicate unlink call parameters into a dictionary to reduce code duplication and improve maintainability. The same parameter list appears in both branches of the if statement.

Code suggestion

Check the AI-generated fix before applying

@@ -1088,21 +1088,19 @@ new_sub_path = self._make_paths_absolute(sub_path) + unlink_params = { + 'missing_ok': missing_ok, + 'anonymous': self._anonymous, + 'nthreads': nthreads, + 'exception_on_fail': exception_on_fail, + 'url_to_path': url_to_path + } + if isinstance(new_sub_path, (list, tuple)): - return self._filecache_to_use.unlink(cast(StrOrPathOrSeqType, - new_sub_path), - missing_ok=missing_ok, - anonymous=self._anonymous, - nthreads=nthreads, - exception_on_fail=exception_on_fail, - url_to_path=url_to_path) + return self._filecache_to_use.unlink(cast(StrOrPathOrSeqType, new_sub_path), + **unlink_params) - return self._filecache_to_use.unlink(cast(StrOrPathOrSeqType, - new_sub_path), - missing_ok=missing_ok, - anonymous=self._anonymous, - nthreads=nthreads, - exception_on_fail=exception_on_fail, - url_to_path=url_to_path)

Code Review Run #87dbbf

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

codecov · 2025-01-06T21:34:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.03%. Comparing base (24647ee) to head (d72d8b6).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
+ Coverage   92.93%   93.03%   +0.10%     
==========================================
  Files           5        5              
  Lines        1784     1767      -17     
  Branches      370      359      -11     
==========================================
- Hits         1658     1644      -14     
+ Misses         75       70       -5     
- Partials       51       53       +2

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

pds-admin · 2025-01-06T22:29:17Z

Code Review Agent Run #a438ce

Actionable Suggestions - 1

filecache/file_cache_path.py - 1
- Consider keeping original operation order · Line 1058-1060

Review Details

Files reviewed - 2 · Commit Range: 64be6e6..b81dc74
- filecache/file_cache_path.py
- tests/test_file_cache.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

pds-admin · 2025-01-06T22:32:31Z

filecache/file_cache_path.py

+        nthreads = self._validate_nthreads(nthreads)
+
+        new_sub_path = self._make_paths_absolute(sub_path)


Consider keeping original operation order

Consider keeping the original order of operations where path validation happens before nthreads validation. The current change could affect error handling flow since invalid paths would not be caught before validating thread count.

Code suggestion

Check the AI-generated fix before applying

@@ -1058,1060 +1058,1060 @@ - nthreads = self._validate_nthreads(nthreads) - new_sub_path = self._make_paths_absolute(sub_path) + new_sub_path = self._make_paths_absolute(sub_path) + nthreads = self._validate_nthreads(nthreads)

Code Review Run #a438ce

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

pds-admin · 2025-01-07T02:11:52Z

Code Review Agent Run #7d2767

Actionable Suggestions - 1

tests/test_file_cache_path.py - 1
- Invalid path joining operation · Line 993-994

Review Details

Files reviewed - 2 · Commit Range: b81dc74..6a5e605
- filecache/file_cache_path.py
- tests/test_file_cache_path.py
Files skipped - 1
- .github/workflows/run-tests.yml - Reason: Filter setting
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

tests/test_file_cache_path.py

pds-admin · 2025-01-07T03:39:17Z

Code Review Agent Run Status

Limitations and other issues: ❌ Failure - Bito Code Review Agent couldn't review this pull request because it is no longer valid. It may have been merged, or the source/target branch may not exist.

Allow relative paths in FCPath

64be6e6

Fix flake8

e2e12ff

pds-admin reviewed Jan 6, 2025

View reviewed changes

rfrenchseti added 2 commits January 6, 2025 13:42

Simplify

7324d7c

Flake8

b81dc74

pds-admin reviewed Jan 6, 2025

View reviewed changes

rfrenchseti added 8 commits January 6, 2025 17:09

Add relative file tests

45919aa

tests

e3e5a74

Tests

4832a1d

tests

ab2d49b

tests

5f10a89

tests

80637e3

tests

961548d

tests

6a5e605

pds-admin reviewed Jan 7, 2025

View reviewed changes

tests/test_file_cache_path.py Show resolved Hide resolved

rfrenchseti added 4 commits January 6, 2025 18:32

tests

71b98bf

tests

0492e8f

tests

2b55947

tests

d72d8b6

rfrenchseti merged commit a2320f5 into main Jan 7, 2025
18 checks passed

rfrenchseti deleted the rf_250106_pdstemplate branch January 7, 2025 02:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow relative paths in FCPath #40

Allow relative paths in FCPath #40

rfrenchseti commented Jan 6, 2025 •

edited by pds-admin

Loading

pds-admin commented Jan 6, 2025 •

edited

Loading

Code Review Agent Run #87dbbf

pds-admin commented Jan 6, 2025 •

edited

Loading

Changelist by Bito

pds-admin Jan 6, 2025

pds-admin Jan 6, 2025

pds-admin Jan 6, 2025

codecov bot commented Jan 6, 2025 •

edited

Loading

pds-admin commented Jan 6, 2025 •

edited

Loading

Code Review Agent Run #a438ce

pds-admin Jan 6, 2025

pds-admin commented Jan 7, 2025 •

edited

Loading

Code Review Agent Run #7d2767

pds-admin commented Jan 7, 2025 •

edited

Loading

		nthreads = self._validate_nthreads(nthreads)

		new_sub_path = self._make_paths_absolute(sub_path)

Allow relative paths in FCPath #40

Allow relative paths in FCPath #40

Conversation

rfrenchseti commented Jan 6, 2025 • edited by pds-admin Loading

Fixed Issues

Summary of Changes

Known Problems

Summary by Bito

pds-admin commented Jan 6, 2025 • edited Loading

Code Review Agent Run #87dbbf

pds-admin commented Jan 6, 2025 • edited Loading

Changelist by Bito

pds-admin Jan 6, 2025

Choose a reason for hiding this comment

pds-admin Jan 6, 2025

Choose a reason for hiding this comment

pds-admin Jan 6, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 6, 2025 • edited Loading

Codecov Report

pds-admin commented Jan 6, 2025 • edited Loading

Code Review Agent Run #a438ce

pds-admin Jan 6, 2025

Choose a reason for hiding this comment

pds-admin commented Jan 7, 2025 • edited Loading

Code Review Agent Run #7d2767

pds-admin commented Jan 7, 2025 • edited Loading

Code Review Agent Run Status

rfrenchseti commented Jan 6, 2025 •

edited by pds-admin

Loading

pds-admin commented Jan 6, 2025 •

edited

Loading

pds-admin commented Jan 6, 2025 •

edited

Loading

codecov bot commented Jan 6, 2025 •

edited

Loading

pds-admin commented Jan 6, 2025 •

edited

Loading

pds-admin commented Jan 7, 2025 •

edited

Loading

pds-admin commented Jan 7, 2025 •

edited

Loading