Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Hoist enum values from subschemas #1051

Merged
merged 4 commits into from Oct 17, 2022
Merged

Hoist enum values from subschemas #1051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c5e5a3
Hoist enum values from subschemas
nightkr Oct 14, 2022
d2f90fb
s/sex/gender/g in test case
nightkr Oct 14, 2022
0faf86a
Merge branch 'main' into fix/hoist-enum-values
nightkr Oct 14, 2022
9dd40ab
Merge branch 'main' into fix/hoist-enum-values
nightkr Oct 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
71 changes: 54 additions & 17 deletions kube-core/src/schema.rs
View file
Open in desktop
Expand Up @@ -32,22 +32,23 @@ impl Visitor for StructuralSchemaRewriter {
fn visit_schema_object(&mut self, schema: &mut schemars::schema::SchemaObject) {
schemars::visit::visit_schema_object(self, schema);

if let Some(one_of) = schema
.subschemas
.as_mut()
.and_then(|subschemas| subschemas.one_of.as_mut())
{
// Tagged enums are serialized using `one_of`
hoist_subschema_properties(one_of, &mut schema.object, &mut schema.instance_type);
}
if let Some(subschemas) = &mut schema.subschemas {
if let Some(one_of) = subschemas.one_of.as_mut() {
// Tagged enums are serialized using `one_of`
hoist_subschema_properties(one_of, &mut schema.object, &mut schema.instance_type);

if let Some(any_of) = schema
.subschemas
.as_mut()
.and_then(|subschemas| subschemas.any_of.as_mut())
{
// Untagged enums are serialized using `any_of`
hoist_subschema_properties(any_of, &mut schema.object, &mut schema.instance_type);
// "Plain" enums are serialized using `one_of` if they have doc tags
hoist_subschema_enum_values(one_of, &mut schema.enum_values, &mut schema.instance_type);

if one_of.is_empty() {
subschemas.one_of = None;
}
}

if let Some(any_of) = &mut subschemas.any_of {
// Untagged enums are serialized using `any_of`
hoist_subschema_properties(any_of, &mut schema.object, &mut schema.instance_type);
}
}

// check for maps without with properties (i.e. flattened maps)
Expand All @@ -65,15 +66,49 @@ impl Visitor for StructuralSchemaRewriter {
}
}

/// Bring all plain enum values up to the root schema,
/// since Kubernetes doesn't allow subschemas to define enum options.
///
/// (Enum here means a list of hard-coded values, not a tagged union.)
fn hoist_subschema_enum_values(
subschemas: &mut Vec<Schema>,
common_enum_values: &mut Option<Vec<serde_json::Value>>,
instance_type: &mut Option<SingleOrVec<InstanceType>>,
) {
subschemas.retain(|variant| {
if let Schema::Object(SchemaObject {
instance_type: variant_type,
enum_values: Some(variant_enum_values),
..
}) = variant
{
if let Some(variant_type) = variant_type {
match instance_type {
None => *instance_type = Some(variant_type.clone()),
Some(tpe) => {
if tpe != variant_type {
panic!("Enum variant set {variant_enum_values:?} has type {variant_type:?} but was already defined as {instance_type:?}. The instance type must be equal for all subschema variants.")
}
}
}
}
common_enum_values
.get_or_insert_with(Vec::new)
.extend(variant_enum_values.iter().cloned());
false
} else {
true
}
})
}

/// Bring all property definitions from subschemas up to the root schema,
/// since Kubernetes doesn't allow subschemas to define properties.
fn hoist_subschema_properties(
subschemas: &mut Vec<Schema>,
common_obj: &mut Option<Box<ObjectValidation>>,
instance_type: &mut Option<SingleOrVec<InstanceType>>,
) {
let common_obj = common_obj.get_or_insert_with(Box::<ObjectValidation>::default);

for variant in subschemas {
if let Schema::Object(SchemaObject {
instance_type: variant_type,
Expand All @@ -82,6 +117,8 @@ fn hoist_subschema_properties(
..
}) = variant
{
let common_obj = common_obj.get_or_insert_with(Box::<ObjectValidation>::default);

if let Some(variant_metadata) = variant_metadata {
// Move enum variant description from oneOf clause to its corresponding property
if let Some(description) = std::mem::take(&mut variant_metadata.description) {
Expand Down
1 change: 1 addition & 0 deletions kube-derive/Cargo.toml
View file
Open in desktop
Expand Up @@ -31,3 +31,4 @@ schemars = { version = "0.8.6", features = ["chrono"] }
validator = { version = "0.16.0", features = ["derive"] }
chrono = { version = "0.4.19", default-features = false }
trybuild = "1.0.48"
assert-json-diff = "2.0.2"
43 changes: 22 additions & 21 deletions kube-derive/tests/crd_schema_test.rs
View file
Open in desktop
@@ -1,5 +1,6 @@
#![recursion_limit = "256"]

use assert_json_diff::assert_json_eq;
use chrono::{DateTime, NaiveDateTime, Utc};
use kube_derive::CustomResource;
use schemars::JsonSchema;
Expand Down Expand Up @@ -78,33 +79,34 @@ enum ComplexEnum {
#[serde(rename_all = "camelCase")]
#[serde(untagged)]
enum UntaggedEnumPerson {
SexAndAge(SexAndAge),
SexAndDateOfBirth(SexAndDateOfBirth),
GenderAndAge(GenderAndAge),
GenderAndDateOfBirth(GenderAndDateOfBirth),
}

#[derive(Serialize, Deserialize, Debug, PartialEq, Clone, JsonSchema)]
#[serde(rename_all = "camelCase")]
struct SexAndAge {
/// Sex of the person
sex: Sex,
struct GenderAndAge {
/// Gender of the person
gender: Gender,
/// Age of the person in years
age: i32,
}

#[derive(Serialize, Deserialize, Debug, PartialEq, Clone, JsonSchema)]
#[serde(rename_all = "camelCase")]
struct SexAndDateOfBirth {
/// Sex of the person
sex: Sex,
struct GenderAndDateOfBirth {
/// Gender of the person
gender: Gender,
/// Date of birth of the person as ISO 8601 date
date_of_birth: String,
}

#[derive(Serialize, Deserialize, Debug, PartialEq, Clone, JsonSchema)]
#[serde(rename_all = "PascalCase")]
enum Sex {
enum Gender {
Female,
Male,
/// This variant has a comment!
Other,
}

Expand All @@ -122,7 +124,7 @@ fn test_shortnames() {

#[test]
fn test_serialized_matches_expected() {
assert_eq!(
assert_json_eq!(
serde_json::to_value(Foo::new("bar", FooSpec {
non_nullable: "asdf".to_string(),
non_nullable_with_default: "asdf".to_string(),
Expand All @@ -132,9 +134,9 @@ fn test_serialized_matches_expected() {
nullable_with_default: None,
timestamp: DateTime::from_utc(NaiveDateTime::from_timestamp(0, 0), Utc),
complex_enum: ComplexEnum::VariantOne { int: 23 },
untagged_enum_person: UntaggedEnumPerson::SexAndAge(SexAndAge {
untagged_enum_person: UntaggedEnumPerson::GenderAndAge(GenderAndAge {
age: 42,
sex: Sex::Male,
gender: Gender::Male,
})
}))
.unwrap(),
Expand All @@ -157,7 +159,7 @@ fn test_serialized_matches_expected() {
},
"untaggedEnumPerson": {
"age": 42,
"sex": "Male"
"gender": "Male"
}
}
})
Expand All @@ -167,9 +169,9 @@ fn test_serialized_matches_expected() {
#[test]
fn test_crd_schema_matches_expected() {
use kube::core::CustomResourceExt;
assert_eq!(
assert_json_eq!(
Foo::crd(),
serde_json::from_value(serde_json::json!({
serde_json::json!({
"apiVersion": "apiextensions.k8s.io/v1",
"kind": "CustomResourceDefinition",
"metadata": {
Expand Down Expand Up @@ -281,18 +283,18 @@ fn test_crd_schema_matches_expected() {
"type": "string",
"description": "Date of birth of the person as ISO 8601 date"
},
"sex": {
"gender": {
"type": "string",
"enum": ["Female", "Male", "Other"],
"description": "Sex of the person"
"description": "Gender of the person"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this drops variant-level documentation, do we want to include it in the enum property's documentation instead?

Do you mean something like this?

"description": "Gender of the person\n\nOther - This variant has a comment!"

I think it's better to keep the current version because users can still add variant-level documentation manually if they'd like. Automatically including them will be a nice opt-in feature, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, basically.

}
},
"anyOf": [
{
"required": ["age", "sex"]
"required": ["age", "gender"]
},
{
"required": ["dateOfBirth", "sex"]
"required": ["dateOfBirth", "gender"]
}
],
"description": "This is a untagged enum with a description"
Expand All @@ -318,8 +320,7 @@ fn test_crd_schema_matches_expected() {
}
]
}
}))
.unwrap()
})
);
}

Expand Down