Refactoring of error propagation and more

[zonia3000]

Checklist before merging

• I added an appropriate entry to CHANGELOG.md
• Let's also add a few words in some docs file (maybe https://github.com/fractal-analytics-platform/fractal-web/blob/main/docs/structure.md)

---

I think that when an error happens while using the modal handled by the ConfirmActionButton component, the error should be displayed inside the modal and the modal should not be closed in that case, so I modified the handleCallbackAction() function inside the ConfirmActionButton component to catch a specific AlertError that can be thrown by the custom callback set on the component. On all the other cases (when we are not using the modal) I expect to check the response.ok value and open directly a StandardErrorAlert, without any kind of error rethrowing logic.

I introduced the src/routes/api/[...path]/+server.js file, that acts like a proxy for the Python backend. The following features have been rewritten calling these proxy endpoints and removing the duplication of calls:

• creation and listing of projects
• CRUD of datasets
• CRUD of dataset resources

Note: I'm adding the @type comments in various places. This suggests the types to the IDE and makes it happier without having to move to TypeScript.

Closes #250
Closes #289
Closes #248
Closes #178
Closes #291

[tcompa]

Here is a first round of (human) tests

creation and listing of projects

• creating a new project -> OK
• creating a new project with the same name as another one -> error is displayed correctly -> OK
• listing projects -> all as expected (both after success/failure in project creation) -> OK

CRUD of datasets

• read a dataset -> OK
• edit dataset name -> OK
• edit dataset read-only property -> OK
• add dataset type -> OK
• unset dataset type: this has a glitch unrelated to this PR (as already identified by @zonia3000), since the action goes through but with no effect (likely because of stripping nulls from request bodies)
• deleting a dataset -> OK
• deleting a dataset related to a job -> error is displayed correctly -> OK

CRUD of dataset resources

• creating resource -> OK
• creating resource with non-absolute path -> error is displayed correctly -> OK
• deleting resource -> OK
• viewing existing resources -> OK

[zonia3000]

Closes #289

[tcompa]

Closes #289

I confirm that 9fcf784 fixes #289 (i.e. I can now create a task without setting its version in the form).

[tcompa]

Some more tests:

• Successful task collection
• Failed task collection -> error displayed
• Successful task edit
• Failed task edit -> to check

[tcompa]

EDIT: this was before the relevant commit, to repeat

• Failed workflow import -> error displayed
• Successful workflow import

GLITCH: after a failed workflow import, close the modal, then perform a successful import. The red alert of the first error is still there.

EDIT: all good, see #288 (comment)

[tcompa]

• Successful workflow creation
• Failed workflow creation (existing name) -> displays error
• Successful workflow delete
• Failed workflow delete -> displays error
• Successful task reordering within workflow

[tcompa]

• workflowtask.meta successful update
• workflowtask.meta failed update
• workflowtask.args successful update
• workflowtask.args failed update

[zonia3000]

GLITCH: after a failed workflow import, close the modal, then perform a successful import. The red alert of the first error is still there.

This should be ok now

[zonia3000]

unset dataset type: this has a glitch unrelated to this PR, since the action goes through but with no effect (likely because of stripping nulls from request bodies)

I tried to set type to null, but I receive String attribute 'type' cannot be None, so it seems that it can't be unset in any case.

[tcompa]

Refactored import workflows, project deletion and project name update

• successful workflow import
• failed workflow import
• successful project-name update
• failed project-name update (see below)
• successful project delete

A failed project-name update results into a very compact error message, with no details:

I guess this was on purpose? Fine with me, when such a limited-scope action fails, but in general let's try to include the error message.

Fixed workflow import error glitch

Fixed, thanks!

[tcompa]

unset dataset type: this has a glitch unrelated to this PR, since the action goes through but with no effect (likely because of stripping nulls from request bodies)

I tried to set type to null, but I receive String attribute 'type' cannot be None, so it seems that it can't be unset in any case.

It's OK.
This would fit into a broader review of request body validation, but it's out of scope here.

[zonia3000]

• Refactoring of error propagation and more #288 (comment) (repeated API calls for job logs/info)
• Refactoring of error propagation and more #288 (comment) (Review use of browser cache for task-collection events #291)

fixed

[tcompa]

Thanks for aa5542d, I confirm it works as expected (or at least as I expected):

[tcompa]

I'm not sure that 5fecc0f works as expected, with respect to #291.

• I perform some task collections (either successful or failed).
• I go and remove one of them from the state table in the db
• I refresh the page, and they are all still listed in the "Task collections" list
• I click on "info" for the one I removed, and then I get something as in the screenshot below. Meanwhile, fastapi responded with a 404, as in

