[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

@@ -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",
+]

dev-tools/xtask/src/main.rs

@@ -46,14 +46,29 @@ 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")
         // Clippy's style nits are useful, but not worth keeping in CI.  This
         // 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: {:?} {}",

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

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<impl RunnableQuery<SiloGroup>, Error> {
+    ) -> Result<impl RunnableQueryNoReturn, Error> {
         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(

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

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;