fix: answer the third review round

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.

services/thumbnails/README.md

@@ -65,10 +65,12 @@ its first frame, and a still source asked for `format(gif)` is palettised.
 Reading an ICO needs libvips with its magick loader; the pure Go build answers
 `406` for one.
 
-The answers are imagor's: `400` for a malformed request, `406` for a source it
-cannot read, for a format this build cannot write and for an upload part that
-is not typed `image/*`, `422` for an image above the configured maximum
-dimensions, `429` when the concurrency limit is reached.
+The answers are imagor's: `400` for a malformed request, which includes a
+`format` nobody writes (anything but `jpg`, `png`, `gif` and `webp`); `406` for a
+source it cannot read, for an upload part that is not typed `image/*`, and for a
+format this build has no encoder for (`webp` without libvips); `422` for an image
+above the configured maximum dimensions; `429` when the concurrency limit is
+reached.
 
 ## Configuration
 

services/thumbnails/pkg/service/http/v0/push.go

@@ -13,8 +13,8 @@ import (
 	"github.com/go-chi/chi/v5"
 )
 
-// gifMagic is the leading signature of a GIF file (GIF87a or GIF89a). It guards
-// against gif.DecodeAll panicking on non-GIF input.
+// gifMagic is the leading signature of a GIF file (GIF87a or GIF89a). It decides
+// whether a request takes the animation path.
 var gifMagic = []byte("GIF8")
 
 // Operations understood by the push endpoint. They map to imagor's resize/crop
@@ -36,9 +36,11 @@ var errFormatUnsupported = fmt.Errorf("format not supported by this build")
 //
 // The filters segment is captured whole and parsed by outputFormatFromFilters; only the
 // format filter is meaningful to this executor, other filters are ignored. When no format
