fix: kube-core::Schema hoisting logic #1
+1,588
−152
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sbernauer
force-pushed
the
test/add-enum-tests
branch
from
6480fee to
295c431
Compare
NickLarsenNZ
reviewed
Oct 9, 2025
NickLarsenNZ
reviewed
Oct 9, 2025
Note: As described on the function, it is overly documented to express intent where it can be a little unclear. Ideally where would be some clearer official Kubernetes documentation on the differences between regular OpenAPI 3 schemas, and what Kubernetes accepts.
… there are no oneOfs at the top level of the schema
Note: At this point, there is some strange interaction with the new hoisting functions and the existing code whereby "properties" disappear.
…l as enum All tests in this PR are passing Note: The function should be renamed, but it needs some work to not only do optional things.
This comment was marked as outdated.
This comment was marked as outdated.
note: test expectaions changed as part of the implementation: Untagged variant doc-comments will not be preserved. Previously they were erroneously added as field descriptions.
Also update function docs, and expected panic messages
sbernauer
commented
Oct 22, 2025
Comment on lines
+271
to
+272
| // TODO (@NickLarsenNZ): At this point, we are seeing duplicate `title` when printing schema as json. | ||
| // This is because `title` is specified under both `extensions` and `other`. |
There was a problem hiding this comment.
I guess I should remove it, but worth sharing the knowledge before I do:
Suggested change
| // TODO (@NickLarsenNZ): At this point, we are seeing duplicate `title` when printing schema as json. | |
| // This is because `title` is specified under both `extensions` and `other`. |
There was a problem hiding this comment.
I think we actually should leave it here (reworded/adjusted) to explicitly share the knowledge.
| // 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. |
There was a problem hiding this comment.
We should probably add a test for:
// panics...wrong. two can't be u32 AND NormalEnum
/// An untagged enum with a nested enum inside
#[derive(Serialize, Deserialize, Debug, Clone, JsonSchema)]
#[serde(untagged)]
enum InvalidUntaggedEnumX {
/// Used in case the `one` field of tpye [`u32`] is present
A { one: String, two: u32 },
/// Used in case the `two` field of type [`NormalEnum`] is present
B { two: NormalEnum },
/// Used in case no fields are present
C {},
}
// This is fine, except that it will fail until he descriptions match (or we fix the invalid descriptions)
/// An untagged enum with a nested enum inside
#[derive(Serialize, Deserialize, Debug, Clone, JsonSchema)]
#[serde(untagged)]
enum UntaggedEnum3 {
/// Used in case the `one` field of tpye [`u32`] is present
A { one: String, two: NormalEnum },
/// Used in case the `two` field of type [`NormalEnum`] is present
B { two: NormalEnum },
/// Used in case no fields are present
C {},
}
Comment on lines
+161
to
+163
| // Set the schema to nullable (as we know we matched the null variant earlier) | ||
| // TODO (@NickLarsenNZ): Do we need to insert `nullable` into `other` too? | ||
| kube_schema.extensions.insert("nullable".to_owned(), true.into()); |
There was a problem hiding this comment.
Anyone know what to do here?
I don't know if it's good enough being in one place, or if it should be in both for compatibility with something.
schemars sends fields that are the same between extensions and other.
NickLarsenNZ
moved this from Development: In Progress
to Development: Waiting for Review
in Stackable Engineering
NickLarsenNZ
changed the title
Comment on lines
+259
to
+263
| #[cfg(test)] | ||
| fn schemars_schema_to_kube_schema(incoming: schemars::Schema) -> Result<Schema, serde_json::Error> { | ||
| serde_json::from_value(incoming.to_value()) | ||
| } | ||
|
|
There was a problem hiding this comment.
This is unused. @sbernauer did you want to do something with is?
Else I will remove it:
Suggested change
| #[cfg(test)] | |
| fn schemars_schema_to_kube_schema(incoming: schemars::Schema) -> Result<Schema, serde_json::Error> { | |
| serde_json::from_value(incoming.to_value()) | |
| } |
sbernauer
moved this from Development: Waiting for Review
to Development: In Review
in Stackable Engineering
|
|
||
| // Enum definitions | ||
|
|
||
| /// A very simple enum with empty variants |
There was a problem hiding this comment.
I want to change this description. It needs changes in the expected test schemas too
Suggested change
| /// A very simple enum with empty variants | |
| /// A very simple enum with unit variants |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes kube Schema conversion since schemars Schema's have changed.
This also fixes an issue where Untagged enum variant doc-comments were being applied to field descriptions.