From 8a80d753633a26629078a344e955fde6096efcb3 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 21 Oct 2026 14:22:54 +0200 Subject: [PATCH] refactor(webdav): keep ignoring the legacy a flag OpenCloud never honored it, so clients were tested against an ignored a and honoring it now would change what they get. --- services/thumbnails/README.md | 4 +- services/webdav/README.md | 8 +-- services/webdav/pkg/dav/requests/thumbnail.go | 16 ----- .../webdav/pkg/dav/requests/thumbnail_test.go | 17 ------ services/webdav/pkg/service/v0/service.go | 13 ----- .../webdav/pkg/thumbnail/workflow/workflow.go | 44 +++++--------- .../pkg/thumbnail/workflow/workflow_test.go | 58 +++---------------- .../previewsAutoAdustedSizing.feature | 4 +- 8 files changed, 27 insertions(+), 137 deletions(-) diff --git a/services/thumbnails/README.md b/services/thumbnails/README.md index 5f846ed4f8..68d73addc9 100644 --- a/services/thumbnails/README.md +++ b/services/thumbnails/README.md @@ -53,8 +53,8 @@ image). Adding the `no_upscale()` filter caps the result at the source size instead (the same request returns 100x100). The `fit-in` route never upscales, imagor would with the `upscale()` filter, this service ignores it. -Which route a preview request ends up on, and the `processor` and `a` -parameters that decide it, are the webdav service's business and documented in +Which route a preview request ends up on, and the `processor` parameter that +decides it, are the webdav service's business and documented in [its README](../webdav/README.md). The request body is a `multipart/form-data` upload with a single file field diff --git a/services/webdav/README.md b/services/webdav/README.md index 82cba43bb5..2ebd79f80d 100644 --- a/services/webdav/README.md +++ b/services/webdav/README.md @@ -24,7 +24,7 @@ The following query parameters are supported: | x | NO | 32, when y is absent too | horizontal target size, absent leaves the width unconstrained, `0` is an error | | y | NO | 32, when x is absent too | vertical target size, absent leaves the height unconstrained, `0` is an error | | scalingup | NO | 0 | accepted for compatibility but ignored; not forwarded to the generator | -| a | NO | none, the chain below decides | aspect ratio (legacy; only honored when no explicit `processor` is given) | +| a | NO | none | accepted for compatibility but ignored; this service has never honored it, so the chain below decides | | c | NO | none | accepted for compatibility but ignored; the cache key holds the file's checksum, so a changed file gets a new thumbnail without it | | processor | NO | none, the chain below decides | preferred thumbnail processor | @@ -56,13 +56,11 @@ The following are **legacy aliases** kept for compatibility: - `thumbnail` is a legacy alias for `fill` (center-crop to the exact box, may upscale). - `fit` is a legacy alias for `fit-in`. -Naming one of them decides the behavior on its own, legacy alias or imagor name: `a` is overruled and `scalingup` is ignored either way. A request that wants `a` to decide has to leave `processor` out, a name nobody knows is treated the same way. +Naming one of them decides the behavior on its own, legacy alias or imagor name. A name nobody knows is treated like no processor at all. To apply one of those, a query parameter has to be added to the request, like `?processor=fit-in`. -Without a processor three things decide, in this order. An unconstrained axis is always fitted, because there is no box to crop to, so `?y=512` needs nothing else. Then `a` decides when the client sent it: `a=1` fits in, `a=0` fills. Otherwise the source type decides, `resize` for gifs and `fill` for everything else, which is what a request without `a` has always got. - -When what a request resolves to contradicts an `a` the client sent, the response carries the header `X-OpenCloud-Thumbnail-Aspect-Ignored` so developers can fix their client. A request that does not send `a` never gets the header. +Without a processor two things decide, in this order. An unconstrained axis is always fitted, because there is no box to crop to, so `?y=512` needs nothing else. Otherwise the source type decides, `resize` for gifs and `fill` for everything else. #### Thumbnail Generator diff --git a/services/webdav/pkg/dav/requests/thumbnail.go b/services/webdav/pkg/dav/requests/thumbnail.go index 9da858f89d..7edfea5317 100644 --- a/services/webdav/pkg/dav/requests/thumbnail.go +++ b/services/webdav/pkg/dav/requests/thumbnail.go @@ -43,10 +43,6 @@ type ThumbnailRequest struct { Height int32 // Indicates which image processor to use Processor string - // Aspect is the legacy ownCloud "a" flag when the client sent it: true wants - // the aspect ratio preserved, false wants the box filled. It is nil when the - // client said nothing, and then nothing is reported as contradicted either. - Aspect *bool // Identifier is the username from /dav/files/{user}/... when present. The // workflow resolves it via GetUserByClaim so the file is looked up in that // user's home; empty for space-ID, /webdav/ and public-link requests. @@ -116,7 +112,6 @@ func ParseThumbnailRequest(r *http.Request) (*ThumbnailRequest, error) { Width: int32(width), Height: int32(height), Processor: processor, - Aspect: aspect(q), Identifier: identifier, }, nil } @@ -165,17 +160,6 @@ func parseDimension(d, name string) (int64, error) { return result, nil } -// aspect reads the legacy "a" flag, nil when the client did not send it. -func aspect(q url.Values) *bool { - if !q.Has("a") { - return nil - } - - preserve := q.Get("a") != "0" - - return &preserve -} - // targetsExactBox reports whether the processor produces exactly the requested // box, by cropping (fill, thumbnail) or by distorting (resize), which an // unconstrained axis cannot describe. fit-in inscribes instead, and so does a diff --git a/services/webdav/pkg/dav/requests/thumbnail_test.go b/services/webdav/pkg/dav/requests/thumbnail_test.go index 35b2d19a02..f597a0916a 100644 --- a/services/webdav/pkg/dav/requests/thumbnail_test.go +++ b/services/webdav/pkg/dav/requests/thumbnail_test.go @@ -8,7 +8,6 @@ import ( . "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" ) @@ -20,22 +19,6 @@ func thumbnailRequest(rawQuery string) *http.Request { } 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()) - - return - } - 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()) diff --git a/services/webdav/pkg/service/v0/service.go b/services/webdav/pkg/service/v0/service.go index d765f16918..78d0e428dd 100644 --- a/services/webdav/pkg/service/v0/service.go +++ b/services/webdav/pkg/service/v0/service.go @@ -342,7 +342,6 @@ func (g Webdav) writeThumbnail(w http.ResponseWriter, r *http.Request, tr *reque return } - setAspectIgnoredHeader(w, thumb.AspectIgnored) generator.WriteThumbnailResponse(w, thumb.Data, thumb.ContentType) } @@ -408,18 +407,6 @@ func (g Webdav) PublicThumbnailHead(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) } -// aspectIgnoredHeader is set on thumbnail responses when what the request -// resolved to contradicts the legacy "a" flag, so developers can tell their -// client to send a consistent request (see the thumbnail docs). -const aspectIgnoredHeader = "X-OpenCloud-Thumbnail-Aspect-Ignored" - -func setAspectIgnoredHeader(w http.ResponseWriter, ignored bool) { - if !ignored { - return - } - w.Header().Set(aspectIgnoredHeader, "1; the processor or the requested box overruled the legacy 'a' flag, see the thumbnail docs") -} - // handleWorkflowError maps a workflow error onto the response. step names what // was attempted, for the log line. func (g Webdav) handleWorkflowError(w http.ResponseWriter, r *http.Request, err error, tr *requests.ThumbnailRequest, logger log.Logger, step string) { diff --git a/services/webdav/pkg/thumbnail/workflow/workflow.go b/services/webdav/pkg/thumbnail/workflow/workflow.go index 4b0fba15c8..ac0b0447ac 100644 --- a/services/webdav/pkg/thumbnail/workflow/workflow.go +++ b/services/webdav/pkg/thumbnail/workflow/workflow.go @@ -266,12 +266,10 @@ func WithUserResolver(r UserResolver) Option { return func(w *ThumbnailWorkflow) { w.userResolver = r } } -// Thumbnail is what a request produces: the image, the type it is returned in, -// and whether an explicit processor overrode a contradicting legacy "a" flag. +// Thumbnail is what a request produces: the image and the type it is returned in. type Thumbnail struct { - Data []byte - ContentType string - AspectIgnored bool + Data []byte + ContentType string } // Execute handles a user thumbnail request (GET). @@ -437,10 +435,8 @@ func (w *ThumbnailWorkflow) generate(ctx context.Context, ref *providerv1beta1.R // The operation is resolved up front so it can be part of the cache key: a // fill and a fit-in request with the same dimensions produce different images // and must not share a cache entry. noUpscale reports whether the generator - // must not enlarge the source (fit-in only); aIgnored reports whether the - // legacy "a" flag was overridden by an explicit processor, so the client can - // be told. - operation, noUpscale, aIgnored := w.matchOperation(tr, mimeType) + // must not enlarge the source (fit-in only). + operation, noUpscale := w.matchOperation(tr, mimeType) // webdav is the sizing brain: it snaps each axis of the requested size onto // the configured ladder. It never inspects the image bytes; the generator is @@ -460,7 +456,7 @@ func (w *ThumbnailWorkflow) generate(ctx context.Context, ref *providerv1beta1.R cached, err := w.cache.Get(cacheKey) switch { case err == nil: - return Thumbnail{Data: cached, ContentType: outputType, AspectIgnored: aIgnored}, nil + return Thumbnail{Data: cached, ContentType: outputType}, nil case !errors.Is(err, cache.ErrCacheMiss): // a miss is the normal answer; anything else is the cache itself // failing, and the thumbnail is made again without a word otherwise @@ -506,7 +502,7 @@ func (w *ThumbnailWorkflow) generate(ctx context.Context, ref *providerv1beta1.R } } - return Thumbnail{Data: thumbBytes, ContentType: outputType, AspectIgnored: aIgnored}, nil + return Thumbnail{Data: thumbBytes, ContentType: outputType}, nil } // sourceImage returns a reader over the source image to send to the generator, @@ -555,10 +551,10 @@ func (w *ThumbnailWorkflow) sourceImage(ctx context.Context, ref *providerv1beta // matchOperation picks how to fit within the box webdav already snapped. A // processor the client named wins, "thumbnail" and "fit" being legacy aliases for -// fill and fit-in; without one, an unconstrained axis, then "a", then the source -// type decide, and "scalingup" is ignored throughout. Only fit-in never enlarges -// its source, so it is the one operation that suppresses upscaling. -func (w *ThumbnailWorkflow) matchOperation(tr *requests.ThumbnailRequest, mimeType string) (operation string, noUpscale, aIgnored bool) { +// fill and fit-in; without one, an unconstrained axis, then the source type +// decide, and "scalingup" is ignored throughout. Only fit-in never enlarges its +// source, so it is the one operation that suppresses upscaling. +func (w *ThumbnailWorkflow) matchOperation(tr *requests.ThumbnailRequest, mimeType string) (operation string, noUpscale bool) { proc := strings.ToLower(strings.TrimSpace(tr.Processor)) switch proc { @@ -568,14 +564,10 @@ func (w *ThumbnailWorkflow) matchOperation(tr *requests.ThumbnailRequest, mimeTy operation = generator.OpFill case "fit", "fit-in": operation = generator.OpFitIn - default: // no processor: a free axis, then "a", then the source type decide + default: // no processor: a free axis, then the source type decide switch { case tr.Width == 0 || tr.Height == 0: operation = generator.OpFitIn - case tr.Aspect != nil && *tr.Aspect: - operation = generator.OpFitIn - case tr.Aspect != nil: - operation = generator.OpFill case strings.HasPrefix(mimeType, "image/gif"): operation = generator.OpStretch default: @@ -585,17 +577,7 @@ func (w *ThumbnailWorkflow) matchOperation(tr *requests.ThumbnailRequest, mimeTy noUpscale = operation == generator.OpFitIn - // stretch distorts either way, so "a" never counts as ignored for it - if tr.Aspect != nil { - switch operation { - case generator.OpFill: - aIgnored = *tr.Aspect // a asked for fit-in but we fill - case generator.OpFitIn: - aIgnored = !*tr.Aspect // a asked for fill but we fit in - } - } - - return operation, noUpscale, aIgnored + return operation, noUpscale } // quoteEscaper matches the escaping mime/multipart applies to part filenames. diff --git a/services/webdav/pkg/thumbnail/workflow/workflow_test.go b/services/webdav/pkg/thumbnail/workflow/workflow_test.go index 1c6d7cbd4e..cefacb5245 100644 --- a/services/webdav/pkg/thumbnail/workflow/workflow_test.go +++ b/services/webdav/pkg/thumbnail/workflow/workflow_test.go @@ -29,7 +29,6 @@ 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" @@ -60,93 +59,52 @@ var _ = Describe("matchOperation", func() { w := &ThumbnailWorkflow{} It("stretches for the resize processor and allows upscaling", func() { - op, noUpscale, aIgnored := w.matchOperation(&requests.ThumbnailRequest{Processor: "resize"}, "image/jpeg") + op, noUpscale := w.matchOperation(&requests.ThumbnailRequest{Processor: "resize"}, "image/jpeg") Expect(op).To(Equal(generator.OpStretch)) Expect(noUpscale).To(BeFalse()) - Expect(aIgnored).To(BeFalse()) }) It("fills for the fill processor and allows upscaling", func() { - op, noUpscale, _ := w.matchOperation(&requests.ThumbnailRequest{Processor: "fill"}, "image/jpeg") + op, noUpscale := w.matchOperation(&requests.ThumbnailRequest{Processor: "fill"}, "image/jpeg") Expect(op).To(Equal(generator.OpFill)) Expect(noUpscale).To(BeFalse()) }) It("treats the thumbnail processor as fill (square tiles) and allows upscaling", func() { - op, noUpscale, _ := w.matchOperation(&requests.ThumbnailRequest{Processor: "thumbnail"}, "image/jpeg") + op, noUpscale := w.matchOperation(&requests.ThumbnailRequest{Processor: "thumbnail"}, "image/jpeg") Expect(op).To(Equal(generator.OpFill)) Expect(noUpscale).To(BeFalse()) }) It("fits in for the fit processor and suppresses upscaling", func() { - op, noUpscale, _ := w.matchOperation(&requests.ThumbnailRequest{Processor: "fit"}, "image/jpeg") + op, noUpscale := w.matchOperation(&requests.ThumbnailRequest{Processor: "fit"}, "image/jpeg") Expect(op).To(Equal(generator.OpFitIn)) Expect(noUpscale).To(BeTrue()) }) It("fits in for the fit-in processor and suppresses upscaling", func() { - op, noUpscale, _ := w.matchOperation(&requests.ThumbnailRequest{Processor: "fit-in"}, "image/jpeg") + op, noUpscale := w.matchOperation(&requests.ThumbnailRequest{Processor: "fit-in"}, "image/jpeg") Expect(op).To(Equal(generator.OpFitIn)) Expect(noUpscale).To(BeTrue()) }) It("defaults to fill for non-gif sources without a processor and allows upscaling", func() { - op, noUpscale, aIgnored := w.matchOperation(&requests.ThumbnailRequest{Width: 64, Height: 64}, "image/jpeg") + op, noUpscale := w.matchOperation(&requests.ThumbnailRequest{Width: 64, Height: 64}, "image/jpeg") Expect(op).To(Equal(generator.OpFill)) Expect(noUpscale).To(BeFalse()) - Expect(aIgnored).To(BeFalse()) }) It("stretches gifs by default (resize for gifs) and allows upscaling", func() { - op, noUpscale, _ := w.matchOperation(&requests.ThumbnailRequest{Width: 64, Height: 64}, "image/gif") + op, noUpscale := w.matchOperation(&requests.ThumbnailRequest{Width: 64, Height: 64}, "image/gif") Expect(op).To(Equal(generator.OpStretch)) Expect(noUpscale).To(BeFalse()) }) It("fits in when an axis is unconstrained, there is nothing to crop to", func() { - op, noUpscale, _ := w.matchOperation(&requests.ThumbnailRequest{Width: 0, Height: 512}, "image/jpeg") + op, noUpscale := w.matchOperation(&requests.ThumbnailRequest{Width: 0, Height: 512}, "image/jpeg") Expect(op).To(Equal(generator.OpFitIn)) Expect(noUpscale).To(BeTrue()) }) - - DescribeTable("honours a when the client sent it and no processor", - func(aspect bool, want string) { - op, _, aIgnored := w.matchOperation( - &requests.ThumbnailRequest{Width: 64, Height: 64, Aspect: &aspect}, "image/jpeg") - Expect(op).To(Equal(want)) - Expect(aIgnored).To(BeFalse(), "nothing contradicted a") - }, - Entry("a=1 preserves the aspect ratio", true, generator.OpFitIn), - Entry("a=0 fills the box", false, generator.OpFill), - ) - - It("reports aIgnored when an unconstrained axis overrules a=0", func() { - op, _, aIgnored := w.matchOperation( - &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: 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: 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: 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: conversions.ToPointer(false)}, "image/jpeg") - Expect(aIgnored).To(BeFalse()) - }) }) func newTestRequest(path string, token string, filePath string) *http.Request { diff --git a/tests/acceptance/features/coreApiWebdavPreviews/previewsAutoAdustedSizing.feature b/tests/acceptance/features/coreApiWebdavPreviews/previewsAutoAdustedSizing.feature index c5de177d42..f7a0438baa 100644 --- a/tests/acceptance/features/coreApiWebdavPreviews/previewsAutoAdustedSizing.feature +++ b/tests/acceptance/features/coreApiWebdavPreviews/previewsAutoAdustedSizing.feature @@ -59,9 +59,7 @@ Feature: sizing of previews of files downloaded through the webdav API # # The OpenCloud web client always sends a processor: list view requests # processor=thumbnail at 64x64, tiles view requests processor=fit at a dynamic - # square between 320 and 768. It currently also sends the legacy a=1 flag on - # every request; that is redundant now that an explicit processor takes - # precedence over a, so web should stop sending it. + # square between 320 and 768. Examples: | request-width | request-height | processor | return-width | return-height | dav-path-version | | 1 | 1 | fit | 16 | 12 | spaces |