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 upload of Zip, extract and continue #2229

Closed
wants to merge 339 commits into from
Closed

Conversation

ildyria
Copy link
Member

@ildyria ildyria commented Jan 21, 2024

Add support of upload of single zip files.

⚠️ this is quite experimental, I tested locally that it works, however there are some caveats to know about.

  1. The uploaded zip file will stay on your server unless it is properly extracted without faults (after which it is removed).
  2. Similarly the extracted files from the zip will stay on your server unless they are properly imported without fault.
  3. On import success, you will see in /path-to-lychee/storage/extract-jobs an empty folder prefixed with YmD so you know when it was created, making easier to clean up later.
  4. ONLY THE FIRST LEVEL OF THE ZIP IS IMPORTED. See explanation below.

In other words, if you have

Something_Stupid.zip
├── img1.jpg
├── img2.jpg
├── img3.jpg
└── SubFolder
    ├── img4.jpg
    ├── img5.jpg
    └── img6.jpg

Only img1.jpg, img2.jpg and img3.jpg will be imported in Something Stupid album (note that _ is automatically replaced by space) and deleted from /path-to-lychee/storage/extract-jobs/yyyymmddSomething_Stupid.
The folder SubFolder will still be in /path-to-lychee/storage/extract-jobs/yyyymmddSomething_Stupid/SubFolder

@ildyria ildyria self-assigned this Jan 21, 2024
@ildyria ildyria requested a review from a team January 21, 2024 18:46
@ildyria ildyria added the Review: hard Difficult review expected: major changes, lots of files modified, and dependencies updated. label Jan 21, 2024
@ildyria ildyria added this to the 5.2.0 milestone Jan 21, 2024
Copy link

codecov bot commented Jan 21, 2024

Codecov Report

Attention: Patch coverage is 67.39288% with 449 lines in your changes missing coverage. Please review.

Project coverage is 84.20%. Comparing base (02e5a20) to head (dfcc429).
Report is 96 commits behind head on master.

Additional details and impacted files

Copy link
Contributor

@kamil4 kamil4 left a comment

Choose a reason for hiding this comment

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

I've got multiple questions...

  • What's with the mangling and demangling of spaces? Why is it necessary?
  • Shouldn't this code extract the zip file and then simply invoke the existing "Import from Server" functionality that already correctly handles subfolders and such? Why reimplement a more limited version?
  • Why is there no proper cleanup? Can't we wrap it all in a big try-catch and do the cleanup there?
  • Why is the empty folder left on the server even on success? Wouldn't a simple rmdir be enough to remove it?

app/Jobs/ExtractZip.php Outdated Show resolved Hide resolved
$baseNameWithoutExtension = substr($this->originalBaseName, 0, -4);

// Save that one (is default if no existing folder found).
$orignalName = str_replace(' ', '_', $baseNameWithoutExtension);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you doing this replacement here? Does ZipArchive not handle spaces correctly or something?

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 may, but I don't trust spaces in paths.
And given that we replace '_' by ' ' later in file names. I figured it would be idem potent.

@ildyria
Copy link
Member Author

ildyria commented Jan 22, 2024

  • What's with the mangling and demangling of spaces? Why is it necessary?

Most of the time when a _ is in the file name when uploading a zip archive, it is meant as a space.
I am planning later to make a renamer module which would make it nicer when uploading in batch.

  • Shouldn't this code extract the zip file and then simply invoke the existing "Import from Server" functionality that already correctly handles subfolders and such? Why reimplement a more limited version?

Import from Server is actually not so great when using from web interface. To the point where I removed it from the v5 GUI (still available via CLI).

The code is old and functionality needs rewriting.
See TODO here: https://github.com/LycheeOrg/Lychee/blob/master/app/Actions/Import/Exec.php#L272

I am planning to revisit the import from server for a cleaner code using Jobs & pipelines, but I wanted to have a simple implementation of import via ZIP as POC.

  • Why is there no proper cleanup? Can't we wrap it all in a big try-catch and do the cleanup there?

It is slightly more complex than that. This is using laravel jobs which you could see as independent work unit which are dispatched on a queue and I am not sure which job will be picked first.
More info on jobs here: https://laravel.com/docs/10.x/queues

  • Why is the empty folder left on the server even on success? Wouldn't a simple rmdir be enough to remove it?

See above, we don't know when all the jobs have been processed.

@ildyria ildyria force-pushed the upload-zip-extract-import branch 4 times, most recently from bc52805 to 23e2382 Compare January 29, 2024 10:07
@ildyria ildyria modified the milestones: 5.2.0, 5.x Apr 6, 2024
@ildyria ildyria marked this pull request as draft July 3, 2024 21:08
@ildyria ildyria force-pushed the upload-zip-extract-import branch from 4961bb6 to 3cf748d Compare July 6, 2024 14:42
@ildyria ildyria removed this from the 5.x milestone Oct 17, 2024
@ildyria ildyria closed this Oct 17, 2024
@ildyria ildyria reopened this Oct 17, 2024
@ildyria ildyria closed this Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: hard Difficult review expected: major changes, lots of files modified, and dependencies updated.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants