From 5f763d334cc1c9a673b6f7b84b832d377064b119 Mon Sep 17 00:00:00 2001 From: Ben Visness Date: Mon, 3 May 2021 09:51:07 -0500 Subject: [PATCH] Start forum category index; fix reflection bugs --- src/db/db.go | 105 +++++++++-- src/db/db_test.go | 53 ++++++ src/templates/src/forum_category.html | 9 + src/templates/src/include/post_list_item.html | 2 +- .../src/include/thread_list_item.html | 36 ++++ src/templates/types.go | 28 ++- src/website/forums.go | 166 ++++++++++++------ src/website/routes.go | 4 +- src/website/urls.go | 14 ++ 9 files changed, 338 insertions(+), 79 deletions(-) create mode 100644 src/db/db_test.go create mode 100644 src/templates/src/forum_category.html create mode 100644 src/templates/src/include/thread_list_item.html diff --git a/src/db/db.go b/src/db/db.go index d24316b0..c5e043b0 100644 --- a/src/db/db.go +++ b/src/db/db.go @@ -3,10 +3,12 @@ package db import ( "context" "errors" + "fmt" "reflect" "strings" "git.handmade.network/hmn/hmn/src/config" + "git.handmade.network/hmn/hmn/src/logging" "git.handmade.network/hmn/hmn/src/oops" "github.com/jackc/pgtype" "github.com/jackc/pgx/v4" @@ -15,6 +17,40 @@ import ( "github.com/rs/zerolog/log" ) +/* +Values of these kinds are ok to query even if they are not directly understood by pgtype. +This is common for custom types like: + + type CategoryKind int +*/ +var queryableKinds = []reflect.Kind{ + reflect.Int, +} + +/* +Checks if we are able to handle a particular type in a database query. This applies only to +primitive types and not structs, since the database only returns individual primitive types +and it is our job to stitch them back together into structs later. +*/ +func typeIsQueryable(t reflect.Type) bool { + _, isRecognizedByPgtype := connInfo.DataTypeForValue(reflect.New(t).Elem().Interface()) // if pgtype recognizes it, we don't need to dig in further for more `db` tags + // NOTE: boy it would be nice if we didn't have to do reflect.New here, considering that pgtype is just doing reflection on the value anyway + + if isRecognizedByPgtype { + return true + } + + // pgtype doesn't recognize it, but maybe it's a primitive type we can deal with + k := t.Kind() + for _, qk := range queryableKinds { + if k == qk { + return true + } + } + + return false +} + var connInfo = pgtype.NewConnInfo() func NewConn() *pgx.Conn { @@ -61,12 +97,35 @@ func (it *StructQueryIterator) Next() (interface{}, bool) { panic(err) } + // Better logging of panics in this confusing reflection process + var currentField reflect.StructField + var currentValue reflect.Value + defer func() { + if r := recover(); r != nil { + if currentValue.IsValid() { + logging.Error(). + Str("field name", currentField.Name). + Stringer("field type", currentField.Type). + Interface("value", currentValue.Interface()). + Stringer("value type", currentValue.Type()). + Msg("panic in iterator") + } + + if currentField.Name != "" { + panic(fmt.Errorf("panic while processing field '%s': %v", currentField.Name, r)) + } else { + panic(r) + } + } + }() + for i, val := range vals { if val == nil { continue } - field := followPathThroughStructs(result, it.fieldPaths[i]) + var field reflect.Value + field, currentField = followPathThroughStructs(result, it.fieldPaths[i]) if field.Kind() == reflect.Ptr { field.Set(reflect.New(field.Type().Elem())) field = field.Elem() @@ -78,6 +137,7 @@ func (it *StructQueryIterator) Next() (interface{}, bool) { if valReflected.Kind() == reflect.Ptr { valReflected = valReflected.Elem() } + currentValue = valReflected switch field.Kind() { case reflect.Int: @@ -85,6 +145,9 @@ func (it *StructQueryIterator) Next() (interface{}, bool) { default: field.Set(valReflected) } + + currentField = reflect.StructField{} + currentValue = reflect.Value{} } return result.Interface(), true @@ -111,22 +174,35 @@ func (it *StructQueryIterator) ToSlice() []interface{} { return result } -func followPathThroughStructs(structVal reflect.Value, path []int) reflect.Value { +func followPathThroughStructs(structPtrVal reflect.Value, path []int) (reflect.Value, reflect.StructField) { if len(path) < 1 { - panic("can't follow an empty path") + panic(oops.New(nil, "can't follow an empty path")) } - val := structVal + if structPtrVal.Kind() != reflect.Ptr || structPtrVal.Elem().Kind() != reflect.Struct { + panic(oops.New(nil, "structPtrVal must be a pointer to a struct; got value of type %s", structPtrVal.Type())) + } + + // more informative panic recovery + var field reflect.StructField + defer func() { + if r := recover(); r != nil { + panic(oops.New(nil, "panic at field '%s': %v", field.Name, r)) + } + }() + + val := structPtrVal for _, i := range path { if val.Kind() == reflect.Ptr && val.Type().Elem().Kind() == reflect.Struct { if val.IsNil() { - val.Set(reflect.New(val.Type())) + val.Set(reflect.New(val.Type().Elem())) } val = val.Elem() } + field = val.Type().Field(i) val = val.Field(i) } - return val + return val, field } func Query(ctx context.Context, conn *pgxpool.Pool, destExample interface{}, query string, args ...interface{}) (*StructQueryIterator, error) { @@ -154,7 +230,7 @@ func Query(ctx context.Context, conn *pgxpool.Pool, destExample interface{}, que }, nil } -func getColumnNamesAndPaths(destType reflect.Type, pathSoFar []int, prefix string) ([]string, [][]int, error) { +func getColumnNamesAndPaths(destType reflect.Type, pathSoFar []int, prefix string) (names []string, paths [][]int, err error) { var columnNames []string var fieldPaths [][]int @@ -172,14 +248,14 @@ func getColumnNamesAndPaths(destType reflect.Type, pathSoFar []int, prefix strin if columnName := field.Tag.Get("db"); columnName != "" { fieldType := field.Type - if destType.Kind() == reflect.Ptr { - fieldType = destType.Elem() + if fieldType.Kind() == reflect.Ptr { + fieldType = fieldType.Elem() } - _, isRecognizedByPgtype := connInfo.DataTypeForValue(reflect.New(fieldType).Elem().Interface()) // if pgtype recognizes it, we don't need to dig in further for more `db` tags - // NOTE: boy it would be nice if we didn't have to do reflect.New here, considering that pgtype is just doing reflection on the value anyway - - if fieldType.Kind() == reflect.Struct && !isRecognizedByPgtype { + if typeIsQueryable(fieldType) { + columnNames = append(columnNames, prefix+columnName) + fieldPaths = append(fieldPaths, path) + } else if fieldType.Kind() == reflect.Struct { subCols, subPaths, err := getColumnNamesAndPaths(fieldType, path, columnName+".") if err != nil { return nil, nil, err @@ -187,8 +263,7 @@ func getColumnNamesAndPaths(destType reflect.Type, pathSoFar []int, prefix strin columnNames = append(columnNames, subCols...) fieldPaths = append(fieldPaths, subPaths...) } else { - columnNames = append(columnNames, prefix+columnName) - fieldPaths = append(fieldPaths, path) + return nil, nil, oops.New(nil, "field '%s' in type %s has invalid type '%s'", field.Name, destType, field.Type) } } } diff --git a/src/db/db_test.go b/src/db/db_test.go new file mode 100644 index 00000000..a969a47e --- /dev/null +++ b/src/db/db_test.go @@ -0,0 +1,53 @@ +package db + +import ( + "reflect" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestPaths(t *testing.T) { + type CustomInt int + type S struct { + I int `db:"I"` + PI *int `db:"PI"` + CI CustomInt `db:"CI"` + PCI *CustomInt `db:"PCI"` + B bool `db:"B"` + PB *bool `db:"PB"` + + NoTag int + } + type Nested struct { + S S `db:"S"` + PS *S `db:"PS"` + + NoTag S + } + + names, paths, err := getColumnNamesAndPaths(reflect.TypeOf(Nested{}), nil, "") + if assert.Nil(t, err) { + assert.Equal(t, []string{ + "S.I", "S.PI", + "S.CI", "S.PCI", + "S.B", "S.PB", + "PS.I", "PS.PI", + "PS.CI", "PS.PCI", + "PS.B", "PS.PB", + }, names) + assert.Equal(t, [][]int{ + {0, 0}, {0, 1}, {0, 2}, {0, 3}, {0, 4}, {0, 5}, + {1, 0}, {1, 1}, {1, 2}, {1, 3}, {1, 4}, {1, 5}, + }, paths) + assert.True(t, len(names) == len(paths)) + } + + testStruct := Nested{} + for i, path := range paths { + val, field := followPathThroughStructs(reflect.ValueOf(&testStruct), path) + assert.True(t, val.IsValid()) + assert.True(t, strings.Contains(names[i], field.Name)) + } +} diff --git a/src/templates/src/forum_category.html b/src/templates/src/forum_category.html new file mode 100644 index 00000000..d658e02c --- /dev/null +++ b/src/templates/src/forum_category.html @@ -0,0 +1,9 @@ +{{ template "base.html" . }} + +{{ define "content" }} +
+ {{ range .Threads }} + {{ template "thread_list_item.html" . }} + {{ end }} +
+{{ end }} diff --git a/src/templates/src/include/post_list_item.html b/src/templates/src/include/post_list_item.html index 407a53a7..5a9fda07 100644 --- a/src/templates/src/include/post_list_item.html +++ b/src/templates/src/include/post_list_item.html @@ -1,5 +1,5 @@ {{/* -This template is intended to display a single post or thread in the context of a forum, the feed, or a similar layout. +This template is intended to display a single post in the context of a forum, the feed, or a similar layout. It should be called with PostListItem. */}} diff --git a/src/templates/src/include/thread_list_item.html b/src/templates/src/include/thread_list_item.html new file mode 100644 index 00000000..801a1298 --- /dev/null +++ b/src/templates/src/include/thread_list_item.html @@ -0,0 +1,36 @@ +{{/* +This template is intended to display a single thread in the context of a forum, the feed, or a similar layout. + +It should be called with ThreadListItem. +*/}} + +
+ +
+
+ {{ range $i, $breadcrumb := .Breadcrumbs }} + {{ if gt $i 0 }} ยป {{ end }} + {{ $breadcrumb.Name }} + {{ end }} +
+
{{ .Title }}
+
+ {{ .FirstUser.Name }} — {{ relativedate .FirstDate }} +
+ {{ with .Content }} +
+ {{ . }} +
+ {{ end }} +
+
+ +
+
Last post {{ relativedate .LastDate }}
+ {{ .LastUser.Name }} +
+
+
+ » +
+
\ No newline at end of file diff --git a/src/templates/types.go b/src/templates/types.go index 5f996212..65c20da1 100644 --- a/src/templates/types.go +++ b/src/templates/types.go @@ -83,11 +83,29 @@ type PostListItem struct { Title string Url string Breadcrumbs []Breadcrumb - User User - Date time.Time - Unread bool - Classes string - Content string + + User User + Date time.Time + + Unread bool + Classes string + Content string +} + +// Data from thread_list_item.html +type ThreadListItem struct { + Title string + Url string + Breadcrumbs []Breadcrumb + + FirstUser User + FirstDate time.Time + LastUser User + LastDate time.Time + + Unread bool + Classes string + Content string } type Breadcrumb struct { diff --git a/src/website/forums.go b/src/website/forums.go index 27d5ad08..26a2956f 100644 --- a/src/website/forums.go +++ b/src/website/forums.go @@ -1,46 +1,35 @@ package website import ( - "fmt" + "context" "math" "net/http" "strconv" "strings" "time" + "git.handmade.network/hmn/hmn/src/templates" + + "github.com/jackc/pgx/v4/pgxpool" + "git.handmade.network/hmn/hmn/src/db" "git.handmade.network/hmn/hmn/src/models" "git.handmade.network/hmn/hmn/src/oops" ) +type forumCategoryData struct { + templates.BaseData + + Threads []templates.ThreadListItem +} + func ForumCategory(c *RequestContext) ResponseData { const threadsPerPage = 25 catPath := c.PathParams["cats"] catSlugs := strings.Split(catPath, "/") - - catSlug := catSlugs[len(catSlugs)-1] - if len(catSlugs) == 1 { - catSlug = "" - } - - // TODO: Is this query right? Do we need to do a better special case for when it's the root category? - currentCatId, err := db.QueryInt(c.Context(), c.Conn, - ` - SELECT id - FROM handmade_category - WHERE - slug = $1 - AND kind = $2 - AND project_id = $3 - `, - catSlug, - models.CatKindForum, - c.CurrentProject.ID, - ) - if err != nil { - panic(oops.New(err, "failed to get current category id")) - } + currentCatId := fetchCatIdFromSlugs(c.Context(), c.Conn, catSlugs, c.CurrentProject.ID) + categoryUrls := GetProjectCategoryUrls(c.Context(), c.Conn, c.CurrentProject.ID) numThreads, err := db.QueryInt(c.Context(), c.Conn, ` @@ -82,6 +71,8 @@ func ForumCategory(c *RequestContext) ResponseData { Thread models.Thread `db:"thread"` FirstPost models.Post `db:"firstpost"` LastPost models.Post `db:"lastpost"` + FirstUser *models.User `db:"firstuser"` + LastUser *models.User `db:"lastuser"` ThreadLastReadTime *time.Time `db:"tlri.lastread"` CatLastReadTime *time.Time `db:"clri.lastread"` } @@ -92,15 +83,16 @@ func ForumCategory(c *RequestContext) ResponseData { handmade_thread AS thread JOIN handmade_post AS firstpost ON thread.first_id = firstpost.id JOIN handmade_post AS lastpost ON thread.last_id = lastpost.id - LEFT OUTER JOIN handmade_threadlastreadinfo AS tlri ON ( + LEFT JOIN auth_user AS firstuser ON firstpost.author_id = firstuser.id + LEFT JOIN auth_user AS lastuser ON lastpost.author_id = lastuser.id + LEFT JOIN handmade_threadlastreadinfo AS tlri ON ( tlri.thread_id = thread.id AND tlri.user_id = $2 ) - LEFT OUTER JOIN handmade_categorylastreadinfo AS clri ON ( + LEFT JOIN handmade_categorylastreadinfo AS clri ON ( clri.category_id = $1 AND clri.user_id = $2 ) - -- LEFT OUTER JOIN auth_user ON post.author_id = auth_user.id WHERE thread.category_id = $1 AND NOT thread.deleted @@ -115,50 +107,112 @@ func ForumCategory(c *RequestContext) ResponseData { if err != nil { panic(oops.New(err, "failed to fetch threads")) } + defer itMainThreads.Close() - var res ResponseData - + var threads []templates.ThreadListItem for _, irow := range itMainThreads.ToSlice() { row := irow.(*mainPostsQueryResult) - res.Write([]byte(fmt.Sprintf("%s\n", row.Thread.Title))) + + threads = append(threads, templates.ThreadListItem{ + Title: row.Thread.Title, + Url: ThreadUrl(row.Thread, models.CatKindForum, categoryUrls[currentCatId]), + + FirstUser: templates.UserToTemplate(row.FirstUser), + FirstDate: row.FirstPost.PostDate, + LastUser: templates.UserToTemplate(row.LastUser), + LastDate: row.LastPost.PostDate, + }) } // --------------------- // Subcategory things // --------------------- - c.Perf.StartBlock("SQL", "Fetch subcategories") - type queryResult struct { - Cat models.Category `db:"cat"` - } - itSubcats, err := db.Query(c.Context(), c.Conn, queryResult{}, - ` - WITH current AS ( + //c.Perf.StartBlock("SQL", "Fetch subcategories") + //type queryResult struct { + // Cat models.Category `db:"cat"` + //} + //itSubcats, err := db.Query(c.Context(), c.Conn, queryResult{}, + // ` + // WITH current AS ( + // SELECT id + // FROM handmade_category + // WHERE + // slug = $1 + // AND kind = $2 + // AND project_id = $3 + // ) + // SELECT $columns + // FROM + // handmade_category AS cat, + // current + // WHERE + // cat.id = current.id + // OR cat.parent_id = current.id + // `, + // catSlug, + // models.CatKindForum, + // c.CurrentProject.ID, + //) + //if err != nil { + // panic(oops.New(err, "failed to fetch subcategories")) + //} + //c.Perf.EndBlock() + + //_ = itSubcats // TODO: Actually query subcategory post data + + baseData := getBaseData(c) + baseData.Title = "yeet" + + var res ResponseData + res.WriteTemplate("forum_category.html", forumCategoryData{ + BaseData: baseData, + Threads: threads, + }, c.Perf) + + return res +} + +func fetchCatIdFromSlugs(ctx context.Context, conn *pgxpool.Pool, catSlugs []string, projectId int) int { + if len(catSlugs) == 1 { + var err error + currentCatId, err := db.QueryInt(ctx, conn, + ` + SELECT cat.id + FROM + handmade_category AS cat + JOIN handmade_project AS proj ON proj.forum_id = cat.id + WHERE + proj.id = $1 + AND cat.kind = $2 + `, + projectId, + models.CatKindForum, + ) + if err != nil { + panic(oops.New(err, "failed to get root category id")) + } + + return currentCatId + } else { + var err error + currentCatId, err := db.QueryInt(ctx, conn, + ` SELECT id FROM handmade_category WHERE slug = $1 AND kind = $2 AND project_id = $3 + `, + catSlugs[len(catSlugs)-1], + models.CatKindForum, + projectId, ) - SELECT $columns - FROM - handmade_category AS cat, - current - WHERE - cat.id = current.id - OR cat.parent_id = current.id - `, - catSlug, - models.CatKindForum, - c.CurrentProject.ID, - ) - if err != nil { - panic(oops.New(err, "failed to fetch subcategories")) + if err != nil { + panic(oops.New(err, "failed to get current category id")) + } + + return currentCatId } - c.Perf.EndBlock() - - _ = itSubcats // TODO: Actually query subcategory post data - - return res } diff --git a/src/website/routes.go b/src/website/routes.go index 49f26cf5..83ce7dd7 100644 --- a/src/website/routes.go +++ b/src/website/routes.go @@ -103,8 +103,8 @@ func getBaseData(c *RequestContext) templates.BaseData { func FetchProjectBySlug(ctx context.Context, conn *pgxpool.Pool, slug string) (*models.Project, error) { subdomainProjectRow, err := db.QueryOne(ctx, conn, models.Project{}, "SELECT $columns FROM handmade_project WHERE slug = $1", slug) if err == nil { - subdomainProject := subdomainProjectRow.(models.Project) - return &subdomainProject, nil + subdomainProject := subdomainProjectRow.(*models.Project) + return subdomainProject, nil } else if !errors.Is(err, db.ErrNoMatchingRows) { return nil, oops.New(err, "failed to get projects by slug") } diff --git a/src/website/urls.go b/src/website/urls.go index 984f9977..152c0215 100644 --- a/src/website/urls.go +++ b/src/website/urls.go @@ -124,3 +124,17 @@ func PostUrl(post models.Post, catKind models.CategoryKind, categoryUrl string) return "" } + +func ThreadUrl(thread models.Thread, catKind models.CategoryKind, categoryUrl string) string { + categoryUrl = strings.TrimRight(categoryUrl, "/") + + switch catKind { + // TODO: All the relevant post types. Maybe it doesn't make sense to lump them all together here. + case models.CatKindBlog: + return fmt.Sprintf("%s/p/%d", categoryUrl, thread.ID) + case models.CatKindForum: + return fmt.Sprintf("%s/t/%d", categoryUrl, thread.ID) + } + + return "" +}