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

New OCS endpoint to list text processing tasks #39680

Merged
merged 6 commits into from
Aug 7, 2023

Conversation

julien-nc
Copy link
Member

@julien-nc julien-nc commented Aug 2, 2023

refs nextcloud/text#4613

This adds a /tasks/app/{appId} endpoint to get a list of tasks for the current user, for a specific app and optionally with a specific identifier.

While playing with the API, I discovered that it's impossible to schedule a task while not being authenticated (mostly because of a NotNull Db constraint on the user_id column). But all the endpoints are public. Maybe we should sort this out first.

  • Do we want to allow non-authenticated users to schedule a job?
    • If so, do we want to be able to list all tasks that were scheduled publicly?
    • If not, should we require auth for all those endpoint?
  • Are we fine with a user only being able to list his/her own tasks?
    • This would mean publicly scheduled tasks can be listed by any other non-authenticated user. Are we fine with that?

TODO

  • add proper index(es)
  • allow authenticated users to list all their tasks by app (and optionally by identifier)

@julien-nc julien-nc force-pushed the enh/tp-list-tasks-by-app-and-identifier branch 2 times, most recently from 2291fd1 to 8ee178b Compare August 3, 2023 21:24
@julien-nc
Copy link
Member Author

What's been done:

  • Convert annotations to Php method attributes
  • Rename languageModelManager to textProcessingManager in the API controller
  • Add endpoint for authenticated users to list their tasks by appId (and optionally by identifier)
  • Add manager and mapper methods to:
    • get a task by user id and task id
    • get a list of task by user id and app id (and optionally with a specific identifier)
  • Allow null user_id in textprocessing_tasks so tasks can be scheduled anonymously
  • Add user_id, app_id, identifier index to textprocessing_tasks

@juliushaertl So it's only possible to list tasks by app while being authenticated. This is to prevent anonymous users to be able to list all tasks that were created by other anonymous users. Any problem with that in the Text integration?

@julien-nc julien-nc marked this pull request as ready for review August 3, 2023 21:26
@juliushaertl
Copy link
Member

So it's only possible to list tasks by app while being authenticated. This is to prevent anonymous users to be able to list all tasks that were created by other anonymous users. Any problem with that in the Text integration?

Fine with me as the MVP. I'd then limit the text integration to logged in users only for now.

@marcelklehr marcelklehr added the pending documentation This pull request needs an associated documentation update label Aug 4, 2023
julien-nc and others added 5 commits August 7, 2023 13:27
add a textprocessing_tasks index
convert anotations to method attributes
refactor TP manager
add mapper methods

Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
@julien-nc julien-nc force-pushed the enh/tp-list-tasks-by-app-and-identifier branch from b8321ef to e9e7db9 Compare August 7, 2023 11:29
@julien-nc
Copy link
Member Author

Rebased on master, bumped server version to 28.0.0.2 to trigger the migration step. All good IMO.

@julien-nc julien-nc requested a review from kesselb August 7, 2023 11:30
Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

Seems good, tested and works for my use case. 👍

Psalm is still not happy though.

Copy link
Member

@marcelklehr marcelklehr left a comment

Choose a reason for hiding this comment

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

please make sure to fix psalm, though

Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc julien-nc force-pushed the enh/tp-list-tasks-by-app-and-identifier branch from 2609d8f to f154fe7 Compare August 7, 2023 16:29
@juliushaertl
Copy link
Member

Failing drone acceptance test is unrelated, also there on master https://drone.nextcloud.com/nextcloud/server/38240/57/4

@juliushaertl juliushaertl merged commit eb2f3bc into master Aug 7, 2023
37 checks passed
@juliushaertl juliushaertl deleted the enh/tp-list-tasks-by-app-and-identifier branch August 7, 2023 19:50
@juliushaertl
Copy link
Member

@julien-nc @marcelklehr Is there already documentation for the text processing to extend on?

@juliushaertl
Copy link
Member

Not triggering the backport yet, but I have it on my list of requirements for 27.1

@marcelklehr
Copy link
Member

@juliushaertl

Yes, text processing docs have already been merged, should be easy to extend for this :)

I've also created #39738 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress backport-request enhancement integration pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants