Skip to content

Address corner case bugs in the check access and get file size helpers in fileio.py #379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jonasbardino merged 3 commits into from
Jan 16, 2026
Merged

Address corner case bugs in the check access and get file size helpers in fileio.py #379

jonasbardino merged 3 commits into from
Jan 16, 2026

Conversation

@jonasbardino
Copy link
Contributor

@jonasbardino jonasbardino commented Nov 1, 2025 •
edited
Loading

Address a few corner case bugs in the check access and get file size helpers of the fileio.py module as explained in issue #378 and covered by the unit tests in PR #377.

Addresses a couple of old unrelated pylint used-before-assignment warnings to make linting actions happy.

@jonasbardino jonasbardino self-assigned this Nov 1, 2025
@jonasbardino jonasbardino added the bug Something isn't working label Nov 1, 2025
@jonasbardino jonasbardino linked an issue Nov 1, 2025 that may be closed by this pull request
Wrong mode constants used in read and write access helpers in the fileio module and missing return value in get size for missing files #378
Closed
@jonasbardino jonasbardino requested a review from a team November 1, 2025 17:31
@albu-diku
Copy link
Contributor

albu-diku commented Nov 2, 2025

I think these changes ought to be covered by a regression test.

@jonasbardino
Copy link
Contributor Author

jonasbardino commented Nov 2, 2025

I think these changes ought to be covered by a regression test.

I agree and they will be in PR #377 as e.g. with the commented out line with a matching TODO in test_handles_missing_file:
https://github.com/ucphhpc/migrid-sync/pull/377/files#diff-1c61395f8eafbd27d885b5dd81c67d2ae2a90778fe101ad9036499a681920f65R428

The other ones in the check access helpers like in
https://github.com/ucphhpc/migrid-sync/pull/377/files#diff-1c61395f8eafbd27d885b5dd81c67d2ae2a90778fe101ad9036499a681920f65R1089
can't be triggered with the existing unit tests in CI as long as they by default run as the super-user inside docker and render permission checks useless. So it currently only shows for make unittest. There's a note about it in the description of #377.

jonasbardino added a commit that referenced this pull request Jan 9, 2026
@jonasbardino
Rework tests to address PR review feedback (thanks @albu-diku):
5b25096 
 * Switch from old `setUp` to `before_each` init structure using 'testconfig'
 * Replace direct `os.makedirs` calls with the `ensure_dirs_exist` helper
 * Converge on string constants for almost everything and greater reuse
 * Strip dir-prefix part of path constants and adjust asserts accordingly

Please note that make unittest still fails the three tests of #378 yet to be
fixed by #379.
jonasbardino added a commit that referenced this pull request Jan 9, 2026
@jonasbardino
Rework tests to address PR review feedback (thanks @albu-diku):
290fbe6 
 * Switch from old `setUp` to `before_each` init structure using 'testconfig'
 * Replace direct `os.makedirs` calls with the `ensure_dirs_exist` helper
 * Converge on string constants for almost everything and greater reuse
 * Strip dir-prefix part of path constants and adjust asserts accordingly

Please note that make unittest still fails the three tests of #378 yet to be
fixed by #379.
jonasbardino added a commit that referenced this pull request Jan 12, 2026
@jonasbardino
Rework tests to address PR review feedback (thanks @albu-diku):
d73c20b 
 * Switch from old `setUp` to `before_each` init structure using 'testconfig'
 * Replace direct `os.makedirs` calls with the `ensure_dirs_exist` helper
 * Converge on string constants for almost everything and greater reuse
 * Strip dir-prefix part of path constants and adjust asserts accordingly

Please note that make unittest still fails the three tests of #378 yet to be
fixed by #379.
@jonasbardino jonasbardino force-pushed the fix/issue-378-corner-case-bugs-in-fileio-module branch from 5d8e46f to 6e2be74 Compare January 12, 2026 08:04
jonasbardino added a commit that referenced this pull request Jan 12, 2026
@jonasbardino
Rework tests to address PR review feedback (thanks @albu-diku):
e4b75c0 
 * Switch from old `setUp` to `before_each` init structure using 'testconfig'
 * Replace direct `os.makedirs` calls with the `ensure_dirs_exist` helper
 * Converge on string constants for almost everything and greater reuse
 * Strip dir-prefix part of path constants and adjust asserts accordingly

Please note that make unittest still fails the three tests of #378 yet to be
fixed by #379.
@jonasbardino jonasbardino force-pushed the fix/issue-378-corner-case-bugs-in-fileio-module branch from 6e2be74 to f338c2a Compare January 12, 2026 11:29
albu-diku
albu-diku requested changes Jan 12, 2026
View reviewed changes
Copy link
Contributor

