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

Add workflows API to create, get, update and list #2792

Merged
merged 47 commits into from
Jan 15, 2025
Merged

Conversation

jyeshe
Copy link
Member

@jyeshe jyeshe commented Dec 17, 2024

Description

This PR adds an API that allows to create, update, get and list workflows. The authentication uses a Bearer token and the user of the Personal Access Token must belong to the project related to the workflow.

For POST and PUT the body should be a JSON encoded workflow having {name, project_id, edges, jobs, triggers}.
A PATCH can be also used to update a workflow passing only a difference of the fields that needs to be changed.

When certain workflow preconditions are not met, it returns 422 (Unprocessable Entity) with an error message describing the violation:

  • "The triggers cannot be replaced, only edited or added." (cannot delete a webhook trigger for example)
  • "A workflow can have only one trigger enabled at a time."

A POST or PUT or PATCH might also fail if the outcome would be having more active workflows than the limit (when running on the billing app). For this case it also returns a 422 with the message:

"Your plan has reached the limit of active workflows."

Some additional information on protocol syntax in regards to paths and responses:

  • All paths follow a nested REST resource on projects (e.g PATCH /api/projects/:project_id/workflows/:id).
  • Getting a single workflow returns a JSON with a workflow
  • Getting a list of workflows returns a JSON with a workflows
  • All other paths return a JSON {"id": <workflow_uuid>, "error": <error_message>}

Closes #1887

Validation steps

Create a Personal Access Token (PAT) on Lightning user profile menu and make these curl requests with authorization: Bearer <the_PAT>, accept: application/jsonand content-type: application/json

Example of POST to create using $token and project_id 86a7888e-8a07-4673-9dbf-43ba016dc790

curl -s -X POST -H "content-type: application/json" -H "authorization: Bearer $token" localhost:4000/api/projects/86a7888e-8a07-4673-9dbf-43ba016dc790/workflows -d "{\"id\":null,\"name\":\"work1\",\"project_id\":\"86a7888e-8a07-4673-9dbf-43ba016dc790\",\"edges\":[{\"enabled\":true,\"id\":\"a8f136af-4527-4009-9122-f6e67fdf0a44\",\"source_trigger_id\":\"858f21e6-f301-4c56-8bcf-66b7378d1e54\",\"target_job_id\":\"66810fb2-e770-4d71-9c3c-44a0354f650d\",\"condition_type\":\"always\",\"source_job_id\":null}],\"jobs\":[{\"id\":\"66810fb2-e770-4d71-9c3c-44a0354f650d\",\"body\":\"fn(state => { return {...state, extra: \\\"data\\\"} })\",\"name\":\"job-9\",\"adaptor\":\"@openfn/language-common@latest\",\"inserted_at\":null,\"updated_at\":null}],\"triggers\":[{\"id\":\"858f21e6-f301-4c56-8bcf-66b7378d1e54\",\"comment\":null,\"custom_path\":null,\"cron_expression\":null,\"type\":\"webhook\",\"enabled\":true}],\"inserted_at\":null,\"updated_at\":null}"
{"error":null,"id":"2eed727b-99a1-4bf8-8312-1ab460f4fd47"}

it shall return a JSON in this format: {"error":null,"id":"2eed727b-99a1-4bf8-8312-1ab460f4fd47"}

After that execute curl commands for:
GET /api/projects/:projectId/workflows
GET /api/projects/:projectId/workflows/:workflowId
PUT /api/projects/:projectId/workflows/:workflowId
PATCH /api/projects/:projectId/workflows/:workflowId

Additional notes for the reviewer

Currently PUT and PATCH are handled by the same update function so can be used interchangeably until a final validation is added.

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

Pre-submission checklist

  • I have performed a self-review of my code.
  • I have implemented and tested all related authorization policies. (e.g., :owner, :admin, :editor, :viewer)
  • I have updated the changelog.
  • I have ticked a box in "AI usage" in this PR

