Avoid calling the image transformation multiple times; Fixes #119

README.md

@@ -108,16 +108,15 @@ Skrop provides a set of filters, which you can use within the routes:
 * **height(size, opt-enlarge)** — resizes the image to the specified height keeping the ratio. If the second arg is specified and it is equals to "DO_NOT_ENLARGE", the image will not be enlarged
 * **blur(sigma, min_ampl)** — blurs the image (for info see [here](http://www.vips.ecs.soton.ac.uk/supported/current/doc/html/libvips/libvips-convolution.html#vips-gaussblur))
 * **imageOverlay(filename, opacity, gravity, opt-top-margin, opt-right-margin, opt-bottom-margin, opt-left-margin)** — puts an image onverlay over the required image
-* **transformFromQueryParams()** - transforms the image based on the request query parameters (supports only crop for now) e.g: localhost:9090/images/S/big-ben.jpg?crop=120,300,500,300.
+* **transformByQueryParams()** - transforms the image based on the request query parameters (supports only crop for now) e.g: localhost:9090/images/S/big-ben.jpg?crop=120,300,500,300.
 * **cropByFocalPoint(targetX, targetY, aspectRatio, minWidth)** — crops the image based on a focal point on both the source as well as on the target and desired aspect ratio of the target. TargetX and TargetY are the definition of the target image focal point defined as relative values for both width and height, i.e. if the focal point of the target image should be right in the center it would be 0.5 and 0.5. This filter expects two PathParams named **focalPointX** and **focalPointY** which are absolute X and Y coordinates of the focal point in the source image. The fourth parameter is optional; when given the filter will ensure that the resulting image has at least the specified minimum width if not it will crop the biggest possible part based on the focal point.
 
 ### About filters
 The eskip file defines a list of configuration. Every configuration is composed by a route and a list of filters to
-apply for that specific route. Skrop adds by default two filters (setupResponse() and finalizeResponse()).
-The filter setupResponse() initialize the response by adding in the context the image received from the backend.
+apply for that specific route.
 The finalizeResponse() needs to be added at the end, because it triggers the last transformation of the image.
 
-Because of performance, each filter does not trigger a transformation of the image, but if possible it is merged with
+Because of performance, most of the filters don't not trigger a transformation of the image, but if possible it is merged with
 the result of the previous filters. The image is actually transformed every time the filter cannot be merged with the
 previous one e.g. both edit the same attribute and also at the end of the filter chain by the finalizeResponse filter.
 

dataclient/dataclient.go

@@ -2,7 +2,6 @@ package dataclient
 
 import (
 	log "github.com/sirupsen/logrus"
-	"github.com/zalando-stups/skrop/filters"
 	"github.com/zalando/skipper/eskip"
 	"github.com/zalando/skipper/eskipfile"
 	"github.com/zalando/skipper/routing"
@@ -16,15 +15,8 @@ type skropDataClient struct {
 // NewSkropDataClient creates a new dataclient specific for skrop
 func NewSkropDataClient(eskipFile string) routing.DataClient {
 
-	emptyArgs := make([]interface{}, 0)
-
-	pre := &eskip.Filter{
-		Name: filters.FinalizeResponseName,
-		Args: emptyArgs,
-	}
 	return skropDataClient{
 		fileName: eskipFile,
-		prepend:  pre,
 	}
 }
 
@@ -41,6 +33,10 @@ func (s skropDataClient) LoadAll() ([]*eskip.Route, error) {
 		return nil, err
 	}
 
+	if s.prepend == nil {
+		return routes, nil
+	}
+
 	for _, route := range routes {
 		route.Filters = append([]*eskip.Filter{s.prepend}, route.Filters...)
 	}

dataclient/dataclient_test.go

@@ -10,19 +10,6 @@ func TestNewSkropDataClient(t *testing.T) {
 	assert.NotNil(t, s)
 }
 
-func TestSkropDataClient_LoadAll(t *testing.T) {
-	//given
-	s := NewSkropDataClient("eskip_test.eskip")
-
-	//when
-	routes, _ := s.LoadAll()
-
-	//then
-	for _, route := range routes {
-		assert.Equal(t, "finalizeResponse", route.Filters[0].Name)
-	}
-}
-
 func TestSkropDataClient_LoadAll_FileErr(t *testing.T) {
 	//given
 	s := NewSkropDataClient("nonexistent.eskip")

eskip/sample.eskip

@@ -22,10 +22,9 @@ smallCached: Path("/cached/images/S/:image")
   -> "http://localhost:9090";
 
 dynamic: Path("/images/dynamic/:image")
-     -> modPath("^/images/dynamic", "/images")
-     -> finalizeResponse()
-     -> transformFromQueryParams()
-     -> "http://localhost:9090";
+  -> modPath("^/images/dynamic", "/images")
+  -> transformByQueryParams()
+  -> "http://localhost:9090";
 
 smallAndSharp: Path("/images/Ss/:image")
   -> modPath("^/images/Ss", "/images")

filters/crop.go

@@ -83,5 +83,6 @@ func (f *crop) CreateFilter(args []interface{}) (filters.Filter, error) {
 func (f *crop) Request(ctx filters.FilterContext) {}
 
 func (f *crop) Response(ctx filters.FilterContext) {
+	log.Debugf("Response %s\n", CropName)
 	HandleImageResponse(ctx, f)
 }

filters/finalizeresponse.go

@@ -1,6 +1,9 @@
 package filters
 
-import "github.com/zalando/skipper/filters"
+import (
+	log "github.com/sirupsen/logrus"
+	"github.com/zalando/skipper/filters"
+)
 
 // This filter is the default filter for every configuration of skrop
 type finalizeResponse struct{}
@@ -29,5 +32,6 @@ func (s *finalizeResponse) Request(ctx filters.FilterContext) {}
 
 //finalize the response calling the transformer for the image one last time before returning the image to the client
 func (s *finalizeResponse) Response(ctx filters.FilterContext) {
+	log.Debugf("Response %s", FinalizeResponseName)
 	FinalizeResponse(ctx)
 }

filters/finalizeresponse_test.go

@@ -1,9 +1,9 @@
 package filters
 
 import (
+	"github.com/danpersa/bimg"
 	"github.com/stretchr/testify/assert"
 	"github.com/zalando-stups/skrop/filters/imagefiltertest"
-	"github.com/danpersa/bimg"
 	"io/ioutil"
 	"testing"
 )
@@ -49,6 +49,7 @@ func TestFinalizeResponse_Response(t *testing.T) {
 	bag[skropOptions] = opt
 	bag[skropImage] = imagefiltertest.PortraitImage()
 	bag[skropInit] = true
+	bag[hasMergedFilters] = true
 
 	ctx := createContext(t, "GET", "url", imagefiltertest.PortraitImageFile, bag)
 

filters/imagefilter.go

@@ -4,10 +4,10 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
+	"github.com/danpersa/bimg"
 	log "github.com/sirupsen/logrus"
 	"github.com/zalando-stups/skrop/messages"
 	"github.com/zalando/skipper/filters"
-	"github.com/danpersa/bimg"
 	"io/ioutil"
 	"net/http"
 	"os"
@@ -26,11 +26,12 @@ const (
 	// Center Gravity
 	Center = "center"
 	// Quality used by default if not specified
-	Quality      = 100
-	doNotEnlarge = "DO_NOT_ENLARGE"
-	skropImage   = "skImage"
-	skropOptions = "skOptions"
-	skropInit    = "skInit"
+	Quality          = 100
+	doNotEnlarge     = "DO_NOT_ENLARGE"
+	skropImage       = "skImage"
+	hasMergedFilters = "hasMergedFilters"
+	skropOptions     = "skOptions"
+	skropInit        = "skInit"
 )
 
 var (
@@ -54,7 +55,7 @@ func init() {
 		Center: bimg.GravityCentre}
 
 	val, exists := os.LookupEnv("STRIP_METADATA")
-	if (exists && strings.ToUpper(val) == "TRUE") {
+	if exists && strings.ToUpper(val) == "TRUE" {
 		stripMetadata = true
 	}
 }
@@ -97,6 +98,8 @@ func buildParameters(ctx filters.FilterContext, image *bimg.Image) *ImageFilterC
 // HandleImageResponse should be called by the Response of every filter. It transforms the image
 func HandleImageResponse(ctx filters.FilterContext, f ImageFilter) error {
 
+	log.Debug("Handle Image Response")
+
 	//in case the response had an error from the backend or from a previous filter
 	if ctx.Response().StatusCode > 300 {
 		return fmt.Errorf("processing skipped, as the backend/filter reported %d status code", ctx.Response().StatusCode)
@@ -106,6 +109,7 @@ func HandleImageResponse(ctx filters.FilterContext, f ImageFilter) error {
 	if _, ok := ctx.StateBag()[skropInit]; !ok {
 		initResponse(ctx)
 		ctx.StateBag()[skropInit] = true
+		ctx.StateBag()[hasMergedFilters] = false
 	}
 
 	image, ok := ctx.StateBag()[skropImage].(*bimg.Image)
@@ -116,52 +120,52 @@ func HandleImageResponse(ctx filters.FilterContext, f ImageFilter) error {
 		return errors.New("processing failed, image not exists in the state bag")
 	}
 
-	opt, err := f.CreateOptions(buildParameters(ctx, image))
+	optionsFromRequest, err := f.CreateOptions(buildParameters(ctx, image))
 	if err != nil {
 		log.Error("Failed to create options ", err.Error())
 		ctx.Serve(errorResponse())
 		return err
 	}
 
-	opts, ok := ctx.StateBag()[skropOptions].(*bimg.Options)
+	optionsFromStateBag, ok := ctx.StateBag()[skropOptions].(*bimg.Options)
 	if !ok {
 		log.Error("context state bag does not contains the key ", skropImage)
 		ctx.Serve(errorResponse())
 		return errors.New("processing failed, initialization of options not successful")
 	}
 
-	if f.CanBeMerged(opts, opt) {
-		ctx.StateBag()[skropOptions] = f.Merge(opts, opt)
+	if f.CanBeMerged(optionsFromStateBag, optionsFromRequest) {
+		ctx.StateBag()[skropOptions] = f.Merge(optionsFromStateBag, optionsFromRequest)
+		ctx.StateBag()[hasMergedFilters] = true
 		log.Debug("Filter ", f, " merged in ", ctx.StateBag()[skropOptions])
 		return nil
 	}
 
-	//transform the image
-	buf, err := transformImage(image, opts)
+	log.Debugf("Transform the image based on the options from request: %+v", optionsFromRequest)
+	buf, err := transformImage(image, optionsFromRequest)
 	if err != nil {
 		log.Error("Failed to process image ", err.Error())
 		ctx.Serve(errorResponse())
 		return err
 	}
 
-	// set opt in the stateBag
 	newImage := bimg.NewImage(buf)
-	newOption, err := f.CreateOptions(buildParameters(ctx, newImage))
 	if err != nil {
 		log.Error("Failed to create new options ", err.Error())
 		ctx.Serve(errorResponse())
 		return err
 	}
 
 	ctx.StateBag()[skropImage] = newImage
-	ctx.StateBag()[skropOptions] = newOption
+	ctx.StateBag()[skropOptions] = &bimg.Options{}
 	return nil
 }
 
 // FinalizeResponse is called at the end of the transformations on an image to empty the queue of
 // operations to perform
 func FinalizeResponse(ctx filters.FilterContext) {
-	//in case the response had an error fromt he backend or from a previous filter
+	log.Debug("Finalize Response")
+	//in case the response had an error from he backend or from a previous filter
 	if ctx.Response().StatusCode > 300 {
 		return
 	}
@@ -173,7 +177,15 @@ func FinalizeResponse(ctx filters.FilterContext) {
 	image := ctx.StateBag()[skropImage].(*bimg.Image)
 	opts := ctx.StateBag()[skropOptions].(*bimg.Options)
 
-	buf, err := transformImage(image, opts)
+	buf := image.Image()
+
+	var err error
+
+	if ctx.StateBag()[hasMergedFilters] == true {
+		buf, err = transformImage(image, opts)
+		ctx.StateBag()[hasMergedFilters] = false
+	}
+
 	if err != nil {
 		log.Error("failed to process image ", err.Error())
 		ctx.Serve(&http.Response{
@@ -193,7 +205,7 @@ func FinalizeResponse(ctx filters.FilterContext) {
 func transformImage(image *bimg.Image, opts *bimg.Options) ([]byte, error) {
 	defOpt := applyDefaults(opts)
 
-	log.Debug("successfully applied the following options on the image: ", opts)
+	log.Debugf("successfully applied the following options on the image: %+v\n", opts)
 
 	transformedImageBytes, err := image.Process(*defOpt)
 

filters/imagefilter_test.go

@@ -7,10 +7,10 @@ import (
 	"net/http"
 	"testing"
 
+	"github.com/danpersa/bimg"
 	"github.com/stretchr/testify/assert"
 	"github.com/zalando-stups/skrop/filters/imagefiltertest"
 	"github.com/zalando/skipper/filters/filtertest"
-	"github.com/danpersa/bimg"
 )
 
 const (
@@ -125,6 +125,7 @@ func createDefaultContext(t *testing.T, url string) *filtertest.Context {
 	bag[skropImage] = bimg.NewImage(buffer)
 	bag[skropOptions] = &bimg.Options{}
 	bag[skropInit] = true
+	bag[hasMergedFilters] = true
 	return createContext(t, "GET", url, imagefiltertest.PNGImageFile, bag)
 }
 

filters/transformFromQueryParams.go → filters/transformbyqueryparams.go

@@ -1,16 +1,16 @@
 package filters
 
 import (
-	"github.com/zalando/skipper/filters"
 	"github.com/danpersa/bimg"
+	log "github.com/sirupsen/logrus"
+	"github.com/zalando/skipper/filters"
 	"strconv"
 	"strings"
 )
 
 const (
-	ExtractArea    = "transformFromQueryParams"
-	cropParameters = "crop"
-	focalPointCropParameters = "focal_point_crop"
+	TransformByQueryParamsName = "transformByQueryParams"
+	cropParameters             = "crop"
 )
 
 type transformFromQueryParams struct{}
@@ -20,7 +20,7 @@ func NewTransformFromQueryParams() filters.Spec {
 }
 
 func (t *transformFromQueryParams) Name() string {
-	return ExtractArea
+	return TransformByQueryParamsName
 }
 
 func (t *transformFromQueryParams) CreateFilter(config []interface{}) (filters.Filter, error) {
@@ -30,20 +30,8 @@ func (t *transformFromQueryParams) CreateFilter(config []interface{}) (filters.F
 func (t *transformFromQueryParams) CreateOptions(ctx *ImageFilterContext) (*bimg.Options, error) {
 	// Get crop prams from the request
 	params, ok := ctx.Parameters[cropParameters]
-	if !ok {
-
-		params, ok = ctx.Parameters[focalPointCropParameters]
-
-		if !ok {
-			return &bimg.Options{}, nil
-		}
-
-		params = strings.Split(params[0], ",")
-		if len(params) != 5 {
-			return &bimg.Options{}, nil
-		}
 
-		// TODO do focal point crop
+	if !ok {
 		return &bimg.Options{}, nil
 	}
 
@@ -89,6 +77,8 @@ func (t *transformFromQueryParams) CreateOptions(ctx *ImageFilterContext) (*bimg
 		width = imgSize.Width - x
 	}
 
+	log.Debugf("Crop options x=%d y=%d width=%d height=%d\n", x, y, width, height)
+
 	return &bimg.Options{
 		Top:        y,
 		Left:       x,
@@ -97,29 +87,19 @@ func (t *transformFromQueryParams) CreateOptions(ctx *ImageFilterContext) (*bimg
 	}, nil
 }
 
-func min(a, b int) int {
-	if a < b {
-		return a
-	}
-	return b
-}
-
 func (t *transformFromQueryParams) CanBeMerged(other *bimg.Options, self *bimg.Options) bool {
 	return false
 }
 
 func (t *transformFromQueryParams) Merge(other *bimg.Options, self *bimg.Options) *bimg.Options {
-	other.AreaWidth = self.AreaWidth
-	other.AreaHeight = self.AreaHeight
-	other.Top = self.Top
-	other.Left = self.Left
-	return other
+	return nil
 }
 
 func (t *transformFromQueryParams) Request(ctx filters.FilterContext) {
-
 }
 
 func (e *transformFromQueryParams) Response(ctx filters.FilterContext) {
+	log.Debugf("Response %s\n", TransformByQueryParamsName)
 	HandleImageResponse(ctx, e)
+	FinalizeResponse(ctx)
 }