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

feat: file conversion provider #49922

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

elzody
Copy link
Contributor

@elzody elzody commented Dec 18, 2024

Summary

This PR will introduce a file conversion API endpoint, which can be called to convert a file from one type to another. Apps can register their own conversion providers (which implement IConversionProvider), and the providers can define which MIME types they support for conversion via ConversionMimeTuples.

TODO

  • OpenAPI specs
  • Flesh out some more details around the IConversionProvider interface
    • What type should it really return? Stream, resource, File, file path?
  • What should happen when multiple apps register similar conversion providers?
  • How should we make the preferred list of conversion providers?

Checklist

@elzody elzody added enhancement 2. developing Work in progress feature: files feature: workflows php Pull requests that update Php code labels Dec 18, 2024
@elzody elzody added this to the Nextcloud 31 milestone Dec 18, 2024
@elzody elzody self-assigned this Dec 18, 2024
@elzody elzody force-pushed the feat/file-conversion-provider branch 2 times, most recently from 7e8ed84 to 3ab63cb Compare December 23, 2024 22:46
@elzody elzody force-pushed the feat/file-conversion-provider branch from b1d9670 to 1bceac6 Compare December 27, 2024 19:58
@elzody elzody force-pushed the feat/file-conversion-provider branch 2 times, most recently from 2cdd54b to e91c447 Compare January 7, 2025 20:24
@elzody elzody marked this pull request as ready for review January 9, 2025 15:37
@elzody elzody requested a review from provokateurin as a code owner January 9, 2025 15:37
elzody and others added 3 commits January 13, 2025 17:31
Signed-off-by: Elizabeth Danzberger <[email protected]>
@elzody elzody force-pushed the feat/file-conversion-provider branch 2 times, most recently from ac0843f to e8d3eac Compare January 13, 2025 22:37
@elzody elzody changed the title [WIP] feat: file conversion provider feat: file conversion provider Jan 13, 2025
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Very nice! I only have a few smaller things to not, feel free to ignore the most nit picky ones 😅

apps/files/lib/Capabilities.php Outdated Show resolved Hide resolved
apps/files/lib/Capabilities.php Outdated Show resolved Hide resolved
lib/public/Files/Conversion/IConversionManager.php Outdated Show resolved Hide resolved
apps/files/lib/Controller/ConversionApiController.php Outdated Show resolved Hide resolved
apps/files/lib/Controller/ConversionApiController.php Outdated Show resolved Hide resolved
lib/public/Files/Conversion/ConversionMimeTuple.php Outdated Show resolved Hide resolved

// Operate in mebibytes
$fileSize = $file->getSize() / (1024 * 1024);
$threshold = $this->config->getValue('max_conversion_filesize', 100);
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make sense to also check that the threshold is a sensible value? Otherwise an admin might configure something like 100 GiB and break their instance. I'm not sure what a reasonable limit is, especially when it comes to high resolution photos or long (high resolution) videos.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a risk of breaking the instance here. If an admin configures long timeouts then it should be fine. At some point we may rather want to have background workers that do an async conversion so we don't run into timeouts (which is the main reason for adding this), but this was discussed out of scope due to the short-term scheduling with @AndyScherzinger.

lib/public/Files/Conversion/IConversionManager.php Outdated Show resolved Hide resolved
lib/public/Files/Conversion/IConversionProvider.php Outdated Show resolved Hide resolved
@elzody elzody force-pushed the feat/file-conversion-provider branch from e8d3eac to 23e4606 Compare January 14, 2025 19:59
@@ -38,6 +41,10 @@ public function getCapabilities(): array {
'max_size' => ChunkedUploadConfig::getMaxChunkSize(),
'max_parallel_count' => ChunkedUploadConfig::getMaxParallelCount(),
],

'file_conversions' => array_map(function (ConversionMimeTuple $mimeTuple) {
return $mimeTuple->jsonSerialize();
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this part is failing to serialize to xml in the integration tests, but have not checked locally, so not sure what the best way to fix this is.

{
  "reqId": "6YTbALAvT2hTLsT4X3ec",
  "level": 3,
  "time": "2025-01-14T22:43:50+00:00",
  "remoteAddr": "::1",
  "user": "admin",
  "app": "PHP",
  "method": "GET",
  "url": "/ocs/v1.php/cloud/capabilities",
  "message": "ValueError: XMLWriter::writeElement(): Argument #2 ($content) must be a valid element name, \"image/png\" given at /home/runner/actions-runner/_work/server/server/lib/private/AppFramework/OCS/BaseResponse.php#149",
  "userAgent": "GuzzleHttp/7",
  "version": "31.0.0.9",
  "data": {
    "app": "PHP"
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Hm yeah that is tricky, seems like / is forbidden as tag name (makes sense since it is part of the XML syntax).
The output format could be changed so that it's no longer used as a key but the entire data structure is a list and the source format is just a property of the objects in the list.

@skjnldsv
Copy link
Member

As a followup, it would be nice to add some integration tests.
Should be easy to test :

  • the api response on failure
  • the proper new png file after the api conversion endpoint have been reached

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🏗️ In progress
Status: 🏗️ In progress
Development

Successfully merging this pull request may close these issues.

5 participants