Nullable<T> for nullable but not optional, fix setting null

Author: david-crespo

common/src/api/external/mod.rs

@@ -43,6 +43,7 @@ use std::fmt::Result as FormatResult;
 use std::net::IpAddr;
 use std::net::Ipv4Addr;
 use std::num::{NonZeroU16, NonZeroU32};
+use std::ops::Deref;
 use std::str::FromStr;
 use tufaceous_artifact::ArtifactHash;
 use uuid::Uuid;
@@ -3266,6 +3267,60 @@ pub enum ImportExportPolicy {
     Allow(Vec<oxnet::IpNet>),
 }
 
+/// Use instead of Option in API request body structs to get a field that can be
+/// null (parsed as `None`) but is not optional. Will fail to parse if the key
+/// is not present.
+#[derive(Clone, Debug, Serialize)]
+pub struct Nullable<T>(pub Option<T>);
+
+impl<T> From<Option<T>> for Nullable<T> {
+    fn from(option: Option<T>) -> Self {
+        Nullable(option)
+    }
+}
+
+impl<T> Deref for Nullable<T> {
+    type Target = Option<T>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+// it looks like we're just using Option's impl here, so why not derive instead?
+// For some reason, deriving JsonSchema + #[serde(transparent)] doesn't work --
+// it almost does, but the field does not end up marked required in the schema.
+// There must be some special handling of Option somewhere causing it to be
+// marked optional rather than nullable + required.
+
+impl<T: JsonSchema> JsonSchema for Nullable<T> {
+    fn schema_name() -> String {
+        T::schema_name()
+    }
+
+    fn json_schema(
+        generator: &mut schemars::r#gen::SchemaGenerator,
+    ) -> schemars::schema::Schema {
+        Option::<T>::json_schema(generator)
+    }
+
+    fn is_referenceable() -> bool {
+        Option::<T>::is_referenceable()
+    }
+}
+
+impl<'de, T: serde::Deserialize<'de>> serde::Deserialize<'de> for Nullable<T> {
+    fn deserialize<D: serde::Deserializer<'de>>(
+        deserializer: D,
+    ) -> Result<Self, D::Error> {
+        // this is what errors if the key isn't present
+        let value = serde_json::Value::deserialize(deserializer)?;
+
+        use serde::de::Error;
+        Option::<T>::deserialize(value).map_err(D::Error::custom).map(Nullable)
+    }
+}
+
 #[cfg(test)]
 mod test {
     use serde::Deserialize;

nexus/db-model/src/silo_settings.rs

@@ -50,16 +50,18 @@ impl From<SiloSettings> for views::SiloSettings {
 #[derive(AsChangeset)]
 #[diesel(table_name = silo_settings)]
 pub struct SiloSettingsUpdate {
-    pub device_token_max_ttl_seconds: Option<i64>,
+    // Needs to be double Option so we can set a value of null in the DB by
+    // passing Some(None). None by itself is ignored by Diesel.
+    pub device_token_max_ttl_seconds: Option<Option<i64>>,
     pub time_modified: DateTime<Utc>,
 }
 
 impl From<params::SiloSettingsUpdate> for SiloSettingsUpdate {
     fn from(params: params::SiloSettingsUpdate) -> Self {
         Self {
-            device_token_max_ttl_seconds: params
-                .device_token_max_ttl_seconds
-                .map(|ttl| ttl.get().into()),
+            device_token_max_ttl_seconds: Some(
+                params.device_token_max_ttl_seconds.map(|ttl| ttl.get().into()),
+            ),
             time_modified: Utc::now(),
         }
     }

nexus/tests/integration_tests/device_auth.rs

@@ -278,13 +278,34 @@ async fn test_device_token_expiration(cptestctx: &ControlPlaneTestContext) {
         let msg = "unable to parse JSON body: device_token_max_ttl_seconds: invalid value";
         assert!(error.message.starts_with(&msg));
     }
+    for value in [-3, 0] {
+        let error = object_put_error(
+            testctx,
+            "/v1/settings",
+            &serde_json::json!({ "device_token_max_ttl_seconds": value }),
+            StatusCode::BAD_REQUEST,
+        )
+        .await;
+        let msg = "unable to parse JSON body: device_token_max_ttl_seconds: invalid value";
+        assert!(error.message.starts_with(&msg));
+    }
+
+    // omitting the key is also a 400
+    let error = object_put_error(
+        testctx,
+        "/v1/settings",
+        &serde_json::json!({}),
+        StatusCode::BAD_REQUEST,
+    )
+    .await;
+    assert!(error.message.starts_with("unable to parse JSON body: missing field `device_token_max_ttl_seconds`"));
 
     // set token expiration on silo to 3 seconds
     let settings: views::SiloSettings = object_put(
         testctx,
         "/v1/settings",
         &params::SiloSettingsUpdate {
-            device_token_max_ttl_seconds: NonZeroU32::new(3),
+            device_token_max_ttl_seconds: NonZeroU32::new(3).into(),
         },
     )
     .await;
@@ -316,6 +337,21 @@ async fn test_device_token_expiration(cptestctx: &ControlPlaneTestContext) {
     project_list(&testctx, &initial_token, StatusCode::OK)
         .await
         .expect("initial token should still work");
+
+    // now test setting the silo max TTL back to null
+    let settings: views::SiloSettings = object_put(
+        testctx,
+        "/v1/settings",
+        &params::SiloSettingsUpdate {
+            device_token_max_ttl_seconds: None.into(),
+        },
+    )
+    .await;
+    assert_eq!(settings.device_token_max_ttl_seconds, None);
+
+    let settings: views::SiloSettings =
+        object_get(testctx, "/v1/settings").await;
+    assert_eq!(settings.device_token_max_ttl_seconds, None);
 }
 
 async fn project_list(

nexus/tests/integration_tests/endpoints.rs

@@ -34,6 +34,7 @@ use omicron_common::api::external::IdentityMetadataUpdateParams;
 use omicron_common::api::external::InstanceCpuCount;
 use omicron_common::api::external::Name;
 use omicron_common::api::external::NameOrId;
+use omicron_common::api::external::Nullable;
 use omicron_common::api::external::RouteDestination;
 use omicron_common::api::external::RouteTarget;
 use omicron_common::api::external::UserId;
@@ -1641,7 +1642,9 @@ pub static VERIFY_ENDPOINTS: LazyLock<Vec<VerifyEndpoint>> =
                     AllowedMethod::Get,
                     AllowedMethod::Put(
                         serde_json::to_value(&params::SiloSettingsUpdate {
-                            device_token_max_ttl_seconds: NonZeroU32::new(3),
+                            device_token_max_ttl_seconds: Nullable(
+                                NonZeroU32::new(3),
+                            ),
                         })
                         .unwrap(),
                     ),

nexus/types/src/external_api/params.rs

@@ -13,8 +13,8 @@ use omicron_common::api::external::{
     AddressLotKind, AffinityPolicy, AllowedSourceIps, BfdMode, BgpPeer,
     ByteCount, FailureDomain, Hostname, IdentityMetadataCreateParams,
     IdentityMetadataUpdateParams, InstanceAutoRestartPolicy, InstanceCpuCount,
-    LinkFec, LinkSpeed, Name, NameOrId, PaginationOrder, RouteDestination,
-    RouteTarget, TxEqConfig, UserId,
+    LinkFec, LinkSpeed, Name, NameOrId, Nullable, PaginationOrder,
+    RouteDestination, RouteTarget, TxEqConfig, UserId,
 };
 use omicron_common::disk::DiskVariant;
 use oxnet::{IpNet, Ipv4Net, Ipv6Net};
@@ -491,12 +491,12 @@ pub struct SiloQuotasUpdate {
 // was discussed.
 
 /// Updateable properties of a silo's settings.
-#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
+#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
 pub struct SiloSettingsUpdate {
-    /// Maximum lifetime of a device token in seconds. If unset (or set to
-    /// null), users will be able to create tokens that do not expire.
+    /// Maximum lifetime of a device token in seconds. If set to null, users
+    /// will be able to create tokens that do not expire.
     #[schemars(range(min = 1))]
-    pub device_token_max_ttl_seconds: Option<NonZeroU32>,
+    pub device_token_max_ttl_seconds: Nullable<NonZeroU32>,
 }
 
 /// Create-time parameters for a `User`

openapi/nexus.json

@@ -22926,12 +22926,15 @@
         "properties": {
           "device_token_max_ttl_seconds": {
             "nullable": true,
-            "description": "Maximum lifetime of a device token in seconds. If unset (or set to null), users will be able to create tokens that do not expire.",
+            "description": "Maximum lifetime of a device token in seconds. If set to null, users will be able to create tokens that do not expire.",
             "type": "integer",
             "format": "uint32",
             "minimum": 1
           }
-        }
+        },
+        "required": [
+          "device_token_max_ttl_seconds"
+        ]
       },
       "SiloUtilization": {
         "description": "View of a silo's resource utilization and capacity",