Create Proper Migration Tests (#15116)

* Create Proper Migration tests

Unfortunately our testing regime has so far meant that migrations do not
get proper testing.

This PR begins the process of creating migration tests for this.

* Add test for v176

* fix mssql drop db

Signed-off-by: Andrew Thornton <art27@cantab.net>

Makefile

@@ -89,7 +89,7 @@ LDFLAGS := $(LDFLAGS) -X "main.MakeVersion=$(MAKE_VERSION)" -X "main.Version=$(G
 
 LINUX_ARCHS ?= linux/amd64,linux/386,linux/arm-5,linux/arm-6,linux/arm64
 
-GO_PACKAGES ?= $(filter-out code.gitea.io/gitea/integrations/migration-test,$(filter-out code.gitea.io/gitea/integrations,$(shell $(GO) list -mod=vendor ./... | grep -v /vendor/)))
+GO_PACKAGES ?= $(filter-out code.gitea.io/gitea/models/migrations code.gitea.io/gitea/integrations/migration-test code.gitea.io/gitea/integrations,$(shell $(GO) list -mod=vendor ./... | grep -v /vendor/))
 
 FOMANTIC_CONFIGS := semantic.json web_src/fomantic/theme.config.less web_src/fomantic/_site/globals/site.variables
 FOMANTIC_DEST := web_src/fomantic/build/semantic.js web_src/fomantic/build/semantic.css
@@ -399,8 +399,9 @@ test-sqlite\#%: integrations.sqlite.test generate-ini-sqlite
 	GITEA_ROOT="$(CURDIR)" GITEA_CONF=integrations/sqlite.ini ./integrations.sqlite.test -test.run $(subst .,/,$*)
 
 .PHONY: test-sqlite-migration
-test-sqlite-migration:  migrations.sqlite.test generate-ini-sqlite
+test-sqlite-migration:  migrations.sqlite.test migrations.individual.sqlite.test generate-ini-sqlite
 	GITEA_ROOT="$(CURDIR)" GITEA_CONF=integrations/sqlite.ini ./migrations.sqlite.test
+	GITEA_ROOT="$(CURDIR)" GITEA_CONF=integrations/sqlite.ini ./migrations.individual.sqlite.test
 
 generate-ini-mysql:
 	sed -e 's|{{TEST_MYSQL_HOST}}|${TEST_MYSQL_HOST}|g' \
@@ -419,8 +420,9 @@ test-mysql\#%: integrations.mysql.test generate-ini-mysql
 	GITEA_ROOT="$(CURDIR)" GITEA_CONF=integrations/mysql.ini ./integrations.mysql.test -test.run $(subst .,/,$*)
 
 .PHONY: test-mysql-migration
-test-mysql-migration: migrations.mysql.test generate-ini-mysql
+test-mysql-migration: migrations.mysql.test migrations.individual.mysql.test generate-ini-mysql
 	GITEA_ROOT="$(CURDIR)" GITEA_CONF=integrations/mysql.ini ./migrations.mysql.test
+	GITEA_ROOT="$(CURDIR)" GITEA_CONF=integrations/mysql.ini ./migrations.individual.mysql.test
 
 generate-ini-mysql8:
 	sed -e 's|{{TEST_MYSQL8_HOST}}|${TEST_MYSQL8_HOST}|g' \
@@ -439,8 +441,9 @@ test-mysql8\#%: integrations.mysql8.test generate-ini-mysql8
 	GITEA_ROOT="$(CURDIR)" GITEA_CONF=integrations/mysql8.ini ./integrations.mysql8.test -test.run $(subst .,/,$*)
 
 .PHONY: test-mysql8-migration
-test-mysql8-migration: migrations.mysql8.test generate-ini-mysql8
+test-mysql8-migration: migrations.mysql8.test migrations.individual.mysql8.test generate-ini-mysql8
 	GITEA_ROOT="$(CURDIR)" GITEA_CONF=integrations/mysql8.ini ./migrations.mysql8.test
+	GITEA_ROOT="$(CURDIR)" GITEA_CONF=integrations/mysql8.ini ./migrations.individual.mysql8.test
 
 generate-ini-pgsql:
 	sed -e 's|{{TEST_PGSQL_HOST}}|${TEST_PGSQL_HOST}|g' \
@@ -460,8 +463,9 @@ test-pgsql\#%: integrations.pgsql.test generate-ini-pgsql
 	GITEA_ROOT="$(CURDIR)" GITEA_CONF=integrations/pgsql.ini ./integrations.pgsql.test -test.run $(subst .,/,$*)
 
 .PHONY: test-pgsql-migration
-test-pgsql-migration: migrations.pgsql.test generate-ini-pgsql
+test-pgsql-migration: migrations.pgsql.test migrations.individual.pgsql.test generate-ini-pgsql
 	GITEA_ROOT="$(CURDIR)" GITEA_CONF=integrations/pgsql.ini ./migrations.pgsql.test
+	GITEA_ROOT="$(CURDIR)" GITEA_CONF=integrations/pgsql.ini ./migrations.individual.pgsql.test
 
 generate-ini-mssql:
 	sed -e 's|{{TEST_MSSQL_HOST}}|${TEST_MSSQL_HOST}|g' \
@@ -480,8 +484,9 @@ test-mssql\#%: integrations.mssql.test generate-ini-mssql
 	GITEA_ROOT="$(CURDIR)" GITEA_CONF=integrations/mssql.ini ./integrations.mssql.test -test.run $(subst .,/,$*)
 
 .PHONY: test-mssql-migration
-test-mssql-migration: migrations.mssql.test generate-ini-mssql
+test-mssql-migration: migrations.mssql.test migrations.individual.mssql.test generate-ini-mssql
 	GITEA_ROOT="$(CURDIR)" GITEA_CONF=integrations/mssql.ini ./migrations.mssql.test -test.failfast
+	GITEA_ROOT="$(CURDIR)" GITEA_CONF=integrations/mssql.ini ./migrations.individual.mssql.test -test.failfast
 
 .PHONY: bench-sqlite
 bench-sqlite: integrations.sqlite.test generate-ini-sqlite
@@ -541,6 +546,26 @@ migrations.mssql.test: $(GO_SOURCES)
 migrations.sqlite.test: $(GO_SOURCES)
 	$(GO) test $(GOTESTFLAGS) -c code.gitea.io/gitea/integrations/migration-test -o migrations.sqlite.test -tags '$(TEST_TAGS)'
 
+.PHONY: migrations.individual.mysql.test
+migrations.individual.mysql.test: $(GO_SOURCES)
+	$(GO) test $(GOTESTFLAGS) -c code.gitea.io/gitea/models/migrations -o migrations.individual.mysql.test
+
+.PHONY: migrations.individual.mysql8.test
+migrations.individual.mysql8.test: $(GO_SOURCES)
+	$(GO) test $(GOTESTFLAGS) -c code.gitea.io/gitea/models/migrations -o migrations.individual.mysql8.test
+
+.PHONY: migrations.individual.pgsql.test
+migrations.individual.pgsql.test: $(GO_SOURCES)
+	$(GO) test $(GOTESTFLAGS) -c code.gitea.io/gitea/models/migrations -o migrations.individual.pgsql.test
+
+.PHONY: migrations.individual.mssql.test
+migrations.individual.mssql.test: $(GO_SOURCES)
+	$(GO) test $(GOTESTFLAGS) -c code.gitea.io/gitea/models/migrations -o migrations.individual.mssql.test
+
+.PHONY: migrations.individual.sqlite.test
+migrations.individual.sqlite.test: $(GO_SOURCES)
+	$(GO) test $(GOTESTFLAGS) -c code.gitea.io/gitea/models/migrations -o migrations.individual.sqlite.test -tags '$(TEST_TAGS)'
+
 .PHONY: check
 check: test
 

models/migrations/fixtures/Test_deleteOrphanedIssueLabels/issue_label.yml

@@ -0,0 +1,29 @@
+# Issue_Label 1 should not be deleted
+-
+  id: 1
+  issue_id: 1
+  label_id: 1
+
+# Issue_label 2 should be deleted
+-
+  id: 2
+  issue_id: 5
+  label_id: 99
+
+# Issue_Label 3 should not be deleted
+-
+  id: 3
+  issue_id: 2
+  label_id: 1
+
+# Issue_Label 4 should not be deleted
+-
+  id: 4
+  issue_id: 2
+  label_id: 4
+
+-
+  id: 5
+  issue_id: 2
+  label_id: 87
+

models/migrations/fixtures/Test_deleteOrphanedIssueLabels/label.yml

@@ -0,0 +1,43 @@
+-
+  id: 1
+  repo_id: 1
+  org_id: 0
+  name: label1
+  color: '#abcdef'
+  num_issues: 2
+  num_closed_issues: 0
+
+-
+  id: 2
+  repo_id: 1
+  org_id: 0
+  name: label2
+  color: '#000000'
+  num_issues: 1
+  num_closed_issues: 1
+-
+  id: 3
+  repo_id: 0
+  org_id:  3
+  name: orglabel3
+  color: '#abcdef'
+  num_issues: 0
+  num_closed_issues: 0
+
+-
+  id: 4
+  repo_id: 0
+  org_id: 3
+  name: orglabel4
+  color: '#000000'
+  num_issues: 1
+  num_closed_issues: 0
+
+-
+  id: 5
+  repo_id: 10
+  org_id: 0
+  name: pull-test-label
+  color: '#000000'
+  num_issues: 0
+  num_closed_issues: 0

models/migrations/fixtures/Test_removeInvalidLabels/comment.yml

@@ -0,0 +1,52 @@
+# type Comment struct {
+#   ID      int64 `xorm:"pk autoincr"`
+#   Type    int   `xorm:"INDEX"`
+#   IssueID int64 `xorm:"INDEX"`
+#   LabelID int64
+# }
+#
+# we are only interested in type 7
+#
+
+-
+  id: 1 # Should remain
+  type: 6
+  issue_id: 1
+  label_id: 0
+  should_remain: true
+-
+  id: 2 # Should remain
+  type: 7
+  issue_id: 1 # repo_id: 1
+  label_id: 1 # repo_id: 1
+  should_remain: true
+-
+  id: 3 # Should remain
+  type: 7
+  issue_id: 2 # repo_id: 2 owner_id: 1
+  label_id: 2 # org_id: 1
+  should_remain: true
+-
+  id: 4 # Should be DELETED
+  type: 7
+  issue_id: 1 # repo_id: 1
+  label_id: 3 # repo_id: 2
+  should_remain: false
+-
+  id: 5 # Should remain
+  type: 7
+  issue_id: 3 # repo_id: 1
+  label_id: 1 # repo_id: 1
+  should_remain: true
+-
+  id: 6 # Should be DELETED
+  type: 7
+  issue_id: 3 # repo_id: 1 owner_id: 2
+  label_id: 2 # org_id: 1
+  should_remain: false
+-
+  id: 7 # Should be DELETED
+  type: 7
+  issue_id: 3 # repo_id: 1 owner_id: 2
+  label_id: 5 # repo_id: 3
+  should_remain: false

models/migrations/fixtures/Test_removeInvalidLabels/issue.yml

@@ -0,0 +1,21 @@
+# type Issue struct {
+#   ID     int64 `xorm:"pk autoincr"`
+#   RepoID int64 `xorm:"INDEX UNIQUE(repo_index)"`
+#   Index  int64 `xorm:"UNIQUE(repo_index)"` // Index in one repository.
+# }
+-
+  id: 1
+  repo_id: 1
+  index: 1
+-
+  id: 2
+  repo_id: 2
+  index: 1
+-
+  id: 3
+  repo_id: 1
+  index: 2
+-
+  id: 4
+  repo_id: 3
+  index: 1

models/migrations/fixtures/Test_removeInvalidLabels/issue_label.yml

@@ -0,0 +1,35 @@
+# type IssueLabel struct {
+#   ID      int64 `xorm:"pk autoincr"`
+#   IssueID int64 `xorm:"UNIQUE(s)"`
+#   LabelID int64 `xorm:"UNIQUE(s)"`
+# }
+-
+  id: 1 # Should remain - matches comment 2
+  issue_id: 1
+  label_id: 1
+  should_remain: true
+-
+  id: 2 # Should remain
+  issue_id: 2
+  label_id: 2
+  should_remain: true
+-
+  id: 3 # Should be deleted
+  issue_id: 1
+  label_id: 3
+  should_remain: false
+-
+  id: 4 # Should remain
+  issue_id: 3
+  label_id: 1
+  should_remain: true
+-
+  id: 5 # Should be deleted
+  issue_id: 3
+  label_id: 2
+  should_remain: false
+-
+  id: 6 # Should be deleted
+  issue_id: 3
+  label_id: 5
+  should_remain: false

models/migrations/fixtures/Test_removeInvalidLabels/label.yml

@@ -0,0 +1,26 @@
+# type Label struct {
+#   ID     int64 `xorm:"pk autoincr"`
+#   RepoID int64 `xorm:"INDEX"`
+#   OrgID  int64 `xorm:"INDEX"`
+# }
+-
+  id: 1
+  repo_id: 1
+  org_id: 0
+-
+  id: 2
+  repo_id: 0
+  org_id: 1
+-
+  id: 3
+  repo_id: 2
+  org_id: 0
+-
+  id: 4
+  repo_id: 1
+  org_id: 0
+-
+  id: 5
+  repo_id: 3
+  org_id: 0
+

models/migrations/fixtures/Test_removeInvalidLabels/repository.yml

@@ -0,0 +1,17 @@
+# type Repository struct {
+#   ID        int64  `xorm:"pk autoincr"`
+#   OwnerID   int64  `xorm:"UNIQUE(s) index"`
+#   LowerName string `xorm:"UNIQUE(s) INDEX NOT NULL"`
+# }
+-
+  id: 1
+  owner_id: 2
+  lower_name: "repo1"
+-
+  id: 2
+  owner_id: 1
+  lower_name: "repo2"
+-
+  id: 3
+  owner_id: 2
+  lower_name: "repo3"

models/migrations/migrations.go

@@ -819,9 +819,24 @@ func dropTableColumns(sess *xorm.Session, tableName string, columnNames ...strin
 		}
 		for _, constraint := range constraints {
 			if _, err := sess.Exec(fmt.Sprintf("ALTER TABLE `%s` DROP CONSTRAINT `%s`", tableName, constraint)); err != nil {
-				return fmt.Errorf("Drop table `%s` constraint `%s`: %v", tableName, constraint, err)
+				return fmt.Errorf("Drop table `%s` default constraint `%s`: %v", tableName, constraint, err)
 			}
 		}
+		sql = fmt.Sprintf("SELECT DISTINCT Name FROM SYS.INDEXES INNER JOIN SYS.INDEX_COLUMNS ON INDEXES.INDEX_ID = INDEX_COLUMNS.INDEX_ID AND INDEXES.OBJECT_ID = INDEX_COLUMNS.OBJECT_ID WHERE INDEXES.OBJECT_ID = OBJECT_ID('%[1]s') AND INDEX_COLUMNS.COLUMN_ID IN (SELECT column_id FROM sys.columns WHERE lower(NAME) IN (%[2]s) AND object_id = OBJECT_ID('%[1]s'))",
+			tableName, strings.ReplaceAll(cols, "`", "'"))
+		constraints = make([]string, 0)
+		if err := sess.SQL(sql).Find(&constraints); err != nil {
+			return fmt.Errorf("Find constraints: %v", err)
+		}
+		for _, constraint := range constraints {
+			if _, err := sess.Exec(fmt.Sprintf("ALTER TABLE `%s` DROP CONSTRAINT IF EXISTS `%s`", tableName, constraint)); err != nil {
+				return fmt.Errorf("Drop table `%s` index constraint `%s`: %v", tableName, constraint, err)
+			}
+			if _, err := sess.Exec(fmt.Sprintf("DROP INDEX IF EXISTS `%[2]s` ON `%[1]s`", tableName, constraint)); err != nil {
+				return fmt.Errorf("Drop index `%[2]s` on `%[1]s`: %v", tableName, constraint, err)
+			}
+		}
+
 		if _, err := sess.Exec(fmt.Sprintf("ALTER TABLE `%s` DROP COLUMN %s", tableName, cols)); err != nil {
 			return fmt.Errorf("Drop table `%s` columns %v: %v", tableName, columnNames, err)
 		}