From 876e254f5fb48c33cde5be260c6e990ed266d0e4 Mon Sep 17 00:00:00 2001 From: Denis Arh Date: Mon, 16 May 2022 15:26:07 +0200 Subject: [PATCH] Refactor filters, add filtrable flag on data.Attribute --- compose/crs/driver.go | 2 +- compose/crs/test/driver.go | 4 +- compose/types/record.go | 37 ++++++ pkg/data/model.go | 6 +- pkg/filter/filter.go | 25 ++++ pkg/filter/sorting.go | 4 - store/adapters/rdbms/crs/connection.go | 6 +- store/adapters/rdbms/crs/fetcher.go | 23 ++-- store/adapters/rdbms/crs/model.go | 123 ++++++++++------- store/adapters/rdbms/crs/model_test.go | 176 ------------------------- store/adapters/rdbms/crs/record.go | 20 --- 11 files changed, 158 insertions(+), 268 deletions(-) create mode 100644 pkg/filter/filter.go delete mode 100644 store/adapters/rdbms/crs/model_test.go diff --git a/compose/crs/driver.go b/compose/crs/driver.go index ae66c8569..f8df4efb8 100644 --- a/compose/crs/driver.go +++ b/compose/crs/driver.go @@ -49,7 +49,7 @@ type ( LookupRecord(context.Context, *data.Model, ValueGetter, ValueSetter) error - SearchRecords(context.Context, *data.Model, any) (Iterator, error) + SearchRecords(context.Context, *data.Model, filter.Filter) (Iterator, error) // --- diff --git a/compose/crs/test/driver.go b/compose/crs/test/driver.go index f43d0afcc..e88d2b86d 100644 --- a/compose/crs/test/driver.go +++ b/compose/crs/test/driver.go @@ -220,7 +220,7 @@ func RecordSearch(t *testing.T, d crs.StoreConnection) { req = require.New(t) ) - i, err := d.SearchRecords(ctx, m, c.f) + i, err := d.SearchRecords(ctx, m, c.f.ToFilter()) req.NoError(err) rr, err := drain(ctx, i) @@ -240,7 +240,7 @@ func RecordSearch(t *testing.T, d crs.StoreConnection) { f.PageCursor = cur f.Limit = lim req.NoError(f.Sort.Set(orderBy)) - i, err := d.SearchRecords(ctx, m, f) + i, err := d.SearchRecords(ctx, m, f.ToFilter()) req.NoError(err) req.NoError(i.Err()) diff --git a/compose/types/record.go b/compose/types/record.go index 172c3bc26..6e7501067 100644 --- a/compose/types/record.go +++ b/compose/types/record.go @@ -73,6 +73,11 @@ type ( filter.Sorting filter.Paging } + + // wrapping struct for recordFilter that + recordFilter struct { + RecordFilter + } ) const ( @@ -81,6 +86,38 @@ const ( OperationTypeDelete OperationType = "delete" ) +// ToFilter wraps RecordFilter with struct that +// imlements filter.Filter interface +func (f RecordFilter) ToFilter() filter.Filter { + return &recordFilter{f} +} + +func (f recordFilter) Constraints() map[string][]any { + c := make(map[string][]any) + + for _, id := range f.LabeledIDs { + c["id"] = append(c["id"], id) + } + + if f.ModuleID > 0 { + c["moduleId"] = []any{f.ModuleID} + } + + if f.ModuleID > 0 { + c["namespaceId"] = []any{f.ModuleID} + } + + return c +} + +func (f recordFilter) Expression() string { return f.Query } +func (f recordFilter) OrderBy() filter.SortExprSet { return f.Sort } +func (f recordFilter) Limit() uint { return f.Paging.Limit } +func (f recordFilter) Cursor() *filter.PagingCursor { return f.Paging.PageCursor } +func (f recordFilter) StateConstraints() map[string]filter.State { + return map[string]filter.State{"deletedAt": f.Deleted} +} + // UserIDs returns a slice of user IDs from all items in the set func (set RecordSet) UserIDs() (IDs []uint64) { IDs = make([]uint64, 0) diff --git a/pkg/data/model.go b/pkg/data/model.go index 1521b19ed..9a95aa154 100644 --- a/pkg/data/model.go +++ b/pkg/data/model.go @@ -33,13 +33,17 @@ type ( // when filtering out deleted items SoftDeleteFlag bool - // Is column sortable? + // Is attribute sortable? // Note: all primary keys are sortable Sortable bool + // Can attribute be used in query expression? + Filterable bool + // Store describes the strategy the underlying storage system should // apply to the underlying value Store StoreCodec + // Type describes what the value represents and how it should be // encoded/decoded Type Type diff --git a/pkg/filter/filter.go b/pkg/filter/filter.go new file mode 100644 index 000000000..9d53bf9b6 --- /dev/null +++ b/pkg/filter/filter.go @@ -0,0 +1,25 @@ +package filter + +type ( + Filter interface { + // Constraints returns map of attribute idents and values + // used for structured filtering ({a1: [v1], a2: [v2, v3]} => "a1 = v1 AND a2 = (v2,v4)") + Constraints() map[string][]any + + // StateConstraints returns map of attribute idents and states + // used for structured filtering ({a1: s1, a2: s2} => "a1 = s1 AND a2 = s2") + StateConstraints() map[string]State + + // Expression returns string, parseable by ql package + Expression() string + + // OrderBy one or more fields + OrderBy() SortExprSet + + // Limit amount of returned results + Limit() uint + + // Cursor from the last fetch + Cursor() *PagingCursor + } +) diff --git a/pkg/filter/sorting.go b/pkg/filter/sorting.go index aaf9d809f..6b0a948ac 100644 --- a/pkg/filter/sorting.go +++ b/pkg/filter/sorting.go @@ -40,10 +40,6 @@ func (s *Sorting) OrderBy() SortExprSet { return s.Sort } -func (s *Sorting) AppendOrderBy(col string, desc bool) { - s.Sort = append(s.Sort, &SortExpr{Column: col, Descending: desc}) -} - // parses sort string // // We allow a simplified version of what SQL supports, so: diff --git a/store/adapters/rdbms/crs/connection.go b/store/adapters/rdbms/crs/connection.go index 4086f59c8..15c765340 100644 --- a/store/adapters/rdbms/crs/connection.go +++ b/store/adapters/rdbms/crs/connection.go @@ -6,8 +6,8 @@ import ( "github.com/cortezaproject/corteza-server/compose/crs" "github.com/cortezaproject/corteza-server/compose/crs/capabilities" - "github.com/cortezaproject/corteza-server/compose/types" "github.com/cortezaproject/corteza-server/pkg/data" + "github.com/cortezaproject/corteza-server/pkg/filter" "github.com/cortezaproject/corteza-server/store/adapters/rdbms/drivers" "github.com/jmoiron/sqlx" ) @@ -67,8 +67,8 @@ func (c *connection) LookupRecord(ctx context.Context, m *data.Model, pkv crs.Va return c.model(m).Lookup(ctx, pkv, r) } -func (c *connection) SearchRecords(ctx context.Context, m *data.Model, filter any) (crs.Iterator, error) { - return c.model(m).Search(filter.(types.RecordFilter)) +func (c *connection) SearchRecords(ctx context.Context, m *data.Model, f filter.Filter) (crs.Iterator, error) { + return c.model(m).Search(f) } func (c *connection) Models(ctx context.Context) (data.ModelSet, error) { diff --git a/store/adapters/rdbms/crs/fetcher.go b/store/adapters/rdbms/crs/fetcher.go index e62e0213b..916a2a376 100644 --- a/store/adapters/rdbms/crs/fetcher.go +++ b/store/adapters/rdbms/crs/fetcher.go @@ -20,8 +20,9 @@ type ( err error query *goqu.SelectDataset - sorting filter.Sorting - paging filter.Paging + sorting filter.SortExprSet + cursor *filter.PagingCursor + limit uint // @todo should filter also be here? @@ -51,9 +52,9 @@ func (i *iterator) More(max uint, last crs.ValueGetter) (err error) { i.rows = nil } - i.paging.Limit = max + i.limit = max if last != nil { - if i.paging.PageCursor, err = i.collectCursorValues(last); err != nil { + if i.cursor, err = i.collectCursorValues(last); err != nil { return fmt.Errorf("could not collect cursor values: %w", err) } } @@ -86,7 +87,7 @@ func (i *iterator) fetch(ctx context.Context) (_ *sql.Rows, err error) { } var ( - cur = i.paging.PageCursor + cur = i.cursor tmp exp.Expression sql string @@ -95,12 +96,12 @@ func (i *iterator) fetch(ctx context.Context) (_ *sql.Rows, err error) { // contains query with ORDER BY and WHERE clauses query = i.query - sort = i.sorting.Sort.Clone() + sort = i.sorting.Clone() ) { // Apply limit from the filter - query = query.Limit(i.paging.GetLimit()) + query = query.Limit(i.limit) if cur != nil { // @todo this needs to work with embedded attributes (non physical columns) as well! @@ -129,7 +130,7 @@ func (i *iterator) fetch(ctx context.Context) (_ *sql.Rows, err error) { query = query.Where(tmp) if cur.IsROrder() { - if i.paging.Limit > 0 { + if i.limit > 0 { // When paging with the reverse cursor AND limit set // we need to do a do sub-query reverse sort to ensure // that we only select the rows that make sense @@ -222,7 +223,7 @@ func (i *iterator) BackCursor(r crs.ValueGetter) (cur *filter.PagingCursor, err // if this cursor is used, we need to flip the conditional operator // from less-then to greater-then - cur.LThen = i.sorting.Sort.Reversed() + cur.LThen = i.sorting.Reversed() return } @@ -232,7 +233,7 @@ func (i *iterator) ForwardCursor(r crs.ValueGetter) (*filter.PagingCursor, error func (i *iterator) collectCursorValues(r crs.ValueGetter) (_ *filter.PagingCursor, err error) { var ( - cur = &filter.PagingCursor{LThen: i.sorting.Sort.Reversed()} + cur = &filter.PagingCursor{LThen: i.sorting.Reversed()} // @todo this will not work when using multiple primary keys! pkUsed bool @@ -252,7 +253,7 @@ func (i *iterator) collectCursorValues(r crs.ValueGetter) (_ *filter.PagingCurso return nil, fmt.Errorf("can not construct cursor without primary key attributes") } - for _, c := range i.sorting.Sort { + for _, c := range i.sorting { if pKeys[c.Column] { pkUsed = true } diff --git a/store/adapters/rdbms/crs/model.go b/store/adapters/rdbms/crs/model.go index d3e70bf5f..988bea990 100644 --- a/store/adapters/rdbms/crs/model.go +++ b/store/adapters/rdbms/crs/model.go @@ -7,7 +7,6 @@ import ( "strings" "github.com/cortezaproject/corteza-server/compose/crs" - "github.com/cortezaproject/corteza-server/compose/types" "github.com/cortezaproject/corteza-server/pkg/data" "github.com/cortezaproject/corteza-server/pkg/filter" "github.com/cortezaproject/corteza-server/store/adapters/rdbms/drivers" @@ -18,10 +17,6 @@ import ( ) type ( - sqlizer interface { - ToSQL() (string, []interface{}, error) - } - queryRunner interface { sqlx.QueryerContext sqlx.ExecerContext @@ -59,6 +54,11 @@ func Model(m *data.Model, c queryRunner, d drivers.Dialect) *model { ms.queryParser = ql.Converter( ql.SymHandler(func(node *ql.ASTNode) (exp.Expression, error) { + attr := ms.model.Attributes.FindByIdent(node.Symbol) + if !attr.Filterable { + return nil, fmt.Errorf("attribute %q can not be used in query expression", attr.Ident) + } + return ms.table.AttributeExpression(node.Symbol) }), ) @@ -106,18 +106,22 @@ func (d *model) Delete(ctx context.Context, r crs.ValueGetter) error { return err } -func (d *model) Search(f types.RecordFilter) (i *iterator, err error) { - if f.PageCursor != nil { +func (d *model) Search(f filter.Filter) (i *iterator, err error) { + var ( + orderBy = f.OrderBy() + ) + + if f.Cursor() != nil { // Page cursor exists; we need to validate it against used sort // To cover the case when paging cursor is set but sorting is empty, we collect the sorting instructions // from the cursor. // This (extracted sorting info) is then returned as part of response - if f.Sort, err = f.PageCursor.Sort(f.Sort); err != nil { + if orderBy, err = f.Cursor().Sort(orderBy); err != nil { return nil, err } } - for _, s := range f.Sort { + for _, s := range orderBy { if _, err = d.table.AttributeExpression(s.Column); err != nil { return nil, err } @@ -130,7 +134,7 @@ func (d *model) Search(f types.RecordFilter) (i *iterator, err error) { } attrIdent := c.Attribute().Ident - if f.Sort.Get(attrIdent) != nil { + if orderBy.Get(attrIdent) != nil { continue } @@ -139,14 +143,15 @@ func (d *model) Search(f types.RecordFilter) (i *iterator, err error) { } // Make sure results are always sorted at least by primary key - f.AppendOrderBy(attrIdent, f.Sort.LastDescending()) + orderBy = append(orderBy, &filter.SortExpr{Column: attrIdent, Descending: orderBy.LastDescending()}) } return &iterator{ ms: d, query: d.searchSql(f), - sorting: f.Sorting, - paging: f.Paging, + sorting: orderBy, + cursor: f.Cursor(), + limit: f.Limit(), }, nil } @@ -186,7 +191,7 @@ func (d *model) Lookup(ctx context.Context, pkv crs.ValueGetter, r crs.ValueSett // converting parts of record filter into conditions // // Does not add any limits, sorting or any cursor conditions! -func (d *model) searchSql(f types.RecordFilter) *goqu.SelectDataset { +func (d *model) searchSql(f filter.Filter) *goqu.SelectDataset { var ( err error base = d.selectSql() @@ -217,48 +222,66 @@ func (d *model) searchSql(f types.RecordFilter) *goqu.SelectDataset { //} } - { - // this can no longer be - //if len(f.LabeledIDs) > 0 { - // // Limit by LabeledIDs (list of record IDs) - // cnd = append(cnd, d.sysColumnID.In(f.LabeledIDs)) - //} - } + for ident, vv := range f.Constraints() { + attr := d.model.Attributes.FindByIdent(ident) + if attr == nil { + return base.SetError(fmt.Errorf("unknown attribute %q used for state constrant", ident)) + } - if f.Deleted != filter.StateInclusive { - //If model supports soft-deletion (= delete-at attribute is present) - //we need to make sure we respect it - var attrIdent exp.LiteralExpression - for _, attr := range d.model.Attributes { - if !attr.SoftDeleteFlag { - continue - } + if !attr.PrimaryKey { + continue + } - if !attr.Type.IsNullable() { - // @todo this must be checked much earlier - return base.SetError(fmt.Errorf("can not use non-nullable attribute %q soft-deleting", attr.Ident)) - } - - attrIdent, err = d.table.AttributeExpression(attr.Ident) - if err != nil { - return base.SetError(err) - } - - switch f.Deleted { - case filter.StateExclusive: - // only not-null values - cnd = append(cnd, attrIdent.IsNotNull()) - - case filter.StateExcluded: - // exclude all non-null values - cnd = append(cnd, attrIdent.IsNull()) - } + var attrExpr exp.LiteralExpression + attrExpr, err = d.table.AttributeExpression(attr.Ident) + if err != nil { + return base.SetError(err) + } + if len(vv) > 0 { + cnd = append(cnd, attrExpr.In(vv...)) } } - if len(strings.TrimSpace(f.Query)) > 0 { - if tmp, err = d.queryParser.Parse(f.Query); err != nil { + for ident, state := range f.StateConstraints() { + attr := d.model.Attributes.FindByIdent(ident) + if attr == nil { + return base.SetError(fmt.Errorf("unknown attribute %q used for state constrant", ident)) + } + + if !attr.SoftDeleteFlag { + continue + } + + if !attr.Type.IsNullable() { + // @todo this must be checked much earlier + return base.SetError(fmt.Errorf("can not use non-nullable attribute %q soft-deleting", attr.Ident)) + } + + if state == filter.StateInclusive { + // not used so we don't rea + continue + } + + var attrExpr exp.LiteralExpression + attrExpr, err = d.table.AttributeExpression(attr.Ident) + if err != nil { + return base.SetError(err) + } + + switch state { + case filter.StateExclusive: + // only not-null values + cnd = append(cnd, attrExpr.IsNotNull()) + + case filter.StateExcluded: + // exclude all non-null values + cnd = append(cnd, attrExpr.IsNull()) + } + } + + if q := strings.TrimSpace(f.Expression()); len(q) > 0 { + if tmp, err = d.queryParser.Parse(q); err != nil { return base.SetError(err) } diff --git a/store/adapters/rdbms/crs/model_test.go b/store/adapters/rdbms/crs/model_test.go deleted file mode 100644 index 9b522f023..000000000 --- a/store/adapters/rdbms/crs/model_test.go +++ /dev/null @@ -1,176 +0,0 @@ -package crs - -import ( - "testing" - "time" - - "github.com/cortezaproject/corteza-server/compose/types" - "github.com/cortezaproject/corteza-server/pkg/data" - "github.com/cortezaproject/corteza-server/store/adapters/rdbms/drivers" - "github.com/doug-martin/goqu/v9" - _ "github.com/doug-martin/goqu/v9/dialect/postgres" - "github.com/doug-martin/goqu/v9/exp" - "github.com/stretchr/testify/require" -) - -type ( - testDialect struct{} -) - -func (testDialect) GOQU() goqu.DialectWrapper { return goqu.Dialect("sqlite3") } -func (testDialect) DeepIdentJSON(ident exp.IdentifierExpression, pp ...any) exp.LiteralExpression { - return drivers.DeepIdentJSON(ident, pp...) -} -func (testDialect) AttributeCast(_ *data.Attribute, val exp.LiteralExpression) (exp.LiteralExpression, error) { - return exp.NewLiteralExpression("?", val), nil -} - -func Test_sqlizers(t *testing.T) { - var ( - m = &data.Model{ - Ident: "test_tbl", - Attributes: data.AttributeSet{ - &data.Attribute{Ident: sysID, Type: &data.TypeID{}, Store: &data.StoreCodecAlias{Ident: "id"}}, - &data.Attribute{Ident: sysCreatedAt, Type: &data.TypeTimestamp{}, Store: &data.StoreCodecAlias{Ident: "created_at"}}, - &data.Attribute{Ident: sysUpdatedAt, Type: &data.TypeTimestamp{}, Store: &data.StoreCodecAlias{Ident: "updated_at"}}, - }, - } - - ms = Model(m, nil, &testDialect{}) - - q sqlizer - sql string - err error - - tenTenTen, _ = time.Parse("2006-01-02", "2010-10-10") - ) - - cases := []struct { - fn func() sqlizer - sql string - err error - }{ - { - fn: func() sqlizer { return ms.lookupByIdSql(10).Prepared(false) }, - sql: `SELECT "test_tbl"."id", "test_tbl"."created_at", "test_tbl"."updated_at" FROM "test_tbl" WHERE ("test_tbl"."id" = 10) LIMIT 1`, - err: nil, - }, - { - fn: func() sqlizer { - return ms.updateSql(&types.Record{ID: 10, CreatedAt: tenTenTen, UpdatedAt: &tenTenTen}).Prepared(false) - }, - sql: `UPDATE "test_tbl" SET "created_at"='2010-10-10T00:00:00Z',"updated_at"='2010-10-10T00:00:00Z' WHERE ("test_tbl"."id" = 10)`, - err: nil, - }, - { - fn: func() sqlizer { return ms.insertSql(&types.Record{ID: 10, CreatedAt: tenTenTen}).Prepared(false) }, - sql: `INSERT INTO "test_tbl" ("created_at", "id", "updated_at") VALUES ('2010-10-10T00:00:00Z', 10, NULL)`, - err: nil, - }, - { - fn: func() sqlizer { return ms.deleteByIdSql(12345).Prepared(false) }, - sql: `DELETE FROM "test_tbl" WHERE ("test_tbl"."id" = 12345)`, - err: nil, - }, - } - - for _, c := range cases { - t.Run("", func(t *testing.T) { - // returns sqlizer with prepared(false): - // values are interpolated into a SQL string - req := require.New(t) - q = c.fn() - - sql, _, err = q.ToSQL() - if c.err == nil { - req.NoError(err) - } else { - req.ErrorIs(err, c.err) - } - - req.Equal(c.sql, sql) - }) - } -} - -func Test_search(t *testing.T) { - var ( - m = &data.Model{ - Ident: "test_tbl", - Attributes: data.AttributeSet{ - &data.Attribute{Ident: sysID, Type: &data.TypeID{}, Store: &data.StoreCodecAlias{Ident: "id"}}, - &data.Attribute{Ident: sysModuleID, Type: &data.TypeID{}, Store: &data.StoreCodecAlias{Ident: "rel_module"}}, - &data.Attribute{Ident: sysCreatedAt, Type: &data.TypeTimestamp{}, Store: &data.StoreCodecAlias{Ident: "created_at"}}, - &data.Attribute{Ident: sysUpdatedAt, Type: &data.TypeTimestamp{}, Store: &data.StoreCodecAlias{Ident: "updated_at"}}, - &data.Attribute{Ident: "foo", Type: &data.TypeText{}, Store: &data.StoreCodecStdRecordValueJSON{Ident: "meta"}}, - &data.Attribute{Ident: "bar", Type: &data.TypeNumber{}, Store: &data.StoreCodecStdRecordValueJSON{Ident: "meta"}}, - &data.Attribute{Ident: "baz", Type: &data.TypeBoolean{}, Store: &data.StoreCodecStdRecordValueJSON{Ident: "meta"}}, - &data.Attribute{Ident: "bbb", Type: &data.TypeUUID{}, Store: &data.StoreCodecStdRecordValueJSON{Ident: "meta"}}, - &data.Attribute{Ident: "phy", Type: &data.TypeText{}, Store: &data.StoreCodecPlain{}}, - }, - } - - sql string - args []any - err error - - moduleID = uint64(1<<64 - 1) - - prefix = `SELECT "test_tbl"."id", "test_tbl"."rel_module", "test_tbl"."created_at", "test_tbl"."updated_at", "test_tbl"."meta", "test_tbl"."phy" FROM "test_tbl"` - ) - - cases := []struct { - f types.RecordFilter - sql string - args []any - err error - }{ - { - f: types.RecordFilter{ - ModuleID: moduleID, - }, - sql: prefix + ` WHERE ("test_tbl"."rel_module" = $1)`, - args: []any{moduleID}, - err: nil, - }, - { - f: types.RecordFilter{ - ModuleID: moduleID, - Query: "moduleID == 1234", - }, - sql: prefix + ` WHERE (("test_tbl"."rel_module" = $1) AND ("test_tbl"."rel_module" = $2))`, - args: []any{moduleID, int64(1234)}, - err: nil, - }, - { - f: types.RecordFilter{ - ModuleID: moduleID, - Query: `bar = 1 AND foo = phy`, - }, - sql: prefix + ` WHERE (("test_tbl"."rel_module" = $1) AND (("meta"->'bar'->0 = $2) AND ("meta"->'foo'->0 = "test_tbl"."phy")))`, - args: []any{moduleID, int64(1)}, - err: nil, - }, - } - - d := Model(m, nil, nil) - - for _, c := range cases { - t.Run("", func(t *testing.T) { - req := require.New(t) - q := d.searchSql(c.f).WithDialect("postgres") - sql, args, err = q.ToSQL() - if c.args == nil { - req.NoError(err) - } else { - req.ErrorIs(err, c.err) - } - - if len(c.sql) > 0 { - req.Equal(c.sql, sql) - } - - req.Equal(c.args, args) - }) - } -} diff --git a/store/adapters/rdbms/crs/record.go b/store/adapters/rdbms/crs/record.go index da8bbfdbe..355a79b2a 100644 --- a/store/adapters/rdbms/crs/record.go +++ b/store/adapters/rdbms/crs/record.go @@ -12,23 +12,3 @@ const ( sysDeletedBy = "deletedBy" sysOwnedBy = "ownedBy" ) - -// is the -func isSystemField(f string) bool { - switch f { - // handle system fields - case sysID, - sysNamespaceID, - sysModuleID, - sysCreatedAt, - sysCreatedBy, - sysUpdatedAt, - sysUpdatedBy, - sysDeletedAt, - sysDeletedBy, - sysOwnedBy: - return true - default: - return false - } -}