Fixed the behavior of default params if they are present (#60)

* Fixed the behavior of default params if they are present

* Moved the default params check to manipulator

* Removed unnecessary imports

* Fixed doc comment

Co-authored-by: Vivek Mittal <[email protected]>

Author: vivekmittal

internal/handler/image.go

@@ -40,7 +40,7 @@ func ImageHandler(deps *service.Dependencies) http.HandlerFunc {
 
 		params := make(map[string]string)
 		values := r.URL.Query()
-		if len(values) > 0 {
+		if len(values) > 0 || deps.Manipulator.HasDefaultParams() {
 			for v := range values {
 				if len(values.Get(v)) != 0 {
 					params[v] = values.Get(v)
@@ -58,7 +58,7 @@ func ImageHandler(deps *service.Dependencies) http.HandlerFunc {
 		w.Header().Set(CacheControlHeader, fmt.Sprintf("public,max-age=%d", config.CacheTime()))
 		// Ref to Google CDN we support: https://cloud.google.com/cdn/docs/caching#cacheability
 		w.Header().Set(VaryHeader, "Accept")
-		
+
 		cl, _ := w.Write(data)
 		w.Header().Set(ContentLengthHeader, fmt.Sprintf("%d", cl))
 	}

internal/handler/image_test.go

@@ -37,11 +37,28 @@ func (s *ImageHandlerTestSuite) SetupTest() {
 		MetricService: s.mockMetricService}
 }
 
-func (s *ImageHandlerTestSuite) TestImageHandler() {
+func (s *ImageHandlerTestSuite) TestImageHandlerWithoutDefaultParams() {
 	r, _ := http.NewRequest(http.MethodGet, "/image-valid", nil)
 	rr := httptest.NewRecorder()
+	data := []byte("validData")
 
-	s.storage.On("Get", mock.Anything, "/image-valid").Return([]byte("validData"), http.StatusOK, nil)
+	s.storage.On("Get", mock.Anything, "/image-valid").Return(data, http.StatusOK, nil)
+	s.manipulator.On("HasDefaultParams").Return(false)
+
+	ImageHandler(s.deps).ServeHTTP(rr, r)
+
+	assert.Equal(s.T(), "validData", rr.Body.String())
+	assert.Equal(s.T(), http.StatusOK, rr.Code)
+}
+
+func (s *ImageHandlerTestSuite) TestImageHandlerWithDefaultParams() {
+	r, _ := http.NewRequest(http.MethodGet, "/image-valid", nil)
+	rr := httptest.NewRecorder()
+	data := []byte("validData")
+
+	s.storage.On("Get", mock.Anything, "/image-valid").Return(data, http.StatusOK, nil)
+	s.manipulator.On("HasDefaultParams").Return(true)
+	s.manipulator.On("Process", mock.AnythingOfType("service.processSpec")).Return(data, nil)
 
 	ImageHandler(s.deps).ServeHTTP(rr, r)
 

pkg/service/dependencies.go

@@ -3,6 +3,7 @@ package service
 
 import (
 	"errors"
+
 	"github.com/gojek/darkroom/pkg/logger"
 	"github.com/gojek/darkroom/pkg/metrics"
 	"github.com/gojek/darkroom/pkg/regex"
@@ -40,8 +41,11 @@ func NewDependencies(registry *prometheus.Registry) (*Dependencies, error) {
 		metricService = metrics.NoOpMetricService{}
 		logger.Warn("NoOpMetricService is being used since metric system is not specified")
 	}
-	deps := &Dependencies{Manipulator: NewManipulator(native.NewBildProcessor(), getDefaultParams(), metricService),
-		MetricService: metricService}
+	deps := &Dependencies{
+		Manipulator:   NewManipulator(native.NewBildProcessor(), getDefaultParams(), metricService),
+		MetricService: metricService,
+	}
+
 	s := config.DataSource()
 	if regex.WebFolderMatcher.MatchString(s.Kind) {
 		deps.Storage = NewWebFolderStorage(s.Value.(config.WebFolder), s.HystrixCommand)

pkg/service/manipulator.go

@@ -44,6 +44,9 @@ const (
 type Manipulator interface {
 	// Process takes ProcessSpec as an argument and returns []byte, error
 	Process(spec processSpec) ([]byte, error)
+
+	// HasDefaultParams returns true if defaultParams are present, returns false otherwise
+	HasDefaultParams() bool
 }
 
 type manipulator struct {
@@ -126,6 +129,11 @@ func (m *manipulator) Process(spec processSpec) ([]byte, error) {
 	return src, err
 }
 
+// HasDefaultParams returns true if defaultParams are present, returns false otherwise
+func (m *manipulator) HasDefaultParams() bool {
+	return len(m.defaultParams) > 0
+}
+
 func joinParams(params map[string]string, defaultParams map[string]string) map[string]string {
 	fp := make(map[string]string)
 	for p := range defaultParams {

pkg/service/manipulator_test.go

@@ -2,6 +2,7 @@ package service
 
 import (
 	"errors"
+
 	"github.com/gojek/darkroom/pkg/metrics"
 	"github.com/gojek/darkroom/pkg/processor"
 	"github.com/gojek/darkroom/pkg/processor/native"
@@ -181,6 +182,14 @@ func TestCleanInt(t *testing.T) {
 	assert.Equal(t, 0, CleanInt("-234"))
 }
 
+func TestManipulator_HasDefaultParams(t *testing.T) {
+	manipulatorWithDefaultParams := NewManipulator(nil, map[string]string{"auto": "compress"}, nil)
+	manipulatorWithoutDefaultParams := NewManipulator(nil, map[string]string{}, nil)
+
+	assert.Equal(t, true, manipulatorWithDefaultParams.HasDefaultParams())
+	assert.Equal(t, false, manipulatorWithoutDefaultParams.HasDefaultParams())
+}
+
 type mockProcessor struct {
 	mock.Mock
 }

pkg/service/mock.go

@@ -12,3 +12,8 @@ func (m *MockManipulator) Process(spec processSpec) ([]byte, error) {
 	args := m.Called(spec)
 	return args.Get(0).([]byte), args.Error(1)
 }
+
+func (m *MockManipulator) HasDefaultParams() bool {
+	args := m.Called()
+	return args.Get(0).(bool)
+}