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

Server does not remove unset optional task args values #196

Closed
rkpasia opened this issue Jun 20, 2023 · 4 comments
Closed

Server does not remove unset optional task args values #196

rkpasia opened this issue Jun 20, 2023 · 4 comments
Labels
to be discussed Not ready to implement

Comments

@rkpasia
Copy link
Contributor

rkpasia commented Jun 20, 2023

With the changes required by #190, the server will no longer receive payloads with properties that have null values.

This will bing to the following scenario:

Given the following schema:

{
  "additionalProperties": false,
  "properties": {
    "sleep_time": {
      "description": "Interval to sleep, in seconds.",
      "title": "Sleep Time",
      "type": "integer"
    }
  },
  "required": [],
  "title": "DummyParallel",
  "type": "object"
}

If an user sets, through the client by saving edits, the sleep_time property, then the server will receive the following HTTP request, with given payload.

PATCH /api/v1/project/1/workflow/2/wftask/4
{
    "args": {
        "raise_error": true,
        "message": "test",
        "sleep_time": 1
    }
}

Upon following refresh of the page, the client will receive the same payload, saved before.

Let's assume that the user wants to remove the optional parameter sleep_time

To do so, the user deletes from the UI the number inside the form and confirms to save changes.

The fractal server, after #190 will receive the following payload.

PATCH /api/v1/project/1/workflow/2/wftask/4
{
    "args": {
        "raise_error": true,
        "message": "test"
    }
}

The user now expects that, upon refreshing the page, or upon running the task, the sleep_time property is not set.
Instead, the server, will handle the client the following payload.

GET /api/v1/project/1/workflow/2
[...]
    "args": {
        "raise_error": true,
        "message": "test",
        "sleep_time": 1
    }
[...]

Shall we expect that the server deletes or sets the sleep_time property to null or deletes it? @tcompa @jluethi

@tcompa
Copy link
Collaborator

tcompa commented Jun 20, 2023

I think we should modify the fractal-server task-parch endpoint, I'll open a more precise issue later.

@tcompa
Copy link
Collaborator

tcompa commented Jun 20, 2023

Shall we expect that the server deletes or sets the sleep_time property to null or deletes it?

If we move on as in fractal-analytics-platform/fractal-server#758, then we should expect that the server deletes args["sleep_time"], when the patch-workflowtask endpoin receives a payload like

{
    "args": {
        "raise_error": true,
        "message": "test"
    }
}

@tcompa
Copy link
Collaborator

tcompa commented Jun 21, 2023

Shall we expect that the server deletes or sets the sleep_time property to null or deletes it?

If we move on as in fractal-analytics-platform/fractal-server#758, then we should expect that the server deletes args["sleep_time"], when the patch-workflowtask endpoin receives a payload like

{
    "args": {
        "raise_error": true,
        "message": "test"
    }
}

We now released fractal-server 1.3.0a9, and this is the correct behavior to expect.

Note that for parameters that do have a default value in the args schema (which is not the case for sleep_time, in this case), removing them corresponds to resetting their value to the default.

@tcompa
Copy link
Collaborator

tcompa commented Jun 22, 2023

Closed as of recent fractal-server update

@tcompa tcompa closed this as completed Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to be discussed Not ready to implement
Projects
None yet
Development

No branches or pull requests

2 participants