From 1766d5decdcab5794fdb3f0a37e6af438a36e14e Mon Sep 17 00:00:00 2001 From: Azareal Date: Wed, 19 Feb 2020 20:32:26 +1000 Subject: [PATCH] more attachment tests Add UpdateLinked method to AttachmentStore. --- common/attachments.go | 46 +++++- common/page_store.go | 5 +- docs/configuration.md | 2 +- misc_test.go | 325 ++++++++++++++++++++++++------------------ routes/attachments.go | 22 +-- routes/reply.go | 3 - routes/topic.go | 6 +- 7 files changed, 241 insertions(+), 168 deletions(-) diff --git a/common/attachments.go b/common/attachments.go index 27acce02..c93b10b5 100644 --- a/common/attachments.go +++ b/common/attachments.go @@ -3,6 +3,8 @@ package common import ( "database/sql" "errors" + + //"fmt" "os" "strings" @@ -34,6 +36,8 @@ type AttachmentStore interface { CountIn(originTable string, oid int) int CountInPath(path string) int Delete(id int) error + + UpdateLinked(otable string, oid int) (err error) } type DefaultAttachmentStore struct { @@ -46,20 +50,27 @@ type DefaultAttachmentStore struct { move *sql.Stmt moveByExtra *sql.Stmt delete *sql.Stmt + + replyUpdateAttachs *sql.Stmt + topicUpdateAttachs *sql.Stmt } func NewDefaultAttachmentStore(acc *qgen.Accumulator) (*DefaultAttachmentStore, error) { a := "attachments" return &DefaultAttachmentStore{ get: acc.Select(a).Columns("originID, sectionID, uploadedBy, path, extra").Where("attachID=?").Prepare(), - getByObj: acc.Select(a).Columns("attachID, sectionID, uploadedBy, path, extra").Where("originTable = ? AND originID = ?").Prepare(), + getByObj: acc.Select(a).Columns("attachID, sectionID, uploadedBy, path, extra").Where("originTable=? AND originID=?").Prepare(), add: acc.Insert(a).Columns("sectionID, sectionTable, originID, originTable, uploadedBy, path, extra").Fields("?,?,?,?,?,?,?").Prepare(), count: acc.Count(a).Prepare(), countIn: acc.Count(a).Where("originTable=? and originID=?").Prepare(), - countInPath: acc.Count(a).Where("path = ?").Prepare(), + countInPath: acc.Count(a).Where("path=?").Prepare(), move: acc.Update(a).Set("sectionID=?").Where("originID=? AND originTable=?").Prepare(), moveByExtra: acc.Update(a).Set("sectionID=?").Where("originTable=? AND extra=?").Prepare(), delete: acc.Delete(a).Where("attachID=?").Prepare(), + + // TODO: Less race-y attachment count updates + replyUpdateAttachs: acc.Update("replies").Set("attachCount=?").Where("rid=?").Prepare(), + topicUpdateAttachs: acc.Update("topics").Set("attachCount=?").Where("tid=?").Prepare(), }, acc.FirstError() } @@ -80,7 +91,14 @@ func (s *DefaultAttachmentStore) MiniGetList(originTable string, originID int) ( a.Image = ImageFileExts.Contains(a.Ext) alist = append(alist, a) } - return alist, rows.Err() + err = rows.Err() + if err != nil { + return nil, err + } + if len(alist) == 0 { + err = sql.ErrNoRows + } + return alist, err } func (s *DefaultAttachmentStore) BulkMiniGetList(originTable string, ids []int) (amap map[int][]*MiniAttachment, err error) { @@ -190,14 +208,34 @@ func (s *DefaultAttachmentStore) Delete(id int) error { return err } +// TODO: Split this out of this store +func (s *DefaultAttachmentStore) UpdateLinked(otable string, oid int) (err error) { + switch otable { + case "topics": + _, err = s.topicUpdateAttachs.Exec(s.CountIn(otable, oid), oid) + if err != nil { + return err + } + err = Topics.Reload(oid) + case "replies": + _, err = s.replyUpdateAttachs.Exec(s.CountIn(otable, oid), oid) + } + if err != nil { + return err + } + return nil +} + // TODO: Add a table for the files and lock the file row when performing tasks related to the file func DeleteAttachment(aid int) error { attach, err := Attachments.Get(aid) if err != nil { + //fmt.Println("o1") return err } err = Attachments.Delete(aid) if err != nil { + //fmt.Println("o2") return err } @@ -205,9 +243,11 @@ func DeleteAttachment(aid int) error { if count == 0 { err := os.Remove("./attachs/" + attach.Path) if err != nil { + //fmt.Println("o3") return err } } + //fmt.Println("o4") return nil } diff --git a/common/page_store.go b/common/page_store.go index 6a22db90..26979c7f 100644 --- a/common/page_store.go +++ b/common/page_store.go @@ -90,10 +90,11 @@ type DefaultPageStore struct { func NewDefaultPageStore(acc *qgen.Accumulator) (*DefaultPageStore, error) { pa := "pages" + allCols := "pid, name, title, body, allowedGroups, menuID" return &DefaultPageStore{ get: acc.Select(pa).Columns("name, title, body, allowedGroups, menuID").Where("pid=?").Prepare(), - getByName: acc.Select(pa).Columns("pid, name, title, body, allowedGroups, menuID").Where("name=?").Prepare(), - getOffset: acc.Select(pa).Columns("pid, name, title, body, allowedGroups, menuID").Orderby("pid DESC").Limit("?,?").Prepare(), + getByName: acc.Select(pa).Columns(allCols).Where("name=?").Prepare(), + getOffset: acc.Select(pa).Columns(allCols).Orderby("pid DESC").Limit("?,?").Prepare(), count: acc.Count(pa).Prepare(), delete: acc.Delete(pa).Where("pid=?").Prepare(), }, acc.FirstError() diff --git a/docs/configuration.md b/docs/configuration.md index 0d1e928f..43fc25d1 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -88,7 +88,7 @@ LastIPCutoff - The number of months which need to pass before the last IP stored PostIPCutoff - The number of days which need to pass before the IP data for a post is automatically deleted. 0 defaults to whatever the current default is, currently 120 and -1 disables this feature. -PollIPCutoff - The number of days which need to pass before the IP data for a poll is automatically deleted. 0 defaults to whatever the current default is, currently 365 and -1 disables this feature. +PollIPCutoff - The number of days which need to pass before the IP data for a poll is automatically deleted. 0 defaults to whatever the current default is, currently 90 and -1 disables this feature. DisableLastIP - Disable storing last IPs for users and purge any existing user last IP data. Default: false diff --git a/misc_test.go b/misc_test.go index b955d9f6..f0a25d2e 100644 --- a/misc_test.go +++ b/misc_test.go @@ -561,92 +561,6 @@ func topicStoreTest(t *testing.T, newID int, ip string) { // TODO: Test topic creation and retrieving that created topic plus reload and inspecting the cache } -// TODO: Implement this -func TestAttachments(t *testing.T) { - miscinit(t) - if !c.PluginsInited { - c.InitPlugins() - } - - filename := "n0-48.png" - srcFile := "./test_data/" + filename - destFile := "./attachs/" + filename - - expect(t, c.Attachments.Count() == 0, "the number of attachments should be 0") - expect(t, c.Attachments.CountIn("topics", 1) == 0, "the number of attachments in topic 1 should be 0") - expect(t, c.Attachments.CountInPath(filename) == 0, fmt.Sprintf("the number of attachments with path '%s' should be 0", filename)) - _, err := c.Attachments.Get(1) - if err != nil && err != sql.ErrNoRows { - t.Error(err) - } - expect(t, err == sql.ErrNoRows, ".Get should have no results") - - // Sim an upload, try a proper upload through the proper pathway later on - _, err = os.Stat(destFile) - if err != nil && !os.IsNotExist(err) { - expectNilErr(t, err) - } else if err == nil { - err := os.Remove(destFile) - expectNilErr(t, err) - } - - input, err := ioutil.ReadFile(srcFile) - expectNilErr(t, err) - err = ioutil.WriteFile(destFile, input, 0644) - expectNilErr(t, err) - - tid, err := c.Topics.Create(2, "Attach Test", "Fillter Body", 1, "") - expectNilErr(t, err) - aid, err := c.Attachments.Add(2, "forums", tid, "topics", 1, filename, "") - expectNilErr(t, err) - expect(t, c.Attachments.Count() == 1, "the number of attachments should be 1") - expect(t, c.Attachments.CountIn("topics", tid) == 1, fmt.Sprintf("the number of attachments in topic %d should be 1", tid)) - expect(t, c.Attachments.CountInPath(filename) == 1, fmt.Sprintf("the number of attachments with path '%s' should be 1", filename)) - - a, err := c.Attachments.Get(aid) - expectNilErr(t, err) - expect(t, a.ID == aid, fmt.Sprintf("a.ID should be %d not %d", aid, a.ID)) - expect(t, a.SectionID == 2, fmt.Sprintf("a.SectionID should be %d not %d", 2, a.SectionID)) - expect(t, a.OriginID == tid, fmt.Sprintf("a.OriginID should be %d not %d", tid, a.OriginID)) - expect(t, a.UploadedBy == 1, fmt.Sprintf("a.UploadedBy should be %d not %d", 1, a.UploadedBy)) - expect(t, a.Path == filename, fmt.Sprintf("a.Path should be %s not %s", filename, a.Path)) - expect(t, a.Extra == "", fmt.Sprintf("a.Extra should be blank not %s", a.Extra)) - expect(t, a.Image, "a.Image should be true") - expect(t, a.Ext == "png", fmt.Sprintf("a.Ext should be png not %s", a.Ext)) - - alist, err := c.Attachments.MiniGetList("topics", tid) - expectNilErr(t, err) - expect(t, len(alist) == 1, fmt.Sprintf("len(alist) should be 1 not %d", len(alist))) - a = alist[0] - expect(t, a.ID == aid, fmt.Sprintf("a.ID should be %d not %d", aid, a.ID)) - expect(t, a.SectionID == 2, fmt.Sprintf("a.SectionID should be %d not %d", 2, a.SectionID)) - expect(t, a.OriginID == tid, fmt.Sprintf("a.OriginID should be %d not %d", tid, a.OriginID)) - expect(t, a.UploadedBy == 1, fmt.Sprintf("a.UploadedBy should be %d not %d", 1, a.UploadedBy)) - expect(t, a.Path == filename, fmt.Sprintf("a.Path should be %s not %s", filename, a.Path)) - expect(t, a.Extra == "", fmt.Sprintf("a.Extra should be blank not %s", a.Extra)) - expect(t, a.Image, "a.Image should be true") - expect(t, a.Ext == "png", fmt.Sprintf("a.Ext should be png not %s", a.Ext)) - - // TODO: Cover the other bits of creation / deletion not covered in the AttachmentStore like updating the reply / topic attachCount - - // TODO: Get attachment tests - // TODO: Move attachment tests - - expectNilErr(t, c.Attachments.Delete(aid)) - expect(t, c.Attachments.Count() == 0, "the number of attachments should be 0") - expect(t, c.Attachments.CountIn("topics", tid) == 0, fmt.Sprintf("the number of attachments in topic %d should be 0", tid)) - expect(t, c.Attachments.CountInPath(filename) == 0, fmt.Sprintf("the number of attachments with path '%s' should be 0", filename)) - _, err = c.Attachments.Get(aid) - if err != nil && err != sql.ErrNoRows { - t.Error(err) - } - expect(t, err == sql.ErrNoRows, ".Get should have no results") - - // TODO: Reply attachment test - // TODO: Delete reply attachment - // TODO: Path overlap tests -} - func TestForumStore(t *testing.T) { miscinit(t) if !c.PluginsInited { @@ -656,7 +570,6 @@ func TestForumStore(t *testing.T) { fcache, ok := c.Forums.(c.ForumCache) expect(t, ok, "Unable to cast ForumStore to ForumCache") - expect(t, c.Forums.Count() == 2, "The forumstore global count should be 2") expect(t, fcache.Length() == 2, "The forum cache length should be 2") @@ -861,16 +774,16 @@ func TestGroupStore(t *testing.T) { recordMustNotExist(t, err, "GID #-1 shouldn't exist") // TODO: Refactor the group store to remove GID #0 - group, err := c.Groups.Get(0) + g, err := c.Groups.Get(0) recordMustExist(t, err, "Couldn't find GID #0") - expect(t, group.ID == 0, fmt.Sprintf("group.ID doesn't not match the requested GID. Got '%d' instead.", group.ID)) - expect(t, group.Name == "Unknown", fmt.Sprintf("GID #0 is named '%s' and not 'Unknown'", group.Name)) + expect(t, g.ID == 0, fmt.Sprintf("g.ID doesn't not match the requested GID. Got '%d' instead.", g.ID)) + expect(t, g.Name == "Unknown", fmt.Sprintf("GID #0 is named '%s' and not 'Unknown'", g.Name)) - group, err = c.Groups.Get(1) + g, err = c.Groups.Get(1) recordMustExist(t, err, "Couldn't find GID #1") - expect(t, group.ID == 1, fmt.Sprintf("group.ID doesn't not match the requested GID. Got '%d' instead.'", group.ID)) - expect(t, len(group.CanSee) > 0, "group.CanSee should not be zero") + expect(t, g.ID == 1, fmt.Sprintf("g.ID doesn't not match the requested GID. Got '%d' instead.'", g.ID)) + expect(t, len(g.CanSee) > 0, "g.CanSee should not be zero") expect(t, !c.Groups.Exists(-1), "GID #-1 shouldn't exist") // 0 aka Unknown, for system posts and other oddities @@ -884,13 +797,13 @@ func TestGroupStore(t *testing.T) { expectNilErr(t, err) expect(t, c.Groups.Exists(gid), "The group we just made doesn't exist") - group, err = c.Groups.Get(gid) + g, err = c.Groups.Get(gid) expectNilErr(t, err) - expect(t, group.ID == gid, "The group ID should match the requested ID") - expect(t, group.IsAdmin, "This should be an admin group") - expect(t, group.IsMod, "This should be a mod group") - expect(t, !group.IsBanned, "This shouldn't be a ban group") - expect(t, len(group.CanSee) == 0, "group.CanSee should be empty") + expect(t, g.ID == gid, "The group ID should match the requested ID") + expect(t, g.IsAdmin, "This should be an admin group") + expect(t, g.IsMod, "This should be a mod group") + expect(t, !g.IsBanned, "This shouldn't be a ban group") + expect(t, len(g.CanSee) == 0, "g.CanSee should be empty") isAdmin = false isMod = true @@ -899,36 +812,36 @@ func TestGroupStore(t *testing.T) { expectNilErr(t, err) expect(t, c.Groups.Exists(gid), "The group we just made doesn't exist") - group, err = c.Groups.Get(gid) + g, err = c.Groups.Get(gid) expectNilErr(t, err) - expect(t, group.ID == gid, "The group ID should match the requested ID") - expect(t, !group.IsAdmin, "This should not be an admin group") - expect(t, group.IsMod, "This should be a mod group") - expect(t, !group.IsBanned, "This shouldn't be a ban group") + expect(t, g.ID == gid, "The group ID should match the requested ID") + expect(t, !g.IsAdmin, "This should not be an admin group") + expect(t, g.IsMod, "This should be a mod group") + expect(t, !g.IsBanned, "This shouldn't be a ban group") // TODO: Make sure this pointer doesn't change once we refactor the group store to stop updating the pointer - err = group.ChangeRank(false, false, true) + err = g.ChangeRank(false, false, true) expectNilErr(t, err) - group, err = c.Groups.Get(gid) + g, err = c.Groups.Get(gid) expectNilErr(t, err) - expect(t, group.ID == gid, "The group ID should match the requested ID") - expect(t, !group.IsAdmin, "This shouldn't be an admin group") - expect(t, !group.IsMod, "This shouldn't be a mod group") - expect(t, group.IsBanned, "This should be a ban group") + expect(t, g.ID == gid, "The group ID should match the requested ID") + expect(t, !g.IsAdmin, "This shouldn't be an admin group") + expect(t, !g.IsMod, "This shouldn't be a mod group") + expect(t, g.IsBanned, "This should be a ban group") - err = group.ChangeRank(true, true, true) + err = g.ChangeRank(true, true, true) expectNilErr(t, err) - group, err = c.Groups.Get(gid) + g, err = c.Groups.Get(gid) expectNilErr(t, err) - expect(t, group.ID == gid, "The group ID should match the requested ID") - expect(t, group.IsAdmin, "This should be an admin group") - expect(t, group.IsMod, "This should be a mod group") - expect(t, !group.IsBanned, "This shouldn't be a ban group") - expect(t, len(group.CanSee) == 0, "len(group.CanSee) should be 0") + expect(t, g.ID == gid, "The group ID should match the requested ID") + expect(t, g.IsAdmin, "This should be an admin group") + expect(t, g.IsMod, "This should be a mod group") + expect(t, !g.IsBanned, "This shouldn't be a ban group") + expect(t, len(g.CanSee) == 0, "len(g.CanSee) should be 0") - err = group.ChangeRank(false, true, true) + err = g.ChangeRank(false, true, true) expectNilErr(t, err) forum, err := c.Forums.Get(2) @@ -944,26 +857,26 @@ func TestGroupStore(t *testing.T) { err = forum.SetPerms(&forumPerms, "custom", gid) expectNilErr(t, err) - group, err = c.Groups.Get(gid) + g, err = c.Groups.Get(gid) expectNilErr(t, err) - expect(t, group.ID == gid, "The group ID should match the requested ID") - expect(t, !group.IsAdmin, "This shouldn't be an admin group") - expect(t, group.IsMod, "This should be a mod group") - expect(t, !group.IsBanned, "This shouldn't be a ban group") - expect(t, group.CanSee != nil, "group.CanSee must not be nil") - expect(t, len(group.CanSee) == 1, "len(group.CanSee) should not be one") - expect(t, group.CanSee[0] == 2, "group.CanSee[0] should be 2") - canSee := group.CanSee + expect(t, g.ID == gid, "The group ID should match the requested ID") + expect(t, !g.IsAdmin, "This shouldn't be an admin group") + expect(t, g.IsMod, "This should be a mod group") + expect(t, !g.IsBanned, "This shouldn't be a ban group") + expect(t, g.CanSee != nil, "g.CanSee must not be nil") + expect(t, len(g.CanSee) == 1, "len(g.CanSee) should not be one") + expect(t, g.CanSee[0] == 2, "g.CanSee[0] should be 2") + canSee := g.CanSee // Make sure the data is static c.Groups.Reload(gid) - group, err = c.Groups.Get(gid) + g, err = c.Groups.Get(gid) expectNilErr(t, err) - expect(t, group.ID == gid, "The group ID should match the requested ID") - expect(t, !group.IsAdmin, "This shouldn't be an admin group") - expect(t, group.IsMod, "This should be a mod group") - expect(t, !group.IsBanned, "This shouldn't be a ban group") + expect(t, g.ID == gid, "The group ID should match the requested ID") + expect(t, !g.IsAdmin, "This shouldn't be an admin group") + expect(t, g.IsMod, "This should be a mod group") + expect(t, !g.IsBanned, "This shouldn't be a ban group") // TODO: Don't enforce a specific order here canSeeTest := func(a, b []int) bool { @@ -981,7 +894,7 @@ func TestGroupStore(t *testing.T) { return true } - expect(t, canSeeTest(group.CanSee, canSee), "group.CanSee is not being reused") + expect(t, canSeeTest(g.CanSee, canSee), "g.CanSee is not being reused") // TODO: Test group deletion // TODO: Test group reload @@ -1051,7 +964,6 @@ func TestReplyStore(t *testing.T) { if !c.PluginsInited { c.InitPlugins() } - _, err := c.Rstore.Get(-1) recordMustNotExist(t, err, "RID #-1 shouldn't exist") _, err = c.Rstore.Get(0) @@ -1074,10 +986,10 @@ func testReplyStore(t *testing.T, newID, newPostCount int, ip string) { } replyTest := func(rid, parentID, createdBy int, content, ip string) { - reply, err := c.Rstore.Get(rid) - replyTest2(reply, err, rid, parentID, createdBy, content, ip) - reply, err = c.Rstore.GetCache().Get(rid) - replyTest2(reply, err, rid, parentID, createdBy, content, ip) + r, err := c.Rstore.Get(rid) + replyTest2(r, err, rid, parentID, createdBy, content, ip) + r, err = c.Rstore.GetCache().Get(rid) + replyTest2(r, err, rid, parentID, createdBy, content, ip) } replyTest(1, 1, 1, "A reply!", "::1") @@ -1145,6 +1057,143 @@ func testReplyStore(t *testing.T, newID, newPostCount int, ip string) { // TODO: Add tests for ReplyCache } +// TODO: Implement this +func TestAttachments(t *testing.T) { + miscinit(t) + if !c.PluginsInited { + c.InitPlugins() + } + + filename := "n0-48.png" + srcFile := "./test_data/" + filename + destFile := "./attachs/" + filename + + expect(t, c.Attachments.Count() == 0, "the number of attachments should be 0") + expect(t, c.Attachments.CountIn("topics", 1) == 0, "the number of attachments in topic 1 should be 0") + expect(t, c.Attachments.CountInPath(filename) == 0, fmt.Sprintf("the number of attachments with path '%s' should be 0", filename)) + _, err := c.Attachments.Get(1) + if err != nil && err != sql.ErrNoRows { + t.Error(err) + } + expect(t, err == sql.ErrNoRows, ".Get should have no results") + _, err = c.Attachments.MiniGetList("topics", 1) + if err != nil && err != sql.ErrNoRows { + t.Error(err) + } + expect(t, err == sql.ErrNoRows, ".MiniGetList should have no results") + _, err = c.Attachments.BulkMiniGetList("topics", []int{1}) + if err != nil && err != sql.ErrNoRows { + t.Error(err) + } + expect(t, err == sql.ErrNoRows, ".BulkMiniGetList should have no results") + + // Sim an upload, try a proper upload through the proper pathway later on + _, err = os.Stat(destFile) + if err != nil && !os.IsNotExist(err) { + expectNilErr(t, err) + } else if err == nil { + err := os.Remove(destFile) + expectNilErr(t, err) + } + + input, err := ioutil.ReadFile(srcFile) + expectNilErr(t, err) + err = ioutil.WriteFile(destFile, input, 0644) + expectNilErr(t, err) + + tid, err := c.Topics.Create(2, "Attach Test", "Fillter Body", 1, "") + expectNilErr(t, err) + aid, err := c.Attachments.Add(2, "forums", tid, "topics", 1, filename, "") + expectNilErr(t, err) + expectNilErr(t, c.Attachments.UpdateLinked("topics", tid)) + expect(t, c.Attachments.Count() == 1, "the number of attachments should be 1") + expect(t, c.Attachments.CountIn("topics", tid) == 1, fmt.Sprintf("the number of attachments in topic %d should be 1", tid)) + expect(t, c.Attachments.CountInPath(filename) == 1, fmt.Sprintf("the number of attachments with path '%s' should be 1", filename)) + + var a *c.MiniAttachment + f := func(aid, sid, oid, uploadedBy int, path, extra, ext string) { + expect(t, a.ID == aid, fmt.Sprintf("ID should be %d not %d", aid, a.ID)) + expect(t, a.SectionID == sid, fmt.Sprintf("SectionID should be %d not %d", sid, a.SectionID)) + expect(t, a.OriginID == oid, fmt.Sprintf("OriginID should be %d not %d", oid, a.OriginID)) + expect(t, a.UploadedBy == uploadedBy, fmt.Sprintf("UploadedBy should be %d not %d", uploadedBy, a.UploadedBy)) + expect(t, a.Path == path, fmt.Sprintf("Path should be %s not %s", path, a.Path)) + expect(t, a.Extra == extra, fmt.Sprintf("Extra should be %s not %s", extra, a.Extra)) + expect(t, a.Image, "Image should be true") + expect(t, a.Ext == ext, fmt.Sprintf("Ext should be %s not %s", ext, a.Ext)) + } + + f2 := func(aid, oid int, extra string, topic bool) { + var tbl string + if topic { + tbl = "topics" + } else { + tbl = "replies" + } + a, err = c.Attachments.Get(aid) + expectNilErr(t, err) + f(aid, 2, oid, 1, filename, extra, "png") + + alist, err := c.Attachments.MiniGetList(tbl, oid) + expectNilErr(t, err) + expect(t, len(alist) == 1, fmt.Sprintf("len(alist) should be 1 not %d", len(alist))) + a = alist[0] + f(aid, 2, oid, 1, filename, extra, "png") + + amap, err := c.Attachments.BulkMiniGetList(tbl, []int{oid}) + expectNilErr(t, err) + expect(t, len(amap) == 1, fmt.Sprintf("len(amap) should be 1 not %d", len(amap))) + alist, ok := amap[oid] + if !ok { + t.Logf("key %d not found in amap", oid) + } + expect(t, len(alist) == 1, fmt.Sprintf("len(alist) should be 1 not %d", len(alist))) + a = alist[0] + f(aid, 2, oid, 1, filename, extra, "png") + } + + topic, err := c.Topics.Get(tid) + expectNilErr(t, err) + expect(t, topic.AttachCount == 1, fmt.Sprintf("topic.AttachCount should be 1 not %d", topic.AttachCount)) + f2(aid, tid, "", true) + + // TODO: Cover the other bits of creation / deletion not covered in the AttachmentStore like updating the reply / topic attachCount + + // TODO: Move attachment tests + + expectNilErr(t, c.Attachments.Delete(aid)) + expect(t, c.Attachments.Count() == 0, "the number of attachments should be 0") + expect(t, c.Attachments.CountIn("topics", tid) == 0, fmt.Sprintf("the number of attachments in topic %d should be 0", tid)) + expect(t, c.Attachments.CountInPath(filename) == 0, fmt.Sprintf("the number of attachments with path '%s' should be 0", filename)) + _, err = c.Attachments.Get(aid) + if err != nil && err != sql.ErrNoRows { + t.Error(err) + } + expect(t, err == sql.ErrNoRows, ".Get should have no results") + _, err = c.Attachments.MiniGetList("topics", tid) + if err != nil && err != sql.ErrNoRows { + t.Error(err) + } + expect(t, err == sql.ErrNoRows, ".MiniGetList should have no results") + _, err = c.Attachments.BulkMiniGetList("topics", []int{tid}) + if err != nil && err != sql.ErrNoRows { + t.Error(err) + } + expect(t, err == sql.ErrNoRows, ".BulkMiniGetList should have no results") + + rid, err := c.Rstore.Create(topic, "Reply Filler", "", 1) + expectNilErr(t, err) + aid, err = c.Attachments.Add(2, "forums", rid, "replies", 1, filename, strconv.Itoa(topic.ID)) + expectNilErr(t, err) + expectNilErr(t, c.Attachments.UpdateLinked("replies", rid)) + r, err := c.Rstore.Get(rid) + expectNilErr(t, err) + expect(t, r.AttachCount == 1, fmt.Sprintf("r.AttachCount should be 1 not %d", r.AttachCount)) + f2(aid, rid, strconv.Itoa(topic.ID), false) + + // TODO: Delete reply attachment + // TODO: Path overlap tests +} + func TestProfileReplyStore(t *testing.T) { miscinit(t) if !c.PluginsInited { diff --git a/routes/attachments.go b/routes/attachments.go index acf4ff9c..d74811d0 100644 --- a/routes/attachments.go +++ b/routes/attachments.go @@ -101,7 +101,7 @@ func deleteAttachment(w http.ResponseWriter, r *http.Request, user c.User, aid i // TODO: Stop duplicating this code // TODO: Use a transaction here // TODO: Move this function to neutral ground -func uploadAttachment(w http.ResponseWriter, r *http.Request, user c.User, sid int, sectionTable string, oid int, originTable, extra string) (pathMap map[string]string, rerr c.RouteError) { +func uploadAttachment(w http.ResponseWriter, r *http.Request, user c.User, sid int, stable string, oid int, otable, extra string) (pathMap map[string]string, rerr c.RouteError) { pathMap = make(map[string]string) files, rerr := uploadFilesWithHash(w, r, user, "./attachs/") if rerr != nil { @@ -109,7 +109,7 @@ func uploadAttachment(w http.ResponseWriter, r *http.Request, user c.User, sid i } for _, filename := range files { - aid, err := c.Attachments.Add(sid, sectionTable, oid, originTable, user.ID, filename, extra) + aid, err := c.Attachments.Add(sid, stable, oid, otable, user.ID, filename, extra) if err != nil { return nil, c.InternalError(err, w, r) } @@ -121,21 +121,9 @@ func uploadAttachment(w http.ResponseWriter, r *http.Request, user c.User, sid i pathMap[filename] = strconv.Itoa(aid) } - switch originTable { - case "topics": - _, err = topicStmts.updateAttachs.Exec(c.Attachments.CountIn(originTable, oid), oid) - if err != nil { - return nil, c.InternalError(err, w, r) - } - err = c.Topics.Reload(oid) - if err != nil { - return nil, c.InternalError(err, w, r) - } - case "replies": - _, err = replyStmts.updateAttachs.Exec(c.Attachments.CountIn(originTable, oid), oid) - if err != nil { - return nil, c.InternalError(err, w, r) - } + err = c.Attachments.UpdateLinked(otable, oid) + if err != nil { + return nil, c.InternalError(err, w, r) } } diff --git a/routes/reply.go b/routes/reply.go index 8902e537..c3a3bb36 100644 --- a/routes/reply.go +++ b/routes/reply.go @@ -15,7 +15,6 @@ import ( ) type ReplyStmts struct { - updateAttachs *sql.Stmt createReplyPaging *sql.Stmt } @@ -25,8 +24,6 @@ var replyStmts ReplyStmts func init() { c.DbInits.Add(func(acc *qgen.Accumulator) error { replyStmts = ReplyStmts{ - // TODO: Less race-y attachment count updates - updateAttachs: acc.Update("replies").Set("attachCount=?").Where("rid=?").Prepare(), createReplyPaging: acc.Select("replies").Cols("rid").Where("rid >= ? - 1 AND tid=?").Orderby("rid ASC").Prepare(), } return acc.FirstError() diff --git a/routes/topic.go b/routes/topic.go index 25ba9c26..5e161aaf 100644 --- a/routes/topic.go +++ b/routes/topic.go @@ -30,7 +30,6 @@ import ( type TopicStmts struct { getLikedTopic *sql.Stmt - updateAttachs *sql.Stmt } var topicStmts TopicStmts @@ -40,8 +39,6 @@ func init() { c.DbInits.Add(func(acc *qgen.Accumulator) error { topicStmts = TopicStmts{ getLikedTopic: acc.Select("likes").Columns("targetItem").Where("sentBy=? && targetItem=? && targetType='topics'").Prepare(), - // TODO: Less race-y attachment count updates - updateAttachs: acc.Update("topics").Set("attachCount=?").Where("tid=?").Prepare(), } return acc.FirstError() }) @@ -134,7 +131,7 @@ func ViewTopic(w http.ResponseWriter, r *http.Request, user c.User, header *c.He if topic.AttachCount > 0 { attachs, err := c.Attachments.MiniGetList("topics", topic.ID) - if err != nil { + if err != nil && err != sql.ErrNoRows { // TODO: We might want to be a little permissive here in-case of a desync? return c.InternalError(err, w, r) } @@ -452,6 +449,7 @@ func CreateTopicSubmit(w http.ResponseWriter, r *http.Request, user c.User) c.Ro return nil } +// TODO: Move this function func uploadFilesWithHash(w http.ResponseWriter, r *http.Request, user c.User, dir string) (filenames []string, rerr c.RouteError) { files, ok := r.MultipartForm.File["upload_files"] if !ok {