INFO:     127.0.0.1:46274 - "GET /api/v1/task/collect/1/?verbose=True HTTP/1.1" 307 Temporary Redirect
INFO:     127.0.0.1:46270 - "GET /api/v1/task/collect/1?verbose=True HTTP/1.1" 404 Not Found

Then in which cases do we automatically remove an item from the list?

[tcompa]

Related to the last comment, note that the logs shown in the screenshot (below the error message) most likely belong to a different item, and not to the one that is being fetched now (which is not surprising, since that fetch will fail with 404).

[tcompa]

Another minor glitch question:
I do observe logs like

INFO:     127.0.0.1:46274 - "GET /api/v1/task/collect/1/?verbose=True HTTP/1.1" 307 Temporary Redirect
INFO:     127.0.0.1:46270 - "GET /api/v1/task/collect/1?verbose=True HTTP/1.1" 404 Not Found

This is probably due to the trailing slash, right? In fractal-server we do not add a trailing slash when the path ends with an ID (while we are a bit less consistent when the path ends with a word, because then it depends on whether the endpoint is our own or one that we take from fastapi-users).

[zonia3000]

Also remove/refactor src/routes/projects/[projectId]/jobs/[jobId]/logs/+server.js

43080bb refactored the download of job logs. Notice that while passing through the "Svelte proxy" the response body isn't streamed, so the user can experience a sort of lag during the download if the content of the file is very big. In any case this happened also before, because the response was read and then converted to a blob object before the download, so I assume it is not a problem. This note is just a consideration in case we will need to handle big files in the future. It could be solved setting up a "real" proxy between the front-end and the Python API.

[zonia3000]

Another minor glitch question: I do observe logs like

INFO:     127.0.0.1:46274 - "GET /api/v1/task/collect/1/?verbose=True HTTP/1.1" 307 Temporary Redirect
INFO:     127.0.0.1:46270 - "GET /api/v1/task/collect/1?verbose=True HTTP/1.1" 404 Not Found

This is probably due to the trailing slash, right? In fractal-server we do not add a trailing slash when the path ends with an ID (while we are a bit less consistent when the path ends with a word, because then it depends on whether the endpoint is our own or one that we take from fastapi-users).

8f370ec mitigates this leaving the trailing slash only in the POST endpoints, that usually doesn't have the id at the end, however it doesn't cover all the cases. Also in this case a "real" proxy can help fixing the glitch, but this non-homogeneous behavior is a bit weird. Could we consider to set the same behavior on all the endpoints?

[zonia3000]

• Review the status of Switch from using formData to binding variables #265

The only remaining action that uses formData is the login. I wouldn't touch this part in this PR, since it involves the call of /auth/ API and we should add another set of "proxy endpoints" to handle it.

[tcompa]

I'm not sure that 5fecc0f works as expected, with respect to #291.

* I perform some task collections (either successful or failed).

* I go and remove one of them from the `state` table in the db

* I refresh the page, and they are all still listed in the "Task collections" list

* I click on "info" for the one I removed, and then I get something as in the screenshot below. Meanwhile, fastapi responded with a 404, as in

INFO:     127.0.0.1:46274 - "GET /api/v1/task/collect/1/?verbose=True HTTP/1.1" 307 Temporary Redirect
INFO:     127.0.0.1:46270 - "GET /api/v1/task/collect/1?verbose=True HTTP/1.1" 404 Not Found

Then in which cases do we automatically remove an item from the list?

I confirm that after 32a6d3c I observe the expected behavior.

[tcompa]

I think the following one is a bug:

If I submit a workflow, and if I do set some specific values of the first/last task to run, the payload for the apply endpoint looks like

{
        'worker_init': None,
        'first_task_index': None,
        'last_task_index': None,
    }

while I would expect first_task_index and last_task_index to be something (e.g. 0 and 2).

[tcompa]

Also remove/refactor src/routes/projects/[projectId]/jobs/[jobId]/logs/+server.js

43080bb refactored the download of job logs. Notice that while passing through the "Svelte proxy" the response body isn't streamed, so the user can experience a sort of lag during the download if the content of the file is very big. In any case this happened also before, because the response was read and then converted to a blob object before the download, so I assume it is not a problem. This note is just a consideration in case we will need to handle big files in the future. It could be solved setting up a "real" proxy between the front-end and the Python API.

Thanks for the note. Agreed, this is not a priority now.

Meanwhile I tested the download-logs feature still works as expected.

[tcompa]

I think the following one is a bug:

If I submit a workflow, and if I do set some specific values of the first/last task to run, the payload for the apply endpoint looks like

{
        'worker_init': None,
        'first_task_index': None,
        'last_task_index': None,
    }

while I would expect first_task_index and last_task_index to be something (e.g. 0 and 2).

We found the error source with Sonia (a typo in the parameter name), and it's easy to fix.

Other than that, I don't have anything more to add for the moment.