diff --git a/internal/repo/activity/vote_repo.go b/internal/repo/activity/vote_repo.go index 389ae18d8..dcd18eae3 100644 --- a/internal/repo/activity/vote_repo.go +++ b/internal/repo/activity/vote_repo.go @@ -184,7 +184,7 @@ func (vr *VoteRepo) GetAndSaveVoteResult(ctx context.Context, objectID, objectTy up, down int64, err error) { up = vr.countVoteUp(ctx, objectID, objectType) down = vr.countVoteDown(ctx, objectID, objectType) - err = vr.updateVotes(ctx, objectID, objectType, int(up-down)) + err = vr.updateVotes(ctx, objectID, objectType) return } @@ -437,16 +437,35 @@ func (vr *VoteRepo) countVote(ctx context.Context, objectID, objectType, action return count, err } -func (vr *VoteRepo) updateVotes(ctx context.Context, objectID, objectType string, voteCount int) (err error) { - session := vr.data.DB.Context(ctx) +// updateVotes refreshes the stored vote count with a single statement that +// recounts the active vote activities. Counting and writing in one statement +// keeps a slower request from overwriting a newer total with a stale one. +func (vr *VoteRepo) updateVotes(ctx context.Context, objectID, objectType string) (err error) { + // A missing activity type (e.g. comments have no vote down) yields id 0, + // which matches no activity, same as the count helpers behave. + upActivityType, _ := vr.activityRepo.GetActivityTypeByObjectType(ctx, objectType, constant.ActVoteUp) + downActivityType, _ := vr.activityRepo.GetActivityTypeByObjectType(ctx, objectType, constant.ActVoteDown) + + var tableName string switch objectType { case constant.QuestionObjectType: - _, err = session.ID(objectID).Cols("vote_count").Update(&entity.Question{VoteCount: voteCount}) + tableName = vr.data.DB.TableName(&entity.Question{}) case constant.AnswerObjectType: - _, err = session.ID(objectID).Cols("vote_count").Update(&entity.Answer{VoteCount: voteCount}) + tableName = vr.data.DB.TableName(&entity.Answer{}) case constant.CommentObjectType: - _, err = session.ID(objectID).Cols("vote_count").Update(&entity.Comment{VoteCount: voteCount}) + tableName = vr.data.DB.TableName(&entity.Comment{}) + default: + return nil } + + activityTable := vr.data.DB.TableName(&entity.Activity{}) + _, err = vr.data.DB.Context(ctx).Exec( + "UPDATE "+tableName+" SET vote_count = "+ + "((SELECT COUNT(*) FROM "+activityTable+" WHERE object_id = ? AND activity_type = ? AND cancelled = 0)"+ + " - "+ + "(SELECT COUNT(*) FROM "+activityTable+" WHERE object_id = ? AND activity_type = ? AND cancelled = 0)) "+ + "WHERE id = ?", + objectID, upActivityType, objectID, downActivityType, objectID) if err != nil { log.Error(err) } diff --git a/internal/repo/repo_test/vote_result_race_test.go b/internal/repo/repo_test/vote_result_race_test.go new file mode 100644 index 000000000..2ca9e4f2d --- /dev/null +++ b/internal/repo/repo_test/vote_result_race_test.go @@ -0,0 +1,146 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package repo_test + +import ( + "context" + "fmt" + "sync" + "testing" + + "github.com/apache/answer/internal/entity" + "github.com/apache/answer/internal/repo/activity" + activityCommonRepo "github.com/apache/answer/internal/repo/activity_common" + configRepo "github.com/apache/answer/internal/repo/config" + "github.com/apache/answer/internal/repo/question" + "github.com/apache/answer/internal/repo/unique" + "github.com/apache/answer/internal/service/config" + "github.com/apache/answer/internal/service/content" + "github.com/apache/answer/pkg/converter" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "xorm.io/builder" +) + +func newVoteRepoForTest() content.VoteRepo { + uniqueIDRepo := unique.NewUniqueIDRepo(testDataSource) + configService := config.NewConfigService(configRepo.NewConfigRepo(testDataSource)) + activityRepo := activityCommonRepo.NewActivityRepo(testDataSource, uniqueIDRepo, configService) + // GetAndSaveVoteResult only needs the data handle and the activity repo. + return activity.NewVoteRepo(testDataSource, activityRepo, nil, nil) +} + +func voteActivityTypeID(t *testing.T, ctx context.Context, key string) int { + cfg := &entity.Config{} + exist, err := testDataSource.DB.Context(ctx).Where(builder.Eq{"key": key}).Get(cfg) + require.NoError(t, err) + require.True(t, exist, "config %s must exist in the seeded test data", key) + return cfg.ID +} + +func addVoteActivity(ctx context.Context, objectID, voterUserID string, activityType int) error { + _, err := testDataSource.DB.Context(ctx).Insert(&entity.Activity{ + ObjectID: objectID, + OriginalObjectID: objectID, + UserID: "9501", + TriggerUserID: converter.StringToInt64(voterUserID), + ActivityType: activityType, + Cancelled: entity.ActivityAvailable, + }) + return err +} + +func addVoteQuestion(t *testing.T, ctx context.Context, title string) *entity.Question { + questionRepo := question.NewQuestionRepo(testDataSource, unique.NewUniqueIDRepo(testDataSource)) + q := &entity.Question{ + UserID: "9502", + Title: title, + OriginalText: title, + ParsedText: title, + Status: entity.QuestionStatusAvailable, + Show: entity.QuestionShow, + } + require.NoError(t, questionRepo.AddQuestion(ctx, q)) + return q +} + +func TestGetAndSaveVoteResultCountsActiveVotes(t *testing.T) { + ctx := context.Background() + voteRepo := newVoteRepoForTest() + upType := voteActivityTypeID(t, ctx, "question.vote_up") + downType := voteActivityTypeID(t, ctx, "question.vote_down") + q := addVoteQuestion(t, ctx, "vote result count question") + + for i := range 3 { + require.NoError(t, addVoteActivity(ctx, q.ID, fmt.Sprintf("9510%d", i), upType)) + } + require.NoError(t, addVoteActivity(ctx, q.ID, "9519", downType)) + + up, down, err := voteRepo.GetAndSaveVoteResult(ctx, q.ID, "question") + require.NoError(t, err) + assert.Equal(t, int64(3), up) + assert.Equal(t, int64(1), down) + + stored := &entity.Question{} + exist, err := testDataSource.DB.Context(ctx).ID(q.ID).Get(stored) + require.NoError(t, err) + require.True(t, exist) + assert.Equal(t, 2, stored.VoteCount) +} + +// Reproduces https://github.com/apache/answer/issues/1646: recalculating the +// total and saving it as an absolute value lets an earlier request overwrite a +// newer result once vote activities commit between the count and the write. +// A single shared connection keeps sqlite free of locking errors while the +// statements of the concurrent calls still interleave freely. One round can +// get lucky and write the correct total last, so the burst repeats over +// several fresh questions; every round must end with the exact total. +func TestGetAndSaveVoteResultConcurrentVotes(t *testing.T) { + ctx := context.Background() + voteRepo := newVoteRepoForTest() + upType := voteActivityTypeID(t, ctx, "question.vote_up") + + testDataSource.DB.SetMaxOpenConns(1) + defer testDataSource.DB.SetMaxOpenConns(0) + + const rounds = 10 + const voters = 32 + for round := range rounds { + q := addVoteQuestion(t, ctx, fmt.Sprintf("vote result race question %d", round)) + var wg sync.WaitGroup + for i := range voters { + wg.Add(1) + go func(i int) { + defer wg.Done() + assert.NoError(t, addVoteActivity(ctx, q.ID, fmt.Sprintf("952%02d%02d", round, i), upType)) + _, _, err := voteRepo.GetAndSaveVoteResult(ctx, q.ID, "question") + assert.NoError(t, err) + }(i) + } + wg.Wait() + + stored := &entity.Question{} + exist, err := testDataSource.DB.Context(ctx).ID(q.ID).Get(stored) + require.NoError(t, err) + require.True(t, exist) + assert.Equal(t, voters, stored.VoteCount, + "round %d: stored vote_count must equal the number of active vote activities", round) + } +}