[CRITICAL BUG] backend removes the target directory if we save an image without a filename

[kswang1029]

Describe the bug If we save an image without giving a filename in the file browser dialog and click the save button, there will be an error toast showing the target saving directory not found. But then the original target directory is actually gone. This is a critical bug as user data will be lost.

To Reproduce

TEST WITH CAUTIONS!

Steps to reproduce the behavior:

1. create a test directory call "test_save_image" somewhere in the directory tree (eg ~/carta_image_pool/test_save_image)
2. load an image
3. use the save image dialog to save it with a filename in ~/carta_image_pool/test_save_image. This should be successful and we do see a new file is created in the target directory.
4. now use the save image dialog to save it once again but with giving a filename (ie empty filename). Then we will see an error toast and if we check the existence of ~/carta_image_pool/test_save_image, it is gone!

Expected behavior We should apply to fixes:

1. at the frontend, if the target saving filename is gone, disable the save button
2. at the backend, if the target saving filename is an empty string, throw error messages and do nothing saving.

Platform info (please complete the following information):

• OS [e.g. macOS Monterey]: macOS Ventura
• Browser [e.g. chrome, safari, electron app]: chrome, electron app
• Browser version [e.g. 22]:
• Backend branch [e.g. dev, v3b2 release]: dev, v4.1-stable
• Frontend branch [e.g. dev, v3b2 release]: dev, v4.1-stable

[kswang1029]

@confluence please arrange a fix for this really critical bug.

[pford]

I can take it @confluence

[confluence]

I can confirm the symptoms reported by @kswang1029 if you choose to save the file in FITS format with a blank name. If you try to save in CASA format with a blank name, the save succeeds, and causes the parent directory to be overwritten with the saved CASA image, which is also a directory (the same data loss occurs, because the original directory is still deleted).

I assume that this is yet another path edge case bug where we're treating a blank filename inside directory foo/bar as the same thing as filename bar in directory foo in one part of the code while simultaneously looking for the original parent directory foo/bar in another part. I'm not sure of the exact mechanism, but I guess that in the CASA case the expected directory is created, whereas in the FITS case it isn't, which later causes an error when it isn't found.

[confluence]

(I agree with @kswang1029's suggested fixes -- for usability, we should disable saving with a blank name in the frontend, but we still need to error out on blank filenames in the backend, because users will be able to bypass the GUI with scripting.)

[confluence]

@kswang1029 @pford bad news: there's a broader problem here, which is that you can also currently overwrite any directory by typing in the directory's name as the name of the file to save. There are no checks in place to prevent overwriting an existing directory -- this protection is only applied to ordinary files, and directories which are detected to be CASA images. It should be extended to normal directories as well.

(I think we should entirely refuse to overwrite a directory of the same name. This is far too dangerous for a single "are you sure" dialog.)

A related issue: while testing, I deleted a CASA image and created a new directory with the same name, but the CARTA file browser still shows this as a CASA image, even if I exit and re-enter the parent directory, or close and reopen the browser. I had to shut down my local CARTA and restart before the directory correctly registered as a directory. I'm not sure if metadata for these paths is being cached somewhere, and if it's a backend or frontend issue.

[pford]

The overwrite dialog only appears for an image (file or directory) in the current directory; with the user's permission, an image can be overwritten. The dialog does not appear for a typed-in existing ordinary file or directory, for a typed-in existing directory/file or directory/directory (where the file and subdirectory may or may not be an image), or for any path when using scripting.

The backend can detect if the SaveFile directory/filename path exists, if it is a file or directory, and if it is an image. But how does the backend know when to overwrite and when to fail? It should fail for a directory which is not an image, but what about the other cases?

[pford]

I could not reproduce the issue where a casa image replaced by an empty directory was shown as CASA. When I used File > Open Image or File > Save Image, the browser showed a directory.

[confluence]

I think the subdirectory/name cases should be consistent with the behaviour for names without subdirectories.

I believe that the confirmation dialog is frontend-only behaviour (the frontend issues the save command to the backend after confirmation, if any). So handling confirmation for overwriting images in subdirectories is something that needs to be changed in the frontend. The backend should only ensure that a non-image directory is never overwritten (regardless of where it is).

[kswang1029]

I think the subdirectory/name cases should be consistent with the behaviour for names without subdirectories.

I believe that the confirmation dialog is frontend-only behaviour (the frontend issues the save command to the backend after confirmation, if any). So handling confirmation for overwriting images in subdirectories is something that needs to be changed in the frontend. The backend should only ensure that a non-image directory is never overwritten (regardless of where it is).

What if before confirmation at the frontend, the directories at the backend have changes (thus the directory state at the frontend is outdated)? Might be simpler (and consistent) to have backend handle the business logic?🤔 But if when the backend has doubts and needs confirmation from the user, yes having a popup at the frontend is fine.

[confluence]

The problem is that I don't think there's currently any way for the frontend and backend to have back-and-forth communication about this (backend tells frontend "that file exists; are you sure?"; frontend pops up a dialog; frontend tells backend "yes, the user is sure"; etc.). That's why the confirmation is just in the frontend, and the backend assumes that it should proceed.

I don't know if we can ever avoid potential staleness -- it's just as possible with the current confirmation dialog for files in the current directory.

[confluence]

We could potentially add a "force overwrite" flag, and have a workflow like:

1. Frontend sends save request without the flag
2. If the file exists, backend returns a failure response with a "refusing to overwrite" message
3. The frontend checks for this message, and if it exists, pops up an "are you sure?" dialog
4. If the user confirms, the frontend resends the request with the flag turned on
5. If the file exists and the flag is on, the backend performs the save (as long as the existing file is not a non-image directory)

The benefit of doing this is that the backend will be 100% responsible for the business logic, and the frontend won't have to do any logic to see if files exist or are images or directories. This would, however, require a protobuf change.

We could also have a "force overwrite recursive" flag, which allows the user to overwrite non-image directories (with a different and more scary confirmation dialog), but I still don't think we should allow this at all. We don't let users add or delete directories through the file manager; it's something they're currently responsible for doing through other means, and I think it's fine to keep it that way (unless these features are requested in future).

[pford]

I also thought that an overwrite flag would be the best way to ensure the right thing happens, and agree the backend should never overwrite non-image directories (maybe non-image files as well?) regardless of this flag. So we need two failure messages: "need confirmation" and "nope, not gonna do it".

If the frontend knows the file is an image (from the most recent file list), it can use the dialog as it does now and set the flag; the backend will still check if it is an image before overwrite. The flag would also be available with scripting, so the user is warned and must be intentional about overwrites.

[confluence]

I think non-image files are OK to overwrite -- it's just directories that have the potential for really bad data loss.

The wrapper doesn't currently implement saving images (although maybe that's a good feature to add soon). I agree that the flag should be exposed to scripting -- it makes sense to add it to the parameters of the low-level frontend function that issues the save command to the backend, and to handle the confirmation / retrying logic in a higher-level function. Then scripting will be able to set that flag when calling the lower-level function directly.

[pford]

I opened draft PRs for the backend and for protobuf. Need frontend development to set overwrite flag.

[pford]

Opened frontend issue #2384