Casa image paths saved incorrectly in workspaces if images added via snippets with a trailing slash

[confluence]

Describe the bug

If a casa image is opened via a snippet with a path which has a trailing space (something that can easily happen if the path was copied from the commandline after tab autocompletion), the image is opened correctly, but when the workspace is saved the name of the image is duplicated at the end of the directory path. This means that when the workspace is loaded the image cannot be opened. This happens because the image's directory is incorrectly set in the metadata when it is opened.

To Reproduce Steps to reproduce the behavior:

1. Open a casa image with snippets, with and without a trailing slash:

   # without slash
   await app.openFile("/home/adrianna/data/casa/test.image");
   # with slash
   await app.appendFile("/home/adrianna/data/casa/test.image/");

2. Save the workspace.
3. Try to load the workspace.
4. If you reproduce this on a local backend (with objects saved as files), you can easily inspect the workspace file and see that the images are saved as follows:

   
   # without slash
   "directory": "/home/adrianna/data/casa",
   "filename": "test.image",

with slash

"directory": "/home/adrianna/data/casa/test.image", "filename": "test.image",

**Expected behavior**

Both images should be saved with the same correct path.

**Platform info (please complete the following information):**

This has been reproduced on Ubuntu Jammy (controller installation saving to a database) and Ubuntu Focal (local installation saving to file). It was present in 4.0 and is still present in 4.1.

**Additional context**

The directory is set incorrectly in the metadata when the image is opened -- you can compare the two open images with snippets:

Snippet code:

console.log(app.frames[0].frameInfo.directory); console.log(app.frames[1].frameInfo.directory);

Result:

09:51:12.574 /home/adrianna/data/casa 09:51:12.575 /home/adrianna/data/casa/test.image

[confluence]

This is a duplicate of this frontend issue. I'm pretty sure that this is still a backend bug and should be fixed in the backend -- the image is correctly being interpreted as an image; a mistake is being made in the splitting of the path.

[pford]

@confluence I do not see where the backend is doing the wrong thing, since it is not involved in saving the workspace which is where the error occurs.

frontend sends OpenFile(directory: "/path/to/casa.image", filename: "") (how do you open an image with no name?), backend responds with OpenFileAck FileInfo(name: "casa.image"). The name field is used for the rendered image and in the Image List so it must be set. The frontend saves the workspace, no protobuf messages exchanged with the backend for this step. If the frontend cannot sort this out, we could add a directory field to FileInfo for the corrected path.

[confluence]

The error doesn't occur when the workspace is saved -- it occurs as soon as the file is opened (see the snippet). But I see what you mean -- I didn't think to compare the OpenFile requests and forgot that a single path parameter to openFile or appendFile is split by the frontend into the directory and filename. So it must be doing the wrong thing here, and that should be fixed in the frontend.

However, the backend's current behaviour is also broken: despite receiving a directory and no file name, it does open the file correctly (is this because the implementation just rejoins the directory and filename, which gives the correct answer?), but returns incorrect metadata (by returning the originally requested directory but deriving the filename from the opened file). Later, other parts of the frontend rely on this metadata to be correct, and things break. The backend should either return an error when it gets this input (which I think would be most correct) or derive both the directory and the filename from the opened file, so that they are consistent.

[pford]

but returns incorrect metadata (by returning the originally requested directory but deriving the filename from the opened file).

No, that is my point, the backend does not return the directory, only the image name (which could be a file, or a directory for CASA and MIRIAD).

The frontend could remove the trailing / before splitting the path, or if the filename is empty then use the last directory in the path for the filename.

[confluence]

Oh, I see. I was assuming that all the file metadata is calculated and returned by the backend -- I will look at this in more detail.

[confluence]

I have created a draft PR in the frontend with a suggested fix.

I do think that the backend's current behaviour is not correct, and that if it is asked to open a file with a blank name in the directory A/B/C it should return an error rather than opening the file called C in the directory A/B -- that isn't the file which was requested! However, if the normalization of the filename is corrected in the frontend, this should prevent that kind of request from being made in the future -- so it's not a high-priority fix.

[pford]

So I think you are saying that the backend's response to the code snippet await app.appendFile("/home/adrianna/data/casa/test.image/"); should be OpenFileAck with success=false and an error message indicating "no filename"? In that case, the frontend could check for an empty filename and issue the error itself instead of sending OpenFile.

Why does the frontend separate the directory and "filename" (image name) instead of using a path? OpenFile("path"), SaveWorkspace("path"), etc. When does it use the directory without the filename?

But I still think CARTA should sort out a trailing /, open the image, and function correctly rather than issue an error to the user. If it cannot be handled in the frontend, change the ICD and add directory to FileInfo in OpenFileAck for future reference.

[confluence]

openFile and appendFile accept various combinations of parameters. I think that the original method of invocation from the GUI was with a pre-split directory path and filename, but if it's invoked from a snippet, there can be a single parameter (which goes in the directory path) and if it's invoked from the URL parameter as in the linked bug it seems that the single parameter goes in the filename and the path is null. Inside loadFile in both these cases the single parameter is split, and this is where the problem is (we're hand-parsing the path and not handling this edge case correctly; I'm suggesting using a path library).

IIRC the workspace saving function also uses a single path, but it constructs this path by joining the directory and filename, which are stored separately in the metadata. This is why the backend returning file C n A/B when asked for blank in A/B/C is a problem (the frontend then combines the "incorrect" filename with its own "correct" directory to get a malformed path -- earlier I forgot where the split happens, and thought that the backend also returns the directory, but that was a red herring). The same thing happens in other parts of the code, e.g. when sessions are saved.

The proposed change in the PR normalizes the parameters to a sensible directory and filename regardless of what form the parameters take. So whether the user's snippet specifies A/B/C or A/B/C/ or A and B/C or A/B and C, the backend will always get A/B and C.

But the backend component should respond with an error if the frontend component is giving it incorrect input (as it has been in this case -- the request is malformed!). Having the backend interpret this as a different, resolvable request has been masking a logic bug in the frontend code -- all the resulting bugs are caused by the frontend assuming that the backend has successfully returned the requested file, when it has actually returned a different file (with a mismatch in the metadata). If making this change causes other parts of the frontend to break, it means that other parts of the frontend are issuing malformed requests, and should also be fixed.