@albu-diku albu-diku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see a test being enabled/changed as part of this commit so that the fix is covered by a regression test.

@jonasbardino jonasbardino force-pushed the fix/issue-378-corner-case-bugs-in-fileio-module branch from f338c2a to 9d67fa4 Compare January 12, 2026 12:08
@jonasbardino
Copy link
Contributor Author

jonasbardino commented Jan 12, 2026

I'd like to see a test being enabled/changed as part of this commit so that the fix is covered by a regression test.

Alright, I pulled them in from #377 now but in the slightly older setUp version for consistency until #377 gets merged.

jonasbardino added a commit that referenced this pull request Jan 13, 2026
@jonasbardino
Rework tests to address PR review feedback (thanks @albu-diku):
6cbbb83 
 * Switch from old `setUp` to `before_each` init structure using 'testconfig'
 * Replace direct `os.makedirs` calls with the `ensure_dirs_exist` helper
 * Converge on string constants for almost everything and greater reuse
 * Strip dir-prefix part of path constants and adjust asserts accordingly

Please note that make unittest still fails the three tests of #378 yet to be
fixed by #379.
@jonasbardino jonasbardino mentioned this pull request Jan 13, 2026
Open
@jonasbardino jonasbardino force-pushed the fix/issue-378-corner-case-bugs-in-fileio-module branch from 9d67fa4 to 7e61e24 Compare January 13, 2026 14:17
jonasbardino added a commit that referenced this pull request Jan 16, 2026
@jonasbardino
Rework tests to address PR review feedback (thanks @albu-diku):
52c44c3 
 * Switch from old `setUp` to `before_each` init structure using 'testconfig'
 * Replace direct `os.makedirs` calls with the `ensure_dirs_exist` helper
 * Converge on string constants for almost everything and greater reuse
 * Strip dir-prefix part of path constants and adjust asserts accordingly

Please note that make unittest still fails the three tests of #378 yet to be
fixed by #379.
@jonasbardino jonasbardino force-pushed the fix/issue-378-corner-case-bugs-in-fileio-module branch from 7e61e24 to 7cfe979 Compare January 16, 2026 14:45
@jonasbardino
Copy link
Contributor Author

jonasbardino commented Jan 16, 2026

Rebased after merging #377 and I'll proceed with merging when CI completes since the previous reviews already cover the code in question.

@jonasbardino jonasbardino merged commit e89d939 into next Jan 16, 2026
7 checks passed
@jonasbardino jonasbardino deleted the fix/issue-378-corner-case-bugs-in-fileio-module branch January 16, 2026 14:50
albu-diku
albu-diku reviewed Jan 16, 2026
View reviewed changes
mig/shared/fileio.py
"""Write message to notify home"""
if not logger:
logger = null_logger("dummy")
filepath = 'UNDEFINED'
Copy link
Contributor

@albu-diku albu-diku Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of an excetion being thrown, will we not now try to remove a file named UNDEFINED in whatever the cwd happens to be?

Copy link
Contributor Author

@jonasbardino jonasbardino Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will effectively try/except on os.remove('UNDEFINED') and fail silently when no such file exists in the cwd - i.e. the mig user home for deployed use.
We could of course make it conditional in the catch-all clean up handler or change filepath to something even more ridiculously improbable if it's a concern.

Copy link
Contributor

@albu-diku albu-diku Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - for me that is clearly explicit is better than implcit, and relying on an exception because we chose
a non-existent name is pretty implicit. I think it is generally better not to attempt a remove when we already know the file can't exist - in this case an empty or None path would be better than a UNDEFINED.

A bare remove like this relies pretty heavily on cwd being the user home, which - while the intent might be true - that is not clear to a reader here and I think it is reasonable to view with some suspicion.

Copy link
Contributor Author

@jonasbardino jonasbardino Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say the reliance is less on the CWD than on the fact that trying to remove a file called 'UNDEFINED' anywhere (most likely in mig-user home) will not cause trouble.
Yet, I addressed it directly with an absolute non-existent path in
5a6fc1a
since this PR was already merged.

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

Reviewers

@albu-diku albu-diku albu-diku requested changes

Assignees

@jonasbardino jonasbardino

Labels

bug Something isn't working

Projects

None yet

Milestone

Unit test coverage of everything

Development

Successfully merging this pull request may close these issues.

Wrong mode constants used in read and write access helpers in the fileio module and missing return value in get size for missing files

2 participants

@jonasbardino @albu-diku