Cleanup attachment serving logic

Remove old workarounds in rest/handlers and replace them with
simple func(http.ResponseWriter, *http.Request) that we return
from controller functions

messaging/rest/attachment.go

@@ -3,15 +3,15 @@ package rest
 import (
 	"context"
 	"io"
-	"time"
+	"net/http"
+	"net/url"
 
 	"github.com/pkg/errors"
 
 	"github.com/crusttech/crust/internal/auth"
+	"github.com/crusttech/crust/messaging/internal/repository"
 	"github.com/crusttech/crust/messaging/internal/service"
-	"github.com/crusttech/crust/messaging/rest/handlers"
 	"github.com/crusttech/crust/messaging/rest/request"
-	"github.com/crusttech/crust/messaging/types"
 )
 
 var _ = errors.Wrap
@@ -20,12 +20,6 @@ type (
 	Attachment struct {
 		att service.AttachmentService
 	}
-
-	file struct {
-		*types.Attachment
-		content  io.ReadSeeker
-		download bool
-	}
 )
 
 func (Attachment) New() *Attachment {
@@ -34,12 +28,12 @@ func (Attachment) New() *Attachment {
 	return ctrl
 }
 
-func (ctrl *Attachment) Original(ctx context.Context, r *request.AttachmentOriginal) (interface{}, error) {
+func (ctrl Attachment) Original(ctx context.Context, r *request.AttachmentOriginal) (interface{}, error) {
 	if err := ctrl.isAccessible(r.AttachmentID, r.UserID, r.Sign); err != nil {
 		return nil, err
 	}
 
-	return ctrl.get(ctx, r.AttachmentID, false, r.Download)
+	return ctrl.serve(ctx, r.AttachmentID, false, r.Download)
 }
 
 func (ctrl *Attachment) Preview(ctx context.Context, r *request.AttachmentPreview) (interface{}, error) {
@@ -47,7 +41,7 @@ func (ctrl *Attachment) Preview(ctx context.Context, r *request.AttachmentPrevie
 		return nil, err
 	}
 
-	return ctrl.get(ctx, r.AttachmentID, true, false)
+	return ctrl.serve(ctx, r.AttachmentID, true, false)
 }
 
 func (ctrl Attachment) isAccessible(attachmentID, userID uint64, signature string) error {
@@ -66,43 +60,42 @@ func (ctrl Attachment) isAccessible(attachmentID, userID uint64, signature strin
 	return nil
 }
 
-func (ctrl Attachment) get(ctx context.Context, ID uint64, preview, download bool) (handlers.Downloadable, error) {
-	rval := &file{download: download}
+func (ctrl Attachment) serve(ctx context.Context, ID uint64, preview, download bool) (interface{}, error) {
+	return func(w http.ResponseWriter, req *http.Request) {
+		att, err := ctrl.att.With(ctx).FindByID(ID)
+
+		if err != nil {
+			switch {
+			case err == repository.ErrAttachmentNotFound:
+				w.WriteHeader(http.StatusNotFound)
+			default:
+				http.Error(w, err.Error(), http.StatusInternalServerError)
+			}
+
+			return
+		}
+
+		var fh io.ReadSeeker
 
-	if att, err := ctrl.att.With(ctx).FindByID(ID); err != nil {
-		return nil, err
-	} else {
-		rval.Attachment = att
 		if preview {
-			rval.content, err = ctrl.att.OpenPreview(att)
+			fh, err = ctrl.att.OpenPreview(att)
 		} else {
-			rval.content, err = ctrl.att.OpenOriginal(att)
+			fh, err = ctrl.att.OpenOriginal(att)
 		}
 
 		if err != nil {
-			return nil, err
+			http.Error(w, err.Error(), http.StatusInternalServerError)
+			return
 		}
-	}
 
-	return rval, nil
-}
+		name := url.QueryEscape(att.Name)
 
-func (f *file) Download() bool {
-	return f.download
-}
+		if download {
+			w.Header().Add("Content-Disposition", "attachment; filename="+name)
+		} else {
+			w.Header().Add("Content-Disposition", "inline; filename="+name)
+		}
 
-func (f *file) Name() string {
-	return f.Attachment.Name
-}
-
-func (f *file) ModTime() time.Time {
-	return f.Attachment.CreatedAt
-}
-
-func (f *file) Content() io.ReadSeeker {
-	return f.content
-}
-
-func (f *file) Valid() bool {
-	return f.content != nil
+		http.ServeContent(w, req, name, att.CreatedAt, fh)
+	}, nil
 }

messaging/rest/handlers/attachment_custom.go

@@ -1,72 +0,0 @@
-package handlers
-
-import (
-	"io"
-	"net/http"
-	"net/url"
-	"time"
-
-	"github.com/crusttech/crust/messaging/rest/request"
-)
-
-// HTTP API interface
-type AttachmentDownloadable struct {
-	Original func(http.ResponseWriter, *http.Request)
-	Preview  func(http.ResponseWriter, *http.Request)
-}
-
-type Downloadable interface {
-	Name() string
-	Download() bool
-	ModTime() time.Time
-	Content() io.ReadSeeker
-	Valid() bool
-}
-
-func NewAttachmentDownloadable(ah AttachmentAPI) *Attachment {
-	serve := func(f interface{}, err error, w http.ResponseWriter, r *http.Request) {
-		if err != nil {
-			switch {
-			// @todo: compare concrete exported error type? Go2 .As() like check?
-			case err.Error() == "crust.messaging.repository.AttachmentNotFound":
-				w.WriteHeader(http.StatusNotFound)
-			default:
-				http.Error(w, err.Error(), http.StatusInternalServerError)
-			}
-		} else if dl, ok := f.(Downloadable); ok {
-			if !dl.Valid() {
-				w.WriteHeader(http.StatusNotFound)
-			} else {
-				if dl.Download() {
-					w.Header().Add("Content-Disposition", "attachment; filename="+url.QueryEscape(dl.Name()))
-				} else {
-					w.Header().Add("Content-Disposition", "inline; filename="+url.QueryEscape(dl.Name()))
-				}
-
-				http.ServeContent(w, r, dl.Name(), dl.ModTime(), dl.Content())
-			}
-		} else {
-			http.Error(w, "Got incompatible type from controller", http.StatusInternalServerError)
-		}
-	}
-
-	return &Attachment{
-		Original: func(w http.ResponseWriter, r *http.Request) {
-			defer r.Body.Close()
-			params := request.NewAttachmentOriginal()
-			_ = params.Fill(r)
-
-			f, err := ah.Original(r.Context(), params)
-			serve(f, err, w, r)
-		},
-
-		Preview: func(w http.ResponseWriter, r *http.Request) {
-			defer r.Body.Close()
-			params := request.NewAttachmentPreview()
-			_ = params.Fill(r)
-
-			f, err := ah.Preview(r.Context(), params)
-			serve(f, err, w, r)
-		},
-	}
-}

messaging/rest/router.go

@@ -11,7 +11,7 @@ func MountRoutes() func(chi.Router) {
 	// Initialize handlers & controllers.
 	return func(r chi.Router) {
 		r.Group(func(r chi.Router) {
-			handlers.NewAttachmentDownloadable(Attachment{}.New()).MountRoutes(r)
+			handlers.NewAttachment(Attachment{}.New()).MountRoutes(r)
 		})
 
 		r.Group(func(r chi.Router) {