From 3572410fc46eaec13bb8ccdffcf81d691986ddda Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Sat, 17 Oct 2026 16:47:10 +0200 Subject: [PATCH] fix: answer the review, and read imagor's path the way imagor does 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. --- .codacy.yml | 3 + .vscode/launch.json | 1 + services/thumbnails/README.md | 15 +- services/thumbnails/pkg/config/config.go | 10 +- .../thumbnails/pkg/parity/grammar_test.go | 60 ++++++++ services/thumbnails/pkg/parity/parity_test.go | 1 + .../thumbnails/pkg/service/http/v0/box.go | 27 ++++ .../thumbnails/pkg/service/http/v0/push.go | 18 ++- .../pkg/service/http/v0/push_helpers_test.go | 10 +- .../pkg/service/http/v0/push_imaging.go | 26 ++-- .../pkg/service/http/v0/push_test.go | 42 +++++- .../pkg/service/http/v0/push_vips.go | 25 +++- .../pkg/service/http/v0/resizegif.go | 15 +- .../thumbnails/pkg/service/http/v0/service.go | 29 ++-- services/webdav/README.md | 2 +- services/webdav/pkg/config/config.go | 4 +- .../pkg/dav/requests/requests_suite_test.go | 13 ++ services/webdav/pkg/dav/requests/thumbnail.go | 1 - .../webdav/pkg/dav/requests/thumbnail_test.go | 140 +++++++----------- services/webdav/pkg/generator/url.go | 2 +- .../webdav/pkg/preprocessor/preprocessor.go | 1 - services/webdav/pkg/service/v0/service.go | 22 +-- services/webdav/pkg/thumbnail/sourcetypes.go | 3 - .../webdav/pkg/thumbnail/typemapping_test.go | 2 +- .../webdav/pkg/thumbnail/workflow/workflow.go | 76 ++++------ .../pkg/thumbnail/workflow/workflow_test.go | 29 +--- tests/config/local/.env-federation | 1 + 27 files changed, 342 insertions(+), 236 deletions(-) create mode 100644 services/thumbnails/pkg/parity/grammar_test.go create mode 100644 services/thumbnails/pkg/service/http/v0/box.go create mode 100644 services/webdav/pkg/dav/requests/requests_suite_test.go diff --git a/.codacy.yml b/.codacy.yml index f5eba526d9..6c901a486e 100644 --- a/.codacy.yml +++ b/.codacy.yml @@ -20,6 +20,9 @@ exclude_paths: - 'tests/acceptance/expected-failures-*.md' # written by the search engine parity suite, table rows are as wide as they are - 'services/search/pkg/parity/README.md' + # thumbnail docs: wide tables and prose that is not hard-wrapped + - 'services/thumbnails/README.md' + - 'services/webdav/README.md' - 'tests/acceptance/bootstrap/**' - 'tests/acceptance/TestHelpers/**' - 'tests/acceptance/scripts/run.sh' diff --git a/.vscode/launch.json b/.vscode/launch.json index 90755d0071..5fe997f712 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -163,6 +163,7 @@ "STORE_GRPC_ADDR": "127.0.0.1:10460", "THUMBNAILS_DEBUG_ADDR": "127.0.0.1:10194", "THUMBNAILS_HTTP_ADDR": "127.0.0.1:10190", + "WEBDAV_THUMBNAIL_GENERATOR_URL": "http://127.0.0.1:10190", "USERLOG_HTTP_ADDR": "127.0.0.1:10211", "USERLOG_DEBUG_ADDR": "127.0.0.1:10210", "USERS_DEBUG_ADDR": "127.0.0.1:10145", diff --git a/services/thumbnails/README.md b/services/thumbnails/README.md index 878600fe59..df9199ea33 100644 --- a/services/thumbnails/README.md +++ b/services/thumbnails/README.md @@ -22,13 +22,22 @@ source file to this endpoint and receives the processed image back. | `POST /unsafe/fit-in/{width}x{height}/filters:{filters}` | Scale to fit within width x height, preserving aspect ratio and never upscaling | | `POST /unsafe/stretch/{width}x{height}/filters:{filters}` | Resize to the exact width x height without preserving aspect ratio (distorts) | -The filters segment is optional, like imagor's. Inside it, `format` and -`no_upscale` are the filters this executor acts on, every other filter is -ignored. `format` itself is optional: without it the input's own format is +The path is imagor's. The filters segment is optional and may be empty +(`filters:`), and an image key behind it is accepted and ignored, because the +source arrives in the upload rather than being loaded by key. A bare box with +nothing behind it is the one place this service and imagor part: imagor reads the +last segment as the image key, so `/unsafe/100x100` is a key and nothing is +resized, while this service treats it as the box. Inside the filters segment, +`format` and `no_upscale` are the filters this executor acts on, every other +filter is ignored. `format` itself is optional: without it the input's own format is preserved, the same way imagor does it. An input this service cannot write (tiff, bmp) becomes a JPEG; imagor writes tiff back in its own format and only falls back to JPEG for the ones it cannot write either. +A side given as `0` is computed from the other one, so `/unsafe/0x100/` keeps the +aspect ratio and `/unsafe/0x0/` returns the source at its own size. A free axis +leaves nothing to crop to, so the operation makes no difference to it. + Like real imagor, the default fill resize **upscales** small sources to fill the box exactly (a 100x100 source requested at `320x320` returns a 320x320 image). Adding the `no_upscale()` filter caps the result at the source size diff --git a/services/thumbnails/pkg/config/config.go b/services/thumbnails/pkg/config/config.go index 639ca1644a..4615524628 100644 --- a/services/thumbnails/pkg/config/config.go +++ b/services/thumbnails/pkg/config/config.go @@ -28,13 +28,9 @@ type Config struct { // still rejected, but the enforcement now lives at decode time in this service // (which owns the image) rather than in webdav. type Thumbnail struct { - // MaxInputWidth is the maximum width of an input image which is being processed. - MaxInputWidth int `yaml:"max_input_width" env:"THUMBNAILS_MAX_INPUT_WIDTH" desc:"The maximum width of an input image which is being processed." introductionVersion:"1.0.0"` - // MaxInputHeight is the maximum height of an input image which is being processed. + MaxInputWidth int `yaml:"max_input_width" env:"THUMBNAILS_MAX_INPUT_WIDTH" desc:"The maximum width of an input image which is being processed." introductionVersion:"1.0.0"` MaxInputHeight int `yaml:"max_input_height" env:"THUMBNAILS_MAX_INPUT_HEIGHT" desc:"The maximum height of an input image which is being processed." introductionVersion:"1.0.0"` - // MaxConcurrentRequests limits the number of thumbnail requests that are - // decoded and resized in parallel. 0 (the default) means unlimited, matching - // main's THUMBNAILS_MAX_CONCURRENT_REQUESTS. Requests arriving while the limit - // is reached get HTTP 429 Too Many Requests. + // 0 (the default) means unlimited; a request arriving while the limit is + // reached gets HTTP 429 with a Retry-After. MaxConcurrentRequests int `yaml:"max_concurrent_requests" env:"THUMBNAILS_MAX_CONCURRENT_REQUESTS" desc:"Maximum number of concurrent thumbnail generation requests. Default is 0 which is unlimited." introductionVersion:"%%NEXT%%"` } diff --git a/services/thumbnails/pkg/parity/grammar_test.go b/services/thumbnails/pkg/parity/grammar_test.go new file mode 100644 index 0000000000..4f1d9659c1 --- /dev/null +++ b/services/thumbnails/pkg/parity/grammar_test.go @@ -0,0 +1,60 @@ +package parity + +// imagor's path is "/unsafe[/{operation}]/{W}x{H}[/filters:{list}]/{image}": the +// filters may be empty, the image key is the last segment and an upload loader +// has no use for it, and a side given as 0 is computed from the other one. +func grammarGroup() parityGroup { + src := func() source { return pngSource(300, 200) } + + return parityGroup{ + name: "grammar", + cases: []parityCase{ + { + title: "an empty filters segment is still a filters segment", path: "/unsafe/100x100/filters:", + source: src(), want: "200 100x100 png", + }, + { + title: "an image key behind the box is ignored", path: "/unsafe/100x100/foo.png", + source: src(), want: "200 100x100 png", + }, + { + title: "an image key behind the filters is ignored", path: "/unsafe/100x100/filters:format(png)/foo.png", + source: src(), want: "200 100x100 png", + }, + { + title: "a zero width follows the height", path: "/unsafe/0x100/filters:format(png)/", + source: src(), want: "200 150x100 png", + }, + { + title: "a zero height follows the width", path: "/unsafe/100x0/filters:format(png)/", + source: src(), want: "200 100x67 png", + }, + { + title: "a free axis enlarges", path: "/unsafe/0x400/filters:format(png)/", + source: src(), want: "200 600x400 png", + }, + { + title: "a free axis under fit-in is the same, there is nothing to crop to", path: "/unsafe/fit-in/0x100/filters:format(png)/", + source: src(), want: "200 150x100 png", + }, + { + title: "a free axis under stretch is the same", path: "/unsafe/stretch/100x0/filters:format(png)/", + source: src(), want: "200 100x67 png", + generatorOverrides: map[string]override{ + "imagor": {answers: "200 100x66 png", because: "imagor truncates the computed side under stretch and rounds it everywhere else"}, + }, + }, + { + title: "a zero box leaves the source alone", path: "/unsafe/0x0/filters:format(png)/", + source: src(), want: "200 300x200 png", + }, + { + title: "a bare box with no tail at all", path: "/unsafe/100x100", + source: src(), want: "200 100x100 png", + generatorOverrides: map[string]override{ + "imagor": {answers: "200 300x200 png", because: "the last segment is imagor's image key, so a bare box is a key and nothing is resized"}, + }, + }, + }, + } +} diff --git a/services/thumbnails/pkg/parity/parity_test.go b/services/thumbnails/pkg/parity/parity_test.go index fe9b555eef..be5bf7508b 100644 --- a/services/thumbnails/pkg/parity/parity_test.go +++ b/services/thumbnails/pkg/parity/parity_test.go @@ -76,6 +76,7 @@ func parityGroups() []parityGroup { limitsGroup(), iconGroup(), transparencyGroup(), + grammarGroup(), } } diff --git a/services/thumbnails/pkg/service/http/v0/box.go b/services/thumbnails/pkg/service/http/v0/box.go new file mode 100644 index 0000000000..0f3076ed98 --- /dev/null +++ b/services/thumbnails/pkg/service/http/v0/box.go @@ -0,0 +1,27 @@ +package svc + +import "math" + +// fitInBox is the size an image of srcW x srcH takes inside the box, aspect +// ratio preserved and the scaled side rounded the way vips and imagor round it. +func fitInBox(srcW, srcH, boxW, boxH int) (int, int) { + scale := math.Min(float64(boxW)/float64(srcW), float64(boxH)/float64(srcH)) + + return max(int(math.Round(float64(srcW)*scale)), 1), max(int(math.Round(float64(srcH)*scale)), 1) +} + +// autoAxis resolves imagor's zero axis: a side given as 0 is computed from the +// other one, so the aspect ratio decides it, and two zero sides mean the image +// is returned at its own size. The bool reports whether anything is left to +// resize, and a resolved box is reached by a plain resize for every operation, +// because with a free axis there is nothing to crop to. +func autoAxis(srcW, srcH, boxW, boxH int) (int, int, bool) { + switch { + case boxW == 0 && boxH == 0: + return srcW, srcH, false + case boxW == 0: + return max(int(math.Round(float64(srcW)*float64(boxH)/float64(srcH))), 1), boxH, true + default: + return boxW, max(int(math.Round(float64(srcH)*float64(boxW)/float64(srcW))), 1), true + } +} diff --git a/services/thumbnails/pkg/service/http/v0/push.go b/services/thumbnails/pkg/service/http/v0/push.go index 09230435ad..2a225ac519 100644 --- a/services/thumbnails/pkg/service/http/v0/push.go +++ b/services/thumbnails/pkg/service/http/v0/push.go @@ -94,7 +94,7 @@ func (s Thumbnails) pushHandler(w http.ResponseWriter, r *http.Request) { return } - filtersSegment := chi.URLParam(r, "filters") + filtersSegment := filtersFromTail(chi.URLParam(r, "*")) ext, hasFormat, err := outputFormatFromFilters(filtersSegment) if err != nil { writeInvalid(w, err.Error()) @@ -183,12 +183,26 @@ func (s Thumbnails) pushHandler(w http.ResponseWriter, r *http.Request) { func parseDim(r *http.Request, name string) (int, error) { v := chi.URLParam(r, name) n, err := strconv.Atoi(v) - if err != nil || n <= 0 { + if err != nil || n < 0 { return 0, fmt.Errorf("invalid %s: %s", name, v) } + return n, nil } +// filtersFromTail reads the filters out of imagor's path tail. The tail is the +// optional "filters:..." segment plus an optional image key behind it; only the +// filters segment is ours, and it may be empty. +func filtersFromTail(tail string) string { + segment, _, _ := strings.Cut(tail, "/") + filters, ok := strings.CutPrefix(segment, "filters:") + if !ok { + return "" + } + + return filters +} + // writeNotAcceptable responds with imagor's status for a source it cannot read // (406 Not Acceptable), as opposed to a malformed request. func writeNotAcceptable(w http.ResponseWriter, msg string) { diff --git a/services/thumbnails/pkg/service/http/v0/push_helpers_test.go b/services/thumbnails/pkg/service/http/v0/push_helpers_test.go index 13380cbc3d..73dc6903a8 100644 --- a/services/thumbnails/pkg/service/http/v0/push_helpers_test.go +++ b/services/thumbnails/pkg/service/http/v0/push_helpers_test.go @@ -13,7 +13,6 @@ import ( "net/textproto" "github.com/go-chi/chi/v5" - "github.com/go-chi/chi/v5/middleware" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "golang.org/x/image/bmp" @@ -142,13 +141,8 @@ func multipartBody(source []byte, partType string) (*bytes.Buffer, string) { // testMux is the production route table with the configured input limits; 0 // disables a bound. func testMux(maxWidth, maxHeight int) *chi.Mux { - mux := chi.NewRouter() - h := Thumbnails{maxWidth: maxWidth, maxHeight: maxHeight}.pushHandler - mux.Use(middleware.StripSlashes) - mux.Post("/unsafe/{operation}/{width}x{height}/filters:{filters}", h) - mux.Post("/unsafe/{width}x{height}/filters:{filters}", h) - mux.Post("/unsafe/{operation}/{width}x{height}", h) - mux.Post("/unsafe/{width}x{height}", h) + mux := chi.NewMux() + mountPushRoutes(mux, Thumbnails{maxWidth: maxWidth, maxHeight: maxHeight}.pushHandler) return mux } diff --git a/services/thumbnails/pkg/service/http/v0/push_imaging.go b/services/thumbnails/pkg/service/http/v0/push_imaging.go index c15ad0a66d..ab9ce83eae 100644 --- a/services/thumbnails/pkg/service/http/v0/push_imaging.go +++ b/services/thumbnails/pkg/service/http/v0/push_imaging.go @@ -10,7 +10,6 @@ import ( "image/jpeg" "image/png" "io" - "math" "github.com/kovidgoyal/imaging" ) @@ -25,10 +24,10 @@ func init() { // processImageImaging resizes the input using the imaging backend. The operation // selects the resize/crop mode: fill (center-crop to the box, upscaling by default -// like real imagor's default resize), fit-in (fit within the box without cropping, -// upscaling small sources by default like real imagor), or stretch (resize to the -// exact box). noUpscale caps the -// default fill at the source size, mirroring imagor's no_upscale() filter. +// like real imagor's default resize), fit-in (fit within the box without cropping +// and without ever enlarging the source, like imagor without its upscale() +// filter), or stretch (resize to the exact box). noUpscale caps the default fill +// at the source size, mirroring imagor's no_upscale() filter. func processImageImaging(r io.Reader, req resizeRequest) (any, error) { if req.KeepAnimation && isGifReader(r) { g, err := gif.DecodeAll(r) @@ -44,6 +43,15 @@ func processImageImaging(r io.Reader, req resizeRequest) (any, error) { srcBounds := img.Bounds() + if req.Width == 0 || req.Height == 0 { + boxW, boxH, resize := autoAxis(srcBounds.Dx(), srcBounds.Dy(), req.Width, req.Height) + if !resize { + return img, nil + } + + return imaging.Resize(img, boxW, boxH, imaging.Lanczos), nil + } + switch req.Operation { case OpStretch: return imaging.Resize(img, req.Width, req.Height, imaging.Lanczos), nil @@ -65,14 +73,6 @@ func processImageImaging(r io.Reader, req resizeRequest) (any, error) { } } -// fitInBox is the size an image of srcW x srcH takes inside the box, aspect -// ratio preserved and the scaled side rounded the way vips and imagor round it. -func fitInBox(srcW, srcH, boxW, boxH int) (int, int) { - scale := math.Min(float64(boxW)/float64(srcW), float64(boxH)/float64(srcH)) - - return max(int(math.Round(float64(srcW)*scale)), 1), max(int(math.Round(float64(srcH)*scale)), 1) -} - // encodeJPEGImaging encodes the processed image as JPEG using the stdlib encoder. func encodeJPEGImaging(w io.Writer, processed any) error { img, ok := processed.(image.Image) diff --git a/services/thumbnails/pkg/service/http/v0/push_test.go b/services/thumbnails/pkg/service/http/v0/push_test.go index 0034286aaf..fa9bbee3ca 100644 --- a/services/thumbnails/pkg/service/http/v0/push_test.go +++ b/services/thumbnails/pkg/service/http/v0/push_test.go @@ -121,6 +121,44 @@ var _ = Describe("the push endpoint", func() { }) }) + // Measured against shumc/imagor:1.9.11: everything behind the box is imagor's + // tail, and a side given as 0 is computed from the other one. + DescribeTable("reads imagor's path the way imagor reads it", + func(path string, source []byte, want image.Rectangle) { + rec := post(path, source) + + Expect(rec.Code).To(Equal(http.StatusOK), rec.Body.String()) + Expect(decodedBounds(rec)).To(Equal(want)) + }, + Entry("an empty filters segment is a filters segment", + "/unsafe/50x50/filters:", pngSource(200, 200), image.Rect(0, 0, 50, 50)), + Entry("an empty filters segment with an operation", + "/unsafe/fit-in/50x50/filters:", pngSource(200, 100), image.Rect(0, 0, 50, 25)), + Entry("an image key behind the box is ignored", + "/unsafe/50x50/foo.png", pngSource(200, 200), image.Rect(0, 0, 50, 50)), + Entry("an image key behind the filters is ignored", + "/unsafe/50x50/filters:format(png)/foo.png", pngSource(200, 200), image.Rect(0, 0, 50, 50)), + Entry("a zero width follows the height", + "/unsafe/0x100/filters:format(png)/", pngSource(300, 200), image.Rect(0, 0, 150, 100)), + Entry("a zero height follows the width", + "/unsafe/100x0/filters:format(png)/", pngSource(300, 200), image.Rect(0, 0, 100, 67)), + Entry("a free axis ignores the operation, there is nothing to crop to", + "/unsafe/fit-in/0x100/filters:format(png)/", pngSource(300, 200), image.Rect(0, 0, 150, 100)), + Entry("a free axis enlarges, like imagor", + "/unsafe/0x200/filters:format(png)/", pngSource(150, 100), image.Rect(0, 0, 300, 200)), + Entry("a zero box leaves the source alone", + "/unsafe/0x0/filters:format(png)/", pngSource(300, 200), image.Rect(0, 0, 300, 200)), + ) + + It("keeps an animated gif animated on a free axis", func() { + rec := post("/unsafe/0x40/filters:format(gif)/", gifSource(200, 100, 3)) + + Expect(rec.Code).To(Equal(http.StatusOK), rec.Body.String()) + out := decodedGIF(rec) + Expect(out.Image).To(HaveLen(3)) + Expect(out.Image[0].Bounds()).To(Equal(image.Rect(0, 0, 80, 40))) + }) + DescribeTable("refuses what it cannot answer", func(path string, source []byte, partType string, want int) { rec := postTo(testMux(0, 0), path, source, partType) @@ -129,8 +167,8 @@ var _ = Describe("the push endpoint", func() { }, Entry("dimensions that are not numbers", "/unsafe/fit-in/abcxdef/filters:format(jpeg)/", pngSource(100, 100), "image/png", http.StatusBadRequest), - Entry("a zero box", - "/unsafe/fit-in/0x0/filters:format(jpeg)/", pngSource(100, 100), "image/png", http.StatusBadRequest), + Entry("a negative box", + "/unsafe/fit-in/-50x50/filters:format(jpeg)/", pngSource(100, 100), "image/png", http.StatusBadRequest), Entry("a format nobody writes", "/unsafe/fit-in/50x50/filters:format(tiff)/", pngSource(100, 100), "image/png", http.StatusBadRequest), Entry("bytes that are not an image", diff --git a/services/thumbnails/pkg/service/http/v0/push_vips.go b/services/thumbnails/pkg/service/http/v0/push_vips.go index cd730fbc81..4c9ee74e23 100644 --- a/services/thumbnails/pkg/service/http/v0/push_vips.go +++ b/services/thumbnails/pkg/service/http/v0/push_vips.go @@ -21,10 +21,10 @@ func init() { // processImageVips resizes the input using libvips. The operation selects the // resize/crop mode: fill (center-crop to the box, upscaling by default like real -// imagor's default resize), fit-in (fit within the box without cropping, upscaling -// small sources by default like real imagor), or stretch (resize to the exact box). -// noUpscale caps the upscaling at the source size, mirroring imagor's no_upscale() -// filter. +// imagor's default resize), fit-in (fit within the box without cropping and +// without ever enlarging the source, like imagor without its upscale() filter), +// or stretch (resize to the exact box). noUpscale caps the default fill at the +// source size, mirroring imagor's no_upscale() filter. func processImageVips(r io.Reader, req resizeRequest) (any, error) { if req.KeepAnimation && isGifReader(r) { g, err := gif.DecodeAll(r) @@ -48,6 +48,23 @@ func processImageVips(r io.Reader, req resizeRequest) (any, error) { // after exporting. Closing it here would free the underlying C image before the // export runs (use-after-free). + if req.Width == 0 || req.Height == 0 { + boxW, boxH, resize := autoAxis(m.Width(), m.Height(), req.Width, req.Height) + if resize { + hScale := float64(boxW) / float64(m.Width()) + vScale := float64(boxH) / float64(m.Height()) + if err := m.ResizeWithVScale(hScale, vScale, vips.KernelLanczos3); err != nil { + return nil, err + } + } + + if err := m.RemoveMetadata(); err != nil { + return nil, err + } + + return m, nil + } + switch req.Operation { case OpStretch: // Resize to the exact box without preserving aspect ratio. diff --git a/services/thumbnails/pkg/service/http/v0/resizegif.go b/services/thumbnails/pkg/service/http/v0/resizegif.go index f31d231714..50e3f44dfd 100644 --- a/services/thumbnails/pkg/service/http/v0/resizegif.go +++ b/services/thumbnails/pkg/service/http/v0/resizegif.go @@ -16,6 +16,15 @@ import ( // https://github.com/willnorris/gifresize. func resizeGIF(m *gif.GIF, width, height int, operation string, noUpscale bool) *gif.GIF { srcX, srcY := m.Config.Width, m.Config.Height + + if width == 0 || height == 0 { + resolvedW, resolvedH, resize := autoAxis(srcX, srcY, width, height) + if !resize { + return m + } + width, height, operation = resolvedW, resolvedH, OpStretch + } + b := image.Rect(0, 0, srcX, srcY) tmp := image.NewRGBA(b) @@ -29,10 +38,12 @@ func resizeGIF(m *gif.GIF, width, height int, operation string, noUpscale bool) case OpStretch: processed = imaging.Resize(tmp, width, height, imaging.Lanczos) case OpFitIn: - if noUpscale && srcX <= width && srcY <= height { + // not imaging.Fit: it truncates the scaled side, vips and imagor round + // it, and fit-in never enlarges whether no_upscale() was sent or not. + if fitW, fitH := fitInBox(srcX, srcY, width, height); fitW >= srcX && fitH >= srcY { processed = tmp } else { - processed = imaging.Fit(tmp, width, height, imaging.Lanczos) + processed = imaging.Resize(tmp, fitW, fitH, imaging.Lanczos) } default: // OpFill if noUpscale && srcX <= width && srcY <= height { diff --git a/services/thumbnails/pkg/service/http/v0/service.go b/services/thumbnails/pkg/service/http/v0/service.go index c3b58b25ac..912d793822 100644 --- a/services/thumbnails/pkg/service/http/v0/service.go +++ b/services/thumbnails/pkg/service/http/v0/service.go @@ -42,19 +42,7 @@ func NewService(opts ...Option) Service { limiter: newConcurrencyLimiter(limits.MaxConcurrentRequests), } - // Push-based thumbnail generation endpoint (imagor-compatible). The optional - // operation segment selects the resize/crop mode; its absence is the default - // center-crop fill. The filters segment is optional too, like imagor's, and - // is captured whole and parsed by the handler so any set and order of imagor - // filters is accepted. - handler := svc.pushHandler - // StripSlashes makes the trailing slash optional, the operation and filters - // segments are the four remaining combinations. - m.Use(middleware.StripSlashes) - m.Post("/unsafe/{operation}/{width}x{height}/filters:{filters}", handler) - m.Post("/unsafe/{width}x{height}/filters:{filters}", handler) - m.Post("/unsafe/{operation}/{width}x{height}", handler) - m.Post("/unsafe/{width}x{height}", handler) + mountPushRoutes(m, svc.pushHandler) _ = chi.Walk(m, func(method string, route string, _ http.Handler, middlewares ...func(http.Handler) http.Handler) error { options.Logger.Debug().Str("method", method).Str("route", route).Int("middlewares", len(middlewares)).Msg("serving endpoint") @@ -64,6 +52,21 @@ func NewService(opts ...Option) Service { return svc } +// mountPushRoutes puts the imagor-compatible push endpoint on a mux. The +// optional operation segment selects the resize/crop mode; its absence is the +// default center-crop fill. Everything after the box is imagor's tail: an +// optional filters segment, which may be empty, followed by an optional image +// key, which an upload loader has no use for and which is ignored here too. +// StripSlashes makes the trailing slash optional, the operation segment and the +// tail are the four remaining combinations. +func mountPushRoutes(m *chi.Mux, handler http.HandlerFunc) { + m.Use(middleware.StripSlashes) + m.Post("/unsafe/{operation}/{width}x{height}/*", handler) + m.Post("/unsafe/{width}x{height}/*", handler) + m.Post("/unsafe/{operation}/{width}x{height}", handler) + m.Post("/unsafe/{width}x{height}", handler) +} + // Thumbnails implements the business logic for Service. type Thumbnails struct { logger log.Logger diff --git a/services/webdav/README.md b/services/webdav/README.md index 077dc3f59a..93d5fd7dd9 100644 --- a/services/webdav/README.md +++ b/services/webdav/README.md @@ -66,7 +66,7 @@ When what a request resolves to contradicts an `a` the client sent, the response #### Thumbnail Generator -The thumbnails themselves are made by a generator the webdav service POSTs to, configured with `WEBDAV_THUMBNAIL_GENERATOR_URL` (and `WEBDAV_THUMBNAIL_GENERATOR_TIMEOUT`). The built-in thumbnails service speaks the imagor image endpoint, so a real [imagor](https://imagor.net) can be put in its place without changing anything else. A source the generator cannot read answers `404`, one that is too large `403`, and `WEBDAV_THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE` caps what is sent at all. A generator that refuses because it is busy answers `429` with its own `Retry-After`, so a client can come back instead of treating it as a missing preview. +The thumbnails themselves are made by a generator the webdav service POSTs to, configured with `WEBDAV_THUMBNAIL_GENERATOR_URL` (and `WEBDAV_THUMBNAIL_GENERATOR_TIMEOUT`). The built-in thumbnails service speaks the imagor image endpoint, so a real [imagor](https://imagor.net) can be put in its place without changing anything else. A source the generator cannot read answers `404`, one that is too large `403`, and `WEBDAV_THUMBNAIL_MAX_INPUT_IMAGE_FILE_SIZE` caps what is sent at all. A generator that refuses because it is busy answers `429` with its own `Retry-After`, so a client can come back instead of treating it as a missing preview. Three settings say what is made of what. They are merged onto the built-in defaults, so naming one type does not take the answer away from the others: diff --git a/services/webdav/pkg/config/config.go b/services/webdav/pkg/config/config.go index eb509715ae..280eb5551f 100644 --- a/services/webdav/pkg/config/config.go +++ b/services/webdav/pkg/config/config.go @@ -29,7 +29,7 @@ type Config struct { ThumbnailGeneratorURL string `yaml:"thumbnail_generator_url" env:"WEBDAV_THUMBNAIL_GENERATOR_URL" desc:"Base URL of the thumbnail generator service, e.g. http://thumbnails:9186" introductionVersion:"%%NEXT%%"` ThumbnailGeneratorTimeout string `yaml:"thumbnail_generator_timeout" env:"WEBDAV_THUMBNAIL_GENERATOR_TIMEOUT" desc:"HTTP timeout for requests to the thumbnail generator, e.g. 30s" introductionVersion:"%%NEXT%%"` // the yaml key stays unprefixed, a config file written against it has to keep working - ThumbnailMaxInputFileSize string `yaml:"max_input_file_size" env:"WEBDAV_THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE;THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE" desc:"Maximum file size of an input image for thumbnail generation. Usable common abbreviations: [KB, KiB, MB, MiB, GB, GiB], example: 50MB" introductionVersion:"%%NEXT%%"` + ThumbnailMaxInputFileSize string `yaml:"max_input_file_size" env:"THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE;WEBDAV_THUMBNAIL_MAX_INPUT_IMAGE_FILE_SIZE" desc:"Maximum file size of an input image for thumbnail generation. Usable common abbreviations: [KB, KiB, MB, MiB, GB, GiB], example: 50MB" introductionVersion:"%%NEXT%%"` ThumbnailCacheBackend string `yaml:"thumbnail_cache_backend" env:"WEBDAV_THUMBNAIL_CACHE_BACKEND" desc:"Cache backend for imagor thumbnails: 'none', 'memory', 'file', or 's3'. Default: file." introductionVersion:"%%NEXT%%"` ThumbnailCacheDir string `yaml:"thumbnail_cache_dir" env:"WEBDAV_THUMBNAIL_CACHE_DIR" desc:"Directory for file-based thumbnail cache (Default: $OC_BASE_DATA_PATH/thumbnails/files). Only used when ThumbnailCacheBackend is 'file'." introductionVersion:"%%NEXT%%"` @@ -47,7 +47,7 @@ type Config struct { ThumbnailTypeMapping []string `yaml:"thumbnail_type_mapping" env:"OC_THUMBNAIL_TYPE_MAPPING;WEBDAV_THUMBNAIL_TYPE_MAPPING" desc:"The type a thumbnail is returned in, per type of the image it is made from, as mime:mime entries, e.g. image/vnd.microsoft.icon:image/png. The target is asked for by the name WEBDAV_THUMBNAIL_GENERATOR_FORMATS gives it. A type that is not listed, and one whose target the generator cannot write, becomes a JPEG." introductionVersion:"%%NEXT%%"` // the yaml key stays unprefixed, a config file written against it has to keep working - ThumbnailFontMapFile string `yaml:"font_map_file" env:"WEBDAV_THUMBNAILS_TXT_FONTMAP_FILE;THUMBNAILS_TXT_FONTMAP_FILE" desc:"The path to a font map file for txt thumbnails." introductionVersion:"%%NEXT%%"` + ThumbnailFontMapFile string `yaml:"font_map_file" env:"THUMBNAILS_TXT_FONTMAP_FILE;WEBDAV_THUMBNAIL_TXT_FONTMAP_FILE" desc:"The path to a font map file for txt thumbnails." introductionVersion:"%%NEXT%%"` Context context.Context `yaml:"-"` } diff --git a/services/webdav/pkg/dav/requests/requests_suite_test.go b/services/webdav/pkg/dav/requests/requests_suite_test.go new file mode 100644 index 0000000000..cce2565068 --- /dev/null +++ b/services/webdav/pkg/dav/requests/requests_suite_test.go @@ -0,0 +1,13 @@ +package requests_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestRequests(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Requests Suite") +} diff --git a/services/webdav/pkg/dav/requests/thumbnail.go b/services/webdav/pkg/dav/requests/thumbnail.go index f7f7df876c..a44b898d5f 100644 --- a/services/webdav/pkg/dav/requests/thumbnail.go +++ b/services/webdav/pkg/dav/requests/thumbnail.go @@ -37,7 +37,6 @@ type ThumbnailRequest struct { Ref *providerv1beta1.Reference // The file name of the source file including the extension Filename string - // The file extension // The requested width of the thumbnail Width int32 // The requested height of the thumbnail diff --git a/services/webdav/pkg/dav/requests/thumbnail_test.go b/services/webdav/pkg/dav/requests/thumbnail_test.go index 7ae31726db..531ee79711 100644 --- a/services/webdav/pkg/dav/requests/thumbnail_test.go +++ b/services/webdav/pkg/dav/requests/thumbnail_test.go @@ -1,101 +1,69 @@ -package requests +package requests_test import ( "context" "net/http" "net/http/httptest" - "testing" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/opencloud-eu/opencloud/pkg/conversions" "github.com/opencloud-eu/opencloud/services/webdav/pkg/constants" + "github.com/opencloud-eu/opencloud/services/webdav/pkg/dav/requests" ) -func newThumbnailRequest(t *testing.T, rawQuery string) *http.Request { - t.Helper() +func thumbnailRequest(rawQuery string) *http.Request { req := httptest.NewRequest("GET", "/files/test.png?"+rawQuery, nil) - ctx := context.WithValue(req.Context(), constants.ContextKeyPath, "/files/test.png") - return req.WithContext(ctx) + + return req.WithContext(context.WithValue(req.Context(), constants.ContextKeyPath, "/files/test.png")) } -var ( - trueValue = true - falseValue = false -) +var _ = Describe("ParseThumbnailRequest", func() { + DescribeTable("the legacy a flag", + func(query string, want *bool) { + tr, err := requests.ParseThumbnailRequest(thumbnailRequest(query)) + Expect(err).ToNot(HaveOccurred()) + if want == nil { + Expect(tr.Aspect).To(BeNil()) -func TestParseThumbnailRequestAspect(t *testing.T) { - tests := []struct { - name string - query string - wantA *bool - }{ - {"a absent is not set at all", "x=1024&y=1024", nil}, - {"a=1 preserves aspect", "x=1024&y=1024&a=1", &trueValue}, - {"a=0 fills the box", "x=1024&y=1024&a=0", &falseValue}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - r := newThumbnailRequest(t, tt.query) - tr, err := ParseThumbnailRequest(r) - if err != nil { - t.Fatalf("ParseThumbnailRequest() error = %v", err) - } - switch { - case tt.wantA == nil && tr.Aspect != nil: - t.Errorf("Aspect = %v, want it unset", *tr.Aspect) - case tt.wantA != nil && tr.Aspect == nil: - t.Errorf("Aspect unset, want %v", *tt.wantA) - case tt.wantA != nil && *tr.Aspect != *tt.wantA: - t.Errorf("Aspect = %v, want %v", *tr.Aspect, *tt.wantA) - } - }) - } -} - -func TestParseThumbnailRequestProcessor(t *testing.T) { - r := newThumbnailRequest(t, "x=320&y=320&processor=fit") - tr, err := ParseThumbnailRequest(r) - if err != nil { - t.Fatalf("ParseThumbnailRequest() error = %v", err) - } - if tr.Processor != "fit" { - t.Errorf("Processor = %q, want %q", tr.Processor, "fit") - } - if tr.Width != 320 || tr.Height != 320 { - t.Errorf("dimensions = %dx%d, want 320x320", tr.Width, tr.Height) - } -} - -func TestParseThumbnailRequestDimensions(t *testing.T) { - tests := []struct { - name string - query string - wantW, wantH int32 - wantErr bool - }{ - {name: "both given", query: "x=448&y=448", wantW: 448, wantH: 448}, - {name: "neither given keeps the legacy default", query: "preview=1", wantW: DefaultWidth, wantH: DefaultHeight}, - {name: "an omitted axis is unconstrained", query: "y=512&processor=fit", wantW: 0, wantH: 512}, - {name: "an explicit zero is rejected", query: "x=0&y=512&processor=fit", wantErr: true}, - {name: "negative is rejected", query: "x=-1&y=512", wantErr: true}, - {name: "an unconstrained axis without a processor is fitted, not rejected", query: "y=512", wantW: 0, wantH: 512}, - {name: "cropping to an unconstrained axis is rejected explicitly", query: "y=512&processor=thumbnail", wantErr: true}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - tr, err := ParseThumbnailRequest(newThumbnailRequest(t, tt.query)) - if tt.wantErr { - if err == nil { - t.Fatalf("ParseThumbnailRequest() error = nil, want an error") - } return } - if err != nil { - t.Fatalf("ParseThumbnailRequest() error = %v", err) - } - if tr.Width != tt.wantW || tr.Height != tt.wantH { - t.Errorf("dimensions = %dx%d, want %dx%d", tr.Width, tr.Height, tt.wantW, tt.wantH) - } - }) - } -} + Expect(tr.Aspect).To(HaveValue(Equal(*want))) + }, + Entry("stays unset when the client does not send it", "x=1024&y=1024", nil), + Entry("preserves the aspect ratio with a=1", "x=1024&y=1024&a=1", conversions.ToPointer(true)), + Entry("fills the box with a=0", "x=1024&y=1024&a=0", conversions.ToPointer(false)), + ) + + It("keeps the processor and both dimensions", func() { + tr, err := requests.ParseThumbnailRequest(thumbnailRequest("x=320&y=320&processor=fit")) + Expect(err).ToNot(HaveOccurred()) + Expect(tr.Processor).To(Equal("fit")) + Expect(tr.Width).To(Equal(int32(320))) + Expect(tr.Height).To(Equal(int32(320))) + }) + + DescribeTable("the requested box", + func(query string, wantWidth, wantHeight int32) { + tr, err := requests.ParseThumbnailRequest(thumbnailRequest(query)) + Expect(err).ToNot(HaveOccurred()) + Expect(tr.Width).To(Equal(wantWidth)) + Expect(tr.Height).To(Equal(wantHeight)) + }, + Entry("takes both axes when they are given", "x=448&y=448", int32(448), int32(448)), + Entry("keeps the legacy default when neither is given", "preview=1", int32(requests.DefaultWidth), int32(requests.DefaultHeight)), + Entry("leaves an omitted axis unconstrained", "y=512&processor=fit", int32(0), int32(512)), + Entry("fits an unconstrained axis without a processor", "y=512", int32(0), int32(512)), + ) + + DescribeTable("a request it refuses", + func(query string) { + _, err := requests.ParseThumbnailRequest(thumbnailRequest(query)) + Expect(err).To(HaveOccurred()) + }, + Entry("an explicit zero", "x=0&y=512&processor=fit"), + Entry("a negative size", "x=-1&y=512"), + Entry("cropping to an unconstrained axis", "y=512&processor=thumbnail"), + ) +}) diff --git a/services/webdav/pkg/generator/url.go b/services/webdav/pkg/generator/url.go index 589b63ab00..66cec0ca70 100644 --- a/services/webdav/pkg/generator/url.go +++ b/services/webdav/pkg/generator/url.go @@ -20,7 +20,7 @@ const ( // operationSegment maps an operation to its imagor URL path segment. The fill // operation is the bare WxH resize (imagor's default), so it contributes no // leading segment; stretch and fit-in use their imagor names. Note that real -// imagor has no /fill/ route — "fill" is only our internal name for the default +// imagor has no /fill/ route, "fill" is only our internal name for the default // center-crop resize. func operationSegment(op string) string { switch op { diff --git a/services/webdav/pkg/preprocessor/preprocessor.go b/services/webdav/pkg/preprocessor/preprocessor.go index b82f1c6f4e..06ffb84f74 100644 --- a/services/webdav/pkg/preprocessor/preprocessor.go +++ b/services/webdav/pkg/preprocessor/preprocessor.go @@ -36,7 +36,6 @@ type Image struct { Data []byte } -// FileConverter is the interface for the file converters // GgsDecoder is a converter for the geogebra slides file type GgsDecoder struct{ thumbnailpath string } diff --git a/services/webdav/pkg/service/v0/service.go b/services/webdav/pkg/service/v0/service.go index 6e81fe271c..7c91cfdb26 100644 --- a/services/webdav/pkg/service/v0/service.go +++ b/services/webdav/pkg/service/v0/service.go @@ -112,10 +112,10 @@ func NewService(opts ...Option) (Service, error) { r.Use(svc.DavUserContext()) if !conf.DisablePreviews { - r.Get("/remote.php/dav/spaces/{id}", svc.SpacesThumbnail) - r.Get("/remote.php/dav/spaces/{id}/*", svc.SpacesThumbnail) - r.Get("/dav/spaces/{id}", svc.SpacesThumbnail) - r.Get("/dav/spaces/{id}/*", svc.SpacesThumbnail) + r.Get("/remote.php/dav/spaces/{id}", svc.Thumbnail) + r.Get("/remote.php/dav/spaces/{id}/*", svc.Thumbnail) + r.Get("/dav/spaces/{id}", svc.Thumbnail) + r.Get("/dav/spaces/{id}/*", svc.Thumbnail) r.Get("/remote.php/dav/files/{id}", svc.Thumbnail) r.Get("/remote.php/dav/files/{id}/*", svc.Thumbnail) @@ -359,18 +359,8 @@ func (g Webdav) writeThumbnail(w http.ResponseWriter, r *http.Request, tr *reque generator.WriteThumbnailResponse(w, thumb.Data, thumb.ContentType) } -// SpacesThumbnail is the endpoint for retrieving thumbnails inside of spaces. -func (g Webdav) SpacesThumbnail(w http.ResponseWriter, r *http.Request) { - logger := g.requestLogger(r) - tr, ok := g.parseThumbnailRequest(w, r, logger) - if !ok { - return - } - - g.writeThumbnail(w, r, tr, r.Header.Get(revactx.TokenHeader), logger) -} - -// Thumbnail implements the Service interface. +// Thumbnail serves a thumbnail for every authenticated DAV path, spaces, files +// and the legacy /webdav root alike: the reference is parsed from the request. func (g Webdav) Thumbnail(w http.ResponseWriter, r *http.Request) { logger := g.requestLogger(r) tr, ok := g.parseThumbnailRequest(w, r, logger) diff --git a/services/webdav/pkg/thumbnail/sourcetypes.go b/services/webdav/pkg/thumbnail/sourcetypes.go index 3495d5b011..461c96ef28 100644 --- a/services/webdav/pkg/thumbnail/sourcetypes.go +++ b/services/webdav/pkg/thumbnail/sourcetypes.go @@ -45,9 +45,6 @@ func (t SourceTypes) Supports(contentType string) bool { return supported } -// All lists the types, so a caller can iterate them. -func (t SourceTypes) All() map[string]struct{} { return t.byType } - // List is the types as a slice, for a configuration that takes one. func (t SourceTypes) List() []string { out := make([]string, 0, len(t.byType)) diff --git a/services/webdav/pkg/thumbnail/typemapping_test.go b/services/webdav/pkg/thumbnail/typemapping_test.go index d3ae199300..4557b75fb1 100644 --- a/services/webdav/pkg/thumbnail/typemapping_test.go +++ b/services/webdav/pkg/thumbnail/typemapping_test.go @@ -12,7 +12,7 @@ var _ = Describe("TypeMapping", func() { m := thumbnail.NewTypeMapping(nil) It("answers for every type a preview is made of", func() { - for mimeType := range thumbnail.NewSourceTypes(nil).All() { + for _, mimeType := range thumbnail.NewSourceTypes(nil).List() { Expect(m.TargetFor(mimeType)).ToNot(BeEmpty(), "%s has no output type", mimeType) } }) diff --git a/services/webdav/pkg/thumbnail/workflow/workflow.go b/services/webdav/pkg/thumbnail/workflow/workflow.go index e1e6af5afb..417f6f455c 100644 --- a/services/webdav/pkg/thumbnail/workflow/workflow.go +++ b/services/webdav/pkg/thumbnail/workflow/workflow.go @@ -118,23 +118,14 @@ func fileIsProcessing(info *providerv1beta1.ResourceInfo) bool { return utils.ReadPlainFromOpaque(info.GetOpaque(), "status") == "processing" } -// processorID normalizes the requested processor to the identifier used in cache -// keys, matching the legacy behavior where an empty processor resolved to "thumbnail". -func processorID(processor string) string { - if processor == "" { - return "thumbnail" - } - return strings.ToLower(processor) -} - // thumbnailStorageKey builds the cache key: checksum[:2]/checksum[2:4]/checksum[4:]/wxh-.format. // The segment (fit-in, fill, stretch, ...) distinguishes requests that // share a size but produce different images, so they never share a cache entry. // The key carries the snapped box, not the requested size, so every request that // rounds to the same box shares one entry. Caches regenerate when the configured // resolutions or operations change. -func thumbnailStorageKey(checksum string, w, h int, format, processor string) string { - name := fmt.Sprintf("%dx%d-%s.%s", w, h, processor, format) +func thumbnailStorageKey(checksum string, w, h int, format, operation string) string { + name := fmt.Sprintf("%dx%d-%s.%s", w, h, operation, format) if len(checksum) >= 6 { return checksum[:2] + "/" + checksum[2:4] + "/" + checksum[4:] + "/" + name } @@ -295,38 +286,48 @@ func (w *ThumbnailWorkflow) Head(ctx context.Context, tr *requests.ThumbnailRequ return err } + _, err = w.statSource(ctx, ref, auth, logger) + + return err +} + +// statSource stats the source and reports whether a thumbnail may be made of it +// at all, which is the same question for a HEAD and for a generation. +func (w *ThumbnailWorkflow) statSource(ctx context.Context, ref *providerv1beta1.Reference, auth string, logger log.Logger) (*providerv1beta1.ResourceInfo, error) { statRsp, err := w.stater.Stat(ctx, ref, auth) if err != nil { logger.Debug().Err(err).Msg("could not stat file") - return fmt.Errorf("stat: %w", err) + + return nil, fmt.Errorf("stat: %w", err) } info := statRsp.GetInfo() if fileIsProcessing(info) { - return ErrFileProcessing + return nil, ErrFileProcessing } if !info.GetPermissionSet().GetInitiateFileDownload() { - return ErrPermissionDenied + return nil, ErrPermissionDenied } if !w.sourceTypes.Supports(info.GetMimeType()) { - return fmt.Errorf("%w: %s", ErrUnsupportedFileType, info.GetMimeType()) + return nil, fmt.Errorf("%w: %s", ErrUnsupportedFileType, info.GetMimeType()) } if w.maxInputSize > 0 && info.GetSize() > w.maxInputSize { logger.Debug().Uint64("size", info.GetSize()).Uint64("max", w.maxInputSize).Msg("file too large for thumbnail") - return ErrImageTooLarge + + return nil, ErrImageTooLarge } - return nil + return info, nil } // resolveReference turns the request's reference into a fully-anchored CS3 // reference, mirroring reva's ocdav split of space-ID-based requests from // path-only requests: -// - If Ref carries a ResourceId (dav/spaces/{id}/...), it is used directly — +// - If Ref carries a ResourceId (dav/spaces/{id}/...), it is used directly, // the URL already identifies the space root. // - Otherwise (path-only request) the absolute CS3 path is resolved to the // user's home-relative path and, when a SpaceLookup is configured, the @@ -403,29 +404,9 @@ func (w *ThumbnailWorkflow) absolutizeUserPath(ctx context.Context, p, identifie } func (w *ThumbnailWorkflow) generate(ctx context.Context, ref *providerv1beta1.Reference, auth string, tr *requests.ThumbnailRequest, logger log.Logger) (Thumbnail, error) { - statRsp, err := w.stater.Stat(ctx, ref, auth) + info, err := w.statSource(ctx, ref, auth, logger) if err != nil { - logger.Debug().Err(err).Msg("could not stat file") - return Thumbnail{}, fmt.Errorf("stat: %w", err) - } - - info := statRsp.GetInfo() - - if fileIsProcessing(info) { - return Thumbnail{}, ErrFileProcessing - } - - if !info.GetPermissionSet().GetInitiateFileDownload() { - return Thumbnail{}, ErrPermissionDenied - } - - if !w.sourceTypes.Supports(info.GetMimeType()) { - return Thumbnail{}, fmt.Errorf("%w: %s", ErrUnsupportedFileType, info.GetMimeType()) - } - - if w.maxInputSize > 0 && info.GetSize() > w.maxInputSize { - logger.Debug().Uint64("size", info.GetSize()).Uint64("max", w.maxInputSize).Msg("file too large for thumbnail") - return Thumbnail{}, ErrImageTooLarge + return Thumbnail{}, err } checksum := info.GetChecksum().GetSum() @@ -467,7 +448,7 @@ func (w *ThumbnailWorkflow) generate(ctx context.Context, ref *providerv1beta1.R // requests that produce the same image share one entry and the number of // entries per file stays bounded by the ladder. It needs nothing but the // request, so a hit still returns without touching storage. - cacheKey := thumbnailStorageKey(checksum, box.Dx(), box.Dy(), outputFormat, processorID(operation)) + cacheKey := thumbnailStorageKey(checksum, box.Dx(), box.Dy(), outputFormat, operation) if w.cache != nil { if cached, err := w.cache.Get(cacheKey); err == nil { @@ -499,8 +480,10 @@ func (w *ThumbnailWorkflow) generate(ctx context.Context, ref *providerv1beta1.R } if thumbType != "" && thumbType != outputType { // the answer is always the type we cache under, so the same request - // cannot report one type now and another one after a cache hit - logger.Debug().Str("asked", outputType).Str("answered", thumbType). + // cannot report one type now and another one after a cache hit. A + // generator that writes something else than it was asked for means the + // configured formats do not match it, which an operator has to fix. + logger.Warn().Str("asked", outputType).Str("answered", thumbType). Msg("generator wrote a type we did not ask for") } @@ -569,10 +552,9 @@ func (w *ThumbnailWorkflow) sourceImage(ctx context.Context, ref *providerv1beta // - (none), gif -> stretch (resize for gifs) // - (none), non-gif -> fill (default = thumbnail = fill) // -// "thumbnail" and "fit" are legacy aliases. They map onto the imagor operations -// above but IGNORE the legacy "a" and "scalingup" query parameters — callers that -// need aspect/upscaling control should use the imagor names directly ("fill", -// "fit-in", "stretch"). The second return value reports whether upscaling must be +// "thumbnail" and "fit" are legacy aliases for "fill" and "fit-in". Naming any +// processor decides the behavior on its own, so "a" is overruled and "scalingup" +// is ignored either way. The second return value reports whether upscaling must be // suppressed (no_upscale): only fit-in never enlarges its source, so it is true // exactly for the fit/fit-in processors; fill, thumbnail and resize all allow the // generator to enlarge a small source to fill the snapped box. The third return diff --git a/services/webdav/pkg/thumbnail/workflow/workflow_test.go b/services/webdav/pkg/thumbnail/workflow/workflow_test.go index dd9469c7cc..0aef504b87 100644 --- a/services/webdav/pkg/thumbnail/workflow/workflow_test.go +++ b/services/webdav/pkg/thumbnail/workflow/workflow_test.go @@ -29,6 +29,7 @@ import ( "github.com/stretchr/testify/mock" "google.golang.org/grpc" + "github.com/opencloud-eu/opencloud/pkg/conversions" "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/webdav/pkg/constants" "github.com/opencloud-eu/opencloud/services/webdav/pkg/dav/requests" @@ -55,24 +56,6 @@ func spaceRef(spaceID, relPath string) *providerv1beta1.Reference { } } -// ptr is the address of a literal, for the optional flags on a request. -func ptr[T any](v T) *T { return &v } - -var _ = Describe("processorID", func() { - It("maps the default (empty) processor to the thumbnail id", func() { - Expect(processorID("")).To(Equal("thumbnail")) - }) - - It("maps an explicit thumbnail processor to the same id as the default", func() { - Expect(processorID("thumbnail")).To(Equal("thumbnail")) - }) - - It("gives opt-in processors their own id so they cache separately", func() { - Expect(processorID("fit")).To(Equal("fit")) - Expect(processorID("fill")).To(Equal("fill")) - }) -}) - var _ = Describe("matchOperation", func() { w := &ThumbnailWorkflow{} @@ -139,29 +122,29 @@ var _ = Describe("matchOperation", func() { It("reports aIgnored when an unconstrained axis overrules a=0", func() { op, _, aIgnored := w.matchOperation( - &requests.ThumbnailRequest{Width: 0, Height: 512, Aspect: ptr(false)}, "image/jpeg") + &requests.ThumbnailRequest{Width: 0, Height: 512, Aspect: conversions.ToPointer(false)}, "image/jpeg") Expect(op).To(Equal(generator.OpFitIn)) Expect(aIgnored).To(BeTrue()) }) It("reports aIgnored when an explicit processor overrides the legacy a flag", func() { // a=0 (Aspect=false) wants fill; fit processor overrides it with fit-in. - op, _, aIgnored := w.matchOperation(&requests.ThumbnailRequest{Processor: "fit", Aspect: ptr(false)}, "image/jpeg") + op, _, aIgnored := w.matchOperation(&requests.ThumbnailRequest{Processor: "fit", Aspect: conversions.ToPointer(false)}, "image/jpeg") Expect(op).To(Equal(generator.OpFitIn)) Expect(aIgnored).To(BeTrue()) // a=1 (Aspect=true) wants fit-in; fill processor overrides it with fill. - op, _, aIgnored = w.matchOperation(&requests.ThumbnailRequest{Processor: "fill", Aspect: ptr(true)}, "image/jpeg") + op, _, aIgnored = w.matchOperation(&requests.ThumbnailRequest{Processor: "fill", Aspect: conversions.ToPointer(true)}, "image/jpeg") Expect(op).To(Equal(generator.OpFill)) Expect(aIgnored).To(BeTrue()) // a=0 wants fill; thumbnail processor implies fill -> no contradiction. - op, _, aIgnored = w.matchOperation(&requests.ThumbnailRequest{Processor: "thumbnail", Aspect: ptr(false)}, "image/jpeg") + op, _, aIgnored = w.matchOperation(&requests.ThumbnailRequest{Processor: "thumbnail", Aspect: conversions.ToPointer(false)}, "image/jpeg") Expect(op).To(Equal(generator.OpFill)) Expect(aIgnored).To(BeFalse()) // resize is neutral: never reports a contradiction. - _, _, aIgnored = w.matchOperation(&requests.ThumbnailRequest{Processor: "resize", Aspect: ptr(false)}, "image/jpeg") + _, _, aIgnored = w.matchOperation(&requests.ThumbnailRequest{Processor: "resize", Aspect: conversions.ToPointer(false)}, "image/jpeg") Expect(aIgnored).To(BeFalse()) }) }) diff --git a/tests/config/local/.env-federation b/tests/config/local/.env-federation index 96801d7057..e7ce2f92ca 100644 --- a/tests/config/local/.env-federation +++ b/tests/config/local/.env-federation @@ -88,6 +88,7 @@ export WEB_DEBUG_ADDR=localhost:10104 export WEB_HTTP_ADDR=localhost:10100 export WEBDAV_DEBUG_ADDR=127.0.0.1:10119 export WEBDAV_HTTP_ADDR=127.0.0.1:10115 +export WEBDAV_THUMBNAIL_GENERATOR_URL=http://127.0.0.1:10190 export WEBFINGER_DEBUG_ADDR=127.0.0.1:10279 export WEBFINGER_HTTP_ADDR=127.0.0.1:10275 export OC_SERVICE_ACCOUNT_ID=serviceaccount