From 857d476ff051b5c91afd23313835a7b6e7bd201d Mon Sep 17 00:00:00 2001 From: Azareal Date: Sat, 18 May 2019 20:31:51 +1000 Subject: [PATCH] ForumStore.Create now rejects blank names. ForumStore.Get should now respond appropriately when a forum has been deleted. Removed a bit of boilerplate in the ForumStore tests. Added forumstore tests for forum deletion and trying to create a forum with a blank name. --- common/forum_store.go | 50 +++++++++++++++++++++++++++---------------- misc_test.go | 39 ++++++++++++++++++++++----------- 2 files changed, 58 insertions(+), 31 deletions(-) diff --git a/common/forum_store.go b/common/forum_store.go index 476f0110..2e2f5da3 100644 --- a/common/forum_store.go +++ b/common/forum_store.go @@ -1,7 +1,7 @@ /* * * Gosora Forum Store -* Copyright Azareal 2017 - 2019 +* Copyright Azareal 2017 - 2020 * */ package common @@ -20,6 +20,7 @@ import ( var forumCreateMutex sync.Mutex var forumPerms map[int]map[int]*ForumPerms // [gid][fid]*ForumPerms // TODO: Add an abstraction around this and make it more thread-safe var Forums ForumStore +var ErrBlankName = errors.New("The name must not be blank") // ForumStore is an interface for accessing the forums and the metadata stored on them type ForumStore interface { @@ -67,7 +68,7 @@ type MemoryForumStore struct { updateCache *sql.Stmt addTopics *sql.Stmt removeTopics *sql.Stmt - updateOrder *sql.Stmt + updateOrder *sql.Stmt } // NewMemoryForumStore gives you a new instance of MemoryForumStore @@ -83,7 +84,7 @@ func NewMemoryForumStore() (*MemoryForumStore, error) { updateCache: acc.Update("forums").Set("lastTopicID = ?, lastReplyerID = ?").Where("fid = ?").Prepare(), addTopics: acc.Update("forums").Set("topicCount = topicCount + ?").Where("fid = ?").Prepare(), removeTopics: acc.Update("forums").Set("topicCount = topicCount - ?").Where("fid = ?").Prepare(), - updateOrder: acc.Update("forums").Set("order = ?").Where("fid = ?").Prepare(), + updateOrder: acc.Update("forums").Set("order = ?").Where("fid = ?").Prepare(), }, acc.FirstError() } @@ -164,21 +165,28 @@ func (mfs *MemoryForumStore) CacheGet(id int) (*Forum, error) { func (mfs *MemoryForumStore) Get(id int) (*Forum, error) { fint, ok := mfs.forums.Load(id) - if !ok || fint.(*Forum).Name == "" { - var forum = &Forum{ID: id} - err := mfs.get.QueryRow(id).Scan(&forum.Name, &forum.Desc, &forum.Active, &forum.Order, &forum.Preset, &forum.ParentID, &forum.ParentType, &forum.TopicCount, &forum.LastTopicID, &forum.LastReplyerID) - if err != nil { - return forum, err + if ok { + forum := fint.(*Forum) + if forum.Name == "" { + return nil, ErrNoRows } + return forum, nil + } - forum.Link = BuildForumURL(NameToSlug(forum.Name), forum.ID) - forum.LastTopic = Topics.DirtyGet(forum.LastTopicID) - forum.LastReplyer = Users.DirtyGet(forum.LastReplyerID) - - mfs.CacheSet(forum) + var forum = &Forum{ID: id} + err := mfs.get.QueryRow(id).Scan(&forum.Name, &forum.Desc, &forum.Active, &forum.Order, &forum.Preset, &forum.ParentID, &forum.ParentType, &forum.TopicCount, &forum.LastTopicID, &forum.LastReplyerID) + if err != nil { return forum, err } - return fint.(*Forum), nil + if forum.Name == "" { + return nil, ErrNoRows + } + forum.Link = BuildForumURL(NameToSlug(forum.Name), forum.ID) + forum.LastTopic = Topics.DirtyGet(forum.LastTopicID) + forum.LastReplyer = Users.DirtyGet(forum.LastReplyerID) + + mfs.CacheSet(forum) + return forum, err } func (mfs *MemoryForumStore) BypassGet(id int) (*Forum, error) { @@ -274,7 +282,10 @@ func (mfs *MemoryForumStore) GetFirstChild(parentID int, parentType string) (*Fo // TODO: Add a query for this rather than hitting cache func (mfs *MemoryForumStore) Exists(id int) bool { forum, ok := mfs.forums.Load(id) - return ok && forum.(*Forum).Name != "" + if !ok { + return false + } + return forum.(*Forum).Name != "" } // TODO: Batch deletions with name blanking? Is this necessary? @@ -284,12 +295,12 @@ func (mfs *MemoryForumStore) CacheDelete(id int) { } // TODO: Add a hook to allow plugin_guilds to detect when one of it's forums has just been deleted? -func (mfs *MemoryForumStore) Delete(id int) error { +func (s *MemoryForumStore) Delete(id int) error { if id == ReportForumID { return errors.New("You cannot delete the Reports forum") } - _, err := mfs.delete.Exec(id) - mfs.CacheDelete(id) + _, err := s.delete.Exec(id) + s.CacheDelete(id) return err } @@ -329,6 +340,9 @@ func (mfs *MemoryForumStore) UpdateLastTopic(tid int, uid int, fid int) error { } func (mfs *MemoryForumStore) Create(forumName string, forumDesc string, active bool, preset string) (int, error) { + if forumName == "" { + return 0, ErrBlankName + } forumCreateMutex.Lock() res, err := mfs.create.Exec(forumName, forumDesc, active, preset) if err != nil { diff --git a/misc_test.go b/misc_test.go index cf6b90c1..fd53c3de 100644 --- a/misc_test.go +++ b/misc_test.go @@ -537,6 +537,9 @@ func TestForumStore(t *testing.T) { c.InitPlugins() } + expect(t, c.Forums.GlobalCount() == 2, "The forumstore global count should be 2") + //expect(t, c.Forums.Length() == 2, "The forumstore length should be 2") + _, err := c.Forums.Get(-1) recordMustNotExist(t, err, "FID #-1 shouldn't exist") _, err = c.Forums.Get(0) @@ -560,22 +563,22 @@ func TestForumStore(t *testing.T) { expectDesc = "A place for general discussions which don't fit elsewhere" expect(t, forum.Desc == expectDesc, fmt.Sprintf("The forum description should be '%s' not '%s'", expectDesc, forum.Desc)) - ok := c.Forums.Exists(-1) - expect(t, !ok, "FID #-1 shouldn't exist") - ok = c.Forums.Exists(0) - expect(t, !ok, "FID #0 shouldn't exist") - ok = c.Forums.Exists(1) - expect(t, ok, "FID #1 should exist") - ok = c.Forums.Exists(2) - expect(t, ok, "FID #2 should exist") - ok = c.Forums.Exists(3) - expect(t, !ok, "FID #3 shouldn't exist") + expect(t, !c.Forums.Exists(-1), "FID #-1 shouldn't exist") + expect(t, !c.Forums.Exists(0), "FID #0 shouldn't exist") + expect(t, c.Forums.Exists(1), "FID #1 should exist") + expect(t, c.Forums.Exists(2), "FID #2 should exist") + expect(t, !c.Forums.Exists(3), "FID #3 shouldn't exist") + + _, err = c.Forums.Create("", "", true, "all") + expect(t, err != nil, "A forum shouldn't be successfully created, if it has a blank name") fid, err := c.Forums.Create("Test Forum", "", true, "all") expectNilErr(t, err) expect(t, fid == 3, "The first forum we create should have an ID of 3") - ok = c.Forums.Exists(3) - expect(t, ok, "FID #2 should exist") + expect(t, c.Forums.Exists(3), "FID #2 should exist") + + expect(t, c.Forums.GlobalCount() == 3, "The forumstore global count should be 3") + //expect(t, c.Forums.Length() == 3, "The forumstore length should be 3") forum, err = c.Forums.Get(3) recordMustExist(t, err, "Couldn't find FID #3") @@ -586,8 +589,18 @@ func TestForumStore(t *testing.T) { expect(t, forum.Desc == "", fmt.Sprintf("The forum description should be blank not '%s'", forum.Desc)) // TODO: More forum creation tests - // TODO: Test forum deletion + + expectNilErr(t, c.Forums.Delete(3)) + expect(t, forum.ID == 3, fmt.Sprintf("forum pointer shenanigans")) + expect(t, c.Forums.GlobalCount() == 2, "The forumstore global count should be 2") + //expect(t, c.Forums.Length() == 2, "The forumstore length should be 2") + expect(t, !c.Forums.Exists(3), "FID #3 shouldn't exist after being deleted") + _, err = c.Forums.Get(3) + recordMustNotExist(t, err, "FID #3 shouldn't exist after being deleted") + + // TODO: Test deleting the reports forum // TODO: Test forum update + // TODO: Other forumstore stuff and forumcache? } // TODO: Implement this