Skip to content
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.

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

Check for legal results filename #366

Merged
merged 4 commits into from May 25, 2022
Merged

Conversation

bboudaoud-nv
Copy link
Collaborator

This branch adds a FilePath::isLegalFilename() check on the filename used for the results database. Previously the results could fail to write due to an illegal character in either an experiment description or a user name.

The new check should throw an exception when any invalid filename character is detected in the results filename. It is worth noting this may catch characters that technically legal but not encouraged for file naming.

Merging this PR closes #351

@bboudaoud-nv bboudaoud-nv added the enhancement New feature or request label Apr 19, 2022
@bboudaoud-nv bboudaoud-nv self-assigned this Apr 19, 2022
@bboudaoud-nv
Copy link
Collaborator Author

bboudaoud-nv commented Apr 19, 2022

It may be worth noting that G3D also has an option for FilePath::makeLegalFilename() that we could use as an alternative to attempt to automatically correct invalid filenames as opposed to just throwing an exception. The behavior of this method is not documented and I haven't looked into it so I didn't use it ahead of throwing the exception for now.

Update: FilePath::makeLegalFilename() replaces any illegal character(s) with _, and has logic to avoid appending multiple _s in a row. This may be a viable approach to avoid illegal names with things like spaces.

@bboudaoud-nv
Copy link
Collaborator Author

Currently this branch also introduces an issue with descriptions/user names that include spaces. This should be resolved before merging...

@bboudaoud-nv
Copy link
Collaborator Author

For now I've moved to using FilePath::makeLegalFilename() as a quick fix here. This is a work-around for the problem of temporary user names (i.e., "Create a new user") and experiment descriptions with spaces in them triggering a runtime exception.

This may only cause issues when something like 2 user IDs/experiment descriptions only differ by sequential illegal characters in which case they may collide... I think this branch is a fine fix for the issue for now

Copy link
Contributor

@jspjutNV jspjutNV left a comment

Choose a reason for hiding this comment

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

All tests passed and this looks like a solid, straightforward improvement.

@jspjutNV
Copy link
Contributor

Actually, it looks like there's a likely bug with an extra + ".db" in there. Checking that before merging.

@jspjutNV
Copy link
Contributor

Actually, it looks like there's a likely bug with an extra + ".db" in there. Checking that before merging.

This should be fixed with 137b0da

@bboudaoud-nv bboudaoud-nv merged commit 9561b4a into master May 25, 2022
@bboudaoud-nv bboudaoud-nv deleted the ResultsFilenameValidation branch May 25, 2022 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with slash in experiment description
2 participants