Refactor DeleteByID #28450

lunny · 2023-12-13T10:17:46Z

This PR introduced a new generic method DeleteByID[T any](ctx context.Context, id int64), so no more deleteByID methods need to be implemented for current models or future models.

lng2020 · 2023-12-17T16:26:38Z

routers/api/actions/artifacts.go

@@ -423,10 +422,10 @@ func (ar artifactRoutes) downloadArtifact(ctx *ArtifactContext) {
 	}

 	artifactID := ctx.ParamsInt64("artifact_id")
-	artifact, err := actions.GetArtifactByID(ctx, artifactID)


xxx, exist, err := db.xxx(xxx)

I think the behavior should be unified. Here it returns 404 first and then returns 500.

lng2020 · 2023-12-17T16:30:45Z

routers/api/v1/repo/mirror.go

 	if err != nil {
 		ctx.Error(http.StatusNotFound, "GetPushMirrors", err)
 		return
+	} else if !exist {
+		ctx.Error(http.StatusNotFound, "GetPushMirrors", nil)
+		return
 	}


But there, they both return 404. And the sequence of !exist and err != nil is different.

lng2020 · 2023-12-17T16:34:25Z

services/mirror/mirror_push.go

@@ -93,7 +94,7 @@ func SyncPushMirror(ctx context.Context, mirrorID int64) bool {
 		log.Error("PANIC whilst syncPushMirror[%d] Panic: %v\nStacktrace: %s", mirrorID, err, log.Stack(2))
 	}()

-	m, err := repo_model.GetPushMirror(ctx, repo_model.PushMirrorOptions{ID: mirrorID})


And there, the exist gets ignored.

lng2020 · 2023-12-17T17:08:15Z

models/db/context.go

+
+func Delete[T any](ctx context.Context, opts FindOptions) (int64, error) {
+	if opts == nil {
+		return 0, ErrConditionRequired{}


I felt this ErrConditionRequired strange.
If I'm going to use the Delete function and forget to add conditions, then it returns an error, I naturally think something is wrong with the database. Maybe the database is somehow disconnected or some other errors. So I probably will use if err != nil and then return the error to the upper function or log the error to remind the user end.
It's unlikely for every developer to check the ErrConditionRequired error. Because 1. it's cumbersome. 2. It's hard to notice because this error was only introduced last week and no one is familiar with it.
Then the error is shown on the user end. But actually, the programmer should be warned instead of the user end.
I prefer to delete this ErrConditionRequired and let the test code find out the problem. But maybe there is a better solution.

lng2020 · 2023-12-17T17:49:16Z

services/user/delete.go

@@ -154,7 +154,7 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error)

 	// ***** START: PublicKey *****
 	if _, err = db.DeleteByBean(ctx, &asymkey_model.PublicKey{OwnerID: u.ID}); err != nil {
-		return fmt.Errorf("deletePublicKeys: %w", err)


Why change this? The code from line 167 to line 180 all uses DeleteByBean and they are not changed.

delvh · 2023-12-15T11:54:01Z

models/system/notice_test.go

@@ -73,7 +73,8 @@ func TestDeleteNotice(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())

 	unittest.AssertExistsAndLoadBean(t, &system.Notice{ID: 3})
-	assert.NoError(t, system.DeleteNotice(db.DefaultContext, 3))
+	_, err := db.DeleteByID[system.Notice](db.DefaultContext, 3)


That test can now be deleted?

delvh · 2023-12-15T11:57:33Z

routers/api/v1/repo/mirror.go

 	if err != nil {
 		ctx.Error(http.StatusNotFound, "GetPushMirrors", err)


NotFound?
Sounds wrong…

delvh · 2023-12-22T11:16:47Z

Is this PR still alive?

Introduce the new generic deletion methods - `func DeleteByID[T any](ctx context.Context, id int64) (int64, error)` - `func DeleteByIDs[T any](ctx context.Context, ids ...int64) error` - `func Delete[T any](ctx context.Context, opts FindOptions) (int64, error)` So, we no longer need any specific deletion method and can just use the generic ones instead. Replacement of #28450 Closes #28450 --------- Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

Introduce the new generic deletion methods - `func DeleteByID[T any](ctx context.Context, id int64) (int64, error)` - `func DeleteByIDs[T any](ctx context.Context, ids ...int64) error` - `func Delete[T any](ctx context.Context, opts FindOptions) (int64, error)` So, we no longer need any specific deletion method and can just use the generic ones instead. Replacement of go-gitea#28450 Closes go-gitea#28450 --------- Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

delete by id

eb4c248

lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Dec 13, 2023

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 13, 2023

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 13, 2023

github-actions bot added the modifies/api This PR adds API routes or modifies them label Dec 13, 2023

lunny added 3 commits December 14, 2023 09:42

Merge branch 'main' into lunny/refactor_deletebyid

ac7f341

more refactors

02dd191

more refactor

4826d05

lunny changed the title ~~WIP: Refactor DeleteByID~~ Refactor DeleteByID Dec 14, 2023

lunny requested review from delvh, fuxiaohei and lng2020 December 17, 2023 11:27

lng2020 reviewed Dec 17, 2023

View reviewed changes

delvh reviewed Dec 17, 2023

View reviewed changes

delvh mentioned this pull request Dec 25, 2023

Refactor deletion #28610

Merged

lunny closed this Dec 25, 2023

lunny deleted the lunny/refactor_deletebyid branch December 25, 2023 12:09

github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor DeleteByID #28450

Refactor DeleteByID #28450

lunny commented Dec 13, 2023 •

edited

Loading

lng2020 Dec 17, 2023

lng2020 Dec 17, 2023

lng2020 Dec 17, 2023

lng2020 Dec 17, 2023

delvh Dec 22, 2023

lng2020 Dec 17, 2023

delvh Dec 15, 2023

delvh Dec 15, 2023

delvh commented Dec 22, 2023

		if err != nil {
		ctx.Error(http.StatusNotFound, "GetPushMirrors", err)

Refactor DeleteByID #28450

Refactor DeleteByID #28450

Conversation

lunny commented Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delvh commented Dec 22, 2023

lunny commented Dec 13, 2023 •

edited

Loading