-// filter is present the input's own format is preserved (detected via imaging), matching
-// imagor. It returns only imagor's defined status codes: 400 for invalid requests or a
-// file exceeding the max size, 422 for an image exceeding the max resolution.
+// filter is present the input's own format is preserved, as imagor does it, and the
+// backend that will decode the image says which format that is. The answers are imagor's:
+// 400 for a malformed request, 406 for a source it cannot read or a format it cannot
+// write, 422 for an image above the configured maximum dimensions, 429 when the
+// concurrency limit is reached.
 func (s Thumbnails) pushHandler(w http.ResponseWriter, r *http.Request) {
 	ok, retryAfter := s.limiter.acquire()
 	if !ok {
@@ -217,7 +219,7 @@ func (s Thumbnails) serveAnimation(w http.ResponseWriter, imgData []byte, req re
 	}
 
 	var buf bytes.Buffer
-	if err := gif.EncodeAll(&buf, resizeGIF(g, req.Width, req.Height, req.Operation, req.NoUpscale)); err != nil {
+	if err := gif.EncodeAll(&buf, resizeGIF(g, req)); err != nil {
 		s.logger.Error().Err(err).Msg("could not encode the gif")
 		w.WriteHeader(http.StatusInternalServerError)
 		fmt.Fprintln(w, "failed to encode image")

services/thumbnails/pkg/service/http/v0/push_imaging.go

@@ -15,8 +15,8 @@ import (
 )
 
 // The backend this build resizes with. The implementations keep their own names
-// so both builds' versions stay reachable by symbol, the variables are what the
-// shared handler calls.
+// so a symbol search finds both builds' versions side by side, the variables are
+// what the shared handler calls.
 var (
 	loadSource         = loadSourceImaging
 	extForSourceFormat = extForSourceFormatImaging

services/thumbnails/pkg/service/http/v0/push_vips.go

@@ -11,8 +11,8 @@ import (
 )
 
 // The backend this build resizes with. The implementations keep their own names
-// so both builds' versions stay reachable by symbol, the variables are what the
-// shared handler calls.
+// so a symbol search finds both builds' versions side by side, the variables are
+// what the shared handler calls.
 //
 // libvips is demand-driven: loading a buffer parses the header and computes
 // pixels only when something pulls them, so the dimension guard has its say

services/thumbnails/pkg/service/http/v0/resizegif.go

@@ -14,8 +14,9 @@ import (
 // imaging either way, because neither libvips nor the pure Go path has anything
 // better for a paletted animation. Code adapted from
 // https://github.com/willnorris/gifresize.
-func resizeGIF(m *gif.GIF, width, height int, operation string, noUpscale bool) *gif.GIF {
+func resizeGIF(m *gif.GIF, req resizeRequest) *gif.GIF {
 	srcX, srcY := m.Config.Width, m.Config.Height
+	width, height, operation, noUpscale := req.Width, req.Height, req.Operation, req.NoUpscale
 
 	if width == 0 || height == 0 {
 		resolvedW, resolvedH, resize := autoAxis(srcX, srcY, width, height)

services/webdav/pkg/config/defaults/defaultconfig.go

@@ -1,10 +1,8 @@
 package defaults
 
 import (
-	"path"
 	"strings"
 
-	cored "github.com/opencloud-eu/opencloud/pkg/config/defaults"
 	"github.com/opencloud-eu/opencloud/pkg/shared"
 	"github.com/opencloud-eu/opencloud/pkg/structs"
 	"github.com/opencloud-eu/opencloud/services/webdav/pkg/config"
@@ -49,7 +47,6 @@ func DefaultConfig() *config.Config {
 		ThumbnailGeneratorTimeout: "30s",
 		ThumbnailMaxInputFileSize: "50MB",
 		ThumbnailCacheBackend:     "file",
-		ThumbnailCacheDir:         path.Join(cored.BaseDataPath(), "thumbnails", "files"),
 		ThumbnailResolutions: []string{
 			"16", "32", "64", "128", "192", "256", "320", "384", "448",
 			"512", "640", "768", "1024", "1280", "1920", "2560", "3840", "7680",
@@ -78,4 +75,10 @@ func Sanitize(cfg *config.Config) {
 	if cfg.HTTP.Root != "/" {
 		cfg.HTTP.Root = strings.TrimSuffix(cfg.HTTP.Root, "/")
 	}
+
+	// a yaml configuration that joined the resolutions into one entry kept
+	// working in the thumbnails service, so it keeps working here
+	if len(cfg.ThumbnailResolutions) == 1 && strings.Contains(cfg.ThumbnailResolutions[0], ",") {
+		cfg.ThumbnailResolutions = strings.Split(cfg.ThumbnailResolutions[0], ",")
+	}
 }

services/webdav/pkg/dav/requests/thumbnail.go

@@ -181,9 +181,11 @@ func aspect(q url.Values) *bool {
 // unconstrained axis cannot describe. Only fit-in inscribes instead.
 func targetsExactBox(processor string) bool {
 	switch strings.ToLower(strings.TrimSpace(processor)) {
-	case "fit", "fit-in":
-		return false
-	default:
+	case "fill", "thumbnail", "resize":
 		return true
+	default:
+		// fit, fit-in, and any name nobody knows, which the workflow resolves
+		// the same way it resolves a request without a processor
+		return false
 	}
 }

services/webdav/pkg/dav/requests/thumbnail_test.go

@@ -55,6 +55,7 @@ var _ = Describe("ParseThumbnailRequest", func() {
 		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)),
+		Entry("treats a processor nobody knows like none at all", "y=512&processor=bogus", int32(0), int32(512)),
 	)
 
 	DescribeTable("a request it refuses",

services/webdav/pkg/service/v0/previews_test.go

@@ -0,0 +1,27 @@
+package svc
+
+import (
+	"net/http"
+	"net/http/httptest"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	"github.com/opencloud-eu/opencloud/pkg/log"
+	"github.com/opencloud-eu/opencloud/services/webdav/pkg/config"
+)
+
+var _ = Describe("with previews disabled", func() {
+	// the routes stay registered, so a missing one cannot let the REPORT
+	// patterns answer a thumbnail request with 405
+	It("answers a thumbnail request with 404 before anything is parsed", func() {
+		g := Webdav{config: &config.Config{DisablePreviews: true}}
+		rec := httptest.NewRecorder()
+
+		tr, ok := g.parseThumbnailRequest(rec, httptest.NewRequest(http.MethodGet, "/dav/files/einstein/photo.png?x=32&y=32&preview=1", nil), log.NopLogger())
+
+		Expect(ok).To(BeFalse())
+		Expect(tr).To(BeNil())
+		Expect(rec.Code).To(Equal(http.StatusNotFound))
+	})
+})

services/webdav/pkg/service/v0/service.go

@@ -111,17 +111,15 @@ func NewService(opts ...Option) (Service, error) {
 		r.Group(func(r chi.Router) {
 			r.Use(svc.DavUserContext())
 
-			if !conf.DisablePreviews {
-				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/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)
-				r.Get("/dav/files/{id}", svc.Thumbnail)
-				r.Get("/dav/files/{id}/*", svc.Thumbnail)
-			}
+			r.Get("/remote.php/dav/files/{id}", svc.Thumbnail)
+			r.Get("/remote.php/dav/files/{id}/*", svc.Thumbnail)
+			r.Get("/dav/files/{id}", svc.Thumbnail)
+			r.Get("/dav/files/{id}/*", svc.Thumbnail)
 
 			r.MethodFunc("REPORT", "/remote.php/dav/spaces*", svc.Search)
 			r.MethodFunc("REPORT", "/dav/spaces*", svc.Search)
@@ -129,25 +127,21 @@ func NewService(opts ...Option) (Service, error) {
 			r.MethodFunc("REPORT", "/dav/files*", svc.Search)
 		})
 
-		if !conf.DisablePreviews {
-			r.Group(func(r chi.Router) {
-				r.Use(svc.DavPublicContext())
+		r.Group(func(r chi.Router) {
+			r.Use(svc.DavPublicContext())
 
-				r.Head("/remote.php/dav/public-files/{token}/*", svc.PublicThumbnailHead)
-				r.Head("/dav/public-files/{token}/*", svc.PublicThumbnailHead)
+			r.Head("/remote.php/dav/public-files/{token}/*", svc.PublicThumbnailHead)
+			r.Head("/dav/public-files/{token}/*", svc.PublicThumbnailHead)
 
-				r.Get("/remote.php/dav/public-files/{token}/*", svc.PublicThumbnail)
-				r.Get("/dav/public-files/{token}/*", svc.PublicThumbnail)
-			})
-		}
+			r.Get("/remote.php/dav/public-files/{token}/*", svc.PublicThumbnail)
+			r.Get("/dav/public-files/{token}/*", svc.PublicThumbnail)
+		})
 
 		r.Group(func(r chi.Router) {
 			r.Use(svc.WebDAVContext())
 
-			if !conf.DisablePreviews {
-				r.Get("/remote.php/webdav/*", svc.Thumbnail)
-				r.Get("/webdav/*", svc.Thumbnail)
-			}
+			r.Get("/remote.php/webdav/*", svc.Thumbnail)
+			r.Get("/webdav/*", svc.Thumbnail)
 
 			r.MethodFunc("REPORT", "/remote.php/webdav*", svc.Search)
 			r.MethodFunc("REPORT", "/webdav*", svc.Search)
@@ -201,6 +195,8 @@ func newThumbnailWorkflow(conf *config.Config, sourceTypes thumbnail.SourceTypes
 		conf.ThumbnailCacheS3SecretKey,
 	)
 
+	// the new setting wins, the deprecated one is the fallback, and an empty
+	// result lets the cache use its own default directory
 	cacheDir := conf.ThumbnailCacheDir
 	if cacheDir == "" {
 		cacheDir = conf.ThumbnailFilesystemStorageRoot
@@ -336,6 +332,15 @@ func (g Webdav) requestLogger(r *http.Request) log.Logger {
 // parseThumbnailRequest reads the request every thumbnail endpoint starts with.
 // It reports false when it already answered with a 400.
 func (g Webdav) parseThumbnailRequest(w http.ResponseWriter, r *http.Request, logger log.Logger) (*requests.ThumbnailRequest, bool) {
+	if g.config.DisablePreviews {
+		// every thumbnail path answers 404, which is what the setting promises.
+		// The routes stay registered: the REPORT patterns match the same paths,
+		// so without them chi would answer a thumbnail request with 405.
+		http.NotFound(w, r)
+
+		return nil, false
+	}
+
 	tr, err := requests.ParseThumbnailRequest(r)
 	if err != nil {
 		logger.Debug().Err(err).Msg("could not create Request")