From 97dfaf09ef8fa5202c2c78ca8561e9910dbf9e56 Mon Sep 17 00:00:00 2001 From: Rain Date: Wed, 14 Feb 2024 22:22:27 -0800 Subject: [PATCH] [nexus-db-queries] remove uses of filter_target (#5052) `filter_target` is required for partial indexes but is easy to misuse for full indexes. To work around that, define an `as_partial_index` method, switch to it, and ban general uses of `filter_target` via clippy. `as_partial_index` adds a `WHERE false` clause. See the comments included in the PR for why that works. The rest of the PR: * Makes sure clippy's `disallowed_` lints are active. `-A clippy::style` disables them. * Adds some missing tests (thanks @jmpesp) * Fixes a `RunnableQuery` that often did not return anything, replacing it with `RunnableQueryNoReturn`. --- clippy.toml | 13 + dev-tools/xtask/src/main.rs | 17 +- nexus/db-queries/src/db/datastore/silo.rs | 2 +- .../db-queries/src/db/datastore/silo_group.rs | 10 +- nexus/db-queries/src/db/datastore/snapshot.rs | 7 +- nexus/db-queries/src/db/mod.rs | 2 + nexus/db-queries/src/db/on_conflict_ext.rs | 329 ++++++++++++++++++ nexus/tests/integration_tests/snapshots.rs | 40 ++- 8 files changed, 404 insertions(+), 16 deletions(-) create mode 100644 clippy.toml create mode 100644 nexus/db-queries/src/db/on_conflict_ext.rs diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 0000000000..ffa3ffac70 --- /dev/null +++ b/clippy.toml @@ -0,0 +1,13 @@ +disallowed-methods = [ + # `ON CONFLICT ... WHERE ... DO UPDATE` is easy to misuse. + # + # If you mean a conditional upsert, import + # diesel::query_dsl::methods::FilterDsl, then use + # `do_update().set(...).filter(...)`. (This is not part of the + # documentation as of Diesel 2.1.4, but it works.) + # + # If you mean to specify a bound on a partial index, use the + # `IncompleteOnConflictExt::as_partial_index` in `nexus-db-queries`. + # See the documentation of that method for more. + "diesel::upsert::DecoratableTarget::filter_target", +] diff --git a/dev-tools/xtask/src/main.rs b/dev-tools/xtask/src/main.rs index 93d91799bc..4ab42736ca 100644 --- a/dev-tools/xtask/src/main.rs +++ b/dev-tools/xtask/src/main.rs @@ -46,6 +46,9 @@ fn cmd_clippy() -> Result<()> { // Make sure we check everything. .arg("--all-targets") .arg("--") + // For a list of lints, see + // https://rust-lang.github.io/rust-clippy/master. + // // We disallow warnings by default. .arg("--deny") .arg("warnings") @@ -53,7 +56,19 @@ fn cmd_clippy() -> Result<()> { // override belongs in src/lib.rs, and it is there, but that doesn't // reliably work due to rust-lang/rust-clippy#6610. .arg("--allow") - .arg("clippy::style"); + .arg("clippy::style") + // But continue to warn on anything in the "disallowed_" namespace. + // (These will be turned into errors by `--deny warnings` above.) + .arg("--warn") + .arg("clippy::disallowed_macros") + .arg("--warn") + .arg("clippy::disallowed_methods") + .arg("--warn") + .arg("clippy::disallowed_names") + .arg("--warn") + .arg("clippy::disallowed_script_idents") + .arg("--warn") + .arg("clippy::disallowed_types"); eprintln!( "running: {:?} {}", diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index a88a27872f..6cfda3c093 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -250,7 +250,7 @@ impl DataStore { .await?; if let Some(query) = silo_admin_group_ensure_query { - query.get_result_async(&conn).await?; + query.execute_async(&conn).await?; } if let Some(queries) = silo_admin_group_role_assignment_queries diff --git a/nexus/db-queries/src/db/datastore/silo_group.rs b/nexus/db-queries/src/db/datastore/silo_group.rs index 29fcb7490b..b8ef759116 100644 --- a/nexus/db-queries/src/db/datastore/silo_group.rs +++ b/nexus/db-queries/src/db/datastore/silo_group.rs @@ -8,13 +8,14 @@ use super::DataStore; use crate::authz; use crate::context::OpContext; use crate::db; -use crate::db::datastore::RunnableQuery; +use crate::db::datastore::RunnableQueryNoReturn; use crate::db::error::public_error_from_diesel; use crate::db::error::ErrorHandler; use crate::db::error::TransactionError; use crate::db::model::SiloGroup; use crate::db::model::SiloGroupMembership; use crate::db::pagination::paginated; +use crate::db::IncompleteOnConflictExt; use async_bb8_diesel::AsyncConnection; use async_bb8_diesel::AsyncRunQueryDsl; use chrono::Utc; @@ -34,16 +35,15 @@ impl DataStore { opctx: &OpContext, authz_silo: &authz::Silo, silo_group: SiloGroup, - ) -> Result, Error> { + ) -> Result { opctx.authorize(authz::Action::CreateChild, authz_silo).await?; use db::schema::silo_group::dsl; Ok(diesel::insert_into(dsl::silo_group) .values(silo_group) .on_conflict((dsl::silo_id, dsl::external_id)) - .filter_target(dsl::time_deleted.is_null()) - .do_nothing() - .returning(SiloGroup::as_returning())) + .as_partial_index() + .do_nothing()) } pub async fn silo_group_ensure( diff --git a/nexus/db-queries/src/db/datastore/snapshot.rs b/nexus/db-queries/src/db/datastore/snapshot.rs index 7a9eb8d2bc..7a3f84bbb2 100644 --- a/nexus/db-queries/src/db/datastore/snapshot.rs +++ b/nexus/db-queries/src/db/datastore/snapshot.rs @@ -20,6 +20,7 @@ use crate::db::model::SnapshotState; use crate::db::pagination::paginated; use crate::db::update_and_check::UpdateAndCheck; use crate::db::update_and_check::UpdateStatus; +use crate::db::IncompleteOnConflictExt; use crate::transaction_retry::OptionalError; use async_bb8_diesel::AsyncRunQueryDsl; use chrono::Utc; @@ -69,7 +70,7 @@ impl DataStore { // As written below, // // .on_conflict((dsl::project_id, dsl::name)) - // .filter_target(dsl::time_deleted.is_null()) + // .as_partial_index() // .do_update() // .set(dsl::time_modified.eq(dsl::time_modified)) // @@ -79,7 +80,7 @@ impl DataStore { // (marked with >>): // // .on_conflict((dsl::project_id, dsl::name)) - // .filter_target(dsl::time_deleted.is_null()) + // .as_partial_index() // .do_update() // .set(dsl::time_modified.eq(dsl::time_modified)) // >> .filter(dsl::id.eq(snapshot.id())) @@ -118,7 +119,7 @@ impl DataStore { diesel::insert_into(dsl::snapshot) .values(snapshot) .on_conflict((dsl::project_id, dsl::name)) - .filter_target(dsl::time_deleted.is_null()) + .as_partial_index() .do_update() .set(dsl::time_modified.eq(dsl::time_modified)), ) diff --git a/nexus/db-queries/src/db/mod.rs b/nexus/db-queries/src/db/mod.rs index e21ba2e3a8..184ebe5f8a 100644 --- a/nexus/db-queries/src/db/mod.rs +++ b/nexus/db-queries/src/db/mod.rs @@ -21,6 +21,7 @@ pub(crate) mod error; mod explain; pub mod fixed_data; pub mod lookup; +mod on_conflict_ext; // Public for doctests. pub mod pagination; mod pool; @@ -44,6 +45,7 @@ pub use nexus_db_model::schema; pub use crate::db::error::TransactionError; pub use config::Config; pub use datastore::DataStore; +pub use on_conflict_ext::IncompleteOnConflictExt; pub use pool::{DbConnection, Pool}; pub use saga_recovery::{recover, CompletionTask, RecoveryTask}; pub use saga_types::SecId; diff --git a/nexus/db-queries/src/db/on_conflict_ext.rs b/nexus/db-queries/src/db/on_conflict_ext.rs new file mode 100644 index 0000000000..25e05d1b0f --- /dev/null +++ b/nexus/db-queries/src/db/on_conflict_ext.rs @@ -0,0 +1,329 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use diesel::dsl::sql; +use diesel::expression::SqlLiteral; +use diesel::prelude::*; +use diesel::sql_types::Bool; +use diesel::upsert::IncompleteOnConflict; + +/// An extension trait for incomplete `INSERT INTO ... ON CONFLICT` +/// expressions. +/// +/// In Diesel, these are represented by [`IncompleteOnConflict`]. This trait is +/// implemented for expressions of that type. +pub trait IncompleteOnConflictExt { + /// The output of [`IncompleteOnConflictExt::as_partial_index`]. + type AsPartialIndexOutput; + + /// Adds a `WHERE false` clause to the `ON CONFLICT` expression, which + /// informs the database that the set of columns is a partial index. + /// + /// This is a replacement for [`DecoratableTarget::filter_target`] which is + /// easier to use, and significantly harder to misuse. In Omicron, we have + /// implemented a general ban on `filter_target` because it's so easy to + /// silently introduce bugs with it. + /// + /// # Background + /// + /// ## 1. Partial indexes + /// + /// Most users of databases are aware of *unique indexes*: a set of columns + /// that uniquely identifies a row in a given table. For example, the + /// primary key of a table is always a unique index. + /// + /// Databases like CockroachDB also support *[partial indexes]*: a unique + /// index formed only on the set of columns that meet a given condition. In + /// Omicron, we have several partial indexes of the form (as described in + /// `dbinit.sql`): + /// + /// ```sql + /// CREATE UNIQUE INDEX IF NOT EXISTS lookup_silo_group_by_silo ON omicron.public.silo_group ( + /// silo_id, + /// external_id + /// ) WHERE + /// time_deleted IS NULL; + /// ``` + /// + /// This index means that in the `omicron.public.silo_group` table, each + /// pair of `silo_id` and `external_id` must be unique. However, the + /// uniqueness is only enforced for rows where `time_deleted` is `NULL`. + /// (Logically, this makes sense: rows that have been soft-deleted should + /// not have an influence on uniqueness constraints.) + /// + /// ## 2. `INSERT INTO ... ON CONFLICT` + /// + /// In general, inserting a new row into an SQL table looks like: + /// + /// ```sql + /// INSERT INTO table_name (column1, column2, ...) VALUES (value1, value2, ...); + /// ``` + /// + /// By default, if you try to insert a row that would violate a unique + /// constraint, the database will raise an error. However, you can + /// customize this behavior with [the `ON CONFLICT` clause]. + /// + /// For example, for a table with five columns `pkey` (primary key), + /// `external_id` (a UUID), `data`, `time_created`, and `time_deleted`, you + /// can write: + /// + /// ```sql + /// INSERT INTO my_table (pkey, external_id, data, time_created, time_deleted) + /// VALUES (1, '70b10c38-36aa-4be0-8ec5-e7923438b3b8', 'foo', now(), NULL) + /// ON CONFLICT (pkey) + /// DO UPDATE SET data = 'foo'; + /// ``` + /// + /// In this case: + /// + /// * If the row doesn't exist, a new row is inserted with the data, and + /// `time_created` set to the current time. + /// * If the row does exist, the `data` column is updated to `'foo'` but + /// `time_created` is not changed. + /// + /// ## 3. Partial indexes and `ON CONFLICT` + /// + /// Now consider what happens if there's a partial index on `my_table`, + /// ensuring that `external_id` is unique if the row hasn't been + /// soft-deleted: + /// + /// ```sql + /// CREATE UNIQUE INDEX IF NOT EXISTS my_table_external_id + /// ON my_table (external_id) + /// WHERE time_deleted IS NULL; + /// ``` + /// + /// You can now try to write a query which takes into account the conflict + /// on `external_id`: + /// + /// ```sql + /// INSERT INTO my_table (pkey, external_id, data, time_created, time_deleted) + /// VALUES (1, '70b10c38-36aa-4be0-8ec5-e7923438b3b8', 'foo', now(), NULL) + /// ON CONFLICT (external_id) + /// DO UPDATE SET data = 'foo'; + /// ``` + /// + /// But this query does not work, and Cockroach errors out with: + /// + /// ```text + /// ERROR: there is no unique or exclusion constraint matching the ON CONFLICT specification + /// ``` + /// + /// The reason for that is that simply specifying the columns is not + /// enough. Because the index is _partial_, Cockroach requires that the + /// `WHERE` clause of the index be specified as well. + /// + /// ```sql + /// INSERT INTO my_table (pkey, external_id, data, time_created, time_deleted) + /// VALUES (1, '70b10c38-36aa-4be0-8ec5-e7923438b3b8', 'foo', now(), NULL) + /// ON CONFLICT (external_id) + /// WHERE time_deleted IS NULL + /// DO UPDATE SET data = 'foo'; + /// ``` + /// + /// Importantly, the `WHERE` clause of the index doesn't have to exactly + /// _match_ the condition on the partial index; it can be anything that + /// _logically implies_ the condition on the partial index. With something + /// like `time_deleted IS NULL` the value of that is not exactly clear, but + /// you can imagine a partial index on something like `col >= 10`, and + /// write `ON CONFLICT (...) WHERE col >= 20`. This is allowed because `col + /// >= 20` implies `col >= 10`. (But `WHERE col >= 5` is not allowed.) + /// + /// ## 4. A similar syntax with a different meaning + /// + /// There's a similar syntax which does something completely different: `ON + /// CONFLICT ... DO UPDATE ... WHERE`. This syntax does a conditional + /// update, whether or not the conflict is in a partial index. For example, + /// `ON CONFLICT (pkey) DO UPDATE SET data = 'foo' WHERE data IS NULL` will + /// update `data` to `'foo'` if the row already exists, but only if `data` + /// is `NULL`. + /// + /// Now, what happens if you accidentally mix the two up, and write + /// something like: `ON CONFLICT (pkey) WHERE data IS NULL DO UPDATE SET + /// data = 'foo'`? You might imagine that this query is rejected, for two + /// reasons: + /// + /// 1. `pkey` is a full index, so you might expect a blanket ban on `WHERE` + /// clauses. + /// 2. `data IS NULL` is irrelevant to `pkey`. + /// + /// But in reality, this query is not only accepted---the `WHERE` clause is + /// **completely, silently ignored**. `data` will be set to `'foo'` no + /// matter if it is `NULL` or not. (This is true as of both CockroachDB + /// 23.2.0 and PostgreSQL 16.2). + /// + /// This seems pretty bad! Why does it happen? + /// + /// ## 5. A digression into logical systems + /// + /// The reason for that ties back into the idea that in `ON CONFLICT ... + /// WHERE`, the `WHERE` clause can be anything that _logically implies_ the + /// index. + /// + /// Let's take a step back and think about what logical implication (often + /// represented by `->`) means. In classical logic, `p -> q` is interpreted + /// as *[material implication]*, and is logically equivalent to `!p || q`. + /// + /// Consider what happens if `q` is a _tautology_, i.e. it is always true. + /// In that case, no matter what `p` is, `p -> q` is always true. + /// + /// Applying this to our SQL example: Since `pkey` is a full index, it is + /// equivalent to a partial index with `WHERE true`. In other words, `q` is + /// always true. This means that `p` (the `WHERE` clause) can be anything. + /// As a result, Cockroach completely ignores the `WHERE` clause. + /// + /// **This seems really bad!** Our intuitive understanding of "p implies q" + /// typically has some kind of notion of _relevance_ attached to it: we + /// expect that `p` being true has to have some bearing on q. But material + /// implication does not capture this idea at all. (Put a pin in this, + /// we're going to come back to it shortly.) + /// + /// ## 6. Towards addressing this + /// + /// To summarize where we're at: + /// + /// * `ON CONFLICT ... WHERE ... DO UPDATE` is easy to mix up with `ON + /// CONFLICT ... DO UPDATE ... WHERE`. + /// * `ON CONFLICT ... WHERE ... DO UPDATE` on a full index silently + /// accepts _anything_ in the `WHERE` clause. + /// * `ON CONFLICT ... WHERE ... DO UPDATE` is *required* for a partial + /// index. + /// + /// How can we prevent misuse while still supporting legitimate uses? + /// + /// One option is to look at [Diesel](https://diesel.rs/), our Rust ORM and + /// query builder. In Diesel, the usual way to add a `WHERE` clause is via + /// [the `filter` method]. But the special case of `ON CONFLICT ... WHERE` + /// is expressed via a separate [`filter_target` method]. + /// + /// Our first inclination might be to just ban `filter_target` entirely, + /// using Clippy's [`disallowed_methods`] lint. This would work great if + /// all we had was full indexes! But we do necessarily have partial + /// indexes, and we must do something about them. + /// + /// That's where `as_partial_index` comes in. + /// + /// ## 7. Back to logical systems + /// + /// How does `as_partial_index` address this problem? Let's go back to + /// material implication: `p -> q` is logically equivalent to `!p || q`. + /// + /// So far, we've discussed the case where `q` is a tautology. But let's + /// look at the other side of it: what if `p` is a _contradiction_? In + /// other words, what if `p` is always false? In that case, it is clear + /// that `p -> q` is always true, no matter what `q` is. + /// + /// It's worth sitting with this for a moment. What this means is that you + /// can make statements that are genuinely nonsense, like "if 2 + 2 = 5, + /// then the sky is green", but are valid in classical logic. + /// + /// The fact that contradictions are "poisonous" to classical logic is a + /// well-known problem, called the [principle of explosion]. This is not + /// new---Aristotle was likely aware of it, and it was first described + /// fully by William of Soissons in the 12th century. Intuitively, **this + /// is even worse** than the earlier case where `q` is a tautology: at + /// least there, `q` is actually true so it doesn't feel too bad that `p` + /// doesn't matter. + /// + /// ## 8. Putting it all together + /// + /// The good news about the principle of explosion, though, is that this + /// gives us a path out. Remember that `ON CONFLICT ... WHERE ...` requires + /// that the bit after `WHERE` logically imply the partial index. Since a + /// contradiction implies anything, a clause like `WHERE 1 = 2`, or `WHERE + /// 'x' = 'y'`, works. + /// + /// In SQL, the simplest contradiction is simply `false`. So we use `WHERE + /// false` here. The effect of this is that it causes the database to + /// automatically infer the right partial index constraints. + /// + /// So the solution we came up in the end is to: + /// + /// * Ban `filter_target` via Clippy's `disallowed_methods` lint, to avoid + /// misuse in cases where there isn't a partial index. + /// * Define `as_partial_index` as a method that adds `WHERE false` to the + /// `ON CONFLICT` expression. + /// * For the cases where there really is a partial index, use + /// `as_partial_index`. + /// + /// `as_partial_index` is also accepted in cases where there's a full + /// index. That isn't a huge problem because a stray `WHERE false` is + /// pretty harmless. The goal is to prevent misuse of `filter_target`, and + /// that part is still upheld. + /// + /// --- + /// + /// ## Further notes + /// + /// The [paradoxes of material implication] are one of the central topics + /// in philosophical logic. Many non-classical logical systems have been + /// proposed to address this, including [paraconsistent logics] and + /// [relevance logics]. The common thread among these systems is that they + /// define implication differently from classical logic, in a way that + /// tries to match our common understanding better. + /// + /// Above, we focused on classical logic. SQL uses the non-classical + /// [three-valued logic] called **K3**. This logic defines implication in + /// the same way, and with the same issues, as material implication in + /// classical logic. + /// + /// With Cockroach 23.2.0 and earlier versions, for a conflict on a full + /// index, you can even specify non-existent columns: `ON CONFLICT ... + /// WHERE non_existent = 1`. This is rejected by Postgres, and has been + /// acknowledged as a bug in Cockroach. + /// + /// There is also an `ON CONFLICT ON CONSTRAINT` syntax which allows users + /// to specify the name of the constraint. That syntax only works for full + /// (non-partial) indexes. + /// + /// Other references: + /// + /// * [Omicron issue + /// #5047](https://github.com/oxidecomputer/omicron/issues/5047) + /// * [CockroachDB issue + /// #119197](https://github.com/cockroachdb/cockroach/issues/119117) + /// + /// [partial indexes]: https://www.cockroachlabs.com/docs/stable/partial-indexes.html + /// [the `ON_CONFLICT` clause]: + /// https://www.cockroachlabs.com/docs/stable/insert#on-conflict-clause + /// [material implication]: + /// https://en.wikipedia.org/wiki/Material_conditional + /// [Diesel]: https://diesel.rs/ + /// [the `filter` method]: + /// https://docs.rs/diesel/2.1.4/diesel/query_dsl/methods/trait.FilterDsl.html#tymethod.filter + /// [`filter_target` method]: + /// https://docs.rs/diesel/2.1.4/diesel/query_dsl/trait.FilterTarget.html#tymethod.filter_targehttps://docs.rs/diesel/2.1.4/diesel/upsert/trait.DecoratableTarget.html#tymethod.filter_targett + /// [`disallowed_methods`]: + /// https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_methods + /// [principle of explosion]: + /// https://en.wikipedia.org/wiki/Principle_of_explosion + /// [paradoxes of material implication]: + /// https://en.wikipedia.org/wiki/Paradoxes_of_material_implication + /// [paraconsistent logics]: + /// https://en.wikipedia.org/wiki/Paraconsistent_logic + /// [relevance logics]: https://en.wikipedia.org/wiki/Relevance_logic + /// [three-valued logic]: https://en.wikipedia.org/wiki/Three-valued_logic + fn as_partial_index(self) -> Self::AsPartialIndexOutput; +} + +impl IncompleteOnConflictExt + for IncompleteOnConflict +where + Target: DecoratableTarget>, +{ + type AsPartialIndexOutput = + IncompleteOnConflict>; + + fn as_partial_index(self) -> Self::AsPartialIndexOutput { + // Bypass this clippy warning in this one place only. + #[allow(clippy::disallowed_methods)] + { + // For why "false", see the doc comment above. + self.filter_target(sql::("false")) + } + } +} + +type DecoratedAsPartialIndex = + >>::FilterOutput; diff --git a/nexus/tests/integration_tests/snapshots.rs b/nexus/tests/integration_tests/snapshots.rs index 3731a80668..63ea81f13f 100644 --- a/nexus/tests/integration_tests/snapshots.rs +++ b/nexus/tests/integration_tests/snapshots.rs @@ -904,13 +904,13 @@ async fn test_create_snapshot_record_idempotent( let project_id = create_project_and_pool(&client).await; let disk_id = Uuid::new_v4(); + let snapshot_name = + external::Name::try_from("snapshot".to_string()).unwrap(); let snapshot = db::model::Snapshot { identity: db::model::SnapshotIdentity { id: Uuid::new_v4(), - name: external::Name::try_from("snapshot".to_string()) - .unwrap() - .into(), + name: snapshot_name.clone().into(), description: "snapshot".into(), time_created: Utc::now(), @@ -961,9 +961,7 @@ async fn test_create_snapshot_record_idempotent( let dupe_snapshot = db::model::Snapshot { identity: db::model::SnapshotIdentity { id: Uuid::new_v4(), - name: external::Name::try_from("snapshot".to_string()) - .unwrap() - .into(), + name: snapshot_name.clone().into(), description: "snapshot".into(), time_created: Utc::now(), @@ -1057,6 +1055,36 @@ async fn test_create_snapshot_record_idempotent( ) .await .unwrap(); + + // Test that a new snapshot can be added with the same name as a deleted + // one. + + let new_snapshot = db::model::Snapshot { + identity: db::model::SnapshotIdentity { + id: Uuid::new_v4(), + name: snapshot_name.into(), + description: "snapshot".into(), + + time_created: Utc::now(), + time_modified: Utc::now(), + time_deleted: None, + }, + + project_id, + disk_id, + volume_id: Uuid::new_v4(), + destination_volume_id: Uuid::new_v4(), + + gen: db::model::Generation::new(), + state: db::model::SnapshotState::Creating, + block_size: db::model::BlockSize::Traditional, + size: external::ByteCount::try_from(1024u32).unwrap().into(), + }; + + let _ = datastore + .project_ensure_snapshot(&opctx, &authz_project, new_snapshot) + .await + .unwrap(); } #[nexus_test]