Use and validate real SHA1s on all uploads

[jamoore]

In many places, we are still setting OriginalFile.sha1 to "pending". These should all be fixed in order to detect file corruptions on uploads.

[jmoore]

Referencing ticket #2595 has changed sprint.

[cblackburn]

Referencing ticket #2595 has changed sprint.

[jburel]

Referencing ticket #2595 has changed sprint.

[mtbcarroll]

Does anybody know why they were set to pending in the first place: is there a need to fire off an asynchronous checksummer for performance reasons, or something? Still looking at the actual cases ...

[jmoore]

Primarily to save the overhead of generating the SHA1s in the calling thread. So they will likely need to be reviewed on a case by case basis. But if we now have the option of selecting something with less overhead than sha1 then perhaps we can do it in-thread.

[mtbcarroll]

True. Also at some point, not tied to a client call, we'll want a separate checksummer job anyway for the subsequent re-checks of files.

[jmoore]

Definitely. Initially a quartz job would probably suffice until we have a proper MQ.

[mtbcarroll]

We seem to have curious use of SHA1 in,

• server
  • PixelsImpl sets it to "Pending..."
• blitz
  • OMEROMetadataStoreClient sets it for "Foo" for a Pixels instance
  • SharedResourcesI sets it to "UNKNOWN" for a new table
  • AbstractRepositoryI sets it to the repository UUID
  • CheckedPath can set it to ""
• insight
  • OMEROGateway sets it to "pending".

[jmoore]

Of all of these, the only one that I know to be used is AbstractRepositoryI all the rest amount to "we didn't calculate it". I don't know of any logic which actively expects any of "UNKNOWN", "pending", "Pending...", etc. (and certainly not Foo!) so it would seem fine to at least standardize though as a first step.

[jmoore]

Not included in fs demo 2 tag. Removing from sprint

[mtbcarroll]

AbstractRepositoryI seems to (ab)use the sha1 column but in develop the RepositoryDaoImpl does use the UUID stored there and it all seems to work. Separately, develop's CheckedPath quite reasonably puts empty strings in there to signify directories and non-existent files, but it really makes me wonder if the column should be made nullable and then these other string constants can also simply be null.

OMEROMetadataStoreClient and PixelsImpl are concerned with Pixel instances' sha1 properties which perhaps this ticket doesn't care about. Though, dev_4_4's OMEROMetadataStoreClient also seems to be using it on OriginalFile instances when archiving on image import, and I haven't yet satisfied myself this gets fixed at the other end after upload.

Similarly, perhaps OMEROGateway's setting of it for file upload is fixed at the other end by the call stack eventually hitting RawFileBean.save but I've not yet seen how this happens.

What's going on with SharedResourcesI and the tables I'm not sure at all; at least, I'm doubtful that tables' checksums get either updated or consulted.

Some of this I'll try to resolve empirically with the help of a debugger and psql but anyone more familiar with the purpose of these methods and the flow of control among them is very welcome to chime in with spoilers.

[jmoore]

For any of the upload scenarios, like the gateway, we should really move to a verification step to know that the file has actually been properly uploaded. As to nullability, certainly an option to consider.

[mtbcarroll]

Regardless of any unresolvedness in the above, proposed first steps for dev_4_4 look to be to ensure that SHA1's are actually checked for,

• OMEROGateway.uploadFile, using the checksum from RawFileBean.save (test using file attachments)
• file import from Insight and CLI,
  • for original metadata import into Files/ (from RawFileBean.save's checksum)
  • for archived files, which currently have an SHA1 of "Pending" from OMEROMetadataStoreClient.createOriginalFileFromFile (different in develop).

[mtbcarroll]

One issue is that of how to write integration tests for ImportLibrary that simulate import-time server/client checksum mismatch.

[jmoore]

Catch a notification event, open a new RawFileStore and overwrite some of the data?

[mtbcarroll]

​http://github.com/openmicroscopy/openmicroscopy/pull/890 addresses some of the above. If it omits anything that is important to resolve in the near term, feel free to comment here in order to draw attention to that gap.

[mtbcarroll]

On the topic of nullability for inappropriate OriginalFile instance properties, ​https://github.com/openmicroscopy/openmicroscopy/pull/892 is now opened.

[mtbcarroll]

A shorter-term to-do here is checksums for attaching files through the web client. A good starting point might be to look at components/tools/OmeroWeb/omeroweb/webclient/controller/container.py which leaves a hash in the DB of 'pending'.

[mtbcarroll]

Split download verification out into ​http://trac.openmicroscopy.org.uk/ome/ticket/10754

[mtbcarroll]

The above shorter-term to-do is addressed in ​https://github.com/openmicroscopy/openmicroscopy/pull/1073

I am not aware of any outstanding to-do's.

[bpindelski]

I think there is still one place that is causing exceptions. OMEROGateway.java has still "pending" strings set as SHA1 files (in the OriginalFile upload(SecurityContext ctx, File file, String mimeType, long originalFileID) method. Try attaching a zero-length file annotation to a P/D/I in Insight. That will cause a checksum mismatch. Quick tests have showed that using the provider for checksum calculation solves the issue. Should be easy to fix.

[bpindelski]

Closing due the amount of work remaining on other fronts before Paris 2013. If the issues still cause bugs, separate tickets can be opened. Comments here might be useful in the future.