Skip to content

Enhancement: Make tables import asynchronous#1801

Merged
enjeck merged 1 commit into
mainfrom
feature/asyncronous-import
May 8, 2026
Merged

Enhancement: Make tables import asynchronous#1801
enjeck merged 1 commit into
mainfrom
feature/asyncronous-import

Conversation

@Koc

@Koc Koc commented May 12, 2025

Copy link
Copy Markdown
Contributor

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

🔍 Preview

Notification once import scheduled
image

Activity item with import stats
image

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

@Koc Koc force-pushed the feature/asyncronous-import branch from 5cbf833 to 3c2422c Compare May 12, 2025 16:00
@blizzz

blizzz commented May 23, 2025

Copy link
Copy Markdown
Member

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

@juliusknorr juliusknorr added enhancement New feature or request 3. to review Waiting for reviews 2. developing Work in progress and removed 3. to review Waiting for reviews labels May 23, 2025
@Koc Koc force-pushed the feature/asyncronous-import branch 5 times, most recently from 9b0c3bd to 1dbdfe8 Compare May 26, 2025 15:47
@Koc Koc marked this pull request as ready for review May 26, 2025 15:47
@Koc Koc requested review from blizzz and enjeck as code owners May 26, 2025 15:47
@Koc

Koc commented May 26, 2025

Copy link
Copy Markdown
Contributor Author

@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)?

Comment thread lib/Db/Row2Mapper.php
Comment thread lib/Service/ImportService.php Outdated
@blizzz

blizzz commented May 26, 2025

Copy link
Copy Markdown
Member

@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 blizzz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

replied to questions

Comment thread lib/Db/Row2Mapper.php
Comment thread lib/Service/ImportService.php Outdated
@Koc Koc force-pushed the feature/asyncronous-import branch 8 times, most recently from 1c49d2c to 599fecd Compare June 9, 2025 14:52
@Koc

Koc commented Jun 9, 2025

Copy link
Copy Markdown
Contributor Author

@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
blizzz previously requested changes Jun 10, 2025

@blizzz blizzz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread lib/Activity/Provider.php Outdated
Comment thread lib/Controller/ImportController.php Outdated
Comment thread lib/Controller/ImportController.php
Comment thread lib/Service/ImportService.php Outdated
@Koc

Koc commented Jul 27, 2025

Copy link
Copy Markdown
Contributor Author

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

Comment thread lib/Service/ImportService.php Outdated
@blizzz

blizzz commented Mar 5, 2026

Copy link
Copy Markdown
Member

Thanks, I tested and the core async works. Only have 2 concerns:

* I never get notified that the upload is done. Where should I be seeing this? I have to refresh the page to see the created table, and I never get a notification

An activity log is created, but a push notification is useful. Can be added in a follow-up PR though.

* I still think that small tables should be created synchronously for better UX. We can have some threshold e.g 200 rows for which creating the table is fast enough to just do it.

This is a valid concern, and I expect that importing non big tables is at least a 50% scenario. Having to wait for it for 5min or such is perhaps degrading experience. We can also have a threshold by file size, but should play it safe indeed.

@Koc Koc force-pushed the feature/asyncronous-import branch from 0cd8194 to 69f312c Compare March 9, 2026 23:47
@Koc Koc force-pushed the feature/asyncronous-import branch 10 times, most recently from 8105ddd to d084b8c Compare March 25, 2026 18:49
@Koc

Koc commented Mar 25, 2026

Copy link
Copy Markdown
Contributor Author

@blizzz @enjeck so, guys, I did changes requested by you. If import file is bigger than 4k cells we go async. Otherwise upload will be the same like it was before

Manually tested next scenarios:

  • import small file to a table from file view
  • import large file to a table from file view
  • import small file to a table via file picker
  • import large file to a table via file picker
  • import small file to a table via file upload
  • import large file to a table via file upload

feel free to test import to a view by yourselves 😉 .

nextcloud-import-2026-03-25_19.50.46.mp4

Cypress tests were extended with scenarios for large file.

@Koc Koc force-pushed the feature/asyncronous-import branch from d084b8c to 08afa01 Compare March 25, 2026 23:04
@blizzz blizzz moved this to 🏗️ In progress in 📝 Productivity team Mar 26, 2026
@Koc Koc force-pushed the feature/asyncronous-import branch 7 times, most recently from fa1c6f4 to f452172 Compare April 10, 2026 22:57
@Koc Koc force-pushed the feature/asyncronous-import branch from f452172 to e7798cc Compare April 27, 2026 09:15

@enjeck enjeck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, a few more comments. These should be the last ones then we can merge. Thanks!

Comment thread lib/Service/ImportService.php Outdated
Comment thread lib/Controller/ImportController.php
Comment thread lib/Controller/ImportController.php
@Koc

Koc commented May 7, 2026

Copy link
Copy Markdown
Contributor Author

@enjeck fixed everything, please check and merge 🙏

Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>

@enjeck enjeck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome, thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2. developing Work in progress enhancement New feature or request

Projects

Status: ☑️ Done

Development

Successfully merging this pull request may close these issues.

4 participants