search

pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.
https://pkp.sfu.ca
GNU General Public License v3.0
299 stars 444 forks source link

Setup foreign key constraints #6093

Closed jonasraoni closed 1 month ago

jonasraoni commented 4 years ago

(Note: Remaining special cases have been filed here: https://github.com/pkp/pkp-lib/issues/8333)

Describe the problem you would like to solve The tables are missing foreign key constraints, and that's a source of orphan data/unexpected statistics.

Describe the solution you'd like Add constraints everywhere, considering carefully if deletes in cascade are the best option (easier to deal with and also to have accidents).

Migrations to review/add constraints to:

pkp-lib:

ojs:

omp:

ops:

asmecher commented 4 years ago

If you're looking into this, make sure you're looking at the master branch -- the schema management has all moved to migrations rather than ADODB's schema management toolset.

I'd suggest adding the constraints, then only considering ON DELETE CASCADE as a secondary step.

There will be quirks around the assoc_type/assoc_id pairs -- it's not possible to specify these as conventional foreign keys.

asmecher commented 4 years ago

Draft PRs:

You can use SchemaSpy to generate documentation on the DB schema. See schemaspy.properties in the OJS root for details.

asmecher commented 4 years ago

Borrowed/rewrote some commits to pull pieces of this into master in advance of solving all the various problems that still need attention:

asmecher commented 3 years ago

A weird note about indexes:

MySQL/MariaDB requires that foreign key columns have indexes. If you don't specify one in the table creation, it'll create one for you. If you subsequently do create one, the implicit one will disappear and the one you created will remain. So there's no harm in specifying index creation alongside foreign key constraints when using MySQL/MariaDB.

jonasraoni commented 3 years ago

I never saw this requirement in other databases, anyway that's a good procedure =]

asmecher commented 3 years ago

@Vitaliy-1, and @jonasraoni, would you mind reviewing these two pull requests?

These involve a number of changes -- I kept the commit list clean in case it helps to read them separately.

In terms of introducing foreign key constraints, they only tackle the announcements tables. But they set patterns that I'd like to roll out in future PRs. Of note:

If this all looks OK, I'll port to the other apps and continue with more foreign key constraints etc. along the same lines, in separate PRs.

Vitaliy-1 commented 3 years ago

Migration looks ok to me and, except for unrelated issues, it went fine on my test instance. Left a comment regarding down() method.

asmecher commented 3 years ago

Thanks, @Vitaliy-1, I've resolved the comments you had! @jonasraoni, could you have a quick look? Then I'll merge these PRs and move on with other foreign key declarations following this pattern.

jonasraoni commented 2 years ago

I just left small comments. I didn't run the upgrade, but it would be useful to check it against real data to catch unexpected issues.

asmecher commented 2 years ago

Thanks, @jonasraoni!

Abort rather than attempt to correct data: If I were an user with no IT support, I'd like to have it automatically corrected, then a message like "orphan submissions were detected, setup a default section at xxxx to receive them", presented on failure or in the beginning of the process, would be great.

So far this is just theoretical, as the scripting I've done does clean (minor) things up automatically. But if it's anything that'll potentially change the scholarly record, I do think we should abort and let them fix it rather than make assumptions.

asmecher commented 2 years ago

Additional PRs for OMP and OPS:

asmecher commented 2 years ago

PRs for the Category classes:

I converted them to the EntityDAO toolset as that has better handling of null-but-that-doesn't-mean-unset constraints. This also fixes the storage of category IDs in publication data, which was broken.

asmecher commented 2 years ago

Genres:

asmecher commented 2 years ago

PRs for controlled vocab tables and common migration:

asmecher commented 1 year ago

FKs for user group relationships:

asmecher commented 1 year ago

FKs for user group relationships:

asmecher commented 1 year ago

FKs for submission file relationships:

asmecher commented 1 year ago

Index fixing:

asmecher commented 1 year ago

All implemented -- the remaining special cases have been filed here: https://github.com/pkp/pkp-lib/issues/8333

asmecher commented 1 month ago

@jonasraoni, a tweak is required on this one: you're using ALTER TABLE t RENAME INDEX old_name TO new_name, which is only available in MariaDB as of version 10.5.2. My Ubuntu ships with 10.3.39 and I think there will probably be lots of ISPs using older versions too. Could you check if there's a workaround that'll support older MariaDBs?

asmecher commented 1 month ago

^ Sorry, wrong issue. Moving to #8333.