Enhancement: Make tables import asynchronous

[Koc]

Right now module not ready to import large tables. This PR improves import flow by moving it into asynchronous job.

🗒️ TODO:

• make import work for uploaded (not selected) files as well
• revert changes to old endpoints, introduce new one instead
• properly parse activity notification
• fix pipelines and old tests
• add new tests?

🔍 Preview

Notification once import scheduled

Activity item with import stats

🎥 Demo
https://github.com/user-attachments/assets/6be864ef-fb26-40a3-9e4c-c98e1d55419d

[blizzz]

Please keep in mind, existing API shall remain stable, but may be flagged deprecated.

[Koc]

@blizzz What exactly do you mean? Do not change already existent endpoint (keep it synchronous) but introduce v2/v3 for async tasks (even if this endpoints will be not in use on FE side)?

[blizzz]

@blizzz What exactly do you mean? Do not change already existent endpoint (keep it synchronous) but introduce v2/v3 for async tasks (even if this endpoints will be not in use on FE side)?

Yes, exactly. The old ones can get the @deprecated annotation and point to the new method.

[blizzz]

replied to questions

[Koc]

@blizzz I've reverted all changes to old endpoints. So they are completely untouched. Now we have also scheduleImportIn(Table|View) + scheduleImportUploadIn(Table|View). FE uses new endpoints.

Also it is possible to import uploaded files (app data used under the hood)

[blizzz]

It is shaping up nicely! It looks like a file was not committed. Did not test yet.

[Koc]

I will rebase current PR and fix conflicts once #1824 got merged. Because both of those 2 PRs changes import a lot. Let's go step by step

[enjeck]

I will rebase current PR and fix conflicts once #1824 got merged

Hi @Koc . That got merged. Have time to continue this? Otherwise, I can take over from here to complete it

[Koc]

@enjeck yeah, I was about to rebase this PR

My plan:

• fix conflicts after rebase and make PR work at least for the basic scenarios ✔️
• display activity ✔️
• start work on tests 🚧

I will try to do as much as I can by myself and ask for your help in case of struggling

[Koc]

@enjeck more or less done

At least we have green pipeline with adjusted old tests. I'm thinking how to extend tests with a new functionality. Meanwhile you can do another round of code review

[enjeck]

@enjeck more or less done

At least we have green pipeline with adjusted old tests. I'm thinking how to extend tests with a new functionality. Meanwhile you can do another round of code review

Thanks for working on this! I don't think we'll need any new tests here, since the current tests are already extensive?

[Koc]

@enjeck in general yes, I've already actualized existent cypress tests

[Koc]

@enjeck @blizzz ok, so if we don't need any other tests here - what's blocks us from merge?

[enjeck]

@enjeck @blizzz ok, so if we don't need any other tests here - what's blocks us from merge?

Sorry, I had this on my to-review list. I was mostly thinking about if multiple simultaneous imports could overload the server and if we want to rate limit on job creation. We recently added rate limits to import previews e.g https://github.com/nextcloud/tables/blob/main/lib/Controller/ImportController.php#L60 since there was the possibilility to DoS the server without these limits. Wondering what @blizzz thinks

[blizzz]

@enjeck @blizzz ok, so if we don't need any other tests here - what's blocks us from merge?

Sorry, I had this on my to-review list. I was mostly thinking about if multiple simultaneous imports could overload the server and if we want to rate limit on job creation. We recently added rate limits to import previews e.g https://github.com/nextcloud/tables/blob/main/lib/Controller/ImportController.php#L60 since there was the possibilility to DoS the server without these limits. Wondering what @blizzz thinks

The factor for the DoS is that you could fire multiple requests at the same time and Tables would be busy parsing everything. The rate limiting makes it less likely by allowing only so much at the same time.

Now with that asynchronous approach, there is the upload, but otherwise the file is not being touched. So the risk that processing the input files leads to the DoS is negligible, as it might run sequentially or with just a few runners.

It could congest the job queue though, therefore it would make sense to apply the rate limit values here just as well.

[Koc]

It could congest the job queue though, therefore it would make sense to apply the rate limit values here just as well.

but how? can we add this in upcoming iterations?