Conversation
butonic
added
Type:Maintenance
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 12 critical |
| CodeStyle | 37 minor |
🟢 Metrics 229 complexity · 312 duplication
Metric Results Complexity 229 Duplication 312
🟢 Coverage 65.15% diff coverage
Metric Results Coverage variation Report missing for 5a7afe01 Diff coverage ✅ 65.15% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (5a7afe0) Report Missing Report Missing Report Missing Head commit (9eb1bcd) 85287 20874 24.48% Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3397) 1145 746 65.15% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
How do audio artwork, geogebra thumbnail and tiff preview extraction fit into this approach? Have you seen that I reworked the tiff extraction pr to use tika? I already upstreamed audio artwork extraction and tiff extraction to tika. Geogebra is still pending, but I expect it to be merged very soon as well. So I would like to see a preprocessing step for extraction of embedded pictures before they are handed over to imagor for resizing (with tika it would be push based and out of process too). I was a bit surprised to see that you are moving so much into the webdav service, my expectation would have been that you just replace the Decoder in the thumbnails service - can you elaborate a little why you chose this way more invasive approach? |
3a7874b to
6aaa4ec
Compare
We can move the workflow into webdav, because webdav already has everything it needs locally (the CS3 gateway client for stat + download, the auth context, the response writer), which lets the generator shrink to a stateless resizer (POST bytes in, resized bytes out) that's swappable with an external imagor via a single URL. The thumbnail generation process is owned, end-to-end, by webdav instead of logic split across two services and two round-trips. And we can use an external container to support HEIC thumbnails, just by changing the URL. And I really want to be able to reuse the thumbnailer like tika, because it now becomes stateless. |
Okay, alright. Works for me.
👍🏻
I'm fine with that, I just wanted you to be aware. No problem at all to port it. It was just important to me that extractors still have a place in the architecture and we don't need a magical one service that can handle all file types.
Fair enough, thanks!
That part was clear :) One more note: It might be the time to at least quickly think about the "honest hasPreview" implementation.
Thoughts? Do you see any problems with your drafted architecture? (I don't see any, just to be sure) |
👀 🤔 I'll comment there...
I would prefer library code. I was thinking of moving it to /pkg, in case other services also call the grpc api.
Yes, they should be cached.
Althought, I plan to add a configurable blobstore for the cache so we can store thumbnails in nats or s3 ... with a checksum based key. But that would be the topic of a future PR. |
|
FYI, all merged:
So we can replace all existing extractors and get this on top: |
78a9428 to
0a9e86a
Compare
|
Some more thoughts on why POST and not GET: With POST the image generator does not need to be granted access to all instances it should GET images from. In a multi instance deployment or at hosters all instances can just POST to the imagor service. New instances do not need to be registered in the firewall or network configuration to let imagor make requests to them. The imagor service itself can just be horizontally scaled. Finally, if malicious bytes manage to take over the container/pod it can still not make any requests to the outside world. So POST really is driven by security and scalability. This also explains why we cannot let clients make signed GET requests directly to the imagor service. It would have to be granted access to all instances. IMO that is too much of an attack surface, even if the requests are signed by the instance itself. At the network level you dont want to inspect every request to ensure it really is a GET / download request for a file ... but if you don't the request might also POST to another endpoint to probe the infrastructure ... nah. Lock it down: no egress whatsoever. As a sidenote: the image cache uses hash based keys, so all instances could share the same objectstorage as well. And the objectstorage can then implement the cache invalidation. |
|
Found a compatibility bug while pointing the generator URL at a real imagor (imagorvideo): Related: for the default fill operation (no operation segment) the router has no route with Fix is building a single segment and matching the routes: --- a/services/webdav/pkg/generator/url.go
+++ b/services/webdav/pkg/generator/url.go
@@
box := fmt.Sprintf("%dx%d", width, height)
- noUpscale := "filters:no_upscale()"
- formatFilter := fmt.Sprintf("filters:format(%s)", outputExt)
+ filterSegment := fmt.Sprintf("filters:no_upscale():format(%s)", outputExt)
segment := operationSegment(operation)
if segment == "" {
- return fmt.Sprintf("%s/unsafe/%s/%s/%s/", base, box, noUpscale, formatFilter)
+ return fmt.Sprintf("%s/unsafe/%s/%s/", base, box, filterSegment)
}
- return fmt.Sprintf("%s/unsafe/%s/%s/%s/%s/", base, segment, box, noUpscale, formatFilter)
+ return fmt.Sprintf("%s/unsafe/%s/%s/%s/", base, segment, box, filterSegment)
--- a/services/thumbnails/pkg/service/http/v0/service.go
+++ b/services/thumbnails/pkg/service/http/v0/service.go
@@
- m.Post("/unsafe/{operation}/{width}x{height}/filters:no_upscale()/filters:format({format})/", handler)
- m.Post("/unsafe/{operation}/{width}x{height}/filters:no_upscale()/filters:format({format})", handler)
- m.Post("/unsafe/{width}x{height}/filters:format({format})/", handler)
- m.Post("/unsafe/{width}x{height}/filters:format({format})", handler)
+ m.Post("/unsafe/{operation}/{width}x{height}/filters:no_upscale():format({format})/", handler)
+ m.Post("/unsafe/{operation}/{width}x{height}/filters:no_upscale():format({format})", handler)
+ m.Post("/unsafe/{width}x{height}/filters:no_upscale():format({format})/", handler)
+ m.Post("/unsafe/{width}x{height}/filters:no_upscale():format({format})", handler)(plus the URL literals in |
026b932 to
07eff1b
Compare
|
The web ui fetches previews with
The tests fetch a 1200x1200 image to test text rendering ... However, the available preview resolutions are configured on the server side and NONE of the resolutions is in the default list (save 32x32): The only real difference is the processor.
THUMBNAILS_RESOLUTIONS was used to limit the number of thumbnail resolutions that will be cached AND returned by the API. So the quare resolutions requested by the web ui with fit (fit-in), produce landscape or portrait previews, depending on the source image dimensions. If the source image happens to be wider or taller than the configured resolution, the largest configured resolution will be generated and cached. This might be smaller than the requested width or height. But since the web ui does not always know the size of the image all it can do is tell the server to fit the image into a rectangular box and the server will fit-in the image. For fit-in the response never is a square, which the web ui already expects. I think we can delete all non square resolutions, because in effect only they are requested. thumbnail and fit are only used to make the server crop the image to the exact resolution (thumbnail), or fill the image to the box without cropping. |
|
Not sure, happy to discuss (if needed at all) - but how do I request previews for ultrawide pictures? I'm requesting non-quadratic previews here: opencloud-eu/web-extensions#529 |
|
Not sure if or how this affects the web client, but if it does in any capacity, please check in with one of us to verify the desired behavior. We've made quite some performance improvements over the last few weeks by optimizing requested and delivered resolutions, it would be a shame if we regressed on that. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 12 critical |
| CodeStyle | 37 minor |
🟢 Metrics 889 complexity
Metric Results Complexity 889
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
@JammingBen This PR is backwards compatible in that it tries to produce the same thumbnails as before. But because I do not want to examine any bytes in process for security reasons I can no longer use the size of the source image when deciding on the dimensions of the preview image. Furthermore, the list of allowed resolutions is not really matching anything web requests (save 64x64 thumbnails), AFAICT. All the fit requests also request a square. But the old code works more by accident, than design. fit + square simensions conceptually tells the "server fit the image inside this box, keep aspect ratio, do not crop, it is ok if I get a landscape or portrait preview back". "thumbnail + square" tells the server give me an image with exatly these dimensions, crop if necessary. That is how I interpret the requests by web. Correct me, if I am interpreting that wrong. But that also means the a and scaleup params are unneeded and do no tneed to be sent by web. In fact Anyway, this PR allows using the the The resulting preview images should now be what the client requested, because on main there are corner cases where it is in fact possible that the generated image was taller or wider than the requested box. What did you do to improve performance with regards to thumbnails? respect cache headers? rely more on the So yeah, I would like to implement preview generation properly on the server side under the mentioned assumbtion about the requests web makes. I also know that domme wants to requests landscape and portrait images. And I think we should be able to finally add proper cache headers. |
Yes this is also my understanding of it.
This is good to know, so Web can safely remove them from the request body then?
We introduced more values to |
yes
ok, to verify: 
web makes requests like this: it specifically tells the server to 
but 500 is wider than 320x320 ... which is plainly wrong. This PR will return a 320x142 image: 
but ... all of that is just an accidental fix. And web also makes the image fit inside the tiles, so this should be fine. |
sounds correct! But that makes it more important to think about ultrawide pictures. |
|
I think it the use case for web is to say:
For the latter case there is bad news, because the server will only produce thumbnails in the configured resolutons. Even for ultrawide images, the current widest resolution is "7680x4320". That is pretty wide. But "give me exactly these dimensions" for thumbnail will only give you a portrait / landscape / square preview in one of the configured resolutions. The imagor docs describe the fill operation as:
Together with the no_upscale() filter (= If the web ui knows the exact dimensions we can extend the preview url parameters to allow more filters. For now I just implemented the minimal set that is needed by the current web ui. The implementation does already pass the |
|
The configured resolutions will be sorted into square, landscape and portrait buckets. And depending of the orientation in the preview request url the corresponding bucket will be used to pick the actual preview size that will be sent to imagor. whatever that returns (which might differ from the requested dimensions) will be returned. I think this is the best tradeoff to flood the cach with every possible dimension to eat the storage and the usability in the web. With this PR we could allow more, but ... that I want to defer that to subsequent PRs. |
|
ok, I think I have the three commits in good shape. |
|
In the beginning I wasn't sure where this was going, I thought you wanted to embed imagor straight away, but this is a nice iteration. Claude says:
|
|
One more on the preprocessor, since it moved as-is: in this branch For a gif request webdav runs What it costs: a large animated gif is fully decoded twice, once inside the webdav process, and Suggestion: add |
8a31e31 to
954eb9b
Compare
Add a stateless POST endpoint that accepts an original image as a multipart
upload and returns the resized thumbnail, mimicking imagor's /unsafe/ API:
POST /unsafe(/fit-in|/stretch)/{width}x{height}(/filters:{filters})
The three forms are fill (center-crop to the exact box, may upscale), fit-in
(preserve aspect ratio, fit within the box, never upscale) and stretch (resize
to the exact box without preserving aspect). Supported output formats are jpg,
png and gif. Image processing is split by build tag: stdlib imaging by default,
libvips when built with -tags enable_vips. The endpoint is stateless: no auth,
no storage, no source fetching. This commit only adds the /unsafe routes; the
legacy /data endpoint is left in place and removed in a later commit.
…erator webdav now owns the complete thumbnail pipeline: stat -> validate -> cache check -> download -> preprocess -> generate -> cache -> respond. The thumbnails service is used only as a stateless imagor-compatible image resizer via its /unsafe push endpoint. - New ThumbnailWorkflow drives the pipeline; space-scoped refs are anchored at the space ResourceId and downloads authenticate with the user's x-access-token. - Preprocessing (audio/geogebra/text/gif) moves from the thumbnails service into webdav (services/webdav/pkg/preprocessor). - Thumbnail caching moves to webdav (memory/file/s3 backends, file cache off by default); resolutions are snapped orientation-aware in webdav. - Generator URL defaults to the local thumbnails service; new WEBDAV_* config for generator, cache and resolutions. - Acceptance tests updated for the new sizing/processor behavior.
The thumbnails service is now a stateless image resizer; remove everything the old gRPC-based pipeline needed: - proto definitions and generated code (service + messages) - gRPC server, handler and decorators - JWT transfer token code and the /data HTTP download endpoint - filesystem storage layer and source fetchers (webdav + CS3 imgsource) - duplicated encoding/generator/processor/resolution utilities - the preprocessor packages moved to webdav in the previous commit - dead config fields (Thumbnail struct, GRPC config, unused go-micro client), no-op metrics/instrumentation wrappers, errors package, stale testdata and the Makefile protobuf target Also remove the now-meaningless thumbnails transfer secret generation from opencloud init and point the graph service at the new webdav thumbnail package. The README is rewritten to document the imagor-like push endpoint.
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
3 participants
This PR tries to fix #1128 in a backwards compatible way.
Thumbnail generation used to live entirely in the thumbnails service behind a gRPC API: webdav called GetThumbnail, the service fetched the source from storage, preprocessed and generated the image, stored it on its own filesystem, and returned a JWT-signed URL that webdav had to follow with a second authenticated HTTP call just to get the bytes back. This branch inverts that: the thumbnails service becomes a stateless imagor-compatible resizer (one POST endpoint — image bytes in, resized bytes out; no auth, storage, or gRPC), and webdav owns the whole workflow via a single ThumbnailWorkflow type: stat via gateway → cache check → download source → preprocess → POST to generator → cache → respond.
Webdav gains what it needs to own that pipeline: the preprocessors (PDF→image, text→image, audio cover art) moved over from the thumbnails service, a new checksum-keyed thumbnail cache with memory/file/S3/noop backends, and a pkg/generator package that builds resizer URLs and posts multipart images. Config shrinks to one generator URL plus timeout, optional auth header (for an external imagor behind a proxy), and max input file size — the URL can point at the built-in thumbnails service or any imagor instance.
The old architecture is deleted from the thumbnails service: proto files, gRPC handler, JWT transfer tokens, filesystem storage, source fetchers, the /data endpoint, and duplicated utilities (~4,000 lines), plus leftover dead config, no-op metrics wrappers, and opencloud init's now-meaningless transfer-secret generation. Net diff: 102 files, +3,337/−4,336; thumbnail requests no longer need the second round-trip, and the resizer is trivially replaceable.
Related:
#630
#3364
#3332
opencloud-eu/reva#781
opencloud-eu/reva#773
owncloud/core#31268 - discusses a and mode parameters
Thumbnails lessons from oc10 #1378
Discussion:
https://github.com/orgs/opencloud-eu/discussions/2368
https://github.com/orgs/opencloud-eu/discussions/1090
@dschmidt This PR is not ready, but I want to bring your attention to this approach, which is why I am pushing this code now.