Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check primary keys for all tables and drop ForeignReference #21721

Merged
merged 18 commits into from
Dec 23, 2022
Merged

Check primary keys for all tables and drop ForeignReference #21721

merged 18 commits into from
Dec 23, 2022

Conversation

wolfogre
Copy link
Member

@wolfogre wolfogre commented Nov 8, 2022

Some dbs require that all tables have primary keys, see

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.

@wolfogre

This comment was marked as resolved.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 8, 2022
@wolfogre wolfogre added status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR type/testing labels Nov 8, 2022
@zeripath
Copy link
Contributor

zeripath commented Dec 4, 2022

@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

Signed-off-by: Andrew Thornton <art27@cantab.net>
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 4, 2022
zeripath and others added 2 commits December 4, 2022 20:00
Signed-off-by: Andrew Thornton <art27@cantab.net>
@wolfogre
Copy link
Member Author

wolfogre commented Dec 5, 2022

@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
Copy link
Contributor

zeripath commented Dec 5, 2022

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
Copy link
Contributor

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

@wolfogre
Copy link
Member Author

wolfogre commented Dec 5, 2022

😂 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.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor

zeripath commented Dec 5, 2022

@wolfogre I've fixed the migration.

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

@wxiaoguang
Copy link
Contributor

@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
Copy link
Member

lunny commented Dec 23, 2022

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 .

@wolfogre wolfogre removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Dec 23, 2022
@wxiaoguang
Copy link
Contributor

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
Copy link
Member Author

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.

@wolfogre wolfogre added the pr/wip This PR is not ready for review label Dec 23, 2022
@wolfogre wolfogre removed the pr/wip This PR is not ready for review label Dec 23, 2022
@wolfogre wolfogre added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Dec 23, 2022
@wolfogre wolfogre changed the title Check primary keys for all tables Check primary keys for all tables and drop ForeignReference Dec 23, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 23, 2022
@lunny lunny merged commit 71ca306 into go-gitea:main Dec 23, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 26, 2022
* upstream/main:
  Add Mermaid copy button, avoid unnecessary tooltip hide (go-gitea#22225)
  [skip ci] Updated licenses and gitignores
  Improve testing for pgsql empty repository (go-gitea#22223)
  JS refactors (go-gitea#22227)
  Check primary keys for all tables and drop ForeignReference (go-gitea#21721)
lunny pushed a commit that referenced this pull request Feb 7, 2023
Fix #22581

TLDR: #18446 made a mess with ForeignIndex and triggered a design
flaw/bug of #16356, then a quick patch #21271 helped #18446, then the
the bug was re-triggered by #21721 .

Related:
* #16356
* BasicIssueContext
https://github.com/go-gitea/gitea/pull/16356/files#diff-7938eb670d42a5ead6b08121e16aa4537a4d716c1cf37923c70470020fb9d036R16-R27
* #18446 
* If some issues were dumped without ForeignIndex, then they would be
imported as ForeignIndex=0
https://github.com/go-gitea/gitea/pull/18446/files#diff-1624a3e715d8fc70edf2db1630642b7d6517f8c359cc69d58c3958b34ba4ce5eR38-R39
* #21271
* It patched the above bug (somewhat), made the issues without
ForeignIndex could have the same value as LocalIndex
* #21721 
    * It re-triggered the zero-ForeignIndex bug.


ps: I am not sure whether the changes in `GetForeignIndex` are ideal (at
least, now it has almost the same behavior as BasicIssueContext in
#16356), it's just a quick fix. Feel free to edit on this PR directly or
replace it.

Co-authored-by: zeripath <art27@cantab.net>
yardenshoham pushed a commit to yardenshoham/gitea that referenced this pull request Feb 7, 2023
…2776)

Fix go-gitea#22581

TLDR: go-gitea#18446 made a mess with ForeignIndex and triggered a design
flaw/bug of go-gitea#16356, then a quick patch go-gitea#21271 helped go-gitea#18446, then the
the bug was re-triggered by go-gitea#21721 .

Related:
* go-gitea#16356
* BasicIssueContext
https://github.com/go-gitea/gitea/pull/16356/files#diff-7938eb670d42a5ead6b08121e16aa4537a4d716c1cf37923c70470020fb9d036R16-R27
* go-gitea#18446 
* If some issues were dumped without ForeignIndex, then they would be
imported as ForeignIndex=0
https://github.com/go-gitea/gitea/pull/18446/files#diff-1624a3e715d8fc70edf2db1630642b7d6517f8c359cc69d58c3958b34ba4ce5eR38-R39
* go-gitea#21271
* It patched the above bug (somewhat), made the issues without
ForeignIndex could have the same value as LocalIndex
* go-gitea#21721 
    * It re-triggered the zero-ForeignIndex bug.


ps: I am not sure whether the changes in `GetForeignIndex` are ideal (at
least, now it has almost the same behavior as BasicIssueContext in
go-gitea#16356), it's just a quick fix. Feel free to edit on this PR directly or
replace it.

Co-authored-by: zeripath <art27@cantab.net>
zeripath pushed a commit that referenced this pull request Feb 8, 2023
…22794)

Backport #22776

Fix #22581

TLDR: #18446 made a mess with ForeignIndex and triggered a design
flaw/bug of #16356, then a quick patch #21271 helped #18446, then the
the bug was re-triggered by #21721 .

Related:
* #16356
* BasicIssueContext
https://github.com/go-gitea/gitea/pull/16356/files#diff-7938eb670d42a5ead6b08121e16aa4537a4d716c1cf37923c70470020fb9d036R16-R27
* #18446 
* If some issues were dumped without ForeignIndex, then they would be
imported as ForeignIndex=0
https://github.com/go-gitea/gitea/pull/18446/files#diff-1624a3e715d8fc70edf2db1630642b7d6517f8c359cc69d58c3958b34ba4ce5eR38-R39
* #21271
* It patched the above bug (somewhat), made the issues without
ForeignIndex could have the same value as LocalIndex
* #21721 
    * It re-triggered the zero-ForeignIndex bug.


ps: I am not sure whether the changes in `GetForeignIndex` are ideal (at
least, now it has almost the same behavior as BasicIssueContext in
#16356), it's just a quick fix. Feel free to edit on this PR directly or
replace it.

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zeripath pushed a commit that referenced this pull request Mar 23, 2023
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Mar 23, 2023
jolheiser pushed a commit that referenced this pull request Mar 24, 2023
Backport #23605 by @wolfogre

Fix
#21086 (comment)

Related to #21721

Co-authored-by: Jason Song <i@wolfogre.com>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Primary keys are required in MySQL DB cluster
6 participants