From fd2f02c986f027ef980409c1b685c6f021d51343 Mon Sep 17 00:00:00 2001 From: sushmith Date: Wed, 18 Jan 2023 13:47:31 +0530 Subject: [PATCH] feat: enhance asset filters in GetAllAssets * Support having `OR` clause in the nested data fields filters for the same key with multiple values --- core/asset/filter.go | 10 ++- internal/server/v1beta1/asset_test.go | 6 +- internal/store/postgres/asset_repository.go | 13 ++-- .../store/postgres/asset_repository_test.go | 72 ++++++++++--------- 4 files changed, 60 insertions(+), 41 deletions(-) diff --git a/core/asset/filter.go b/core/asset/filter.go index 4fe8cafa..b638a3da 100644 --- a/core/asset/filter.go +++ b/core/asset/filter.go @@ -15,7 +15,7 @@ type Filter struct { SortDirection string `validate:"omitempty,oneof=asc desc"` QueryFields []string Query string - Data map[string]string + Data map[string][]string } func (f *Filter) Validate() error { @@ -90,7 +90,13 @@ func (fb *filterBuilder) Build() (Filter, error) { SortBy: fb.sortBy, SortDirection: fb.sortDirection, Query: fb.q, - Data: fb.data, + } + + if len(fb.data) != 0 { + flt.Data = make(map[string][]string) + for k, v := range fb.data { + flt.Data[k] = strings.Split(v, ",") + } } if fb.types != "" { diff --git a/internal/server/v1beta1/asset_test.go b/internal/server/v1beta1/asset_test.go index aab6a679..ef376618 100644 --- a/internal/server/v1beta1/asset_test.go +++ b/internal/server/v1beta1/asset_test.go @@ -87,9 +87,9 @@ func TestGetAllAssets(t *testing.T) { SortDirection: "asc", QueryFields: []string{"name", "urn"}, Query: "internal", - Data: map[string]string{ - "dataset": "booking", - "project": "p-godata-id", + Data: map[string][]string{ + "dataset": {"booking"}, + "project": {"p-godata-id"}, }, } as.EXPECT().GetAllAssets(ctx, cfg, false).Return([]asset.Asset{}, 0, nil) diff --git a/internal/store/postgres/asset_repository.go b/internal/store/postgres/asset_repository.go index a044b954..26808b05 100644 --- a/internal/store/postgres/asset_repository.go +++ b/internal/store/postgres/asset_repository.go @@ -828,8 +828,8 @@ func (r *AssetRepository) BuildFilterQuery(builder sq.SelectBuilder, flt asset.F } if len(flt.Data) > 0 { - for key, val := range flt.Data { - if val == "_nonempty" { + for key, vals := range flt.Data { + if len(vals) == 1 && vals[0] == "_nonempty" { field := r.buildDataField(key, true) whereClause := sq.And{ sq.NotEq{field: nil}, // IS NOT NULL (field exists) @@ -840,8 +840,13 @@ func (r *AssetRepository) BuildFilterQuery(builder sq.SelectBuilder, flt asset.F } builder = builder.Where(whereClause) } else { - finalQuery := r.buildDataField(key, false) - builder = builder.Where(fmt.Sprintf("%s = '%s'", finalQuery, val)) + dataOrClause := sq.Or{} + for _, v := range vals { + finalQuery := r.buildDataField(key, false) + dataOrClause = append(dataOrClause, sq.Eq{finalQuery: v}) + } + + builder = builder.Where(dataOrClause) } } } diff --git a/internal/store/postgres/asset_repository_test.go b/internal/store/postgres/asset_repository_test.go index 6efe4e42..672e754f 100644 --- a/internal/store/postgres/asset_repository_test.go +++ b/internal/store/postgres/asset_repository_test.go @@ -182,20 +182,28 @@ func (r *AssetRepositoryTestSuite) TestBuildFilterQuery() { // inconsistent test. description: "should return sql query with asset's data fields filter", config: asset.Filter{ - Data: map[string]string{ - "entity": "odpf", + Data: map[string][]string{ + "entity": {"odpf"}, }, }, - expectedQuery: `data->>'entity' = 'odpf'`, + expectedQuery: `(data->>'entity' = $1)`, }, { description: "should return sql query with asset's nested data fields filter", config: asset.Filter{ - Data: map[string]string{ - "landscape.properties.project-id": "compass_001", + Data: map[string][]string{ + "landscape.properties.project-id": {"compass_001"}, }, }, - expectedQuery: `data->'landscape'->'properties'->>'project-id' = 'compass_001'`, + expectedQuery: `(data->'landscape'->'properties'->>'project-id' = $1)`, + }, + { + description: "should return sql query with asset's nested multiple data fields filter ", + config: asset.Filter{ + Data: map[string][]string{ + "properties.attributes.entity": {"alpha", "beta"}, + }}, + expectedQuery: `(data->'properties'->'attributes'->>'entity' = $1 OR data->'properties'->'attributes'->>'entity' = $2)`, }, } @@ -328,9 +336,9 @@ func (r *AssetRepositoryTestSuite) TestGetAll() { r.Run("should filter using asset's data fields", func() { results, err := r.repository.GetAll(r.ctx, asset.Filter{ - Data: map[string]string{ - "entity": "odpf", - "country": "th", + Data: map[string][]string{ + "entity": {"odpf"}, + "country": {"th"}, }, }) r.Require().NoError(err) @@ -344,9 +352,9 @@ func (r *AssetRepositoryTestSuite) TestGetAll() { r.Run("should filter using asset's nested data fields", func() { results, err := r.repository.GetAll(r.ctx, asset.Filter{ - Data: map[string]string{ - "landscape.properties.project-id": "compass_001", - "country": "vn", + Data: map[string][]string{ + "landscape.properties.project-id": {"compass_001"}, + "country": {"vn"}, }, }) r.Require().NoError(err) @@ -360,8 +368,8 @@ func (r *AssetRepositoryTestSuite) TestGetAll() { r.Run("should filter using asset's nonempty data fields", func() { results, err := r.repository.GetAll(r.ctx, asset.Filter{ - Data: map[string]string{ - "properties.dependencies": "_nonempty", + Data: map[string][]string{ + "properties.dependencies": {"_nonempty"}, }, }) r.Require().NoError(err) @@ -375,11 +383,11 @@ func (r *AssetRepositoryTestSuite) TestGetAll() { r.Run("should filter using asset's different nonempty data fields", func() { results, err := r.repository.GetAll(r.ctx, asset.Filter{ - Data: map[string]string{ - "properties.dependencies": "_nonempty", - "entity": "odpf", - "urn": "j-xcvcx", - "country": "vn", + Data: map[string][]string{ + "properties.dependencies": {"_nonempty"}, + "entity": {"odpf"}, + "urn": {"j-xcvcx"}, + "country": {"vn"}, }, }) r.Require().NoError(err) @@ -463,9 +471,9 @@ func (r *AssetRepositoryTestSuite) TestGetTypes() { { Description: "should filter using asset's data fields", Filter: asset.Filter{ - Data: map[string]string{ - "entity": "odpf", - "country": "th", + Data: map[string][]string{ + "entity": {"odpf"}, + "country": {"th"}, }, }, Expected: map[asset.Type]int{ @@ -477,9 +485,9 @@ func (r *AssetRepositoryTestSuite) TestGetTypes() { { Description: "should filter using asset's nested data fields", Filter: asset.Filter{ - Data: map[string]string{ - "landscape.properties.project-id": "compass_001", - "country": "vn", + Data: map[string][]string{ + "landscape.properties.project-id": {"compass_001"}, + "country": {"vn"}, }, }, Expected: map[asset.Type]int{ @@ -489,8 +497,8 @@ func (r *AssetRepositoryTestSuite) TestGetTypes() { { Description: "should filter using asset's nonempty data fields", Filter: asset.Filter{ - Data: map[string]string{ - "properties.dependencies": "_nonempty", + Data: map[string][]string{ + "properties.dependencies": {"_nonempty"}, }, }, Expected: map[asset.Type]int{ @@ -500,11 +508,11 @@ func (r *AssetRepositoryTestSuite) TestGetTypes() { { Description: "should filter using asset's different nonempty data fields", Filter: asset.Filter{ - Data: map[string]string{ - "properties.dependencies": "_nonempty", - "entity": "odpf", - "urn": "j-xcvcx", - "country": "vn", + Data: map[string][]string{ + "properties.dependencies": {"_nonempty"}, + "entity": {"odpf"}, + "urn": {"j-xcvcx"}, + "country": {"vn"}, }, }, Expected: map[asset.Type]int{