Merge pull request #1677 from Bee-lee/cluster-toggle-cluster-disable-…

…user-id Remove `user_id` from all functionality regarding the `rule_disable` table
RedHatInsights · Nov 7, 2022 · 897606a · 897606a
2 parents ecf2310 + f2d2ed7
commit 897606a
Show file tree

Hide file tree

Showing 13 changed files with 316 additions and 249 deletions.
diff --git a/migration/actual_migrations_test.go b/migration/actual_migrations_test.go
@@ -1083,3 +1083,110 @@ func TestMigration28(t *testing.T) {
 	// not possible to insert more than 1 row per org
 	assert.Error(t, err)
 }
+
+func TestMigration29(t *testing.T) {
+	db, dbDriver, closer := prepareDBAndInfo(t)
+	defer closer()
+
+	if dbDriver == types.DBDriverSQLite3 {
+		// nothing worth testing for sqlite
+		return
+	}
+
+	err := migration.SetDBVersion(db, dbDriver, 28)
+	helpers.FailOnError(t, err)
+
+	_, err = db.Exec(`
+		INSERT INTO cluster_rule_toggle
+		(cluster_id, user_id, org_id, rule_id, error_key, disabled, disabled_at, updated_at)
+		VALUES
+		($1, $2, $3, $4, $5, $6, $7, $8)
+	`,
+		testdata.ClusterName,
+		testdata.UserID,
+		testdata.OrgID,
+		testdata.Rule1ID,
+		testdata.ErrorKey1,
+		1,
+		time.Now(),
+		time.Now(),
+	)
+	helpers.FailOnError(t, err)
+
+	err = migration.SetDBVersion(db, dbDriver, 29)
+	helpers.FailOnError(t, err)
+
+	err = db.QueryRow(`SELECT user_id FROM cluster_rule_toggle`).Err()
+	assert.Error(t, err, "user_id column should not exist")
+
+	err = migration.SetDBVersion(db, dbDriver, 28)
+	helpers.FailOnError(t, err)
+
+	var userID types.UserID
+	err = db.QueryRow(`
+	SELECT
+		user_id
+	FROM
+		cluster_rule_toggle
+	`,
+	).Scan(
+		&userID,
+	)
+	helpers.FailOnError(t, err)
+
+	// default value on stepdown
+	assert.Equal(t, userID, types.UserID("-1"))
+}
+
+func TestMigration30(t *testing.T) {
+	db, dbDriver, closer := prepareDBAndInfo(t)
+	defer closer()
+
+	if dbDriver == types.DBDriverSQLite3 {
+		// nothing worth testing for sqlite
+		return
+	}
+
+	err := migration.SetDBVersion(db, dbDriver, 29)
+	helpers.FailOnError(t, err)
+
+	_, err = db.Exec(`
+		INSERT INTO rule_disable
+		(user_id, org_id, rule_id, error_key, justification, created_at, updated_at)
+		VALUES
+		($1, $2, $3, $4, $5, $6, $7)
+	`,
+		testdata.UserID,
+		testdata.OrgID,
+		testdata.Rule1ID,
+		testdata.ErrorKey1,
+		"",
+		time.Now(),
+		time.Now(),
+	)
+	helpers.FailOnError(t, err)
+
+	err = migration.SetDBVersion(db, dbDriver, 30)
+	helpers.FailOnError(t, err)
+
+	err = db.QueryRow(`SELECT user_id FROM rule_disable`).Err()
+	assert.Error(t, err, "user_id column should not exist")
+
+	err = migration.SetDBVersion(db, dbDriver, 29)
+	helpers.FailOnError(t, err)
+
+	var userID types.UserID
+	err = db.QueryRow(`
+	SELECT
+		user_id
+	FROM
+		rule_disable
+	`,
+	).Scan(
+		&userID,
+	)
+	helpers.FailOnError(t, err)
+
+	// default value on stepdown
+	assert.Equal(t, userID, types.UserID("-1"))
+}
diff --git a/migration/mig_0029_alter_cluster_rule_toggle_user_id.go b/migration/mig_0029_alter_cluster_rule_toggle_user_id.go
@@ -12,11 +12,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 //
-// This migration drops the user_id columns from all tables.
-// Some tables have the user_id in the PRIMARY KEY, but we should be OK dropping it
-// anyway because user_id was actually account_number, so the tables shouldn't have
-// duplicate records per rule (or per cluster) per organization.
-// Organization IDs were added and populated in previous migrations.
+// This migration drops the user_id columns from the
+// cluster_rule_toggle table. This table doesn't have the user_id
+// in the constraint(s), so we can remove the column without needing to
+// alter it.
 
 package migration
 

diff --git a/migration/mig_0030_alter_rule_disable_drop_user_id.go b/migration/mig_0030_alter_rule_disable_drop_user_id.go
@@ -0,0 +1,56 @@
+// Copyright 2022 Red Hat, Inc
+//
+// Licensed 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.
+//
+// This migration drops the user_id columns from the
+// rule_disable table. This table doesn't have the user_id
+// in the constraint(s), so we can remove the column without needing to
+// alter it.
+
+package migration
+
+import (
+	"database/sql"
+	"fmt"
+
+	"github.com/RedHatInsights/insights-results-aggregator/types"
+)
+
+const (
+	ruleDisableTable = "rule_disable"
+)
+
+var mig0030DropRuleDisableUserIDColumn = Migration{
+	StepUp: func(tx *sql.Tx, driver types.DBDriver) error {
+		if driver == types.DBDriverPostgres {
+			dropColumnQuery := fmt.Sprintf(alterTableDropColumnQuery, ruleDisableTable, userIDColumn)
+			_, err := tx.Exec(dropColumnQuery)
+			if err != nil {
+				return err
+			}
+		}
+
+		return nil
+	},
+	StepDown: func(tx *sql.Tx, driver types.DBDriver) error {
+		if driver == types.DBDriverPostgres {
+			addColumnQuery := fmt.Sprintf(alterTableAddVarcharColumn, ruleDisableTable, userIDColumn)
+			_, err := tx.Exec(addColumnQuery)
+			if err != nil {
+				return err
+			}
+		}
+
+		return nil
+	},
+}
diff --git a/migration/migrations.go b/migration/migrations.go
@@ -46,4 +46,5 @@ var migrations = []Migration{
 	mig0027CleanupInvalidRowsMissingOrgID,
 	mig0028AlterRuleDisablePKAndIndex,
 	mig0029DropClusterRuleToggleUserIDColumn,
+	mig0030DropRuleDisableUserIDColumn,
 }
diff --git a/openapi.json b/openapi.json
@@ -1124,7 +1124,7 @@
         ]
       }
     },
-    "/clusters/{clusterId}/rules/{ruleId}/error_key/{errorKey}/organizations/{orgId}/users/{userId}/enable": {
+    "/clusters/{clusterId}/rules/{ruleId}/error_key/{errorKey}/organizations/{orgId}/enable": {
       "put": {
         "summary": "Re-enables a rule/health check recommendation for specified cluster",
         "operationId": "enableRule",
@@ -1172,16 +1172,6 @@
               "type": "string"
             },
             "example": "42"
-          },
-          {
-            "name": "userId",
-            "in": "path",
-            "required": true,
-            "description": "ID of the user",
-            "schema": {
-              "type": "string"
-            },
-            "example": "1234"
           }
         ],
         "responses": {
@@ -1208,7 +1198,7 @@
         ]
       }
     },
-    "/clusters/{clusterId}/rules/{ruleId}/error_key/{errorKey}/organizations/{orgId}/users/{userId}/disable": {
+    "/clusters/{clusterId}/rules/{ruleId}/error_key/{errorKey}/organizations/{orgId}/disable": {
       "put": {
         "summary": "Disables a rule/health check recommendation for specified cluster",
         "operationId": "disableRule",
@@ -1256,16 +1246,6 @@
               "type": "string"
             },
             "example": "42"
-          },
-          {
-            "name": "userId",
-            "in": "path",
-            "required": true,
-            "description": "ID of the user",
-            "schema": {
-              "type": "string"
-            },
-            "example": "1234"
           }
         ],
         "responses": {
@@ -1924,7 +1904,7 @@
         }
       }
     },
