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 6b55e636..72a25fbf 100644 Binary files a/tests/data/data.db and b/tests/data/data.db differ