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

Fix FileToDatasetConverter picking up non-existing files #479

Merged
merged 1 commit into from Aug 26, 2022

Conversation

imagejan
Copy link
Member

It turns out that io.canOpen actually returns true for a file like blobs.gif even if it doesn't exist on the given path (see #434 (comment)). FileToDatasetConverter should really only be accepting requests where the file exists (or the name ends with .fake for the special case of the fake file format).

The tests are adjusted to be testing for unsupported formats as well as non-existing but theoretically supported files.

Closes #434.
Should also fix scijava/scijava-common#442.

It turns out that io.canOpen actually returns true for a file like 'blobs.gif' even if it doesn't exist on the given path. FileToDatasetConverter should really only be accepting requests where the file exists (or the name ends with '.fake' for the special case of the fake file format).

The tests are adjusted to be testing for unsupported formats as well as non-existing but theoretically supported files.
@hinerm
Copy link
Member

hinerm commented Aug 25, 2022

Thanks for this @imagejan!

I am getting concerned that we have parallel systems performing the same tasks, in the ConvertService and LocationService - maybe unfounded, but I want to sanity check ourselves here.

For example, if I open a .fake string (whether directly from a string or from a file named on disk) it goes through the TestImgLocationResolver and now that it's a Location it can unambiguously be opened as an ImgPlus and wrapped into a Dataset if desired.

This PR is necessary because of how script parameters are run through the ConvertService - is that correct (in summary)? But we're special-casing fake images, when that logic is already encapsulated in the TestImgLocationResolver.. right?

Looking at your explanation here, I am wondering why any of our formats claim support for a FileLocation that doesn't exist. Would it be reasonable if that was handled somewhere like in AbstractChecker instead of FileToDatasetConverter? That would also address io.canOpen, right?

@imagejan
Copy link
Member Author

Thanks @hinerm for the information.

I didn't know enough about the details of Checker, and when looking at the source of io.canOpen, thought it would be easiest (and maybe safest) to do the check for file existence right here in the converter. And after fixing it, the tests would be failing for .fake so I added the special-casing.

But I agree with you that it would probably preferable to fix it in AbstractChecker or somewhere else upstream. It's just that I didn't know how to best do it, that's also why this issue remained known and open for so long.

@imagejan
Copy link
Member Author

Now that I'm thinking of it: Checker is used in both opening and saving files, right? So checking for file existence in AbstractChecker will break file saving (where the destination usually doesn't exist yet). @hinerm @ctrueden do you have some better hint where the check should be made?

@hinerm
Copy link
Member

hinerm commented Aug 26, 2022

Now that I'm thinking of it: Checker is used in both opening and saving files, right?

Ah good point. This is reason enough for me to just merge as-is 😄

@hinerm hinerm merged commit b16216a into master Aug 26, 2022
@hinerm hinerm deleted the fix-file-to-dataset-converter branch August 26, 2022 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants