fix: do not iteratively delete records (#3766)

Resolves performance issues on some databases when deleting consent. BREAKING CHANGES: Deleting consents no longer returns 404 in certain edge cases but instead always 204.
ory · Jun 7, 2024 · 5ef20a2 · 5ef20a2
1 parent 7563907
commit 5ef20a2
Show file tree

Hide file tree

Showing 17 changed files with 397 additions and 74 deletions.
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
@@ -69,7 +69,7 @@ jobs:
     steps:
       - run: |
           docker create --name cockroach -p 26257:26257 \
-            cockroachdb/cockroach:v22.1.10 start-single-node --insecure
+            cockroachdb/cockroach:v24.1.0 start-single-node --insecure
           docker start cockroach
         name: Start CockroachDB
       - uses: ory/ci/checkout@master
@@ -170,7 +170,7 @@ jobs:
     steps:
       - run: |
           docker create --name cockroach -p 26257:26257 \
-            cockroachdb/cockroach:v22.1.10 start-single-node --insecure
+            cockroachdb/cockroach:v24.1.0 start-single-node --insecure
           docker start cockroach
         name: Start CockroachDB
       - uses: ory/ci/checkout@master

diff --git a/consent/test/manager_test_helpers.go b/consent/test/manager_test_helpers.go
@@ -744,8 +744,8 @@ func ManagerTests(deps Deps, m consent.Manager, clientManager client.Manager, fo
 				})
 			}
 
