feat!: change record filter behaviour (#23)

* feat(search)!: change filter behaviour

* feat!: change filter behaviour when fetching records

Author: StewartJingga

api/handlers/search_handler.go

@@ -135,20 +135,6 @@ func newCachingTypeRepo(repo models.TypeRepository) *cachingTypeRepo {
 	}
 }
 
-func filterConfigFromValues(values url.Values) map[string][]string {
-	var filter = make(map[string][]string)
-	for key, fields := range values {
-		// filters are of form "filter.{field}", apart from "filter.type", which is used
-		// for building the type whitelist.
-		if !strings.HasPrefix(key, filterPrefix) || strings.EqualFold(key, whiteListQueryParamKey) {
-			continue
-		}
-		filterKey := strings.TrimPrefix(key, filterPrefix)
-		filter[filterKey] = fields
-	}
-	return filter
-}
-
 func parseTypeWhiteList(values url.Values) (types []string) {
 	for _, typ := range values[whiteListQueryParamKey] {
 		typList := strings.Split(typ, ",")

api/handlers/search_handler_test.go

@@ -9,24 +9,24 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"reflect"
-	"strconv"
 	"testing"
 
 	"github.com/odpf/columbus/api/handlers"
 	"github.com/odpf/columbus/lib/mock"
 	"github.com/odpf/columbus/models"
 
+	"github.com/stretchr/testify/assert"
 	testifyMock "github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 )
 
 func TestSearchHandler(t *testing.T) {
 	ctx := context.Background()
 	// todo: pass testCase to ValidateResponse
 	type testCase struct {
 		Title            string
-		SearchText       string
 		ExpectStatus     int
-		RequestParams    map[string][]string
+		Querystring      string
 		InitRepo         func(testCase, *mock.TypeRepository)
 		InitSearcher     func(testCase, *mock.RecordSearcher)
 		ValidateResponse func(testCase, io.Reader) error
@@ -56,11 +56,11 @@ func TestSearchHandler(t *testing.T) {
 			Title:            "should return HTTP 400 if 'text' parameter is empty or missing",
 			ExpectStatus:     http.StatusBadRequest,
 			ValidateResponse: func(tc testCase, body io.Reader) error { return nil },
-			SearchText:       "",
+			Querystring:      "",
 		},
 		{
-			Title:      "should report HTTP 500 if record searcher fails",
-			SearchText: "test",
+			Title:       "should report HTTP 500 if record searcher fails",
+			Querystring: "text=test",
 			InitSearcher: func(tc testCase, searcher *mock.RecordSearcher) {
 				err := fmt.Errorf("service unavailable")
 				searcher.On("Search", ctx, testifyMock.AnythingOfType("models.SearchConfig")).
@@ -69,8 +69,8 @@ func TestSearchHandler(t *testing.T) {
 			ExpectStatus: http.StatusInternalServerError,
 		},
 		{
-			Title:      "should return an error if looking up an type detail fails",
-			SearchText: "test",
+			Title:       "should return an error if looking up an type detail fails",
+			Querystring: "text=test",
 			InitSearcher: func(tc testCase, searcher *mock.RecordSearcher) {
 				results := []models.SearchResult{
 					{
@@ -88,55 +88,31 @@ func TestSearchHandler(t *testing.T) {
 			ExpectStatus: http.StatusInternalServerError,
 		},
 		{
-			Title:      "should pass filter to search config format",
-			SearchText: "resource",
-			RequestParams: map[string][]string{
-				// "landscape" is not a valid filter key. All filters
-				// begin with the "filter." prefix. Adding this here is just a little
-				// extra check to make sure that the handler correctly parses the filters.
-				"landscape":        {"id", "vn"},
-				"filter.landscape": {"th"},
-				"filter.type":      {"dagger"},
-			},
-			InitRepo: withTypes(testdata.Type),
+			Title:       "should pass filter to search config format",
+			Querystring: "text=resource&landscape=id,vn&filter.data.landscape=th&filter.type=topic&filter.service=kafka,rabbitmq",
 			InitSearcher: func(tc testCase, searcher *mock.RecordSearcher) {
 				cfg := models.SearchConfig{
-					Text:          tc.SearchText,
-					TypeWhiteList: tc.RequestParams["filter.type"],
+					Text:          "resource",
+					TypeWhiteList: []string{"topic"},
 					Filters: map[string][]string{
-						"landscape": tc.RequestParams["filter.landscape"],
+						"service":        {"kafka", "rabbitmq"},
+						"data.landscape": {"th"},
 					},
 				}
 
-				results := []models.SearchResult{
-					{
-						TypeName: testdata.Type.Name,
-						Record: models.Record{
-							Urn:     "test-1",
-							Name:    "test 1",
-							Service: "test-service",
-							Data: map[string]interface{}{
-								"id":        "test-1",
-								"title":     "test 1",
-								"landscape": "th",
-								"entity":    "odpf",
-							},
-						},
-					},
-				}
-				searcher.On("Search", ctx, cfg).Return(results, nil)
+				searcher.On("Search", ctx, cfg).Return([]models.SearchResult{}, nil)
 				return
 			},
 			ValidateResponse: func(tc testCase, body io.Reader) error {
 				return nil
 			},
 		},
 		{
-			Title:      "should return the matched documents",
-			SearchText: "test",
+			Title:       "should return the matched documents",
+			Querystring: "text=test",
 			InitSearcher: func(tc testCase, searcher *mock.RecordSearcher) {
 				cfg := models.SearchConfig{
-					Text:    tc.SearchText,
+					Text:    "test",
 					Filters: make(map[string][]string),
 				}
 				response := []models.SearchResult{
@@ -190,17 +166,13 @@ func TestSearchHandler(t *testing.T) {
 			},
 		},
 		{
-			Title: "should return the requested number of records",
-			RequestParams: map[string][]string{
-				"size": {"15"},
-			},
-			SearchText: "resource",
-			InitRepo:   withTypes(testdata.Type),
+			Title:       "should return the requested number of records",
+			Querystring: "text=resource&size=10",
+			InitRepo:    withTypes(testdata.Type),
 			InitSearcher: func(tc testCase, searcher *mock.RecordSearcher) {
-				maxResults, _ := strconv.Atoi(tc.RequestParams["size"][0])
 				cfg := models.SearchConfig{
-					Text:       tc.SearchText,
-					MaxResults: maxResults,
+					Text:       "resource",
+					MaxResults: 10,
 					Filters:    make(map[string][]string),
 				}
 
@@ -239,10 +211,11 @@ func TestSearchHandler(t *testing.T) {
 				if err != nil {
 					return fmt.Errorf("error reading response body: %v", err)
 				}
-				expectedResults, _ := strconv.Atoi(tc.RequestParams["size"][0])
-				actualResults := len(payload)
-				if expectedResults != actualResults {
-					return fmt.Errorf("expected search request to return %d results, returned %d results instead", expectedResults, actualResults)
+
+				expectedSize := 10
+				actualSize := len(payload)
+				if expectedSize != actualSize {
+					return fmt.Errorf("expected search request to return %d results, returned %d results instead", expectedSize, actualSize)
 				}
 				return nil
 			},
@@ -263,15 +236,9 @@ func TestSearchHandler(t *testing.T) {
 			defer recordSearcher.AssertExpectations(t)
 			defer typeRepo.AssertExpectations(t)
 
-			params := url.Values{}
-			params.Add("text", testCase.SearchText)
-			if testCase.RequestParams != nil {
-				for key, values := range testCase.RequestParams {
-					for _, value := range values {
-						params.Add(key, value)
-					}
-				}
-			}
+			params, err := url.ParseQuery(testCase.Querystring)
+			require.NoError(t, err)
+
 			requestURL := "/?" + params.Encode()
 			rr := httptest.NewRequest(http.MethodGet, requestURL, nil)
 			rw := httptest.NewRecorder()
@@ -283,10 +250,7 @@ func TestSearchHandler(t *testing.T) {
 			if expectStatus == 0 {
 				expectStatus = http.StatusOK
 			}
-			if rw.Code != expectStatus {
-				t.Errorf("expected handler to return http status %d, was %d instead", expectStatus, rw.Code)
-				return
-			}
+			assert.Equal(t, expectStatus, rw.Code)
 			if testCase.ValidateResponse != nil {
 				if err := testCase.ValidateResponse(testCase, rw.Body); err != nil {
 					t.Errorf("error validating handler response: %v", err)

api/handlers/type_handler_test.go

@@ -780,36 +780,39 @@ func TestTypeHandler(t *testing.T) {
 			{
 				Description:  "should return an http 200 irrespective of environment value",
 				TypeName:     "dagger",
-				QueryStrings: "filter.environment=nonexisting",
+				QueryStrings: "filter.data.environment=nonexisting",
 				ExpectStatus: http.StatusOK,
 				Setup: func(tc *testCase, er *mock.TypeRepository, rrf *mock.RecordRepositoryFactory) {
 					er.On("GetByName", ctx, daggerType.Name).Return(daggerType, nil)
 					rr := new(mock.RecordRepository)
-					rr.On("GetAll", ctx, map[string][]string{"environment": {"nonexisting"}}).Return(daggerRecords, nil)
+					rr.On("GetAll", ctx, map[string][]string{"data.environment": {"nonexisting"}}).Return(daggerRecords, nil)
 					rrf.On("For", daggerType).Return(rr, nil)
 				},
 			},
 			{
-				Description:  "should return an http 200 even if the environment is not provided",
+				Description:  "should create filter from querystring",
 				TypeName:     "dagger",
-				QueryStrings: "",
+				QueryStrings: "filter.service=kafka,rabbitmq&filter.data.company=appel",
 				ExpectStatus: http.StatusOK,
 				Setup: func(tc *testCase, er *mock.TypeRepository, rrf *mock.RecordRepositoryFactory) {
 					er.On("GetByName", ctx, daggerType.Name).Return(daggerType, nil)
 					rr := new(mock.RecordRepository)
-					rr.On("GetAll", ctx, map[string][]string{}).Return(daggerRecords, nil)
+					rr.On("GetAll", ctx, map[string][]string{
+						"service":      {"kafka", "rabbitmq"},
+						"data.company": {"appel"},
+					}).Return(daggerRecords, nil)
 					rrf.On("For", daggerType).Return(rr, nil)
 				},
 			},
 			{
 				Description:  "should return all records for an type",
 				TypeName:     "dagger",
-				QueryStrings: "filter.environment=test",
+				QueryStrings: "filter.data.environment=test",
 				ExpectStatus: http.StatusOK,
 				Setup: func(tc *testCase, er *mock.TypeRepository, rrf *mock.RecordRepositoryFactory) {
 					er.On("GetByName", ctx, daggerType.Name).Return(daggerType, nil)
 					rr := new(mock.RecordRepository)
-					rr.On("GetAll", ctx, map[string][]string{"environment": {"test"}}).Return(daggerRecords, nil)
+					rr.On("GetAll", ctx, map[string][]string{"data.environment": {"test"}}).Return(daggerRecords, nil)
 					rrf.On("For", daggerType).Return(rr, nil)
 				},
 				PostCheck: func(tc *testCase, resp *http.Response) error {
@@ -828,12 +831,12 @@ func TestTypeHandler(t *testing.T) {
 			{
 				Description:  "should return the subset of fields specified via select parameter",
 				TypeName:     "dagger",
-				QueryStrings: "filter.environment=test&select=" + url.QueryEscape("urn,owner"),
+				QueryStrings: "filter.data.environment=test&select=" + url.QueryEscape("urn,owner"),
 				ExpectStatus: http.StatusOK,
 				Setup: func(tc *testCase, er *mock.TypeRepository, rrf *mock.RecordRepositoryFactory) {
 					er.On("GetByName", ctx, daggerType.Name).Return(daggerType, nil)
 					rr := new(mock.RecordRepository)
-					rr.On("GetAll", ctx, map[string][]string{"environment": {"test"}}).Return(daggerRecords, nil)
+					rr.On("GetAll", ctx, map[string][]string{"data.environment": {"test"}}).Return(daggerRecords, nil)
 					rrf.On("For", daggerType).Return(rr, nil)
 				},
 				PostCheck: func(tc *testCase, resp *http.Response) error {
@@ -867,38 +870,10 @@ func TestTypeHandler(t *testing.T) {
 					return nil
 				},
 			},
-			{
-				Description:  "should support landscape and entity filters",
-				TypeName:     "dagger",
-				QueryStrings: "filter.environment=test&filter.landscape=id&filter.entity=odpf",
-				ExpectStatus: http.StatusOK,
-				Setup: func(tc *testCase, er *mock.TypeRepository, rrf *mock.RecordRepositoryFactory) {
-					er.On("GetByName", ctx, daggerType.Name).Return(daggerType, nil)
-					rr := new(mock.RecordRepository)
-					filters := map[string][]string{
-						"landscape":   {"id"},
-						"entity":      {"odpf"},
-						"environment": {"test"},
-					}
-					rr.On("GetAll", ctx, filters).Return(daggerRecords, nil)
-					rrf.On("For", daggerType).Return(rr, nil)
-				},
-				PostCheck: func(tc *testCase, resp *http.Response) error {
-					var response []models.Record
-					err := json.NewDecoder(resp.Body).Decode(&response)
-					if err != nil {
-						return fmt.Errorf("error parsing response payload: %v", err)
-					}
-					if reflect.DeepEqual(response, daggerRecords) == false {
-						return fmt.Errorf("expected handler to return %v, returned %v instead", daggerRecords, response)
-					}
-					return nil
-				},
-			},
 			{
 				Description:  "(internal) should return http 500 if the handler fails to construct record repository",
 				TypeName:     "dagger",
-				QueryStrings: "filter.environment=test",
+				QueryStrings: "filter.data.environment=test",
 				ExpectStatus: http.StatusInternalServerError,
 				Setup: func(tc *testCase, er *mock.TypeRepository, rrf *mock.RecordRepositoryFactory) {
 					er.On("GetByName", ctx, daggerType.Name).Return(daggerType, nil)
@@ -910,13 +885,13 @@ func TestTypeHandler(t *testing.T) {
 			{
 				Description:  "(internal) should return an http 500 if calling recordRepository.GetAll fails",
 				TypeName:     "dagger",
-				QueryStrings: "filter.environment=test",
+				QueryStrings: "filter.data.environment=test",
 				ExpectStatus: http.StatusInternalServerError,
 				Setup: func(tc *testCase, er *mock.TypeRepository, rrf *mock.RecordRepositoryFactory) {
 					er.On("GetByName", ctx, daggerType.Name).Return(daggerType, nil)
 					rr := new(mock.RecordRepository)
 					err := fmt.Errorf("temporarily unavailable")
-					rr.On("GetAll", ctx, map[string][]string{"environment": {"test"}}).Return([]models.Record{}, err)
+					rr.On("GetAll", ctx, map[string][]string{"data.environment": {"test"}}).Return([]models.Record{}, err)
 					rrf.On("For", daggerType).Return(rr, nil)
 				},
 			},

api/handlers/utils.go

@@ -0,0 +1,28 @@
+package handlers
+
+import (
+	"net/url"
+	"strings"
+)
+
+func filterConfigFromValues(querystring url.Values) map[string][]string {
+	var filter = make(map[string][]string)
+	for key, values := range querystring {
+		// filters are of form "filter.{field}", apart from "filter.type", which is used
+		// for building the type whitelist.
+		if !strings.HasPrefix(key, filterPrefix) || strings.EqualFold(key, whiteListQueryParamKey) {
+			continue
+		}
+
+		var filterValues []string
+		for _, value := range values {
+			for _, v := range strings.Split(value, ",") {
+				filterValues = append(filterValues, v)
+			}
+		}
+
+		filterKey := strings.TrimPrefix(key, filterPrefix)
+		filter[filterKey] = filterValues
+	}
+	return filter
+}

store/record_repository.go

@@ -217,7 +217,7 @@ func (repo *RecordRepository) termsQuery(filters models.RecordFilter) (io.Reader
 			values = append(values, val)
 		}
 
-		key := fmt.Sprintf("data.%s.keyword", key)
+		key := fmt.Sprintf("%s.keyword", key)
 		termQueries = append(termQueries, elastic.NewTermsQuery(key, values...))
 	}
 	boolQuery := elastic.NewBoolQuery().Must(termQueries...)