Fix owner_rels.cid foreign key reference
#926
Conversation
|
I also checked the code, everywhere uses |
| SELECT relname | ||
| FROM pg_class | ||
| INNER JOIN pg_namespace | ||
| ON pg_class.relnamespace = pg_namespace.oid | ||
| WHERE pg_class.relkind = 'S' | ||
| AND pg_namespace.nspname = $1 |
| ALTER TABLE owner_rels DROP CONSTRAINT owner_rels_cid_fkey; | ||
| ALTER TABLE owner_rels ADD FOREIGN KEY (cid) REFERENCES crates(id); |
There was a problem hiding this comment.
So if I understand this correctly:
- The code uses
cidto meancrate_ideverywhere. - The database foreign key referenced the
releases.idcolumn, which didn't match the usage. - The reason we didn't catch it until now is because both are strictly increasing, and there are always more releases than crates. So the foreign constraint held even though the semantic meaning was incorrect.
- This changes the foreign key constraint but nothing else, because all the joins are already done on
owners INNER JOIN crates ON owners.cid = crates.id, which is the correct behavior.
However I don't think the last point is entirely true: When I wrote delete_crate I thought this was actually the release id. So I think that delete_crate is actually very broken right now and needs to be fixed as part of this change:
Line 65 in 7ab8306
Line 102 in 7ab8306
cc @pietroalbini: do not delete any more crates until this is fixed.
There was a problem hiding this comment.
Yep, that all sounds correct, I've fixed it in the latest commit. I don't see any easy way to really test the behaviour.
There was a problem hiding this comment.
Fortunately this part of the code is (since #917) self-correcting so the worst that can happen is that the owner is missing until the next publish. Thanks for working on this :)
Nemo157
force-pushed
the
fix-owner-rels-refs
branch
from
f4cb1c4 to
43940d5
Compare
| @@ -105,6 +104,7 @@ fn delete_crate_from_database(conn: &Connection, name: &str, crate_id: i32) -> R | |||
| &[&crate_id], | |||
| )?; | |||
| } | |||
| transaction.execute("DELETE FROM owner_rels WHERE cid = $1;", &[&crate_id])?; | |||
There was a problem hiding this comment.
I was going to say you only do this for deleting a crate, not a version, then I realized that's the right behavior and it was just buggy before 😅
Also improves testing to catch attempts to use incorrect foreign keys.