@jyeshe jyeshe force-pushed the workflows-api branch 2 times, most recently from 4adef60 to d1cda5c Compare December 17, 2024 16:48
@jyeshe jyeshe mentioned this pull request Dec 17, 2024
@jyeshe jyeshe force-pushed the workflows-api branch 2 times, most recently from 3a78e2c to 4830973 Compare December 18, 2024 12:10
@jyeshe jyeshe self-assigned this Dec 18, 2024
@jyeshe jyeshe force-pushed the workflows-api branch 3 times, most recently from f2d6262 to bf7cc45 Compare December 18, 2024 20:39
@jyeshe jyeshe marked this pull request as ready for review December 18, 2024 20:39
@jyeshe jyeshe force-pushed the workflows-api branch 3 times, most recently from 7dfbc4b to 85550ce Compare December 18, 2024 20:49
@jyeshe jyeshe force-pushed the workflows-api branch 6 times, most recently from 0aa06ac to 8db3687 Compare December 18, 2024 21:55
@jyeshe jyeshe changed the title Add workflows api to create, get and list Add workflows API to create, get, update and list Dec 18, 2024
@jyeshe
Copy link
Member Author

jyeshe commented Jan 10, 2025

This is where I'm at now:
⛔ get workflows with invalid project id (expect 404 and error object, got 400 webpage)

Sorry Joe, previously it was implemented only on the get workflow, now it's done for the index

⛔ get workflows for project that does not exist (expect 404, got 401)

The reason it's returning 401 Unauthorized it's because the project id is used for ACL. The user could check the project id on 401 but if 404 is really helpful for the user we can add a query or subquery for all operations. In any case I've added the 401 on the spec with a description "User not allowed to access the project" for existing projects.

⛔ PUT a new job with arbitrary ids

The arbitrary ids are supported only on the POST because the triggers cannot be replaced. If arbitrary ids are allowed only for jobs we could have a single rule for POST and PUT. However the PUT we need to be aware that all jobs would be replaced and I need to check what is behaviour of the edges on this jobs full update.

⛔ PUT a new orphaned job without an id

On PUT everything is expected to have an id for being an update. It's doable however.

@jyeshe
Copy link
Member Author

jyeshe commented Jan 13, 2025

Take aways from our meeting:

  • There is no particular rule for disconnected/orphan jobs in regards to ids. Client apps need to pass ids on all jobs.
  • All sucessfull operations will return the resulting workflow.
  • Make sure that PUT is able to remove jobs and edges.
  • RESTful PATCH shall be available (put back/reenable).

According to Stu, the Workflows Policy are prone to be rewriting.

This reverts commit 8f2e5b7.
@jyeshe
Copy link
Member Author

jyeshe commented Jan 15, 2025

@jyeshe any idea why the tests are timing out 🤔 ?

to be continued here: #2822

Copy link
Collaborator

@midigofrank midigofrank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jyeshe , this looks really great.

There's a comment I've left regarding using if..else instead of cond.
Also, there's a conflict on the CHANGELOG.

defp save_workflow(%{"project_id" => project_id} = params, user) do
active_triggers_count = count_enabled_triggers(params)

cond do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we just use if..else here?

@jyeshe jyeshe requested review from midigofrank and stuartc and removed request for stuartc January 15, 2025 10:59
Copy link
Member

@stuartc stuartc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @jyeshe, left some comments; I think at least we need to add the other condition_... fields to the Edge @derive property.

Comment on lines 34 to 42
@derive {Jason.Encoder,
only: [
:id,
:condition_type,
:enabled,
:source_job_id,
:source_trigger_id,
:target_job_id
]}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we also need condition_expression and condition_label to allow for JS based conditions no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 4 to +5
msgid "has already been taken"
msgstr "This email address already exists."
msgstr "This value should be unique."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there not a way to make the distinction between emails (in this case) and other uniqueness violations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible but we gonna need to call a different translation. Opened an issue for identifying and changing on all places of the app to have a specific message for email: #2824

@midigofrank midigofrank merged commit 4967c6b into main Jan 15, 2025
7 of 8 checks passed
@midigofrank midigofrank deleted the workflows-api branch January 15, 2025 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

API for Workflows
4 participants