Check primary keys for all tables and drop ForeignReference

[wolfogre]

Some dbs require that all tables have primary keys, see

• Primary key required in MySQL DB clusters  #16802
• Primary keys are required in MySQL DB cluster #21086

We can add a test to keep it from being broken again.

Edit:

Added missing primary key for ForeignReference Dropped the ForeignReference table to satisfy the check, so it closes #21086.

More context can be found in comments.

[zeripath]

@wolfogre just add a primary key to the ForeignReference table and make a migration to add it. This should do it:

PATCH

From 961c87a313255a1124a7d78786d1a38f7add0e55 Mon Sep 17 00:00:00 2001
From: Andrew Thornton <art27@cantab.net>
Date: Sun, 4 Dec 2022 11:03:50 +0000
Subject: [PATCH] add primary key to foreignreference

Signed-off-by: Andrew Thornton <art27@cantab.net>
---
 models/foreignreference/foreignreference.go   |  2 ++
 .../foreign_reference.yml                     | 25 ++++++++++++++
 models/migrations/migrations.go               |  2 ++
 models/migrations/v1_19/v236.go               | 21 ++++++++++++
 models/migrations/v1_19/v236_test.go          | 34 +++++++++++++++++++
 5 files changed, 84 insertions(+)
 create mode 100644 models/migrations/fixtures/Test_AddPrimaryKeyToForeignReference/foreign_reference.yml
 create mode 100644 models/migrations/v1_19/v236.go
 create mode 100644 models/migrations/v1_19/v236_test.go

