Check field order of DB models against DB column order

nexus/db-model/src/lib.rs

@@ -135,6 +135,7 @@ mod certificate;
 mod clickhouse_policy;
 mod cockroachdb_node_id;
 mod collection;
+mod column_order;
 mod console_session;
 mod crucible_dataset;
 mod dataset_kind;
@@ -288,6 +289,7 @@ pub use certificate::*;
 pub use clickhouse_policy::*;
 pub use cockroachdb_node_id::*;
 pub use collection::*;
+pub use column_order::FullRowModel;
 pub use console_session::*;
 pub use crucible_dataset::*;
 pub use dataset_kind::*;

nexus/db-model/src/webhook_rx.rs

@@ -80,12 +80,12 @@ impl TryFrom<WebhookReceiverConfig> for alert::WebhookReceiver {
 pub struct AlertReceiver {
     #[diesel(embed)]
     pub identity: AlertReceiverIdentity,
-    pub endpoint: String,
 
     /// child resource generation number for secrets, per RFD 192
     pub secret_gen: Generation,
     /// child resource generation number for event subscriptions, per RFD 192
     pub subscription_gen: Generation,
+    pub endpoint: String,
 }
 
 impl DatastoreCollectionConfig<WebhookSecret> for AlertReceiver {

nexus/db-queries/src/db/datastore/alert_rx.rs

@@ -1233,6 +1233,71 @@ mod test {
         logctx.cleanup_successful();
     }
 
+    #[tokio::test]
+    async fn test_queryable_silently_swaps_same_typed_columns() {
+        let logctx = dev::test_setup_log(
+            "test_queryable_silently_swaps_same_typed_columns",
+        );
+        let db = TestDatabase::new_with_datastore(&logctx.log).await;
+        let (opctx, datastore) = (db.opctx(), db.datastore());
+        let rx = create_receiver(
+            datastore,
+            opctx,
+            "positional-decoding",
+            Vec::new(),
+        )
+        .await;
+        let rx_id = rx.rx.id().into_untyped_uuid();
+        let secret_gen = Generation::try_from(2_i64).unwrap();
+        let subscription_gen = Generation::try_from(3_i64).unwrap();
+        let conn = datastore.pool_connection_authorized(opctx).await.unwrap();
+
+        diesel::update(rx_dsl::alert_receiver.find(rx_id))
+            .set((
+                rx_dsl::secret_gen.eq(secret_gen),
+                rx_dsl::subscription_gen.eq(subscription_gen),
+            ))
+            .execute_async(&*conn)
+            .await
+            .unwrap();
+
+        let correctly_ordered = rx_dsl::alert_receiver
+            .find(rx_id)
+            .select(AlertReceiver::as_select())
+            .get_result_async(&*conn)
+            .await
+            .unwrap();
+        assert_eq!(correctly_ordered.secret_gen, secret_gen);
+        assert_eq!(correctly_ordered.subscription_gen, subscription_gen);
+
+        // This has the same SQL type as `AlertReceiver::as_select()`, so
+        // Diesel accepts it even though the two generation columns are in the
+        // opposite order from the model fields.
+        let incorrectly_ordered = rx_dsl::alert_receiver
+            .find(rx_id)
+            .select((
+                (
+                    rx_dsl::id,
+                    rx_dsl::name,
+                    rx_dsl::description,
+                    rx_dsl::time_created,
+                    rx_dsl::time_modified,
+                    rx_dsl::time_deleted,
+                ),
+                rx_dsl::subscription_gen,
+                rx_dsl::secret_gen,
+                rx_dsl::endpoint,
+            ))
+            .get_result_async::<AlertReceiver>(&*conn)
+            .await
+            .unwrap();
+        assert_eq!(incorrectly_ordered.secret_gen, subscription_gen);
+        assert_eq!(incorrectly_ordered.subscription_gen, secret_gen);
+
+        db.terminate().await;
+        logctx.cleanup_successful();
+    }
+
     #[tokio::test]
     async fn test_webhook_rx_update_fails_when_receiver_deleted() {
         let logctx = dev::test_setup_log(

nexus/db-queries/src/db/datastore/dns.rs

@@ -761,6 +761,7 @@ mod test {
     use async_bb8_diesel::AsyncConnection;
     use async_bb8_diesel::AsyncRunQueryDsl;
     use chrono::Utc;
+    use diesel::prelude::*;
     use futures::FutureExt;
     use nexus_db_errors::TransactionError;
     use nexus_db_model::DnsGroup;
@@ -778,6 +779,64 @@ mod test {
     use std::num::NonZeroU32;
     use uuid::Uuid;
 
+    #[derive(Queryable)]
+    struct MisorderedDnsVersion {
+        dns_group: DnsGroup,
+        version: Generation,
+        time_created: chrono::DateTime<Utc>,
+        comment: String,
+        creator: String,
+    }
+
+    #[tokio::test]
+    async fn test_default_selection_silently_swaps_same_typed_model_fields() {
+        let logctx = dev::test_setup_log(
+            "test_default_selection_silently_swaps_same_typed_model_fields",
+        );
+        let db = TestDatabase::new_with_datastore(&logctx.log).await;
+        let datastore = db.datastore();
+        let conn = datastore.pool_connection_for_tests().await.unwrap();
+        let version = Generation::try_from(1_i64).unwrap();
+        let time_created = Utc::now();
+
+        use nexus_db_schema::schema::dns_version::dsl;
+        diesel::insert_into(dsl::dns_version)
+            .values(DnsVersion {
+                dns_group: DnsGroup::Internal,
+                version,
+                time_created,
+                creator: "creator value".to_string(),
+                comment: "comment value".to_string(),
+            })
+            .execute_async(&*conn)
+            .await
+            .unwrap();
+
+        // No explicit selection: Diesel selects the table's columns in table
+        // order and decodes them into the model in field order. `creator` and
+        // `comment` have the same SQL and Rust types, so their order mismatch
+        // is accepted and the values are silently swapped.
+        let misordered = dsl::dns_version
+            .find((DnsGroup::Internal, version))
+            .get_result_async::<MisorderedDnsVersion>(&*conn)
+            .await
+            .unwrap();
+        assert_eq!(misordered.dns_group, DnsGroup::Internal);
+        assert_eq!(misordered.version, version);
+        assert_eq!(
+            misordered
+                .time_created
+                .signed_duration_since(time_created)
+                .num_seconds(),
+            0
+        );
+        assert_eq!(misordered.comment, "creator value");
+        assert_eq!(misordered.creator, "comment value");
+
+        db.terminate().await;
+        logctx.cleanup_successful();
+    }
+
     // Tests reading various uninitialized or partially-initialized DNS data
     #[tokio::test]
     async fn test_read_dns_config_uninitialized() {

nexus/db-queries/src/db/update_and_check.rs

@@ -16,6 +16,7 @@ use diesel::query_source::Table;
 use diesel::result::Error as DieselError;
 use diesel::sql_types::Nullable;
 use nexus_db_lookup::DbConnection;
+use nexus_db_model::FullRowModel;
 use std::marker::PhantomData;
 
 /// A simple wrapper type for Diesel's [`UpdateStatement`], which
@@ -57,7 +58,17 @@ where
     /// Nests the existing update statement in a CTE which
     /// identifies if the row exists (by ID), even if the row
     /// cannot be successfully updated.
-    fn check_if_exists<Q>(self, key: K) -> UpdateAndQueryStatement<US, K, Q>;
+    ///
+    /// The found row is selected in the table's `AllColumns` order but
+    /// decoded positionally into `Q`, whose field order comes from its
+    /// `Selectable` derive. The type system doesn't tie the two orders
+    /// together, so `Q` is required to be a [`FullRowModel`] for the updated
+    /// table: implementing that trait (via `full_row_models!` in
+    /// nexus-db-model's `column_order` module) generates a test verifying the
+    /// field order.
+    fn check_if_exists<Q>(self, key: K) -> UpdateAndQueryStatement<US, K, Q>
+    where
+        Q: FullRowModel<Table = US::Table>;
 }
 
 // UpdateStatement has four generic parameters:
@@ -83,7 +94,10 @@ where
         QueryFragment<Pg> + Send + 'static,
     K: 'static + Copy + Send,
 {
-    fn check_if_exists<Q>(self, key: K) -> UpdateAndQueryStatement<US, K, Q> {
+    fn check_if_exists<Q>(self, key: K) -> UpdateAndQueryStatement<US, K, Q>
+    where
+        Q: FullRowModel<Table = US::Table>,
+    {
         let find_subquery = Box::new(US::Table::table().find(key));
         UpdateAndQueryStatement {
             update_statement: self.statement(),