Skip to content

Fix owner_rels.cid foreign key reference #926

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 1, 2020
Merged

Fix owner_rels.cid foreign key reference #926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e7407bf
Offset primary key sequences during testing to test referencing incor…
Nemo157 Aug 1, 2020
9944854
Make build queue test independent of database ids
Nemo157 Aug 1, 2020
0da6269
Fix owner_rels.cid foreign key reference
Nemo157 Aug 1, 2020
43940d5
Fix deleting crates to only remove owner relationships when deleting …
Nemo157 Aug 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 9 additions & 20 deletions src/build_queue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,8 +283,8 @@ mod tests {
let queue = env.build_queue();

let test_crates = [
("foo", "1.0.0", -10),
("bar", "1.0.0", 0),
("foo", "1.0.0", -10),
("baz", "1.0.0", 10),
];
for krate in &test_crates {
Expand All @@ -293,26 +293,15 @@ mod tests {

assert_eq!(
vec![
QueuedCrate {
id: 1,
name: "foo".into(),
version: "1.0.0".into(),
priority: -10,
},
QueuedCrate {
id: 2,
name: "bar".into(),
version: "1.0.0".into(),
priority: 0,
},
QueuedCrate {
id: 3,
name: "baz".into(),
version: "1.0.0".into(),
priority: 10,
},
("foo", "1.0.0", -10),
("bar", "1.0.0", 0),
("baz", "1.0.0", 10),
],
queue.queued_crates()?
queue
.queued_crates()?
.iter()
.map(|c| (c.name.as_str(), c.version.as_str(), c.priority))
.collect::<Vec<_>>()
);

Ok(())
Expand Down
8 changes: 4 additions & 4 deletions src/db/delete.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,8 @@ fn get_id(conn: &Connection, name: &str) -> Result<i32, Error> {

// metaprogramming!
// WARNING: these must be hard-coded and NEVER user input.
const METADATA: [(&str, &str); 5] = [
const METADATA: &[(&str, &str)] = &[
("author_rels", "rid"),
("owner_rels", "cid"),
("keyword_rels", "rid"),
("builds", "rid"),
("compression_rels", "release"),
Expand All @@ -60,7 +59,7 @@ const METADATA: [(&str, &str); 5] = [
fn delete_version_from_database(conn: &Connection, name: &str, version: &str) -> Result<(), Error> {
let crate_id = get_id(conn, name)?;
let transaction = conn.transaction()?;
for &(table, column) in &METADATA {
for &(table, column) in METADATA {
transaction.execute(
&format!("DELETE FROM {} WHERE {} IN (SELECT id FROM releases WHERE crate_id = $1 AND version = $2)", table, column),
&[&crate_id, &version],
Expand Down Expand Up @@ -96,7 +95,7 @@ fn delete_crate_from_database(conn: &Connection, name: &str, crate_id: i32) -> R
"DELETE FROM sandbox_overrides WHERE crate_name = $1",
&[&name],
)?;
for &(table, column) in &METADATA {
for &(table, column) in METADATA {
transaction.execute(
&format!(
"DELETE FROM {} WHERE {} IN (SELECT id FROM releases WHERE crate_id = $1)",
Expand All @@ -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])?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

transaction.execute("DELETE FROM releases WHERE crate_id = $1;", &[&crate_id])?;
transaction.execute("DELETE FROM crates WHERE id = $1;", &[&crate_id])?;

Expand Down
16 changes: 16 additions & 0 deletions src/db/migrate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,22 @@ pub fn migrate(version: Option<Version>, conn: &Connection) -> CratesfyiResult<(
"DROP TABLE compression_rels;
ALTER TABLE files DROP COLUMN compression;"
),
migration!(
context,
// version
15,
// description
"Fix owner_rels.cid foreign key reference",
// upgrade query
"
ALTER TABLE owner_rels DROP CONSTRAINT owner_rels_cid_fkey;
ALTER TABLE owner_rels ADD FOREIGN KEY (cid) REFERENCES crates(id);
Comment on lines +375 to +376
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understand this correctly:

  • The code uses cid to mean crate_id everywhere.
  • The database foreign key referenced the releases.id column, 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:

docs.rs/src/db/delete.rs

Line 65 in 7ab8306

&format!("DELETE FROM {} WHERE {} IN (SELECT id FROM releases WHERE crate_id = $1 AND version = $2)", table, column),
and

docs.rs/src/db/delete.rs

Line 102 in 7ab8306

"DELETE FROM {} WHERE {} IN (SELECT id FROM releases WHERE crate_id = $1)",

cc @pietroalbini: do not delete any more crates until this is fixed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

",
// downgrade query
"
-- Nope, this is a pure database fix, no going back.
"
),
];

for migration in migrations {
Expand Down
23 changes: 23 additions & 0 deletions src/test/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,29 @@ impl TestDatabase {
))?;
crate::db::migrate(None, &conn)?;

// Move all sequence start positions 10000 apart to avoid overlapping primary keys
let query: String = conn
.query(
"
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
Comment on lines +246 to +251
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dark magics 😨

",
&[&schema],
)?
.into_iter()
.map(|row| row.get(0))
.enumerate()
.map(|(i, sequence): (_, String)| {
let offset = (i + 1) * 10000;
format!(r#"ALTER SEQUENCE "{}" RESTART WITH {};"#, sequence, offset)
})
.collect();
conn.batch_execute(&query)?;

Ok(TestDatabase {
pool: Pool::new_with_schema(config, &schema)?,
schema,
Expand Down