Using uri_file_path to properly check for file existence

[douglowe]

Correcting logical checks inside upload_directory and
upload_file, to use schema_salad.ref_resolver.uri_file_path
for resolving file:/// paths. This ensures that encoded
special characters within the path are properly decoded again.

Will fix #3973

Changelog Entry

To be copied to the draft changelog by merger:

• CWL input uploading to the file store: decode file paths that were previously URL encoded

Reviewer Checklist

• Make sure it is coming from issues/XXXX-fix-the-thing in the Toil repo, or from an external repo.
  • If it is coming from an external repo, make sure to pull it in for CI with:

    contrib/admin/test-pr otheruser theirbranchname issues/XXXX-fix-the-thing
    

  • If there is no associated issue, create one.
• Read through the code changes. Make sure that it doesn't have:
  • Addition of trailing whitespace.
  • New variable or member names in camelCase that want to be in snake_case.
  • New functions without type hints.
  • New functions or classes without informative docstrings.
  • Changes to semantics not reflected in the relevant docstrings.
  • New or changed command line options for Toil workflows that are not reflected in docs/running/{cliOptions,cwl,wdl}.rst
  • New features without tests.
• Comment on the lines of code where problems exist with a review comment. You can shift-click the line numbers in the diff to select multiple lines.
• Finish the review with an overall description of your opinion.

Merger Checklist

• Make sure the PR passes tests.
• Make sure the PR has been reviewed since its last modification. If not, review it.
• Merge with the Github "Squash and merge" feature.
  • If there are multiple authors' commits, add Co-authored-by to give credit to all contributing authors.
• Copy its recommended changelog entry to the Draft Changelog.
• Append the issue number in parentheses to the changelog entry.

[mr-c]

Thanks @douglowe ! Is there an example path that this fixes?

[douglowe]

Thanks @douglowe ! Is there an example path that this fixes?

My problem path was an output directory we were naming after a mutation (A:Gln2Cys) - the file string for this became: "file:///Users/douglowe/cwl-mutations/A%3AGln2Cys".

I could zip up an example script and input molecular structure to send over, if that would help?

[mr-c]

file:///Users/douglowe/cwl-mutations/A%3AGln2Cys

Thanks! I think we can write a fast test that uses A:Gln2Cys as an input name to test this PR

[mr-c]

I've pushed your branch to our repo as https://github.com/DataBiosphere/toil/tree/issues/3973-decode-uri-filepaths to enable CI testing while I write a targeted test

[mr-c]

@douglowe Are you running with --relax-path-checks?

[douglowe]

@douglowe Are you running with --relax-path-checks?

No - I was running without that option. I've tried enabling it just now, and it failed the same test in upload_file as before.

From the help text it looks like this option only deals with allowing spaces and hashes in paths. I've not run any tests to see if my change helps with those or not.

[mr-c]

Even with this change, I think cwltool would need common-workflow-language/cwltool#1579

[mr-c]

Here is my test setup; does it cover the same use case for you? common-workflow-language/cwl-v1.2#144

[douglowe]

Even with this change, I think cwltool would need common-workflow-language/cwltool#1579

Ahh - I had not checked --relax-path-checks for cwltool.

I had actually tested this workflow with cwltool - and found that it coped fine with these paths (which made me look more into the Toil code, to see where it might be doing things differently).

[mr-c]

I had actually tested this workflow with cwltool - and found that it coped fine with these paths (which made me look more into the Toil code, to see where it might be doing things differently).

Can you share your workflow with me?

[douglowe]

I had actually tested this workflow with cwltool - and found that it coped fine with these paths (which made me look more into the Toil code, to see where it might be doing things differently).

Can you share your workflow with me?

Yep - you can get a copy here: https://github.com/douglowe/cwl-mutation-example

[mr-c]

https://github.com/DataBiosphere/toil/tree/issues/3973-decode-uri-filepaths has been updated with 7580c73

[DailyDreaming]

LGTM.

[DailyDreaming]

Merging since all of the CWL tests have passed and completed and this only touches the CWL code. Thank you @douglowe .