-			require.EqualError(t, m.RevokeSubjectConsentSession(ctx, "i-do-not-exist"), x.ErrNotFound.Error())
-			require.EqualError(t, m.RevokeSubjectClientConsentSession(ctx, "i-do-not-exist", "i-do-not-exist"), x.ErrNotFound.Error())
+			require.NoError(t, m.RevokeSubjectConsentSession(ctx, "i-do-not-exist"))
+			require.NoError(t, m.RevokeSubjectClientConsentSession(ctx, "i-do-not-exist", "i-do-not-exist"))
 		})
 
 		t.Run("case=list-used-consent-requests", func(t *testing.T) {

diff --git a/internal/driver.go b/internal/driver.go
@@ -9,19 +9,16 @@ import (
 	"testing"
 
 	"github.com/go-jose/go-jose/v3"
-
-	"github.com/ory/x/configx"
-
 	"github.com/stretchr/testify/require"
 
-	"github.com/ory/hydra/v2/x"
-	"github.com/ory/x/contextx"
-	"github.com/ory/x/sqlcon/dockertest"
-
 	"github.com/ory/hydra/v2/driver"
 	"github.com/ory/hydra/v2/driver/config"
 	"github.com/ory/hydra/v2/jwk"
+	"github.com/ory/hydra/v2/x"
+	"github.com/ory/x/configx"
+	"github.com/ory/x/contextx"
 	"github.com/ory/x/logrusx"
+	"github.com/ory/x/sqlcon/dockertest"
 )
 
 func resetConfig(p *config.DefaultProvider) {
@@ -87,7 +84,7 @@ func ConnectToPG(t testing.TB) string {
 }
 
 func ConnectToCRDB(t testing.TB) string {
-	return dockertest.RunTestCockroachDBWithVersion(t, "v22.1.2")
+	return dockertest.RunTestCockroachDBWithVersion(t, "v24.1.0")
 }
 
 func ConnectDatabases(t *testing.T, migrate bool, ctxer contextx.Contextualizer) (pg, mysql, crdb driver.Registry, clean func(*testing.T)) {

diff --git a/internal/httpclient/go.sum b/internal/httpclient/go.sum
diff --git a/persistence/sql/migrations/20230908104443000000_change_client_pk.cockroach.up.sql b/persistence/sql/migrations/20230908104443000000_change_client_pk.cockroach.up.sql
@@ -1 +1 @@
-ALTER TABLE hydra_client ALTER PRIMARY KEY USING COLUMNS (id, nid) USING HASH;
+ALTER TABLE hydra_client ALTER PRIMARY KEY USING COLUMNS (id, nid);
diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go
@@ -12,21 +12,18 @@ import (
 
 	"github.com/gobuffalo/pop/v6"
 	"github.com/gofrs/uuid"
-
-	"github.com/ory/hydra/v2/oauth2/flowctx"
-	"github.com/ory/x/otelx"
-	"github.com/ory/x/sqlxx"
-
-	"github.com/ory/x/errorsx"
-
 	"github.com/pkg/errors"
 
 	"github.com/ory/fosite"
 	"github.com/ory/hydra/v2/client"
 	"github.com/ory/hydra/v2/consent"
 	"github.com/ory/hydra/v2/flow"
+	"github.com/ory/hydra/v2/oauth2/flowctx"
 	"github.com/ory/hydra/v2/x"
+	"github.com/ory/x/errorsx"
+	"github.com/ory/x/otelx"
 	"github.com/ory/x/sqlcon"
+	"github.com/ory/x/sqlxx"
 )
 
 var _ consent.Manager = &Persister{}
@@ -51,43 +48,47 @@ func (p *Persister) revokeConsentSession(whereStmt string, whereArgs ...interfac
 		if err := p.QueryWithNetwork(ctx).
 			Where(whereStmt, whereArgs...).
 			Select("consent_challenge_id").
-			All(&fs); err != nil {
-			if errors.Is(err, sql.ErrNoRows) {
-				return errorsx.WithStack(x.ErrNotFound)
-			}
-
+			All(&fs); errors.Is(err, sql.ErrNoRows) {
+			return errorsx.WithStack(x.ErrNotFound)
+		} else if err != nil {
 			return sqlcon.HandleError(err)
 		}
 
-		var count int
+		ids := make([]interface{}, 0, len(fs))
+		nid := p.NetworkID(ctx)
 		for _, f := range fs {
-			if err := p.RevokeAccessToken(ctx, f.ConsentChallengeID.String()); errors.Is(err, fosite.ErrNotFound) {
-				// do nothing
-			} else if err != nil {
-				return err
-			}
-
-			if err := p.RevokeRefreshToken(ctx, f.ConsentChallengeID.String()); errors.Is(err, fosite.ErrNotFound) {
-				// do nothing
-			} else if err != nil {
-				return err
-			}
-
-			localCount, err := c.RawQuery("DELETE FROM hydra_oauth2_flow WHERE consent_challenge_id = ? AND nid = ?", f.ConsentChallengeID, p.NetworkID(ctx)).ExecWithCount()
-			if err != nil {
-				if errors.Is(err, sql.ErrNoRows) {
-					return errorsx.WithStack(x.ErrNotFound)
-				}
-				return sqlcon.HandleError(err)
-			}
-
-			// If there are no sessions to revoke we should return an error to indicate to the caller
-			// that the request failed.
-			count += localCount
+			ids = append(ids, f.ConsentChallengeID.String())
+		}
+
+		if len(ids) == 0 {
+			return nil
+		}
+
+		if err := p.QueryWithNetwork(ctx).
+			Where("nid = ?", nid).
+			Where("request_id IN (?)", ids...).
+			Delete(&OAuth2RequestSQL{Table: sqlTableAccess}); errors.Is(err, fosite.ErrNotFound) {
+			// do nothing
+		} else if err != nil {
+			return err
 		}
 
-		if count == 0 {
+		if err := p.QueryWithNetwork(ctx).
+			Where("nid = ?", nid).
+			Where("request_id IN (?)", ids...).
+			Delete(&OAuth2RequestSQL{Table: sqlTableRefresh}); errors.Is(err, fosite.ErrNotFound) {
+			// do nothing
+		} else if err != nil {
+			return err
+		}
+
+		if err := p.QueryWithNetwork(ctx).
+			Where("nid = ?", nid).
+			Where("consent_challenge_id IN (?)", ids...).
+			Delete(new(flow.Flow)); errors.Is(err, sql.ErrNoRows) {
 			return errorsx.WithStack(x.ErrNotFound)
+		} else if err != nil {
+			return sqlcon.HandleError(err)
 		}
 
 		return nil
@@ -642,7 +643,7 @@ SELECT DISTINCT c.* FROM hydra_client as c
 JOIN hydra_oauth2_flow as f ON (c.id = f.client_id AND c.nid = f.nid)
 WHERE
 	f.subject=? AND
-	c.%schannel_logout_uri!='' AND
+	c.%schannel_logout_uri != '' AND
 	c.%schannel_logout_uri IS NOT NULL AND
 	f.login_session_id = ? AND
 	f.nid = ? AND

diff --git a/persistence/sql/persister_nid_test.go b/persistence/sql/persister_nid_test.go
@@ -1834,7 +1834,7 @@ func (s *PersisterTestSuite) TestRevokeSubjectClientConsentSession() {
 
 			actual := flow.Flow{}
 
-			require.Error(t, r.Persister().RevokeSubjectClientConsentSession(s.t2, "sub", client.ID))
+			require.NoError(t, r.Persister().RevokeSubjectClientConsentSession(s.t2, "sub", client.ID), "should not error if nothing was found")
 			require.NoError(t, r.Persister().Connection(context.Background()).Find(&actual, f.ID))
 			require.NoError(t, r.Persister().RevokeSubjectClientConsentSession(s.t1, "sub", client.ID))
 			require.Error(t, r.Persister().Connection(context.Background()).Find(&actual, f.ID))

diff --git a/quickstart-cockroach.yml b/quickstart-cockroach.yml
@@ -9,7 +9,6 @@
 # endpoint can only be used if you follow the steps in the tutorial.      #
 #                                                                         #
 ###########################################################################
-version: "3.7"
 services:
   hydra-migrate:
     environment:

diff --git a/quickstart-cors.yml b/quickstart-cors.yml
@@ -9,9 +9,6 @@
 # endpoint can only be used if you follow the steps in the tutorial.      #
 #                                                                         #
 ###########################################################################
-
-version: "3.7"
-
 services:
   hydra:
     environment:

diff --git a/quickstart-debug.yml b/quickstart-debug.yml
@@ -9,9 +9,6 @@
 # endpoint can only be used if you follow the steps in the tutorial.      #
 #                                                                         #
 ###########################################################################
-
-version: "3.7"
-
 services:
   hydra:
     environment:

diff --git a/quickstart-hsm.yml b/quickstart-hsm.yml
@@ -9,9 +9,6 @@
 # endpoint can only be used if you follow the steps in the tutorial.      #
 #                                                                         #
 ###########################################################################
-
-version: "3.7"
-
 services:
   hydra:
     build:

diff --git a/quickstart-jwt.yml b/quickstart-jwt.yml
@@ -9,9 +9,6 @@
 # endpoint can only be used if you follow the steps in the tutorial.      #
 #                                                                         #
 ###########################################################################
-
-version: "3.7"
-
 services:
   hydra:
     environment:

diff --git a/quickstart-mysql.yml b/quickstart-mysql.yml
@@ -9,7 +9,6 @@
 # endpoint can only be used if you follow the steps in the tutorial.      #
 #                                                                         #
 ###########################################################################
-version: "3.7"
 services:
   hydra-migrate:
     environment:

diff --git a/quickstart-postgres.yml b/quickstart-postgres.yml
@@ -9,7 +9,6 @@
 # endpoint can only be used if you follow the steps in the tutorial.      #
 #                                                                         #
 ###########################################################################
-version: "3.7"
 services:
   hydra-migrate:
     environment:

diff --git a/quickstart-prometheus.yml b/quickstart-prometheus.yml
@@ -9,9 +9,6 @@
 # endpoint can only be used if you follow the steps in the tutorial.      #
 #                                                                         #
 ###########################################################################
-
-version: "3.7"
-
 services:
   prometheus:
     image: prom/prometheus:v2.12.0

diff --git a/quickstart-tracing.yml b/quickstart-tracing.yml
@@ -9,9 +9,6 @@
 # endpoint can only be used if you follow the steps in the tutorial.      #
 #                                                                         #
 ###########################################################################
-
-version: "3.7"
-
 services:
   hydra:
     depends_on:

diff --git a/quickstart.yml b/quickstart.yml
@@ -9,7 +9,6 @@
 # endpoint can only be used if you follow the steps in the tutorial.      #
 #                                                                         #
 ###########################################################################
-version: "3.7"
 services:
   hydra:
     image: oryd/hydra:v2.2.0