Nicely handle missing user in collaborations #17049

zeripath · 2021-09-14T19:40:58Z

zeripath commented

2021-09-14 19:40:58 +00:00

It is possible to have a collaboration in a repository which refers to a no-longer
existing user. This causes the repository transfer to fail with an unusual error.

This PR makes repo.getCollaborators() nicely handle the missing user by ghosting
the collaboration but also adds consistency check. It also adds an
Access consistency check.

Fix #17044

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

It is possible to have a collaboration in a repository which refers to a no-longer existing user. This causes the repository transfer to fail with an unusual error. This PR makes `repo.getCollaborators()` nicely handle the missing user by ghosting the collaboration but also adds consistency check. It also adds an Access consistency check. Fix #17044 Signed-off-by: Andrew Thornton <art27@cantab.net>

delvh reviewed 2021-09-14 22:04:39 +00:00

modules/doctor/dbconsistency.go

delvh commented

2021-09-14 22:04:39 +00:00

I would say that you can combine all these lines into four calls of a new function named checkOrphanedAndAutoFix or something like that takes nothing else than a few string parameters to specify what is queried and what is logged.

I would say that you can combine all these lines into four calls of a new function named `checkOrphanedAndAutoFix` or something like that takes nothing else than a few string parameters to specify what is queried and what is logged.

zeripath reviewed 2021-09-15 06:34:53 +00:00

modules/doctor/dbconsistency.go

zeripath commented

2021-09-15 06:34:53 +00:00

I would also agree - there's a definite need to completely refactor this consistency check function.

lafriks (Migrated from github.com) approved these changes 2021-09-15 22:24:03 +00:00

delvh reviewed 2021-09-15 22:38:09 +00:00

modules/doctor/dbconsistency.go

delvh commented

2021-09-15 22:38:09 +00:00

Is it a good idea to cancel all db consistency checks immediately when one consistency check throws an error?
I think it would be better to
a) only log them.
or
b) collect them and report them afterward.
Otherwise, the database can be more broken than necessary as potentially helpful consistency checks will not be executed because of one error.

The only thing I am unsure about is how to handle the return value in that case.

Is it a good idea to cancel all db consistency checks immediately when one consistency check throws an error? I think it would be better to a) only log them. or b) collect them and report them afterward. Otherwise, the database can be more broken than necessary as potentially helpful consistency checks will not be executed because of one error. The only thing I am unsure about is how to handle the return value in that case.

zeripath reviewed 2021-09-15 22:42:52 +00:00

modules/doctor/dbconsistency.go

zeripath commented

2021-09-15 22:42:52 +00:00

Look at what kind of thing causes an error here.

These are critical errors that mean that the db is not working properly

So it is appropriate to stop.

Look at what kind of thing causes an error here. These are critical errors that mean that the db is not working properly So it is appropriate to stop.

KN4CK3R (Migrated from github.com) reviewed 2021-09-16 09:06:05 +00:00

models/repo_collaboration.go

KN4CK3R (Migrated from github.com) commented

2021-09-16 09:06:04 +00:00

			if IsErrUserNotExist(err) {
				log.Warn("Inconsistent DB: User: %d is listed as collaborator of %-v but does not exist", c.UserID, repo)
				user = NewGhostUser()
			} else {
				return nil, err
			}

```suggestion if IsErrUserNotExist(err) { log.Warn("Inconsistent DB: User: %d is listed as collaborator of %-v but does not exist", c.UserID, repo) user = NewGhostUser() } else { return nil, err } ```