Thumbnails Service #3

C0rby · 2020-03-10T14:16:25Z

Can generate thumbnails and store them in the filesystem.

Still missing:

Proper API (Implement proper API #4)
Security measures to prevent users to see other users thumbnails. (Implement security measures to prevent information leaks #5)
An idea for proper authorization (#6)

Example request:

GET /thumbnails?file_path=oc.png&width=500&height=300&type=png&etag=979f4c8db98f7b82e768ef478d3c8612 HTTP/1.1
> Host: localhost:9190
> User-Agent: curl/7.69.0
> Accept: */*
> Authorization: Bearer <JWT>

Currently uses in memory caching and loads the file from the local filesystem.

If the file is not cached we have to get it from somewhere. For that I implemented a webdav file source which gets the file from reva.

update-docs · 2020-03-10T14:16:28Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

IljaN · 2020-03-10T17:04:35Z

pkg/service/v0/service.go

-	w.Write([]byte("Hello ocis-thumbnails!"))
+	encoder := thumbnails.EncoderForType(fileType)
+	if encoder == nil {
+		// TODO: better error responses


Use https://golang.org/pkg/net/http/#Error here and in all subsequent cases.

You are right. But since the API will be changed in another PR anyways I would leave it like that for now.

refs

👌 a bit of nitty-gritty regarding functionality and style. Other than that looks good. Still need to run it locally tho.

refs · 2020-03-11T09:14:57Z

pkg/flagset/flagset.go

@@ -89,7 +89,7 @@ func ServerWithConfig(cfg *config.Config) []cli.Flag {
 		},
 		&cli.StringFlag{
 			Name:        "debug-addr",
-			Value:       "0.0.0.0:9114",
+			Value:       "0.0.0.0:9194",


make sure to reserve a port range here

Oh I wanted to change that back... But yes we should define a port for the service.

refs · 2020-03-11T09:17:11Z

pkg/thumbnails/storage/filesystem.go

+// NewFileSystemStorage creates a new instanz of FileSystem
+func NewFileSystemStorage(cfg config.FileSystemStorage) FileSystem {
+	return FileSystem{
+		dir: cfg.RootDirectory,


Would this work on non UNIX systems?

I'd leave UNIX by default and on a Before hook check the host OS and update config

refs · 2020-03-11T09:26:35Z

pkg/thumbnails/storage/filesystem.go

+func (s FileSystem) Set(key string, img []byte) error {
+	path := filepath.Join(s.dir, key)
+	dir := filepath.Dir(path)
+	if err := os.MkdirAll(dir, 0700); err != nil {


does it need execution permissions? or can this be set to 0600?

Yes, execution permission is needed to be able to read or write files in the directory.

refs · 2020-03-11T09:27:13Z

pkg/thumbnails/storage/filesystem.go

+
+	f, err := os.Create(path)
+	if err != nil {
+		fmt.Println(err.Error())


use a proper logger

refs · 2020-03-11T09:30:42Z

pkg/service/v0/service.go

 	}

 	m.Route(options.Config.HTTP.Root, func(r chi.Router) {
-		r.Get("/", svc.Dummy)
+		r.Get("/thumbnails", svc.Thumbnails)
 	})

 	return svc
 }

 // Thumbnails defines implements the business logic for Service.
 type Thumbnails struct {


change annotation to:

Thumbnails implements the business logic for Service.

Also use the singular, as in Thumbnail 💃

refs · 2020-03-11T09:31:56Z

pkg/service/v0/service.go

+	width, _ := strconv.Atoi(query.Get("width"))
+	height, _ := strconv.Atoi(query.Get("height"))


add error handling here, make it more explicit where a failure might happen

refs · 2020-03-11T09:33:13Z

pkg/service/v0/service.go

+	height, _ := strconv.Atoi(query.Get("height"))
+	fileType := query.Get("type")
+	filePath := query.Get("file_path")
+	etag := query.Get("etag")


Generally speaking I'd declare a struct and try to unmarshal the request query string onto it, then operate with typed fields. It is the same concept you're doing here but a bit more explicit. We could give it a try together

refs · 2020-03-11T09:37:17Z

pkg/service/v0/service.go

+	if encoder == nil {
+		// TODO: better error responses
+		w.WriteHeader(http.StatusBadRequest)
+		w.Write([]byte("can't encode that"))


use a logger from ocis-pkg so we have persisted logs and not only the user can see it. I'm also fond of using the passive voice, as in:

can't be encoded

Furthermore the error message is not informative enough, since you're checking whether the fileType is supported or not by fetching an encoder for it, it should read more like:

encoding for filetype: .XYZ is not supported

I wanted to replace that when the proper API is implemented.

refs · 2020-03-11T09:40:04Z

pkg/thumbnails/imgsource/imgsource.go

+}
+
+// NewContext creates a new SourceContext instance
+func NewContext() SourceContext {


have you considered using context.Context here? You can still store key-values on a typed and thread safe manner 🗡

refs · 2020-03-11T09:47:29Z

pkg/thumbnails/thumbnails.go

+}
+
+func mapToStorageContext(ctx Context) storage.Context {
+	sCtx := storage.Context{


same as with the previous context.Context comment. By adopting that idiom you could even chain them like middlewares

refs · 2020-03-11T09:50:11Z

Also don't forget to add a changelog 💃

C0rby · 2020-03-11T15:16:27Z

The only things left now are the things I want to change in another PR when I'll implement the proper API. Or should I do it here?

C0rby · 2020-03-13T13:55:39Z

So now I added the grpc service. @refs could you have another look?

refs

👍 Thanks for adding traces and metrics!

refs · 2020-03-18T10:25:35Z

pkg/service/v0/logging.go

+	} else {
+		logger.Debug().
+			Msg("")
+	}


leftover? or intended to populate this with a message?

To be honest, I copied it from ocis-hello.
I thought it would then log at least the stuff from line 30 and 31

David Christofas and others added 13 commits March 3, 2020 16:40

first prototype of the thumbnail service

a98df0b

Currently uses in memory caching and loads the file from the local filesystem.

implement file sources

34dcaf9

If the file is not cached we have to get it from somewhere. For that I implemented a webdav file source which gets the file from reva.

rename cache to storage

0978653

implement prototype filesystem storage

f55199f

add config for filesystem storage

9753878

clean up and add comments

4647168

rename Context

d8c3593

clean up and add comments

2a587b3

Fix Formatting

e0354f2

Update README.md

61d0e52

Update manifest.tmpl

78b5231

Typo in CHANGELOG.tmpl

e36a7ef

Merge branch 'master' into thumbnail_service

261429e

C0rby requested review from butonic and refs March 10, 2020 14:23

IljaN reviewed Mar 10, 2020

View reviewed changes

refs suggested changes Mar 11, 2020

View reviewed changes

refs changed the title ~~First working version.~~ Thumbnails Service Mar 11, 2020

refs added the enhancement New feature or request label Mar 11, 2020

refs assigned C0rby Mar 11, 2020

David Christofas added 6 commits March 11, 2020 14:18

refactoring: implement review feedback

985c118

add proper logging

1389cd7

add comment

f9d0f7e

add changelog

0164715

use os.TempDir() for portable temporary directory

f5f68e2

use filepath.Join instead of path.Join

78a92cf

C0rby requested review from IljaN and refs March 11, 2020 15:15

implement grpc service and remove http service

35141c8

David Christofas added 2 commits March 16, 2020 16:55

implement metrics, logging and tracing

06151de

add mimetype to service response

024e466

refs approved these changes Mar 18, 2020

View reviewed changes

C0rby merged commit 96e3ec5 into master Mar 18, 2020

delete-merged-branch bot deleted the thumbnail_service branch March 18, 2020 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thumbnails Service #3

Thumbnails Service #3

C0rby commented Mar 10, 2020 •

edited

Loading

update-docs bot commented Mar 10, 2020

IljaN Mar 10, 2020

C0rby Mar 11, 2020

refs left a comment

refs Mar 11, 2020

C0rby Mar 11, 2020

refs Mar 11, 2020

refs Mar 11, 2020

refs Mar 11, 2020

C0rby Mar 11, 2020

refs Mar 11, 2020

refs Mar 11, 2020

refs Mar 11, 2020

refs Mar 11, 2020

refs Mar 11, 2020

C0rby Mar 11, 2020

refs Mar 11, 2020

refs Mar 11, 2020

refs commented Mar 11, 2020

C0rby commented Mar 11, 2020

C0rby commented Mar 13, 2020

refs left a comment

refs Mar 18, 2020

C0rby Mar 18, 2020

		width, _ := strconv.Atoi(query.Get("width"))
		height, _ := strconv.Atoi(query.Get("height"))

Thumbnails Service #3

Thumbnails Service #3

Conversation

C0rby commented Mar 10, 2020 • edited Loading

update-docs bot commented Mar 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refs commented Mar 11, 2020

C0rby commented Mar 11, 2020

C0rby commented Mar 13, 2020

refs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C0rby commented Mar 10, 2020 •

edited

Loading