From 06d75cb654c016f6e5b34c89d9253b970b86c654 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Fri, 9 Oct 2026 20:56:53 +0200 Subject: [PATCH] fix(cabana): search and sort to-many relation columns through subqueries - relationSubquery reads the pivot, key and table names from GORM's parsed relationship (Schema.Relationships.Relations), never from a convention - list search matches every relation column through a correlated EXISTS, so the count query gains no JOIN and belongsToMany search stops failing - sorting a hasMany or belongsToMany column orders by a correlated string_agg subquery before the primary-key tiebreak - polymorphic relations and unknown related columns answer 422 - DryRun smoke test TestListRelationSearchSmoke needs no Docker --- docs/backend/lists-and-filters.md | 2 +- modules/cabana/query.go | 182 ++++++++++++++++++++---- modules/cabana/query_relation_test.go | 194 ++++++++++++++++++++++++++ 3 files changed, 353 insertions(+), 25 deletions(-) create mode 100644 modules/cabana/query_relation_test.go diff --git a/docs/backend/lists-and-filters.md b/docs/backend/lists-and-filters.md index 1f31d01..cf8a11d 100644 --- a/docs/backend/lists-and-filters.md +++ b/docs/backend/lists-and-filters.md @@ -59,7 +59,7 @@ columns: type: datetime ``` -A column takes `label`, `type` (`text`, `datetime`, `date`, `time` or `switch`), `searchable` (default false) and `sortable` (default true, as in WinterCMS), and, for a column read through a relation, `relation` and `select`. Other WinterCMS column types and options are refused at boot. When `type` is omitted, a `time.Time` column is `datetime`, a `lagoon.Date` column is `date` and a `lagoon.TimeOfDay` column is `time`; every other column stays untyped (the admin treats that as text). An explicit `type` always wins. The compiled list keeps the columns in file order: +A column takes `label`, `type` (`text`, `datetime`, `date`, `time` or `switch`), `searchable` (default false) and `sortable` (default true, as in WinterCMS), and, for a column read through a relation, `relation` and `select`. Other WinterCMS column types and options are refused at boot. When `type` is omitted, a `time.Time` column is `datetime`, a `lagoon.Date` column is `date` and a `lagoon.TimeOfDay` column is `time`; every other column stays untyped (the admin treats that as text). An explicit `type` always wins. A searchable relation column matches a record when any related record matches: a `categories` column with `relation: categories` and `select: name` on a blog post finds the posts with at least one matching category, for belongsTo, hasOne, hasMany and belongsToMany relations alike, each through a subquery, so a post is listed and counted once. A sortable hasMany or belongsToMany column sorts by its comma-joined values, the text the list shows. The compiled list keeps the columns in file order: ```go src=modules/cabana/example_test.go#ExampleCompileList // The plugin embeds its controllers/ and models/ trees; the example reads diff --git a/modules/cabana/query.go b/modules/cabana/query.go index 53c91be..b8ea7cf 100644 --- a/modules/cabana/query.go +++ b/modules/cabana/query.go @@ -17,6 +17,7 @@ import ( "git.golem15.com/golem15/summercms/modules/pact" "gorm.io/gorm" "gorm.io/gorm/clause" + gormschema "gorm.io/gorm/schema" ) // ListInput is the raw admin list query. Identifiers are matched later @@ -129,7 +130,10 @@ func ExecuteList(ctx context.Context, db *gorm.DB, cc *CompiledController, in Li if err := q.Session(&gorm.Session{}).Count(&total).Error; err != nil { return nil, err } - q = applyListOrder(q, cc.Controller, cc.List, model, sortCol, sortDesc) + q, err = applyListOrder(q, cc.Controller, cc.List, model, sortCol, sortDesc) + if err != nil { + return nil, err + } for _, col := range cc.List.Columns { if col.Relation == "" { continue @@ -290,12 +294,11 @@ func normalizeSort(schema *ListSchema, in ListInput) (string, bool, error) { return match.Key, dir == "desc", nil } -func applyListOrder(db *gorm.DB, controller pact.AdminController, schema *ListSchema, model any, column string, desc bool) *gorm.DB { +func applyListOrder(db *gorm.DB, controller pact.AdminController, schema *ListSchema, model any, column string, desc bool) (*gorm.DB, error) { table := tableName(model) pk := primaryColumn(model) if column != "" { - orderTable := table - orderColumn := column + order := clause.OrderByColumn{Column: clause.Column{Table: table, Name: column}, Desc: desc} for _, col := range schema.Columns { if col.Key != column || col.Relation == "" { continue @@ -304,22 +307,149 @@ func applyListOrder(db *gorm.DB, controller pact.AdminController, schema *ListSc if !ok { continue } - orderTable = field - orderColumn = listRelationColumn(controller, col.Relation, col.Select) + sub, err := relationSubquery(db, model, field, listRelationColumn(controller, col.Relation, col.Select), 0, "sort") + if err != nil { + return nil, err + } + if sub.toMany { + // Winter sorts a to-many column on its group_concat sub-select; + // string_agg in the same order is the comma-joined value the + // list displays (relatedSelectMany). A raw column merges with + // the primary-key tiebreak below into one ORDER BY. + value := "CAST(" + sub.column + " AS TEXT)" + order.Column = clause.Column{ + Name: fmt.Sprintf("(SELECT string_agg(%s, ', ' ORDER BY %s) FROM %s WHERE %s)", value, value, sub.from, sub.where), + Raw: true, + } + break + } + // A to-one relation is joined; GORM aliases the joined + // association with the Go field name. db = db.Joins(field) + order.Column = clause.Column{Table: field, Name: sub.name} break } - db = db.Order(clause.OrderByColumn{ - Column: clause.Column{Table: orderTable, Name: orderColumn}, - Desc: desc, - }) + db = db.Order(order) } if column != pk { db = db.Order(clause.OrderByColumn{Column: clause.Column{Table: table, Name: pk}}) } - return db + return db, nil } +// relationSQL is the FROM and WHERE of a subquery over the related records +// of one relation, correlated to the outer list query's main table. It holds +// no bound values: every name in it comes from GORM's parsed relationship and +// is quoted by the dialector. +type relationSQL struct { + // from is the related table aliased cabana_r; a belongsToMany + // relation adds its pivot aliased cabana_p, joined to the related + // table. + from string + // where correlates the related records to the main table's row and + // skips soft-deleted related records. + where string + // column is the selected related column, qualified with the alias. + column string + // name is the selected column's physical name. + name string + // toMany is true for hasMany and belongsToMany relations. + toMany bool +} + +// relationSubquery builds the subquery parts for the relation behind the Go +// field of model, selecting column from the related table. Table, pivot and +// key names come from GORM's parsed relationship, never from a naming +// convention, so custom foreignKey, joinForeignKey and many2many tags and the +// naming strategy are honored. The aliases carry n, which keeps a +// self-referencing relation apart from the main table and two subqueries of +// one statement apart from each other. A missing relation, a column the +// related model lacks and polymorphic relations are refused as validation +// failures on param. +func relationSubquery(db *gorm.DB, model any, field, column string, n int, param string) (relationSQL, error) { + invalid := listInvalid(param, "is not a searchable relation column") + if param == "sort" { + invalid = listInvalid(param, "is not a sortable column") + } + stmt := &gorm.Statement{DB: db} + if err := stmt.Parse(model); err != nil { + return relationSQL{}, err + } + rel, ok := stmt.Schema.Relationships.Relations[field] + if !ok || rel == nil || rel.FieldSchema == nil || rel.Polymorphic != nil || !identifier(column) { + return relationSQL{}, invalid + } + selected := rel.FieldSchema.LookUpField(column) + if selected == nil || selected.DBName == "" { + return relationSQL{}, invalid + } + for _, ref := range rel.References { + if ref == nil || ref.PrimaryValue != "" || ref.PrimaryKey == nil || ref.ForeignKey == nil { + return relationSQL{}, invalid + } + } + main := stmt.Schema.Table + related := fmt.Sprintf("cabana_r%d", n) + out := relationSQL{ + column: qualifiedColumn(db, related, selected.DBName), + name: selected.DBName, + } + var on, where []string + switch rel.Type { + case gormschema.Many2Many: + if rel.JoinTable == nil { + return relationSQL{}, invalid + } + pivot := fmt.Sprintf("cabana_p%d", n) + for _, ref := range rel.References { + if ref.OwnPrimaryKey { + // pivot owner key = main primary key + where = append(where, qualifiedColumn(db, pivot, ref.ForeignKey.DBName)+" = "+qualifiedColumn(db, main, ref.PrimaryKey.DBName)) + } else { + // pivot related key = related primary key + on = append(on, qualifiedColumn(db, pivot, ref.ForeignKey.DBName)+" = "+qualifiedColumn(db, related, ref.PrimaryKey.DBName)) + } + } + if len(on) == 0 || len(where) == 0 { + return relationSQL{}, invalid + } + out.from = fmt.Sprintf("%s AS %s JOIN %s AS %s ON %s", + quotedIdent(db, rel.JoinTable.Table), quotedIdent(db, pivot), + quotedIdent(db, rel.FieldSchema.Table), quotedIdent(db, related), + strings.Join(on, " AND ")) + out.toMany = true + case gormschema.HasMany, gormschema.HasOne: + for _, ref := range rel.References { + // related foreign key = main primary key + where = append(where, qualifiedColumn(db, related, ref.ForeignKey.DBName)+" = "+qualifiedColumn(db, main, ref.PrimaryKey.DBName)) + } + out.from = quotedIdent(db, rel.FieldSchema.Table) + " AS " + quotedIdent(db, related) + out.toMany = rel.Type == gormschema.HasMany + case gormschema.BelongsTo: + for _, ref := range rel.References { + // related primary key = main foreign key + where = append(where, qualifiedColumn(db, related, ref.PrimaryKey.DBName)+" = "+qualifiedColumn(db, main, ref.ForeignKey.DBName)) + } + out.from = quotedIdent(db, rel.FieldSchema.Table) + " AS " + quotedIdent(db, related) + default: + return relationSQL{}, invalid + } + if len(where) == 0 { + return relationSQL{}, invalid + } + // Winter's whereHas applies the related model's soft-delete scope. + for _, f := range rel.FieldSchema.Fields { + if f.DBName != "" && f.FieldType == deletedAtType { + where = append(where, qualifiedColumn(db, related, f.DBName)+" IS NULL") + break + } + } + out.where = strings.Join(where, " AND ") + return out, nil +} + +var deletedAtType = reflect.TypeOf(gorm.DeletedAt{}) + func applyListSearch(db *gorm.DB, controller pact.AdminController, schema *ListSchema, model any, term string) (*gorm.DB, error) { term = strings.TrimSpace(term) if term == "" { @@ -335,32 +465,36 @@ func applyListSearch(db *gorm.DB, controller pact.AdminController, schema *ListS return nil, listInvalid("search", "is not searchable") } pattern := "%" + escapeLike(strings.ToLower(term)) + "%" - joined := map[string]struct{}{} parts := make([]string, 0, len(cols)) args := make([]any, 0, len(cols)) main := tableName(model) + n := 0 for _, col := range cols { - table := main - column := col.Key if col.Relation != "" { field, ok := relationFieldName(model, col.Relation) - physical := listRelationColumn(controller, col.Relation, col.Select) - if !ok || !identifier(physical) { + if !ok { return nil, listInvalid("search", "is not searchable") } - // GORM aliases the joined association with the Go field name. - table = field - column = physical - if _, done := joined[field]; !done { - db = db.Joins(field) - joined[field] = struct{}{} + // Every relation type is searched through a correlated EXISTS + // (Winter's orWhereHas), so the outer query gains no JOIN: a + // record matching through several related rows is counted and + // listed once, and unqualified ListExtendQuery or scope + // predicates stay unambiguous. + sub, err := relationSubquery(db, model, field, listRelationColumn(controller, col.Relation, col.Select), n, "search") + if err != nil { + return nil, err } - } else if !identifier(column) { + n++ + parts = append(parts, fmt.Sprintf("EXISTS (SELECT 1 FROM %s WHERE %s AND LOWER(CAST(%s AS TEXT)) LIKE ? ESCAPE '\\')", sub.from, sub.where, sub.column)) + args = append(args, pattern) + continue + } + if !identifier(col.Key) { return nil, listInvalid("search", "is not searchable") } // CAST makes a numeric, date or boolean column searchable as its text // form (Winter casts to TEXT too); PostgreSQL has no lower(integer). - parts = append(parts, fmt.Sprintf("LOWER(CAST(%s AS TEXT)) LIKE ? ESCAPE '\\'", qualifiedColumn(db, table, column))) + parts = append(parts, fmt.Sprintf("LOWER(CAST(%s AS TEXT)) LIKE ? ESCAPE '\\'", qualifiedColumn(db, main, col.Key))) args = append(args, pattern) } return db.Where(strings.Join(parts, " OR "), args...), nil diff --git a/modules/cabana/query_relation_test.go b/modules/cabana/query_relation_test.go new file mode 100644 index 0000000..5c5f404 --- /dev/null +++ b/modules/cabana/query_relation_test.go @@ -0,0 +1,194 @@ +package cabana + +import ( + "context" + "net/http" + "strings" + "sync" + "testing" + "time" + + "git.golem15.com/golem15/summercms/modules/backpack" + "git.golem15.com/golem15/summercms/modules/pact" + pgdriver "gorm.io/driver/postgres" + "gorm.io/gorm" + "gorm.io/gorm/logger" +) + +// The relation fixtures cover every relation type a list column can name: +// belongsTo (Author), belongsToMany through a pivot (Tags) and hasMany (Notes). + +type relAuthor struct { + ID uint `gorm:"column:id;primaryKey"` + Name string `gorm:"column:name"` +} + +func (relAuthor) TableName() string { return "cabana_rel_authors" } + +type relTag struct { + ID uint `gorm:"column:id;primaryKey"` + Name string `gorm:"column:name"` +} + +func (relTag) TableName() string { return "cabana_rel_tags" } + +type relNote struct { + ID uint `gorm:"column:id;primaryKey"` + PostID uint `gorm:"column:post_id"` + Body string `gorm:"column:body"` +} + +func (relNote) TableName() string { return "cabana_rel_notes" } + +type relPost struct { + ID uint `gorm:"column:id;primaryKey"` + Title string `gorm:"column:title"` + AuthorID *uint `gorm:"column:author_id"` + Author relAuthor `gorm:"foreignKey:AuthorID"` + Tags []relTag `gorm:"many2many:cabana_rel_post_tags"` + Notes []relNote `gorm:"foreignKey:PostID"` +} + +func (relPost) TableName() string { return "cabana_rel_posts" } + +type relController struct{} + +func (relController) ID() string { return "acme.demo.widgets" } +func (relController) ModelName() string { return "Widget" } +func (relController) ConfigDir() string { return "controllers/widgets" } +func (relController) NewRecord() any { return &relPost{} } + +var _ pact.AdminRecordSource = relController{} + +const relListConfig = `modelClass: Widget +list: ~/plugins/acme/demo/models/widget/columns.yaml +recordsPerPage: 20 +showSorting: true +` + +const relColumns = `columns: + title: + label: Title + searchable: true + tags: + label: Tags + relation: tags + select: name + searchable: true + sortable: true + notes: + label: Notes + relation: notes + select: body + searchable: true + author: + label: Author + relation: author + select: name + searchable: true + sortable: true +` + +// sqlRecorder is a GORM logger that keeps every statement GORM traces, with +// its bound values inlined, so DryRun tests can assert on the SQL shape. +type sqlRecorder struct { + mu sync.Mutex + sqls []string +} + +func (r *sqlRecorder) LogMode(logger.LogLevel) logger.Interface { return r } +func (r *sqlRecorder) Info(context.Context, string, ...any) {} +func (r *sqlRecorder) Warn(context.Context, string, ...any) {} +func (r *sqlRecorder) Error(context.Context, string, ...any) {} +func (r *sqlRecorder) Trace(_ context.Context, _ time.Time, fc func() (string, int64), _ error) { + sql, _ := fc() + r.mu.Lock() + defer r.mu.Unlock() + r.sqls = append(r.sqls, sql) +} + +// statements returns the recorded statements and forgets them. +func (r *sqlRecorder) statements() []string { + r.mu.Lock() + defer r.mu.Unlock() + out := r.sqls + r.sqls = nil + return out +} + +// statement returns the first recorded statement starting with prefix. +func (r *sqlRecorder) statement(t *testing.T, prefix string, all []string) string { + t.Helper() + for _, sql := range all { + if strings.HasPrefix(sql, prefix) { + return sql + } + } + t.Fatalf("no statement starting with %q in %q", prefix, all) + return "" +} + +// openDryRunDB opens a Postgres-dialect GORM handle that never connects: +// DryRun builds every statement without executing it, and the DSN points at +// a port nothing listens on, so a test that needs no Docker can still assert +// on the SQL that ExecuteList would send. +func openDryRunDB(t *testing.T) (*gorm.DB, *sqlRecorder) { + t.Helper() + rec := &sqlRecorder{} + db, err := gorm.Open(pgdriver.New(pgdriver.Config{DSN: "host=127.0.0.1 port=1 user=cabana dbname=cabana sslmode=disable"}), + &gorm.Config{DryRun: true, DisableAutomaticPing: true, Logger: rec}) + if err != nil { + t.Fatalf("dry run db: %v", err) + } + return db, rec +} + +// newDryRunListService builds a service whose app publishes db and whose +// registry holds the compiled list of ctl as acme.demo.widgets, the +// controller controllerRequest targets. +func newDryRunListService(t *testing.T, db *gorm.DB, ctl pact.AdminController, config, columns string) *service { + t.Helper() + schema, err := CompileList("acme.demo", ctl, filterFS(config, columns, "")) + if err != nil { + t.Fatalf("schema: %v", err) + } + app := backpack.New(nil) + if err := app.Publish(db); err != nil { + t.Fatal(err) + } + return &service{app: app, reg: &Registry{byID: map[string]*CompiledController{ + "acme.demo.widgets": {Controller: ctl, List: schema}, + }}} +} + +func TestListRelationSearchSmoke(t *testing.T) { + db, rec := openDryRunDB(t) + svc := newDryRunListService(t, db, relController{}, relListConfig, relColumns) + + decodeList(t, getList(t, svc, "search=fungi"), http.StatusOK) + count := rec.statement(t, "SELECT count(*)", rec.statements()) + for _, want := range []string{ + `EXISTS (SELECT 1 FROM "cabana_rel_post_tags" AS "cabana_p0" JOIN "cabana_rel_tags" AS "cabana_r0" ON "cabana_p0"."rel_tag_id" = "cabana_r0"."id"`, + `WHERE "cabana_p0"."rel_post_id" = "cabana_rel_posts"."id" AND LOWER(CAST("cabana_r0"."name" AS TEXT)) LIKE '%fungi%'`, + `EXISTS (SELECT 1 FROM "cabana_rel_notes" AS "cabana_r1" WHERE "cabana_r1"."post_id" = "cabana_rel_posts"."id"`, + `EXISTS (SELECT 1 FROM "cabana_rel_authors" AS "cabana_r2" WHERE "cabana_r2"."id" = "cabana_rel_posts"."author_id"`, + } { + if !strings.Contains(count, want) { + t.Fatalf("count statement lacks %s:\n%s", want, count) + } + } + if strings.Contains(count, "JOIN \"cabana_rel_tags\" \"Tags\"") || strings.Contains(count, "LEFT JOIN") { + t.Fatalf("count statement joins a relation:\n%s", count) + } + + decodeList(t, getList(t, svc, "sort=tags&dir=desc"), http.StatusOK) + page := rec.statement(t, "SELECT * FROM", rec.statements()) + order := page[strings.Index(page, "ORDER BY"):] + if !strings.Contains(order, `ORDER BY (SELECT string_agg(CAST("cabana_r0"."name" AS TEXT), ', ' ORDER BY CAST("cabana_r0"."name" AS TEXT)) FROM "cabana_rel_post_tags" AS "cabana_p0"`) || + !strings.Contains(order, `) DESC,"cabana_rel_posts"."id"`) { + t.Fatalf("to-many sort is not a string_agg subquery with the id tiebreak:\n%s", page) + } + if strings.Contains(page, "LEFT JOIN") { + t.Fatalf("to-many sort joins the relation:\n%s", page) + } +}