Move null schema into NULL_SCHEMA static

Author: sbernauer

kube-core/src/schema/mod.rs

@@ -13,7 +13,10 @@ mod transform_properties;
 use schemars::{transform::Transform, JsonSchema};
 use serde::{Deserialize, Serialize};
 use serde_json::Value;
-use std::collections::{BTreeMap, BTreeSet};
+use std::{
+    collections::{BTreeMap, BTreeSet},
+    sync::LazyLock,
+};
 
 use crate::schema::{
     transform_any_of::hoist_any_of_subschema_with_a_nullable_variant,
@@ -22,6 +25,14 @@ use crate::schema::{
     transform_properties::hoist_properties_for_any_of_subschemas,
 };
 
+/// This is the signature for the `null` variant produced by schemars.
+static NULL_SCHEMA: LazyLock<Value> = LazyLock::new(|| {
+    serde_json::json!({
+        "enum": [null],
+        "nullable": true
+    })
+});
+
 /// schemars [`Visitor`] that rewrites a [`Schema`] to conform to Kubernetes' "structural schema" rules
 ///
 /// The following two transformations are applied

kube-core/src/schema/transform_any_of.rs

@@ -1,6 +1,6 @@
 use std::ops::DerefMut;
 
-use crate::schema::{Schema, SchemaObject, SubschemaValidation};
+use crate::schema::{Schema, SchemaObject, SubschemaValidation, NULL_SCHEMA};
 
 /// Replace the schema with the anyOf subschema and set to nullable when the
 /// only other subschema is the nullable entry.
@@ -36,19 +36,10 @@ pub(crate) fn hoist_any_of_subschema_with_a_nullable_variant(kube_schema: &mut S
         return;
     }
 
-    // This is the signature for the null variant, indicating the "other"
-    // variant is the subschema that needs hoisting
-    let null = serde_json::json!({
-        "enum": [null],
-        "nullable": true
-    });
-
-    // iter through any_of entries, converting them to Value,
-    // and build a truth table for null matches
     let entry_is_null: [bool; 2] = any_of
         .iter()
         .map(|x| serde_json::to_value(x).expect("schema should be able to convert to JSON"))
-        .map(|x| x == null)
+        .map(|x| x == *NULL_SCHEMA)
         .collect::<Vec<_>>()
         .try_into()
         .expect("there should be exactly 2 elements. We checked earlier");
@@ -63,8 +54,8 @@ pub(crate) fn hoist_any_of_subschema_with_a_nullable_variant(kube_schema: &mut S
     // At this point, we can be reasonably sure we need to hoist the non-null
     // anyOf subschema up to the schema level, and unset the anyOf field.
     // From here, anything that looks wrong will panic instead of return.
-    // TODO (@NickLarsenNZ): Return errors instead of panicking, leave panicking up to the infallible schemars::Transform
-
+    // TODO (@NickLarsenNZ): Return errors instead of panicking, leave panicking up to the
+    // infallible schemars::Transform
     let Schema::Object(to_hoist) = subschema_to_hoist else {
         panic!("the non-null anyOf subschema is a bool. That is not expected here");
     };
@@ -90,6 +81,8 @@ pub(crate) fn hoist_any_of_subschema_with_a_nullable_variant(kube_schema: &mut S
 
 #[cfg(test)]
 mod tests {
+    use assert_json_diff::assert_json_eq;
+
     use super::*;
 
     #[test]
@@ -168,7 +161,7 @@ mod tests {
         let mut actual_converted_schema_object = original_schema_object.clone();
         hoist_any_of_subschema_with_a_nullable_variant(&mut actual_converted_schema_object);
 
-        assert_json_diff::assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
+        assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
     }
 
     #[test]
@@ -216,6 +209,6 @@ mod tests {
         let mut actual_converted_schema_object = original_schema_object.clone();
         hoist_any_of_subschema_with_a_nullable_variant(&mut actual_converted_schema_object);
 
-        assert_json_diff::assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
+        assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
     }
 }

kube-core/src/schema/transform_one_of.rs

@@ -41,7 +41,8 @@ pub(crate) fn hoist_one_of_enum_with_unit_variants(kube_schema: &mut SchemaObjec
     // At this point, we can be reasonably sure we need to hoist the oneOf
     // subschema enums and types up to the schema level, and unset the oneOf field.
     // From here, anything that looks wrong will panic instead of return.
-    // TODO (@NickLarsenNZ): Return errors instead of panicking, leave panicking up to the infallible schemars::Transform
+    // TODO (@NickLarsenNZ): Return errors instead of panicking, leave panicking up to the
+    // infallible schemars::Transform
 
     // Prepare to ensure each variant schema has a type
     let mut types = one_of.iter().map(|schema| match schema {
@@ -92,6 +93,8 @@ pub(crate) fn hoist_one_of_enum_with_unit_variants(kube_schema: &mut SchemaObjec
 
 #[cfg(test)]
 mod tests {
+    use assert_json_diff::assert_json_eq;
+
     use super::*;
 
     #[test]
@@ -143,6 +146,6 @@ mod tests {
         let mut actual_converted_schema_object = original_schema_object.clone();
         hoist_one_of_enum_with_unit_variants(&mut actual_converted_schema_object);
 
-        assert_json_diff::assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
+        assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
     }
 }

kube-core/src/schema/transform_optional_enum_with_null.rs

@@ -15,12 +15,14 @@ pub(crate) fn remove_optional_enum_null_variant(kube_schema: &mut SchemaObject)
         return;
     };
 
-    // It only makes sense to remove `null` enum values in case the enum is
-    // nullable (thus optional).
+    // It only makes sense to remove `null` enum values in case the enum is nullable (thus optional)
+    // This is a safety mechanism, "nullable" should always be set to true, if one enum variant is
+    // null.
     if let Some(Value::Bool(true)) = extensions.get("nullable") {
-        // Don't remove the single last enum variant. This often happens for
-        // `Option<XXX>`, which is represented as
-        // `"anyOf": [XXX, {"enum": [null], "optional": true}]`
+        // Don't remove the single last enum variant. This often happens for `Option<XXX>`, which is
+        // represented as `"anyOf": [XXX, {"enum": [null], "optional": true}]`.
+        // We need to keep `"enum": [null]` (as opposed to `"enum": []`), because other hoisting
+        // code uses `kube::core::NULL_SCHEMA` to detect null variants.
         if enum_values.len() > 1 {
             enum_values.retain(|enum_value| enum_value != &Value::Null);
         }
@@ -29,6 +31,8 @@ pub(crate) fn remove_optional_enum_null_variant(kube_schema: &mut SchemaObject)
 
 #[cfg(test)]
 mod tests {
+    use assert_json_diff::assert_json_eq;
+
     use super::*;
 
     #[test]
@@ -64,6 +68,6 @@ mod tests {
         let mut actual_converted_schema_object = original_schema_object.clone();
         remove_optional_enum_null_variant(&mut actual_converted_schema_object);
 
-        assert_json_diff::assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
+        assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
     }
 }

kube-core/src/schema/transform_properties.rs

@@ -1,4 +1,4 @@
-use crate::schema::{InstanceType, Metadata, Schema, SchemaObject, SingleOrVec};
+use crate::schema::{InstanceType, Metadata, Schema, SchemaObject, SingleOrVec, NULL_SCHEMA};
 
 /// Take oneOf or anyOf subschema properties and move them them into the schema
 /// properties.
@@ -35,28 +35,21 @@ pub(crate) fn hoist_properties_for_any_of_subschemas(kube_schema: &mut SchemaObj
         return;
     }
 
-    // Ensure we aren't looking at the one with a null
+    // Ensure we aren't looking at the one with a null, as that is hoisted by another transformer
     if subschemas.len() == 2 {
-        // This is the signature for the null variant, indicating the "other"
-        // variant is the subschema that needs hoisting
-        let null = serde_json::json!({
-            "enum": [null],
-            "nullable": true
-        });
-
-        // Return if one of the two entries are nulls
-        for value in subschemas
+        // Return if there is a null entry
+        if subschemas
             .iter()
             .map(|x| serde_json::to_value(x).expect("schema should be able to convert to JSON"))
+            .any(|x| x == *NULL_SCHEMA)
         {
-            if value == null {
-                return;
-            }
+            return;
         }
     }
 
     // At this point, we can be reasonably sure we need operate on the schema.
-    // TODO (@NickLarsenNZ): Return errors instead of panicking, leave panicking up to the infallible schemars::Transform
+    // TODO (@NickLarsenNZ): Return errors instead of panicking, leave panicking up to the
+    // infallible schemars::Transform
 
     let subschemas = subschemas
         .iter_mut()
@@ -67,7 +60,8 @@ pub(crate) fn hoist_properties_for_any_of_subschemas(kube_schema: &mut SchemaObj
         .collect::<Vec<_>>();
 
     for subschema in subschemas {
-        // This will clear out any objects that don't have required/properties fields (so that it appears as: {}).
+        // This will clear out any objects that don't have required/properties fields (so that it
+        // appears as: {}).
         let metadata = subschema.metadata.take();
         subschema.instance_type.take();
 
@@ -91,10 +85,12 @@ pub(crate) fn hoist_properties_for_any_of_subschemas(kube_schema: &mut SchemaObj
             }
 
             // If properties are set, hoist them to the schema properties.
-            // This will panic if duplicate properties are encountered that do not have the same shape.
-            // That can happen when the untagged enum variants each refer to structs which contain the same field name but with different types or doc-comments.
+            // This will panic if duplicate properties are encountered that do not have the same
+            // shape. That can happen when the untagged enum variants each refer to structs which
+            // contain the same field name but with different types or doc-comments.
             // The developer needs to make them the same.
-            // TODO (@NickLarsenNZ): Add a case for a structural variant, and a tuple variant containing a structure where the same field name is used.
+            // TODO (@NickLarsenNZ): Add a case for a structural variant, and a tuple variant
+            // containing a structure where the same field name is used.
             while let Some((property_name, Schema::Object(property_schema_object))) =
                 object.properties.pop_first()
             {
@@ -105,8 +101,9 @@ pub(crate) fn hoist_properties_for_any_of_subschemas(kube_schema: &mut SchemaObj
                     .properties
                     .get(&property_name)
                 {
-                    // TODO (@NickLarsenNZ): Here we could do another check to see if it only differs by description.
-                    // If the schema property description is not set, then we could overwrite it and not panic.
+                    // TODO (@NickLarsenNZ): Here we could do another check to see if it only
+                    // differs by description. If the schema property description is not set, then
+                    // we could overwrite it and not panic.
                     assert_eq!(
                         existing_property,
                         &Schema::Object(property_schema_object.clone()),
@@ -126,6 +123,8 @@ pub(crate) fn hoist_properties_for_any_of_subschemas(kube_schema: &mut SchemaObj
 
 #[cfg(test)]
 mod tests {
+    use assert_json_diff::assert_json_eq;
+
     use super::*;
 
     #[test]
@@ -314,7 +313,7 @@ mod tests {
         let mut actual_converted_schema_object = original_schema_object.clone();
         hoist_properties_for_any_of_subschemas(&mut actual_converted_schema_object);
 
-        assert_json_diff::assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
+        assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
     }
 
     #[test]
@@ -439,7 +438,7 @@ mod tests {
         let mut actual_converted_schema_object = original_schema_object.clone();
         hoist_properties_for_any_of_subschemas(&mut actual_converted_schema_object);
 
-        assert_json_diff::assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
+        assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
     }
 
     #[test]
@@ -558,7 +557,7 @@ mod tests {
         let mut actual_converted_schema_object = original_schema_object.clone();
         hoist_properties_for_any_of_subschemas(&mut actual_converted_schema_object);
 
-        assert_json_diff::assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
+        assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
     }
 
     #[test]
@@ -647,7 +646,7 @@ mod tests {
         let mut actual_converted_schema_object = original_schema_object.clone();
         hoist_properties_for_any_of_subschemas(&mut actual_converted_schema_object);
 
-        assert_json_diff::assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
+        assert_json_eq!(actual_converted_schema_object, expected_converted_schema_object);
     }
 
     #[test]