From adb5d6e9987b9fef1195cbed1601c0177cb9284e Mon Sep 17 00:00:00 2001 From: Gani Georgiev Date: Fri, 11 Aug 2023 14:29:18 +0300 Subject: [PATCH] [#3110] normalized view queries with numeric or expression ids --- .github/workflows/release.yaml | 2 +- CHANGELOG.md | 7 +- apis/collection_test.go | 13 +-- apis/record_crud_test.go | 26 ++++++ daos/collection.go | 52 ++++++++++++ daos/collection_test.go | 116 +++++++++++++++++++++++++- daos/view.go | 45 ++++++++-- daos/view_test.go | 8 +- forms/collections_import_test.go | 2 +- migrations/1691747912_resave_views.go | 28 +++++++ tests/data/data.db | Bin 249856 -> 249856 bytes 11 files changed, 277 insertions(+), 22 deletions(-) create mode 100644 migrations/1691747912_resave_views.go diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index bc8ba5f6..09797872 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -21,7 +21,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v3 with: - go-version: '>=1.20.3' + go-version: '>=1.21.0' # This step usually is not needed because the /ui/dist is pregenerated locally # but its here to ensure that each release embeds the latest admin ui artifacts. diff --git a/CHANGELOG.md b/CHANGELOG.md index e3dbf5d1..4c1a29d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,7 @@ -## v0.17.4-WIP +## v0.17.4 + +- Fixed Views record retrieval when numeric id is used ([#3110](https://github.com/pocketbase/pocketbase/issues/3110)). + _With this fix we also now properly recognize `CAST(... as TEXT)` and `CAST(... as BOOLEAN)` as `text` and `bool` fields._ - Fixed `relation` "Cascade delete" tooltip message ([#3098](https://github.com/pocketbase/pocketbase/issues/3098)). @@ -6,6 +9,8 @@ - Disabled the initial Admin UI admins counter cache when there are no initial admins to allow detecting externally created accounts (eg. with the `admin` command) ([#3106](https://github.com/pocketbase/pocketbase/issues/3106)). +- Downgraded `google/go-cloud` dependency to v0.32.0 until v0.34.0 is released to prevent the `os.TempDir` `cross-device link` errors as too many users complained about it. + ## v0.17.3 diff --git a/apis/collection_test.go b/apis/collection_test.go index 84bfcae1..7b15df7d 100644 --- a/apis/collection_test.go +++ b/apis/collection_test.go @@ -47,7 +47,7 @@ func TestCollectionsList(t *testing.T) { ExpectedContent: []string{ `"page":1`, `"perPage":30`, - `"totalItems":10`, + `"totalItems":11`, `"items":[{`, `"id":"_pb_users_auth_"`, `"id":"v851q4r790rhknl"`, @@ -57,6 +57,7 @@ func TestCollectionsList(t *testing.T) { `"id":"wzlqyes4orhoygb"`, `"id":"4d1blo5cuycfaca"`, `"id":"9n89pl5vkct6330"`, + `"id":"ib3m2700k5hlsjz"`, `"type":"auth"`, `"type":"base"`, }, @@ -75,9 +76,9 @@ func TestCollectionsList(t *testing.T) { ExpectedContent: []string{ `"page":2`, `"perPage":2`, - `"totalItems":10`, + `"totalItems":11`, `"items":[{`, - `"id":"kpv709sk2lqbqk8"`, + `"id":"v9gwnfh02gjq1q0"`, `"id":"9n89pl5vkct6330"`, }, ExpectedEvents: map[string]int{ @@ -1164,7 +1165,7 @@ func TestCollectionUpdate(t *testing.T) { } func TestCollectionsImport(t *testing.T) { - totalCollections := 10 + totalCollections := 11 scenarios := []tests.ApiScenario{ { @@ -1421,8 +1422,8 @@ func TestCollectionsImport(t *testing.T) { ExpectedEvents: map[string]int{ "OnCollectionsAfterImportRequest": 1, "OnCollectionsBeforeImportRequest": 1, - "OnModelBeforeDelete": 8, - "OnModelAfterDelete": 8, + "OnModelBeforeDelete": 9, + "OnModelAfterDelete": 9, "OnModelBeforeUpdate": 2, "OnModelAfterUpdate": 2, "OnModelBeforeCreate": 1, diff --git a/apis/record_crud_test.go b/apis/record_crud_test.go index 85d27adf..d879475e 100644 --- a/apis/record_crud_test.go +++ b/apis/record_crud_test.go @@ -462,6 +462,22 @@ func TestRecordCrudList(t *testing.T) { }, ExpectedEvents: map[string]int{"OnRecordsListRequest": 1}, }, + { + Name: "view collection with numeric ids", + Method: http.MethodGet, + Url: "/api/collections/numeric_id_view/records", + ExpectedStatus: 200, + ExpectedContent: []string{ + `"page":1`, + `"perPage":30`, + `"totalPages":1`, + `"totalItems":2`, + `"items":[{`, + `"id":"1"`, + `"id":"2"`, + }, + ExpectedEvents: map[string]int{"OnRecordsListRequest": 1}, + }, } for _, scenario := range scenarios { @@ -731,6 +747,16 @@ func TestRecordCrudView(t *testing.T) { }, ExpectedEvents: map[string]int{"OnRecordViewRequest": 1}, }, + { + Name: "view record with numeric id", + Method: http.MethodGet, + Url: "/api/collections/numeric_id_view/records/1", + ExpectedStatus: 200, + ExpectedContent: []string{ + `"id":"1"`, + }, + ExpectedEvents: map[string]int{"OnRecordViewRequest": 1}, + }, } for _, scenario := range scenarios { diff --git a/daos/collection.go b/daos/collection.go index 56527c29..9bde5b27 100644 --- a/daos/collection.go +++ b/daos/collection.go @@ -378,6 +378,12 @@ func (dao *Dao) saveViewCollection(newCollection, oldCollection *models.Collecti } } + // wrap view query if necessary + query, err = txDao.normalizeViewQueryId(query) + if err != nil { + return fmt.Errorf("failed to normalize view query id: %w", err) + } + // (re)create the view if err := txDao.SaveView(newCollection.Name, query); err != nil { return err @@ -389,6 +395,52 @@ func (dao *Dao) saveViewCollection(newCollection, oldCollection *models.Collecti }) } +// @todo consider removing once custom id types are supported +// +// normalizeViewQueryId wraps (if necessary) the provided view query +// with a subselect to ensure that the id column is a text since +// currently we don't support non-string model ids +// (see https://github.com/pocketbase/pocketbase/issues/3110). +func (dao *Dao) normalizeViewQueryId(query string) (string, error) { + parsed, err := dao.parseQueryToFields(query) + if err != nil { + return "", err + } + + needWrapping := true + + idField := parsed[schema.FieldNameId] + if idField != nil && idField.field != nil && + idField.field.Type != schema.FieldTypeJson && + idField.field.Type != schema.FieldTypeNumber && + idField.field.Type != schema.FieldTypeBool { + needWrapping = false + } + + if !needWrapping { + return query, nil // no changes needed + } + + // raw parse to preserve the columns order + rawParsed := new(identifiersParser) + if err := rawParsed.parse(query); err != nil { + return "", err + } + + columns := make([]string, 0, len(rawParsed.columns)) + for _, col := range rawParsed.columns { + if col.alias == schema.FieldNameId { + columns = append(columns, fmt.Sprintf("cast(%s as text) %s", col.alias, col.alias)) + } else { + columns = append(columns, col.alias) + } + } + + query = fmt.Sprintf("SELECT %s FROM (%s)", strings.Join(columns, ","), query) + + return query, nil +} + // resaveViewsWithChangedSchema updates all view collections with changed schemas. func (dao *Dao) resaveViewsWithChangedSchema(excludeIds ...string) error { collections, err := dao.FindCollectionsByType(models.CollectionTypeView) diff --git a/daos/collection_test.go b/daos/collection_test.go index d4e19fc3..7b341c9f 100644 --- a/daos/collection_test.go +++ b/daos/collection_test.go @@ -210,11 +210,12 @@ func TestDeleteCollection(t *testing.T) { {colUnsaved, true}, {colReferenced, true}, {colSystem, true}, + {colBase, true}, // depend on view1, view2 and view2 {colView1, true}, // view2 depend on it {colView2, false}, {colView1, false}, // no longer has dependent collections - {colBase, false}, - {colAuth, false}, // should delete also its related external auths + {colBase, false}, // no longer has dependent views + {colAuth, false}, // should delete also its related external auths } for i, s := range scenarios { @@ -393,8 +394,117 @@ func TestSaveCollectionIndirectViewsUpdate(t *testing.T) { } } +func TestSaveCollectionViewWrapping(t *testing.T) { + viewName := "test_wrapping" + + scenarios := []struct { + name string + query string + expected string + }{ + { + "no wrapping - text field", + "select text as id, bool from demo1", + "CREATE VIEW `test_wrapping` AS SELECT * FROM (select text as id, bool from demo1)", + }, + { + "no wrapping - id field", + "select text as id, bool from demo1", + "CREATE VIEW `test_wrapping` AS SELECT * FROM (select text as id, bool from demo1)", + }, + { + "no wrapping - relation field", + "select rel_one as id, bool from demo1", + "CREATE VIEW `test_wrapping` AS SELECT * FROM (select rel_one as id, bool from demo1)", + }, + { + "no wrapping - select field", + "select select_many as id, bool from demo1", + "CREATE VIEW `test_wrapping` AS SELECT * FROM (select select_many as id, bool from demo1)", + }, + { + "no wrapping - email field", + "select email as id, bool from demo1", + "CREATE VIEW `test_wrapping` AS SELECT * FROM (select email as id, bool from demo1)", + }, + { + "no wrapping - datetime field", + "select datetime as id, bool from demo1", + "CREATE VIEW `test_wrapping` AS SELECT * FROM (select datetime as id, bool from demo1)", + }, + { + "no wrapping - url field", + "select url as id, bool from demo1", + "CREATE VIEW `test_wrapping` AS SELECT * FROM (select url as id, bool from demo1)", + }, + { + "wrapping - bool field", + "select bool as id, text as txt, url from demo1", + "CREATE VIEW `test_wrapping` AS SELECT * FROM (SELECT cast(id as text) id,txt,url FROM (select bool as id, text as txt, url from demo1))", + }, + { + "wrapping - bool field (different order)", + "select text as txt, url, bool as id from demo1", + "CREATE VIEW `test_wrapping` AS SELECT * FROM (SELECT txt,url,cast(id as text) id FROM (select text as txt, url, bool as id from demo1))", + }, + { + "wrapping - json field", + "select json as id, text, url from demo1", + "CREATE VIEW `test_wrapping` AS SELECT * FROM (SELECT cast(id as text) id,text,url FROM (select json as id, text, url from demo1))", + }, + { + "wrapping - numeric id", + "select 1 as id", + "CREATE VIEW `test_wrapping` AS SELECT * FROM (SELECT cast(id as text) id FROM (select 1 as id))", + }, + { + "wrapping - expresion", + "select ('test') as id", + "CREATE VIEW `test_wrapping` AS SELECT * FROM (SELECT cast(id as text) id FROM (select ('test') as id))", + }, + { + "no wrapping - cast as text", + "select cast('test' as text) as id", + "CREATE VIEW `test_wrapping` AS SELECT * FROM (select cast('test' as text) as id)", + }, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + app, _ := tests.NewTestApp() + defer app.Cleanup() + + collection := &models.Collection{ + Name: viewName, + Type: models.CollectionTypeView, + Options: types.JsonMap{ + "query": s.query, + }, + } + + err := app.Dao().SaveCollection(collection) + if err != nil { + t.Fatal(err) + } + + var sql string + + rowErr := app.Dao().DB().NewQuery("SELECT sql FROM sqlite_master WHERE type='view' AND name={:name}"). + Bind(dbx.Params{"name": viewName}). + Row(&sql) + if rowErr != nil { + t.Fatalf("Failed to retrieve view sql: %v", rowErr) + } + + if sql != s.expected { + t.Fatalf("Expected query \n%v, \ngot \n%v", s.expected, sql) + } + }) + } +} + func TestImportCollections(t *testing.T) { - totalCollections := 10 + totalCollections := 11 scenarios := []struct { name string diff --git a/daos/view.go b/daos/view.go index 421b4fab..2e38d3bc 100644 --- a/daos/view.go +++ b/daos/view.go @@ -235,6 +235,8 @@ func defaultViewField(name string) *schema.SchemaField { } } +var castRegex = regexp.MustCompile(`(?i)^cast\s*\(.*\s+as\s+(\w+)\s*\)$`) + func (dao *Dao) parseQueryToFields(selectQuery string) (map[string]*queryField, error) { p := new(identifiersParser) if err := p.parse(selectQuery); err != nil { @@ -257,15 +259,8 @@ func (dao *Dao) parseQueryToFields(selectQuery string) (map[string]*queryField, for _, col := range p.columns { colLower := strings.ToLower(col.original) - // numeric expression cast - if strings.Contains(colLower, "(") && - (strings.HasPrefix(colLower, "count(") || - strings.HasPrefix(colLower, "total(") || - strings.Contains(colLower, " as numeric") || - strings.Contains(colLower, " as real") || - strings.Contains(colLower, " as int") || - strings.Contains(colLower, " as integer") || - strings.Contains(colLower, " as decimal")) { + // numeric aggregations + if strings.HasPrefix(colLower, "count(") || strings.HasPrefix(colLower, "total(") { result[col.alias] = &queryField{ field: &schema.SchemaField{ Name: col.alias, @@ -275,6 +270,38 @@ func (dao *Dao) parseQueryToFields(selectQuery string) (map[string]*queryField, continue } + castMatch := castRegex.FindStringSubmatch(colLower) + + // numeric casts + if len(castMatch) == 2 { + switch castMatch[1] { + case "real", "integer", "int", "decimal", "numeric": + result[col.alias] = &queryField{ + field: &schema.SchemaField{ + Name: col.alias, + Type: schema.FieldTypeNumber, + }, + } + continue + case "text": + result[col.alias] = &queryField{ + field: &schema.SchemaField{ + Name: col.alias, + Type: schema.FieldTypeText, + }, + } + continue + case "boolean", "bool": + result[col.alias] = &queryField{ + field: &schema.SchemaField{ + Name: col.alias, + Type: schema.FieldTypeBool, + }, + } + continue + } + } + parts := strings.Split(col.original, ".") var fieldName string diff --git a/daos/view_test.go b/daos/view_test.go index a47fb016..f5997a94 100644 --- a/daos/view_test.go +++ b/daos/view_test.go @@ -330,7 +330,7 @@ func TestCreateViewSchema(t *testing.T) { }, }, { - "query with numeric casts", + "query with casts", `select a.id, count(a.id) count, @@ -339,6 +339,9 @@ func TestCreateViewSchema(t *testing.T) { cast(a.id as real) cast_real, cast(a.id as decimal) cast_decimal, cast(a.id as numeric) cast_numeric, + cast(a.id as text) cast_text, + cast(a.id as bool) cast_bool, + cast(a.id as boolean) cast_boolean, avg(a.id) avg, sum(a.id) sum, total(a.id) total, @@ -354,6 +357,9 @@ func TestCreateViewSchema(t *testing.T) { "cast_real": schema.FieldTypeNumber, "cast_decimal": schema.FieldTypeNumber, "cast_numeric": schema.FieldTypeNumber, + "cast_text": schema.FieldTypeText, + "cast_bool": schema.FieldTypeBool, + "cast_boolean": schema.FieldTypeBool, // json because they are nullable "sum": schema.FieldTypeJson, "avg": schema.FieldTypeJson, diff --git a/forms/collections_import_test.go b/forms/collections_import_test.go index f426856d..0d740e70 100644 --- a/forms/collections_import_test.go +++ b/forms/collections_import_test.go @@ -38,7 +38,7 @@ func TestCollectionsImportValidate(t *testing.T) { } func TestCollectionsImportSubmit(t *testing.T) { - totalCollections := 10 + totalCollections := 11 scenarios := []struct { name string diff --git a/migrations/1691747912_resave_views.go b/migrations/1691747912_resave_views.go new file mode 100644 index 00000000..121cbbf8 --- /dev/null +++ b/migrations/1691747912_resave_views.go @@ -0,0 +1,28 @@ +package migrations + +import ( + "github.com/pocketbase/dbx" + "github.com/pocketbase/pocketbase/daos" + "github.com/pocketbase/pocketbase/models" +) + +// Resave all view collections to ensure that the proper id normalization is applied. +// (see https://github.com/pocketbase/pocketbase/issues/3110) +func init() { + AppMigrations.Register(func(db dbx.Builder) error { + dao := daos.New(db) + + collections, err := dao.FindCollectionsByType(models.CollectionTypeView) + if err != nil { + return nil + } + + for _, collection := range collections { + // ignore errors to allow users to adjust + // the view queries after app start + dao.Save(collection) + } + + return nil + }, nil) +} diff --git a/tests/data/data.db b/tests/data/data.db index 6b55e636ea826849dd372ddcc176b5195b2fa45d..72a25fbfc6f5b708351f292b1fff6f1f17ab50ec 100644 GIT binary patch delta 2055 zcmbVNZ)g)|9M0X_HK%FrZLGDmZhC9Av_|7yE_auUx>{=lskKtO7SwXRCYSV@f6e9c z$8=k2JJD{!I`ZKtGXKrNHs-Ka&0sL>#{42o#$YfI_F?RU-DIDJFl6p7?betTCxg7) zd++-^_dd_>`Q>i>A{@U6r#ILdO(xTBeeBW)qW==_ZljUL*}*Gtb9L1N6Z{!Y;oa6x ztT}v}^*1;GoAE#K@9|HqUDgd%5Af4-RP}sSj$9A=D=C(zI2Xq;oRA##i>l-oqREJ) z$UZ^#3n^JjJ4OB6|3HW1OTZl2+IH|G` zAm6=_$(dfL*wO&n|F+6gPW?#fW5`2nOEO9AtV(yiFObrl?gGm?TqG?W06)~C! z#FU7xG%TueESeD1*i(9kr)TzVT?0CcG@9aRmNjfis-9msETqJ6QYv~g7t1xp=BGA| zZLYSSF~J3R9zTmC*86xI-vzU<6WXB#{{^47UbmiscjqW#(AwJy`jO{qj`bzTXE#NC z{{Hr3>o7f8-UTB1)60^5(bfQXV|j%{tMg|aj@)XU%d%|giBC6GSx?n%0v$^(Rt$7q z$^J?~)|T$Gl=QCv*E)b`9PpgM)ii<5BK#SFt;wI?(b{Czoo)o}OX9f|nzmtXVMoK^ z1`FI`g6}~QK7>3>!mr^J?1DeQS-54+w#N+D)&M(_6^9cF?{`Mzj5Cl5S@QflcXi?^ z6Z}f2{iMV1!K<(UPr*mQPZM9|kR!G*&P# zn2(_%a$A43>_b{Aj+%ZnMr~6QHVKSw7gQ-SDsX|$!)(b^6MI07y|_9^=!8oKVZyvj zQ1@pK^(_*L(;}h5m}<9rKp=P0eb8KMw__7_gHtBA?6#xF+urLX`VM&eiNUhHL8AQ# zam0Jj+tEvGCHD7pcM&a1w>~kUw#a@$Oc1J+QCkVwzfFpWa#)A$G;j$iEcsNTrKh`J zIP_}QUT;rJE79F&Tv`p5jwcog0VNh8d|_FNs)^RZh9lqrf^-$6USLMi$_HAs4~$i; zx?@tbhkYOlPF8hlzxIK%g@u05P`?@=Z3uXRwBhr5jVVNHi-Qhq$j@XlQO2#k6$d`d z9~l{@#o&l`{04dh%ld|7GDv0%vkLGaHurXlA}G>BIz1HUpcz_woB;c=Sim_f$w9yN z?KSjGEa{I0hC-=~HZY40VRA4Tlf%Au&7tIp za99Z#Le<>BSYtdXDJL2|jsGw9#@C&slip4iHIux@MR`cl!SmeFlJ(M*bT0#jW>9(A MgcnwggGb$e0k)!kp8x;= delta 1137 zcmaKqZAep57{|}KXY)>TI~rwTzO0Na%z5wboV&}=kfjl!=38oYmATS}N988(6y@&TI)h>WP<6%va`hnUNZq3BzRjEZ+Qd4w81Z&LZ zd6Ut^Fyi&3__&ob_4D2)%~f#NpO_evp3i0k5hm}Wl|Rkjm*0BCqU0y=#Gm~ zYh=KMicW>m;)qV?`m%-KC_D$|*YnMCBT0ULEe|JFMe?Cw9+bMlDUIiB;KCXZPG|(V zpdgTkm8m3oS|Hy_q(U6eJP7xTPg3?kYQ~qDP#hnHd4TWWI=qZ|+>M{(VSEyM@HBCl zghl?wOG3B-en#>oQh$vfVi~XCyVy;L9!&3WTx#yKqOCY?snphP zs7tdYC@zgH?By#0)xXCuw7)L>DnaMv#8OlU;R)GRhG>eSd!^P2