-    "/rules/{rule_id}/error_key/{error_key}/organizations/{org_id}/users/{user_id}/update": {
+    "/rules/{rule_id}/error_key/{error_key}/organizations/{org_id}/update": {
       "post": {
         "summary": "Updates disable justification of a rule disabled for all clusters",
         "operationId": "UpdateRuleSystemWide",
@@ -1956,15 +1936,6 @@
             "schema": {
               "type": "string"
             }
-          },
-          {
-            "name": "user_id",
-            "in": "path",
-            "required": true,
-            "description": "Numeric ID of the user. An example: `42`",
-            "schema": {
-              "type": "string"
-            }
           }
         ],
         "requestBody": {
@@ -1986,7 +1957,7 @@
         }
       }
     },
-    "/rules/{rule_id}/error_key/{error_key}/organizations/{org_id}/users/{user_id}": {
+    "/rules/{rule_id}/error_key/{error_key}/organizations/{org_id}": {
       "get": {
         "summary": "Returns info about rule disabled system-wide",
         "operationId": "ReadRuleSystemWide",
@@ -2018,15 +1989,6 @@
             "schema": {
               "type": "string"
             }
-          },
-          {
-            "name": "user_id",
-            "in": "path",
-            "required": true,
-            "description": "Numeric ID of the user. An example: `42`",
-            "schema": {
-              "type": "string"
-            }
           }
         ],
         "responses": {
@@ -2039,7 +2001,7 @@
         }
       }
     },
-    "/rules/organizations/{org_id}/users/{user_id}/disabled_system_wide": {
+    "/rules/organizations/{org_id}/disabled_system_wide": {
       "get": {
         "summary": "Returns a list of all rules disabled system-wide",
         "operationId": "ListOfDisabledRulesSystemWide",
@@ -2053,15 +2015,6 @@
             "schema": {
               "type": "string"
             }
-          },
-          {
-            "name": "user_id",
-            "in": "path",
-            "required": true,
-            "description": "Numeric ID of the user. An example: `42`",
-            "schema": {
-              "type": "string"
-            }
           }
         ],
         "responses": {

diff --git a/server/endpoints.go b/server/endpoints.go
@@ -75,15 +75,15 @@ const (
 	// Endpoints to handle rules to be enabled, disabled, updated, and queried system-wide
 
 	// EnableRuleSystemWide re-enables a rule for all clusters
-	EnableRuleSystemWide = "rules/{rule_id}/error_key/{error_key}/organizations/{org_id}/users/{user_id}/enable"
+	EnableRuleSystemWide = "rules/{rule_id}/error_key/{error_key}/organizations/{org_id}/enable"
 	// DisableRuleSystemWide disables a rule for all clusters
-	DisableRuleSystemWide = "rules/{rule_id}/error_key/{error_key}/organizations/{org_id}/users/{user_id}/disable"
+	DisableRuleSystemWide = "rules/{rule_id}/error_key/{error_key}/organizations/{org_id}/disable"
 	// UpdateRuleSystemWide updates disable justification of a rule for all clusters
-	UpdateRuleSystemWide = "rules/{rule_id}/error_key/{error_key}/organizations/{org_id}/users/{user_id}/update"
+	UpdateRuleSystemWide = "rules/{rule_id}/error_key/{error_key}/organizations/{org_id}/update"
 	// ReadRuleSystemWide queries rule disabled system-wide
-	ReadRuleSystemWide = "rules/{rule_id}/error_key/{error_key}/organizations/{org_id}/users/{user_id}"
+	ReadRuleSystemWide = "rules/{rule_id}/error_key/{error_key}/organizations/{org_id}/"
 	// ListOfDisabledRulesSystemWide returns a list of rules disabled from current account
-	ListOfDisabledRulesSystemWide = "rules/organizations/{org_id}/users/{user_id}/disabled_system_wide"
+	ListOfDisabledRulesSystemWide = "rules/organizations/{org_id}/disabled_system_wide"
 
 	// RecommendationsListEndpoint receives a list of clusters in POST body and returns a list of all recommendations hitting for them
 	RecommendationsListEndpoint = "recommendations/organizations/{org_id}/users/{user_id}/list"