diff --git a/models/foreignreference/foreignreference.go b/models/foreignreference/foreignreference.go
index 2d2ad04c5..93f7879fd 100644
--- a/models/foreignreference/foreignreference.go
+++ b/models/foreignreference/foreignreference.go
@@ -19,6 +19,8 @@ const (
 
 // ForeignReference represents external references
 type ForeignReference struct {
+	ID int64 `xorm:"pk autoincr"`
+
 	// RepoID is the first column in all indices. now we only need 2 indices: (repo, local) and (repo, foreign, type)
 	RepoID       int64  `xorm:"UNIQUE(repo_foreign_type) INDEX(repo_local)" `
 	LocalIndex   int64  `xorm:"INDEX(repo_local)"` // the resource key inside Gitea, it can be IssueIndex, or some model ID.
diff --git a/models/migrations/fixtures/Test_AddPrimaryKeyToForeignReference/foreign_reference.yml b/models/migrations/fixtures/Test_AddPrimaryKeyToForeignReference/foreign_reference.yml
new file mode 100644
index 000000000..4512cef79
--- /dev/null
+++ b/models/migrations/fixtures/Test_AddPrimaryKeyToForeignReference/foreign_reference.yml
@@ -0,0 +1,25 @@
+-
+  repo_id: 1
+  local_index: 1
+  foreign_index: "foo"
+  type: "foo"
+-
+  repo_id: 1
+  local_index: 2
+  foreign_index: "bar"
+  type: "foo"
+-
+  repo_id: 2
+  local_index: 2
+  foreign_index: "foo"
+  type: "foo"
+-
+  repo_id: 3
+  local_index: 1024
+  foreign_index: "1"
+  type: "normal"
+-
+  repo_id: 2
+  local_index: 1
+  foreign_index: "bar"
+  type: "foo"
diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go
index e718355f8..5f6eda0fd 100644
--- a/models/migrations/migrations.go
+++ b/models/migrations/migrations.go
@@ -442,6 +442,8 @@ var migrations = []Migration{
 	NewMigration("Add package cleanup rule table", v1_19.CreatePackageCleanupRuleTable),
 	// v235 -> v236
 	NewMigration("Add index for access_token", v1_19.AddIndexForAccessToken),
+	// v236 -> v237
+	NewMigration("Add primary key to foreign reference", v1_19.AddPrimaryKeyToForeignReference),
 }
 
 // GetCurrentDBVersion returns the current db version
diff --git a/models/migrations/v1_19/v236.go b/models/migrations/v1_19/v236.go
new file mode 100644
index 000000000..4271217a4
--- /dev/null
+++ b/models/migrations/v1_19/v236.go
@@ -0,0 +1,21 @@
+// Copyright 2022 Gitea. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package v1_19 //nolint
+
+import "xorm.io/xorm"
+
+func AddPrimaryKeyToForeignReference(x *xorm.Engine) error {
+	// ForeignReference represents external references
+	type ForeignReference struct {
+		ID int64 `xorm:"pk autoincr"`
+
+		// RepoID is the first column in all indices. now we only need 2 indices: (repo, local) and (repo, foreign, type)
+		RepoID       int64  `xorm:"UNIQUE(repo_foreign_type) INDEX(repo_local)" `
+		LocalIndex   int64  `xorm:"INDEX(repo_local)"` // the resource key inside Gitea, it can be IssueIndex, or some model ID.
+		ForeignIndex string `xorm:"INDEX UNIQUE(repo_foreign_type)"`
+		Type         string `xorm:"VARCHAR(16) INDEX UNIQUE(repo_foreign_type)"`
+	}
+
+	return x.Sync(new(ForeignReference))
+}
diff --git a/models/migrations/v1_19/v236_test.go b/models/migrations/v1_19/v236_test.go
new file mode 100644
index 000000000..d3f109845
--- /dev/null
+++ b/models/migrations/v1_19/v236_test.go
@@ -0,0 +1,34 @@
+// Copyright 2022 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package v1_19 //nolint
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/migrations/base"
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_AddPrimaryKeyToForeignReference(t *testing.T) {
+	// ForeignReference represents external references
+	type ForeignReference struct {
+		// RepoID is the first column in all indices. now we only need 2 indices: (repo, local) and (repo, foreign, type)
+		RepoID       int64  `xorm:"UNIQUE(repo_foreign_type) INDEX(repo_local)" `
+		LocalIndex   int64  `xorm:"INDEX(repo_local)"` // the resource key inside Gitea, it can be IssueIndex, or some model ID.
+		ForeignIndex string `xorm:"INDEX UNIQUE(repo_foreign_type)"`
+		Type         string `xorm:"VARCHAR(16) INDEX UNIQUE(repo_foreign_type)"`
+	}
+
+	// Prepare and load the testing database
+	x, deferable := base.PrepareTestEnv(t, 0, new(ForeignReference))
+	defer deferable()
+	if x == nil || t.Failed() {
+		return
+	}
+
+	if err := AddPrimaryKeyToForeignReference(x); err != nil {
+		assert.NoError(t, err)
+		return
+	}
+}
-- 
2.34.1

[wolfogre]

@wolfogre just add a primary key to the ForeignReference table and make a migration to add it. This should do it:

PATCH

@zeripath Maybe we should open another PR to add a primary key and mark it closing #21086, I'll follow up once it has been merged. What do you think?

[zeripath]

Nah we'd be better off getting the primary key in in this pr and adding the check at the same time.

This is almost there. The only problem is that the migration isn't working for sqlite which probably should be changed to use the recreate table for that db. There's a previous migration that does similar for other tables so those should be checked in case I've missed something.

Then we can merge the fix and do the enforcement at the same time. If we don't then the enforcement may never be possible.

[wxiaoguang]

My opinion is to drop that table and remove all dead code.

[wolfogre]

😂 See? The context is too complicated for me, so I prefer to solve the problem of ForeignReference table in another PR. We need a better place to discuss how to do it, adding a primary key? or dropping it?

I believe it should be neither here nor #21086, so a new PR is needed.

[zeripath]

@wolfogre I've fixed the migration.

@wxiaoguang I'm not sure what you mean - is this stuff entirely unused?

[wxiaoguang]

@wolfogre I've fixed the migration.

@wxiaoguang I'm not sure what you mean - is this stuff entirely unused?

Yes, I have read the logic again:

1. It's not used at the moment (only inserts, no read)
2. It's badly designed and doesn't look like to be used any more. Primary keys are required in MySQL DB cluster #21086 (comment)

[lunny]

I think removing ForeignReference needs more work. I prefer to merge this one and removing it on another PR if we really want to remove it. We can find some context in #18446 .

[wxiaoguang]

I think removing ForeignReference needs more work. I prefer to merge this one and removing it on another PR if we really want to remove it. We can find some context in #18446 .

What do you mean by "more work"? At most, 30-minute work is enough, that's far less than the time wasted, then no more dirty and dead code.

[wolfogre]

And now we have a new situation, the CI failed with:

        testlogger.go:77: 2022/12/23 03:20:41 ...igrations/base/db.go:67:RecreateTable() [E] Unable to create uniques for table tmp_recreate__foreign_reference. Error: Error 1061: Duplicate key name 'UQE_tmp_recreate__foreign_reference_repo_foreign_type'
        migration_test.go:307: 
            	Error Trace:	/drone/src/migration_test.go:307
            	            				/drone/src/migration_test.go:335
            	Error:      	Received unexpected error:
            	            	migrate: Error 1061: Duplicate key name 'UQE_tmp_recreate__foreign_reference_repo_foreign_type'
            	Test:       	TestMigrations/Migrate-mysql-1.7.0

It was caused by recreating the ForeignReference table, I have no idea why. As wxiaoguang said, maybe it's a waste of time to figure it out.

I've changed my mind. I'll try to drop the table, let's see if it's easier.