HERA-Team / librarian

The HERA Librarian.
BSD 2-Clause "Simplified" License
6 stars 11 forks source link

{dest-path} ending in '/' result in registered file named ' ' #70

Open BrianJKoopman opened 4 years ago

BrianJKoopman commented 4 years ago

I realize I'm probably breaking the rules in my file naming during manual testing and that this case might not come up when programmatically uploading directories, but in testing I've successfully uploading directories with {local-path} containing a trailing slash:

$ librarian upload --null-obsid TestUser directory_test/ foo/directory_test

However, I find that if I have a trailing slash on the {dest-path} (might be easy to do manually if tab completing a directory and not giving the destination any parent directory), things go wrong:

$ librarian upload --null-obsid TestUser directory_test/ directory_test11/
error: upload failed: RPC call {'store_name': 'host2store', 'staging_dir': 'staging.uC98iT', 'dest_store_path': 'directory_test11/', 'meta_mode': 'infer', 'deletion_policy': 'disallowed', 'staging_was_known': False, 'null_obsid': True, 'authenticator': 'I am a bot'} failed: cannot move upload to its destination (is there already a file there, unknown to this Librarian?): RPC call ['ssh', 'Host2', "mkdir -p '/tmp/librarian/directory_test11' && chmod u+w '/tmp/librarian/staging.uC98iT/' && mv -nT '/tmp/librarian/staging.uC98iT/' '/tmp/librarian/directory_test11/' && test ! -e '/tmp/librarian/staging.uC98iT/' && chmod -R 'ugoa-w' '/tmp/librarian/directory_test11/'"] failed: exit code 1; stdout:

''

stderr:

''

The librarian webpage then reports a file existing with the name of ' ':

Name Created Observation Type Source Size
details 2020-02-03 17:14:52 null uC98iT/ TestUser 36 Bytes

I understand why, given the rules for naming, this might error out, but I think the ' ' file registering might be a bug.

pkgw commented 4 years ago

Definitely a bug, the interfaces should sanitize file paths for things like trailing slashes. Other fun things to think about are trailing /., internal /../, etc.

plaplant commented 4 years ago

We can probably handle most of these issues in a self-consistent way by using pathlib to canonicalize file/directory paths as part of uploading. I can try to take a stab at this in the next ~week or so.