From e030384bcd6f278088754c19f469d7b944a639b5 Mon Sep 17 00:00:00 2001
From: Rikkuru <5867722+Rikkuru@users.noreply.github.com>
Date: Thu, 21 Nov 2024 20:27:24 +0300
Subject: [PATCH] Don't return error to caller with RetryType Ignore
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

RetryType Ignore is documented and should work according to the documentation. Error is not returned to caller on ignore but can be seen in ObservedQuery. RetryType should be checked even if RetryPolicy.Attempt returns false, otherwise Ignore will not work on last attempt.

Patch by Rikkuru; reviewed by João Reis, Jackson Fleming for CASSGO-28
---
 CHANGELOG.md      |  1 +
 query_executor.go | 28 ++++++++++++++------
 session_test.go   | 67 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 8 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6c0054952..a83e6f861 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - Retry policy now takes into account query idempotency (CASSGO-27)
+- Don't return error to caller with RetryType Ignore (CASSGO-28)
 
 ## [1.7.0] - 2024-09-23
 
diff --git a/query_executor.go b/query_executor.go
index 03687361a..d6be02e53 100644
--- a/query_executor.go
+++ b/query_executor.go
@@ -165,27 +165,39 @@ func (q *queryExecutor) do(ctx context.Context, qry ExecutableQuery, hostIter Ne
 		}
 
 		// Exit if the query was successful
-		// or no retry policy defined or retry attempts were reached
-		if iter.err == nil || !qry.IsIdempotent() || rt == nil || !rt.Attempt(qry) {
+		// or query is not idempotent or no retry policy defined
+		if iter.err == nil || !qry.IsIdempotent() || rt == nil {
 			return iter
 		}
-		lastErr = iter.err
+
+		attemptsReached := !rt.Attempt(qry)
+		retryType := rt.GetRetryType(iter.err)
+
+		var stopRetries bool
 
 		// If query is unsuccessful, check the error with RetryPolicy to retry
-		switch rt.GetRetryType(iter.err) {
+		switch retryType {
 		case Retry:
 			// retry on the same host
-			continue
-		case Rethrow, Ignore:
-			return iter
 		case RetryNextHost:
 			// retry on the next host
 			selectedHost = hostIter()
-			continue
+		case Ignore:
+			iter.err = nil
+			stopRetries = true
+		case Rethrow:
+			stopRetries = true
 		default:
 			// Undefined? Return nil and error, this will panic in the requester
 			return &Iter{err: ErrUnknownRetryType}
 		}
+
+		if stopRetries || attemptsReached {
+			return iter
+		}
+
+		lastErr = iter.err
+		continue
 	}
 
 	if lastErr != nil {
diff --git a/session_test.go b/session_test.go
index 0319a8a4c..c7bafbb04 100644
--- a/session_test.go
+++ b/session_test.go
@@ -347,3 +347,70 @@ func TestIsUseStatement(t *testing.T) {
 		}
 	}
 }
+
+type simpleTestRetryPolycy struct {
+	RetryType  RetryType
+	NumRetries int
+}
+
+func (p *simpleTestRetryPolycy) Attempt(q RetryableQuery) bool {
+	return q.Attempts() <= p.NumRetries
+}
+
+func (p *simpleTestRetryPolycy) GetRetryType(error) RetryType {
+	return p.RetryType
+}
+
+// TestRetryType_IgnoreRethrow verify that with Ignore/Rethrow retry types:
+// - retries stopped
+// - return error is nil on Ignore
+// - return error is not nil on Rethrow
+// - observed error is not nil
+func TestRetryType_IgnoreRethrow(t *testing.T) {
+	session := createSession(t)
+	defer session.Close()
+
+	var observedErr error
+	var observedAttempts int
+
+	resetObserved := func() {
+		observedErr = nil
+		observedAttempts = 0
+	}
+
+	observer := funcQueryObserver(func(ctx context.Context, o ObservedQuery) {
+		observedErr = o.Err
+		observedAttempts++
+	})
+
+	for _, caseParams := range []struct {
+		retries   int
+		retryType RetryType
+	}{
+		{0, Ignore},  // check that error ignored even on last attempt
+		{1, Ignore},  // check thet ignore stops retries
+		{1, Rethrow}, // check thet rethrow stops retries
+	} {
+		retryPolicy := &simpleTestRetryPolycy{RetryType: caseParams.retryType, NumRetries: caseParams.retries}
+
+		err := session.Query("INSERT INTO gocql_test.invalid_table(value) VALUES(1)").Idempotent(true).RetryPolicy(retryPolicy).Observer(observer).Exec()
+
+		if err != nil && caseParams.retryType == Ignore {
+			t.Fatalf("[%v] Expected no error, got: %s", caseParams.retryType, err)
+		}
+
+		if err == nil && caseParams.retryType == Rethrow {
+			t.Fatalf("[%v] Expected unconfigured table error, got: nil", caseParams.retryType)
+		}
+
+		if observedErr == nil {
+			t.Fatal("Expected unconfigured table error in Obserer, got: nil")
+		}
+
+		if observedAttempts > 1 {
+			t.Fatalf("Expected one attempt, got: %d", observedAttempts)
+		}
+
+		resetObserved()
+	}
+}