From 2b0de82aa9eadf7175c299a3604105abaa491e7e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Joan=20L=C3=B3pez=20de=20la=20Franca=20Beltran?=
 <5459617+joanlopez@users.noreply.github.com>
Date: Mon, 23 Jan 2023 14:17:56 +0100
Subject: [PATCH] SQLStore: Add test for nested transactions events (#60500)

* SQLStore: Add test for nested transactions events

* Replace fmt.Print* with t.Log*

* Add different test cases
---
 pkg/services/sqlstore/session.go           |  2 +-
 pkg/services/sqlstore/transactions.go      |  8 +--
 pkg/services/sqlstore/transactions_test.go | 74 ++++++++++++++++++++++
 3 files changed, 78 insertions(+), 6 deletions(-)

diff --git a/pkg/services/sqlstore/session.go b/pkg/services/sqlstore/session.go
index b0cc6a804d8..cffd88231a0 100644
--- a/pkg/services/sqlstore/session.go
+++ b/pkg/services/sqlstore/session.go
@@ -68,7 +68,7 @@ func startSessionOrUseExisting(ctx context.Context, engine *xorm.Engine, beginTr
 
 // WithDbSession calls the callback with the session in the context (if exists).
 // Otherwise it creates a new one that is closed upon completion.
-// A session is stored in the context if sqlstore.InTransaction() has been been previously called with the same context (and it's not committed/rolledback yet).
+// A session is stored in the context if sqlstore.InTransaction() has been previously called with the same context (and it's not committed/rolledback yet).
 // In case of sqlite3.ErrLocked or sqlite3.ErrBusy failure it will be retried at most five times before giving up.
 func (ss *SQLStore) WithDbSession(ctx context.Context, callback DBTransactionFunc) error {
 	return ss.withDbSession(ctx, ss.engine, callback)
diff --git a/pkg/services/sqlstore/transactions.go b/pkg/services/sqlstore/transactions.go
index a3ed3527428..3a67f3cae2b 100644
--- a/pkg/services/sqlstore/transactions.go
+++ b/pkg/services/sqlstore/transactions.go
@@ -85,11 +85,9 @@ func (ss *SQLStore) inTransactionWithRetryCtx(ctx context.Context, engine *xorm.
 		return err
 	}
 
-	if len(sess.events) > 0 {
-		for _, e := range sess.events {
-			if err = bus.Publish(ctx, e); err != nil {
-				ctxLogger.Error("Failed to publish event after commit.", "error", err)
-			}
+	for _, e := range sess.events {
+		if err = bus.Publish(ctx, e); err != nil {
+			ctxLogger.Error("Failed to publish event after commit.", "error", err)
 		}
 	}
 
diff --git a/pkg/services/sqlstore/transactions_test.go b/pkg/services/sqlstore/transactions_test.go
index 709cd9cd055..508b0bb71db 100644
--- a/pkg/services/sqlstore/transactions_test.go
+++ b/pkg/services/sqlstore/transactions_test.go
@@ -2,8 +2,10 @@ package sqlstore
 
 import (
 	"context"
+	"errors"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -65,3 +67,75 @@ func TestIntegrationReuseSessionWithTransaction(t *testing.T) {
 		}))
 	})
 }
+
+func TestIntegrationPublishAfterCommitWithNestedTransactions(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping integration test")
+	}
+
+	ss := InitTestDB(t)
+	ctx := context.Background()
+
+	// On X success
+	var xHasSucceeded bool
+	ss.Bus().AddEventListener(func(ctx context.Context, e *X) error {
+		xHasSucceeded = true
+		t.Logf("Succeeded and committed: %T\n", e)
+		return nil
+	})
+
+	// On Y success
+	var yHasSucceeded bool
+	ss.Bus().AddEventListener(func(ctx context.Context, e *Y) error {
+		yHasSucceeded = true
+		t.Logf("Succeeded and committed: %T\n", e)
+		return nil
+	})
+
+	t.Run("When no session is stored into the context, each transaction should be independent", func(t *testing.T) {
+		t.Cleanup(func() { xHasSucceeded = false; yHasSucceeded = false })
+
+		err := ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error {
+			t.Logf("Outer transaction: doing X... success!")
+			sess.PublishAfterCommit(&X{})
+
+			require.NoError(t, ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error {
+				t.Log("Inner transaction: doing Y... success!")
+				sess.PublishAfterCommit(&Y{})
+				return nil
+			}))
+
+			t.Log("Outer transaction: doing Z... failure, rolling back...")
+			return errors.New("z failed")
+		})
+
+		assert.NotNil(t, err)
+		assert.False(t, xHasSucceeded)
+		assert.True(t, yHasSucceeded)
+	})
+
+	t.Run("When the session is stored into the context, the inner transaction should depend on the outer one", func(t *testing.T) {
+		t.Cleanup(func() { xHasSucceeded = false; yHasSucceeded = false })
+
+		err := ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error {
+			t.Logf("Outer transaction: doing X... success!")
+			sess.PublishAfterCommit(&X{})
+
+			require.NoError(t, ss.InTransaction(ctx, func(ctx context.Context) error {
+				t.Log("Inner transaction: doing Y... success!")
+				sess.PublishAfterCommit(&Y{})
+				return nil
+			}))
+
+			t.Log("Outer transaction: doing Z... failure, rolling back...")
+			return errors.New("z failed")
+		})
+
+		assert.NotNil(t, err)
+		assert.False(t, xHasSucceeded)
+		assert.False(t, yHasSucceeded)
+	})
+}
+
+type X struct{}
+type Y struct{}