ome / omero-py

Python project containing Ice remoting code for OMERO
https://www.openmicroscopy.org/omero
GNU General Public License v2.0
22 stars 32 forks source link

download: Check for null file size #180

Closed joshmoore closed 4 years ago

joshmoore commented 4 years ago

If an unexpected download ID is given, the client can fail with:

$ omero download 123456 my.ome.tif
...
Traceback (most recent call last):
  File "OMERO/bin/omero", line 130, in <module>
    rv = omero.cli.argv()
  File "OMERO/lib/python/omero/cli.py", line 1620, in argv
    cli.invoke(args[1:])
  File "OMERO/lib/python/omero/cli.py", line 1095, in invoke
    stop = self.onecmd(line, previous_args)
  File "OMERO/lib/python/omero/cli.py", line 1172, in onecmd
    self.execute(line, previous_args)
  File "OMERO/lib/python/omero/cli.py", line 1254, in execute
    args.func(args)
  File "OMERO/lib/python/omero/plugins/download.py", line 66, in __call__
    client.download(orig_file, target_file)
  File "OMERO/lib/python/omero/clients.py", line 863, in download
    if block_size > ofile.size.val:

(line numbers are from 5.4.10)

cc: @chris-allan

chris-allan commented 4 years ago

Thanks, @joshmoore. @kkoz is working on a variety of scenarios surrounding this. We were unable to find anything other than broken imports where size was null but there were also lots of other conditions that we found were the error checking was sub par. I'll let him comment when he's ready.

joshmoore commented 4 years ago

:+1: Also feel free to just open separately and I can close this. Right now I'm looking into what the behavior is when one accidentally tries to download a Directory or a Repository. If you know what the original target was (called 123456 above) that would be useful.

chris-allan commented 4 years ago

Yep, those are exactly conditions we were seeing as well. Also seen with trying to download an OMERO.tables instance. The original 123456 target was what we could only assume to be a broken import from many years ago. File was not actually in the ManagedRepository and the OriginalFile instance only partially filled out. The reason it was selected was misunderstanding the documentation and assuming 123456 was an Image ID (requiring Image:123456 as a command line option) rather than an OriginalFile ID.

joshmoore commented 4 years ago

Minor update to hide the stack trace from the user, e.g.:

ClientError: invalid size for OriginalFile 'Meas_04(2011-02-22_19-24-25)' (mimetype:Directory)
joshmoore commented 4 years ago

--exclude