mirror of
https://github.com/opencloud-eu/opencloud.git
synced 2026-10-31 02:28:31 -04:00
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.
This commit is contained in:
1 parent
83a0facc08
commit
8a80d75363
8 files changed
+27
-137
No files matched your search
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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())
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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 |
|
||||
|
||||
Reference in new issue
Block a user