Repository navigation
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 941 complexity
Metric Results Complexity 941
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.
The multipart part carries the source's own type, because imagor's upload loader accepts image/* only. A public link that needs a password is a 404 again, a preview carries nosniff and a content length, and the input size limit is read with reva's bytesize again.
A source type maps onto the type its thumbnail is returned in, and that type is asked for by the name the generator knows it under. The answer carries that type whether it was just generated or came from the cache, which is also what the cache key says.
Nothing is decoded in webdav any more, which also ends the libvips build split in the preview path.
webdav, graph and ocdav read the same list, so a configured type is announced and attempted by all three. A type the generator cannot answer is refused at startup, and ocdav takes the types from its own configuration instead of importing an OpenCloud package, which the vendored reva carries until opencloud-eu/reva#885 lands.
A converter that found nothing returns nil, so the workflow decides what to do with it instead of reading an error.
The upload part has to be typed, a malformed request is a 400 while a source it cannot read is a 406, an animated gif flattens when the output is still, a still source can be written as a gif, libvips writes webp, and fit-in rounds the way vips and imagor round it.
The fixtures are named after what they are, and the leftovers of the deleted grpc service are gone.
A cache backend nobody can serve fails at startup instead of dropping entries silently, the memory backend is bounded, a busy generator's 429 reaches the client with its Retry-After, and a 400 from the generator is read as this service's own bug instead of a missing preview.
An unconstrained axis is fitted instead of refused, a decides when the client sent it, and an empty resolution ladder is refused at startup instead of letting the client pick the cache key.
The gif loop is shared by both builds, the executor takes a request struct, the route table halves, the aspect flag is a pointer instead of two bools, and the cache's budget is named after what it is.
…gors The same image goes to the built-in generator and to both imagor images, and status, dimensions, format, frames and alpha are compared. A generator that answers differently is held to that answer by a recorded override, so the test fails once the divergence disappears. It runs once per resize implementation, because make test builds the pure Go path while the images ship vips.
The repeated post-and-assert bodies collapse into tables, and the sources are built without gomega because they run while the spec tree is built.
An explicit x=0 or y=0 is an error again, only an omitted axis is unconstrained, and the text preview fixtures carry the resolution the generator writes now.
The a and processor defaults, that every named processor overrules a, the shared OC_THUMBNAIL_* names, the gif output type, the ladder making the response a different size, the empty filters segment that answers 404, and 406 instead of 400 for a format the build cannot write.
The push endpoint takes an empty filters segment, ignores an image key behind the box and computes a side given as 0 from the other one, all measured against both imagors and pinned in the parity suite. Plus the review leftovers: no em dashes, the request test on ginkgo, comments that contradicted the code, one stat for Head and generate, one thumbnail handler, the gif fit-in rounding of vips, and the new env names last so they win.
The dimension guard and the input-format detection used Go's decoder registry in both builds, so with libvips they were silently skipped for everything Go cannot read, and a webp source came back as a JPEG where imagor keeps it a webp. Each build now reads the header with the library that will decode the image, the guard runs on that, and the animated gif path is shared instead of living in both encoders. The hook names stay per build, the function variables are typed per build, so the any and its seven type assertions are gone.
A processor that needs the whole box is not a cropping one, so targetsExactBox says what it decides. An output format without an encoder answers 406 like every other thing this build cannot write. The configured defaults are no longer seeded twice, the constructors merge them anyway, like they do for the source types. And webp needs no build tag: both builds read it, the pure Go one answers the JPEG the mapping asks for, which the parity suite pins.
The thumbnails service kept a single map of supported mime types that nothing in that service used any more, only webdav read it, and the comment still claimed reva reads it there. It is webdav's announcement, so DefaultSourceTypes lives next to SourceTypes now, the leftover package is gone, and no webdav file imports the thumbnails service any longer.
With previews off a thumbnail path answered 405, because the REPORT patterns match the same paths and the GET routes were left out; the routes stay now and the shared parse step answers 404, which is what the setting promises. The deprecated cache directory is read again: its default no longer sits in the config struct, where it made the fallback unreachable. A processor nobody knows is treated like none, in the parser too. A comma-joined resolutions entry in yaml keeps working. Plus the comments and the readme line that described something else than the code does.
The four writable formats were a whitelist in the filter parser and a second list of content types in the encode step; they are one map now, so a format nobody writes stays a 400 and one without an encoder in this build stays a 406. The fit-in shape existed twice since the gif loop started rounding like vips, it lives in box.go now. And the push routes are back inline: mountPushRoutes only existed so a test mux could not drift from them, and the specs go through the real NewService instead.
…thing else The aspect-ignored header blamed a processor for a decision the free axis can make too. fit-in does not letterbox, it answers smaller than the box. The s3 cache checks bucket, region and credentials, not the endpoint. A mapping onto a type the generator cannot write is refused at startup instead of quietly becoming a JPEG. And a processor name nobody knows resolves like none, which three comments and a readme line still denied.
Two readme sentences promised a startup refusal without saying that nothing reads these settings when OC_DISABLE_PREVIEWS is set, so a typo surfaces when previews come back on.
Overall: the architecture is right and I would merge this after a short list of fixes, not a rework. What this gets right
Blocking, in my opinion
Should fix, small
Process Commits titled "answer the third review round" with zero review comments on GitHub mean the review happened somewhere else. For a change that deletes a public gRPC API and changes client-visible sizing, it would help outside readers a lot if that trail were on the PR. |
Both builds and imagor now crop the same region, a gif is bounded by its declared frames, the posted body by THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE and an image extracted from a container by the configured input size. The s3 cache reads a miss where minio reports it. The push endpoint's HTTP behaviour is asserted in the parity suite now, where imagor answers the same request, so 41 duplicated service specs are gone and the 32 that remain are what a non-default config or a helper decides.
3c456b7 to
dd91deb
Compare
A cropping resize now carries imagor's smart segment, which is how the libvips build and a real imagor keep the region that carries the image, the way main cropped before the generator moved out. A build without libvips answers the same request with a centre crop, and the parity suite records that.
| probe: cropProbe, | ||
| generatorOverrides: map[string]override{ | ||
| "imaging": {answers: "200 100x100 png drops-marker", because: "an attention crop is libvips work; this build takes the segment and crops the centre, which is what the thumbnails service has always done without libvips"}, | ||
| }, |
There was a problem hiding this comment.
This means that text in .txt can be cut off in pure go builds while it works in imagor and vips builds.
It's not a regression it's a preexisting bug on main.
Claude says it can be fixed in about 80 LoC, but I'm not convinced it's worth it. Noone should be using it in production...
4 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 → respondstat via gateway → snap the requested size → cache check → download source → preprocess → POST to generator → cache → respond.Webdav gains what it needs to own that pipeline: the preprocessors (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 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,336118 files, +6,863/−4,861; thumbnail requests no longer need the second round-trip, and the resizer is trivially replaceable.Update: sizing and caching
THUMBNAILS_RESOLUTIONSis a ladder of edge lengths, not a list of boxes: each axis is rounded up on its own, so a square request can no longer land in a landscape box, which is what cropped and shrank portrait previews. LegacyWidthxHeightentries still count with both sides, and the cache is keyed on the snapped box, sox=447andx=448share an entry.Clients no longer need to know the list:
xandyare independently optional, an omitted axis is unconstrained and capped at the largest length, while an explicit0stays the 400 it has always been. A cropping processor plus an unconstrained axis is a 400, no dimensions at all stays the legacy 32x32. A public link that needs a password answers 404 again instead of 500.Update: what a thumbnail is made of
Three settings, read as a chain. For a
.heicon a deployment running a stock imagor:Each one merges onto the built-in defaults, so naming a type does not take the answer away from the others. A target the generator has no format for is refused at startup, a source it cannot read answers 404.
image/heicandimage/avif, both unsupported today, were verified end to end this way, no code change.Update: parity suite
services/thumbnails/pkg/parityposts the same image to the built-in generator and to two real imagor images and compares status, dimensions, format, frames and alpha, with imagor from testcontainers locally and service containers in CI. A generator that answers differently is held to that answer by a recorded override carrying the reason, which fails once the divergence disappears.It runs twice, once per resize implementation, because
make testcompiles the pure Go path while the images ship vips, so the shipped resize code had no test at all. That exposed an older problem, not fixed here: CI's libvips is not the one the images ship, soenable_vipstests measure capabilities we do not deliver (#3746).Follow-ups
oc:has-previewis answered from the same configured list, which needed a change in reva: fix(ocdav): answer has-preview from the ocdav configuration reva#885. ocdav takes the types from its own configuration instead of importing an OpenCloud package, and the frontend hands in what webdav attempts. The vendored copy carries it until that PR lands. It also removes the import that makes a module cycle, reva's go.mod requires opencloud v1.1.0, so a standalone reva announced previews from a frozen OpenCloud list. The requirement itself stays,pkg/app/registry/microandpkg/micro/ocdavstill useopencloud/pkg/registry.image/jpgcannot occur, reva mapsjpgtoimage/jpeg. Remove it from the supported list and the encode step if nothing outside the repo depends on it.THUMBNAILS_MAX_ANIMATION_FRAMESnow caps a gif from its block structure before a frame is decoded, butTHUMBNAILS_MAX_INPUT_WIDTHand_HEIGHTdescribe one page, so a multi-page heic or tiff is bounded only by both backends loading a single page today.S3Cachehas two gaps the unit tests cannot see, because the mock is not minio.Secure: trueis hard-wired inBuildS3CacheConfig, andminio.Newprepends the scheme itself, so thehttp://host:portform the config text asks for is refused as an invalid endpoint. Deriving the scheme from the endpoint and handing minio a bare host fixes both, and a real MinIO behind testcontainers (theimagortestpattern) is what should pin it.ThumbnailCache.Gettakes no context, so the s3 backend reads oncontext.Background()and ignores the request deadline.http.ClientcarryingWEBDAV_THUMBNAIL_GENERATOR_TIMEOUTis shared by the generator POST and the source download, so the generator timeout silently doubles as the download timeout for a source of up toWEBDAV_THUMBNAIL_MAX_INPUT_IMAGE_FILE_SIZE.FontLoaderis rebuilt for every text preview, becausepreprocessor.ForTyperuns per request.ETagand noCache-Control, although the cache key (checksum, box, operation, format) is a content-addressed ETag already.push_test.goand in the parity suite. A parity case runs the built-in generator in-process, so it needs no container and compares imagor on top; the service specs should keep only what a non-default config decides (the concurrency limiter, the input limits) and the helper unit tests.processorand withouta, the default is now fill with upscaling, where main capped the result at the source size;previewsAutoAdustedSizing.featurecarries the new numbers. Web always sends a processor, so this is visible only to a client that sends a barex/y.THUMBNAILS_DATA_ENDPOINTandTHUMBNAILS_TRANSFER_TOKENare gone without a deprecation path: they configured the pull pipeline this PR removes, so there is nothing to point them at.THUMBNAILS_FILESYSTEMSTORAGE_ROOTis still read as a fallback forWEBDAV_THUMBNAIL_CACHE_DIRand marked deprecated.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.Taken over from @butonic with his and the team's agreement, and rebased onto current main. The sizing and caching changes described above are mine. ~ @dschmidt