GMOD / Chado

the GMOD database schema
http://gmod.org/wiki/Chado
Artistic License 2.0
38 stars 25 forks source link

Fixing foreign key for db_relationship type_id #118

Closed scottcain closed 1 year ago

scottcain commented 3 years ago

Per #117 , the type_id should point at the cvterm table, no the db table.

scottcain commented 3 years ago

Definitely a small change, so a 2 person review team should certainly be sufficient.

spficklin commented 2 years ago

This PR is still not quite right and it failed a flyway migrate test.

Here's the problem:

foreign key (type_id) references db (cvterm_id) on delete cascade INITIALLY DEFERRED,

it should be:

foreign key (type_id) references cvterm (cvterm_id) on delete cascade INITIALLY DEFERRED,

I've fixed the PR and run Flyway on it and now it works and can be merged

Database: jdbc:postgresql://localhost:5432/chado (PostgreSQL 12.9)
Current version of schema "public": 1.3.3.001
Migrating schema "public" to version "1.3.3.002 - add db relationship table"
Migrating schema "public" to version "1.3.3.003 - alter analysis unique constraint"
Migrating schema "public" to version "1.3.3.004 - add synoynm indices cvtermsynonym"
Migrating schema "public" to version "1.3.3.005 - add type id 2 project table"
Migrating schema "public" to version "1.3.3.006 - modify mage"
Migrating schema "public" to version "1.3.3.007 - fix search path"
laceysanderson commented 2 years ago

Further discussion on this while testing #129 made us realize that this should be a new migration rather than fix an existing one. The idea with flyway is that people can use it to get access to updates to the schema before an official release is made. By altering an existing migration, people only get the fix if they hadn't applied the previous migration... so instead, we should make a new one just to be safe.

laceysanderson commented 2 years ago

Removed myself from review since I've now made significant changes 😬 @scottcain do you agree with the new approach?

spficklin commented 2 years ago

Since this is a small change PR and you only made other small changes @laceysanderson I propose that we just all do another round of checks rather than you bowing out of review.... just my two cents.