Database should use foreign keys

[lesderid]

• Gitea version (or commit ref): 1.1.0
• Git version: Not relevant
• Operating system: Not relevant
• Database:
  • PostgreSQL
  • MySQL
  • MSSQL
  • SQLite
• Can you reproduce the bug at https://try.gitea.io:
  • Yes (provide example URL)
  • No
  • Not relevant
• Log gist:

Description

Gitea's database should use foreign keys. This is much cleaner and makes it easier to do maintenance on the database (e.g. removing spam) without forgetting to delete certain records or deleting too many records.

[andreynering]

Foreign keys are important, and it was discussed before.

The problem is that a migration that adds foreign keys will break on installations that already have orphan DB registries.

Maybe we should try to create references but ignore any error.

[lunny]

I don't like foreign key. It means your database data could be changed by your Go code and database events.

[travnick]

I like foreign keys.
It ensures your data integrity. Also you will get an error if trying do to same stupid things (bugs).

http://dbadiaries.com/foreign-key-constraints

[strk]

I agree about using foreign key, but before being able to do that we'll need a way to ensure they can be added and a way to deal with the case in which they cannot. So maybe an admin panel to find orphaned/broken records and repair them ?

[sailfish009]

i don't like foreign key, data must be Immutable.

[tboerger]

Foreign keys are important to enforce a sane database

[travnick]

@sailfish009 No one enforces you to set foreign keys as CASCADE. There is also RESTRICT option.

[bkcsoft]

While I like the idea of foreign keys for some things (comments on issues, repos on org/users), you'd still need to be careful with that relations you setup. If a user deletes their account, do you want their comments to go away as well? For any kind of corporate development, that would be considered a bug as potentially vital information might go lost if a user is deleted when an employee quits.

Aside from consistency, there's also performance gains to be had by having the RMDBS taking care of clean-up instead of having Go do that (Go really shouldn't be doing that...). But at the same time, SQLite and FK aren't really friends. While I don't suggest hampering performance for the sake of being friendly with SQLite, we must still consider these things.

Just my 2 cents.

[jgdye]

I admit that I haven't reviewed the code, but why would you ever delete a user row from the database? Shouldn't you just set a flag to disallow login? If the concern is allowing the username to be recycled, wouldn't that cause the same issue with regard to comments, etc as FKs and deleting the user?

[lafriks]

@jgdye but currently everywhere they are deleted so it is not possible to set on existing installs

[lesderid]

@jgdye Removing spam is my main use case.

[jgdye]

@lafricks Any FK solution is going to require large database adjustments on existing installs. I'm not a Go programmer, but I would suggest two stages to implementation. Start with adding the active flag and deactivating instead of removing rows, and when a FK solution that works is finished, have a maintenance option to unorphan comments by creating deactivated users. Perhaps also a config flag that tells Gitea whether to try to create the FKs or not.
@lesderid For the spam case I would think offering a choice of disabling and keeping comments or a cascading delete that removes all comments if a spammer would work well. If it's a spammer that has also made real contributions you don't want to lose, disabling instead of deleting would also prevent them coming registering again with the same name. FKs would help in the nuke the entire account case.

[jgdye]

@lafriks apologies for the misspelling. Editing on my phone and couldn't see your username while typing.

[bkcsoft]

FKs in SQLite is a no-op if not enabled so you could just setup the FKs and then continue to delete children as well, for Postgres etc the Delete-op would essentially be a no-op, and for SQLite we ensure that we don't leave orphans around.

As for already orphaned data, just have the migration delete orphans before running Sync on that table.

Also, as always I'd recommend doing this incrementally, 1 or 2 tables per PR. Less bugs, easier reviews, easier to revert if it breaks 🙂