fix: kube-core::Schema hoisting logic #1839

NickLarsenNZ · 2025-10-29T07:56:53Z

Fixes #1821

This updates the kube Schema hoisting since schemars Schema's have changed.

Add tests for a variety of enum use-cases:
- Tagged vs Untagged
- Unit vs Tuple vs Structural variants
- With and without doc-comments (descriptions)
Rewrite the hoisting logic
- This is annotated with dev-comments to help understand intend and to ease future schemars changes.

This also fixes other issues that appeared along the way:

Untagged enum variant doc-comments were being applied to field descriptions.
Additional null entry added to enums.

See also: stackabletech#1, where this work was originally done.

Motivation

Since kube 1.0.0, schemas became invalid when optional enums contained comments on the variants.

Solution

Break out the hoisting logic into separate modules with scoped tests.
- Heavily document the steps to help developers with future schemars Schema generation changes.
Add tests to try and catch this in future.

Fixes kube Schema conversion since schemars Schema's have changed. - Add tests for a variety of enum use-cases: - Tagged vs Untagged - Unit vs Tuple vs Structural variants - With and without doc-comments (descriptions) - Rewrite the hoisting logic - This is annotated with dev-comments to help understand intend and to ease future schemars changes. This also fixes other issues: - Untagged enum variant doc-comments were being applied to field descriptions. - Additional `null` entry added to enums. --------- Co-authored-by: Nick Larsen <[email protected]> Co-authored-by: Sebastian Bernauer <[email protected]> Co-authored-by: Techassi <[email protected]> Signed-off-by: Nick Larsen <[email protected]>

codecov · 2025-10-29T08:07:11Z

Codecov Report

❌ Patch coverage is 97.31801% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.3%. Comparing base (a12559d) to head (77af8e5).
⚠️ Report is 38 commits behind head on main.

Files with missing lines	Patch %	Lines
kube-core/src/schema/transform_one_of.rs	92.2%	3 Missing ⚠️
kube-core/src/schema/transform_any_of.rs	95.2%	2 Missing ⚠️
kube-core/src/schema/transform_properties.rs	97.6%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1839     +/-   ##
=======================================
+ Coverage   74.7%   75.3%   +0.6%     
=======================================
  Files         84      90      +6     
  Lines       7938    8133    +195     
=======================================
+ Hits        5927    6120    +193     
- Misses      2011    2013      +2

Files with missing lines	Coverage Δ
kube-core/src/schema/mod.rs	`100.0% <100.0%> (ø)`
...re/src/schema/transform_optional_enum_with_null.rs	`100.0% <100.0%> (ø)`
kube-derive/src/custom_resource.rs	`84.3% <100.0%> (-<0.1%)`	⬇️
kube-derive/tests/crd_complex_enum_tests.rs	`100.0% <100.0%> (ø)`
kube-derive/tests/crd_mixed_enum_test.rs	`100.0% <100.0%> (ø)`
kube-derive/tests/crd_schema_test.rs	`100.0% <100.0%> (ø)`
kube-derive/tests/crd_top_level_enum_test.rs	`100.0% <ø> (ø)`
kube-core/src/schema/transform_any_of.rs	`95.2% <95.2%> (ø)`
kube-core/src/schema/transform_properties.rs	`97.6% <97.6%> (ø)`
kube-core/src/schema/transform_one_of.rs	`92.2% <92.2%> (ø)`

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

NickLarsenNZ · 2025-10-29T08:11:31Z

I'm unsure what to do about the CI error:

error: rustc 1.86.0 is not supported by the following package:
  [email protected] requires rustc 1.88

For me, I get [email protected]. Should I pin the dependency?

kube/Cargo.toml

Line 50 in 7c63f56

home = "0.5.4"

sbernauer · 2025-10-29T09:20:50Z

Created #1840 to address the home CI failures

Techassi · 2025-11-04T09:29:29Z

I've just synced this PR branch to include the CI fixes (done in #1840). This PR should now be ready for review.

NickLarsenNZ · 2025-11-26T12:44:25Z

Hi @clux, are you able to take a look at this?

We saw a new PR come up that fixes part of the problem: #1853

sbernauer · 2025-11-26T13:26:35Z

(Also a small draft PR with a fix popped up: #1841)

clux · 2025-11-26T13:45:28Z

Heyyy, sorry not had much time to deeply look at things the last month. This is definitely on the larger size, so only done a quick pass now, but the testing rigor is definitely super helpful! Thanks a lot for all this.

One thing that stands out as different in the approaches here is the use of Transform impls; this PR calls multiple functions, in one giant Transform, and the new PR is adding a more isolated option enum transform. I do wonder if it's better to follow the multiples-transform approach to allow users some flexibility (without us having to expose a bunch more config parameters for "our kube rewriter"). I have a hunch that it can make testing a bit more isolatable as well perhaps, but maybe not.

As a quick check first. Would you be OK with rebasing (possibly also splitting to expedite) this if I were to merge #1853 first? That's a pretty digestible PR, although it does seem to clash with your remove_optional_enum_null_variant parts (for which the optional_enum_with_null tests would probably be useful to combine).

NickLarsenNZ · 2025-11-27T09:14:24Z

Heyyy, sorry not had much time to deeply look at things the last month.

No problem. I know how it gets.

This is definitely on the larger size

Yeah, we are aware of that. We started off with very minimal changes (much like the other PR(s)) but after creating a set of tests we found at least three bugs which lead to a larger refactor after seeing a more general transformation pattern.

The other PRs appear to only be solving one at a time and I believe they would not pass the test suite in this PR.

One thing that stands out as different in the approaches here is the use of Transform impls; this PR calls multiple functions, in one giant Transform, and the new PR is adding a more isolated option enum transform. I do wonder if it's better to follow the multiples-transform approach to allow users some flexibility (without us having to expose a bunch more config parameters for "our kube rewriter"). I have a hunch that it can make testing a bit more isolatable as well perhaps, but maybe not.

I just didn't think of that, but I think we can just make multiple transformers and move the chained calls inside the existing one into each new transformer. I think I can spend a little bit of time to see how easy it is to convert.

As a quick check first. Would you be OK with rebasing (possibly also splitting to expedite) this if I were to merge #1853 first? That's a pretty digestible PR, although it does seem to clash with your remove_optional_enum_null_variant parts (for which the optional_enum_with_null tests would probably be useful to combine).

I guess I'd prefer not to, partly because of the possible merge conflicts, but also because I think they only solve part of the problem without seeing the broader issue.

Given that, if you are not convinced to hold off merging the other PR(s), I understand.

Signed-off-by: Nick Larsen <[email protected]>

NickLarsenNZ · 2025-11-27T10:57:05Z

I have updated the implementation to split the single transformer into multiple transformers.

The original StructuralSchemaRewriter should be renamed because it now does two random other things (should it just be nameds OtherSchemaRewriter, or should they too be split into separate Transform impls?

I did try moving the body of the transform functions into the Transform impl but then the unit tests would need rewriting (I can try that is you prefer).

Anyway, I will leave it as is and await your feedback.

clux · 2025-11-27T16:07:11Z

I have updated the implementation to split the single transformer into multiple transformers.

I think this makes the intent between the various functionality a bit clearer at the very least. 👍

The other PRs appear to only be solving one at a time and I believe they would not pass the test suite in this PR.

yeah. that is a concern I have as well. i need to look a bit more tomorrow to understand the various test cases here, but afaikt the tests are fairly isolated as is, and each transformer now looks like something that can be reviewed independently, but maybe there's a subtletly i am missing.

i'll get back with more tomorrow.

clux · 2025-11-28T07:23:46Z

I guess I'd prefer not to, partly because of the possible merge conflicts, but also because I think they only solve part of the problem without seeing the broader issue.

Yeah, that is fair. Will hold off.

I did try moving the body of the transform functions into the Transform impl but then the unit tests would need rewriting (I can try that is you prefer).

I like the idea of this if it doesn't make the testing too difficult. At least then we are testing the actual interface and not only all our internal functions/structs (which i at some point hoped we could reduce). Maybe it can make the tests a little easier to read as well if we try stuff like schemars::json_schema!. EDIT: looking at schemars own unit testing, i like their style

clux

some initial comments on various bits. suggestions on some code-minimisation.
feels like i mostly want more use of slice-patterns (to remove some implicit panics), more let-else use, more comments on the invariants that cause us to panic, and possibly (possibly also testing of the interface direct as per previous comment).

otherwise, i think this approach is nice. it is very thorough, and you're dealing with a more complicated codebase at stackable than i have to deal with so happy to take changes like this from someone with a bit of skin in the game.

clux · 2025-11-27T15:21:10Z

kube-core/src/schema/transform_properties.rs

+        // Apply this (self) transform to all subschemas
+        schemars::transform::transform_subschemas(self, transform_schema);
+
+        let mut schema: SchemaObject = match serde_json::from_value(transform_schema.clone().to_value()).ok()


this match might be able to be a let-else;

let mut Some(schema) = serde_json::... { else return; }

if the compiler allows the mut in there with a type

Done in 14457d1

clux · 2025-11-27T15:45:09Z

kube-core/src/schema/transform_any_of.rs

+    if any_of.len() != 2 {
+        return;
+    }
+
+    let entry_is_null: [bool; 2] = any_of
+        .iter()
+        .map(|schema| serde_json::to_value(schema).expect("schema should be able to convert to JSON"))
+        .map(|value| value == *NULL_SCHEMA)
+        .collect::<Vec<_>>()
+        .try_into()
+        .expect("there should be exactly 2 elements. We checked earlier");
+
+    // Get the `any_of` subschema that isn't the null entry
+    let subschema_to_hoist = match entry_is_null {
+        [true, false] => &any_of[1],
+        [false, true] => &any_of[0],
+        _ => return,
+    };


technically 3 panic-using apis here; array indexing + 2 expects. indexing is checked, but would be nice to use slice-patterns to avoid the implicit linkage between the length check code and the indexing code.

The first expect should not happen unless everything is broken (a SchemaObject should be able to convert to a serde_json::Value).

The second expect should not be reachable due to the early return.

I'm happy to change things around to make this more compact. Could you elaborate what you mean about the slice-pattern? I tried a few things, but that introduced more conversions and clones.

clux · 2025-11-27T15:47:44Z

kube-core/src/schema/transform_any_of.rs

+    // TODO (@NickLarsenNZ): Return errors instead of panicking, leave panicking up to the
+    // infallible schemars::Transform


been thinking about the panicing here a bit, and i think at least the panicing here would generally apply to "setup code". even if it's not, i would rather have a panic here than to accidentally do a runtime "error" log that people miss and as a result perhaps modify a schema incorrectly (making people potentially have to do migrations).

the tradeoff here is that we could panic a controller that does something with schemas live, but controllers should be idempotent and resilient against temporary failures like a controller crashing..

It's really unfortunate that the schemars::Transform is infallible.

As for the comment I left in the code, that was more about making the functions return errors (easier to tests too) and then panic on error inside the transform.

Would you like me to remove the comment?

Suggested change

// TODO (@NickLarsenNZ): Return errors instead of panicking, leave panicking up to the

// infallible schemars::Transform

clux · 2025-11-28T07:34:27Z

kube-core/src/schema/transform_one_of.rs

+///
+/// This will return early without modifications unless:
+/// - There are `oneOf` subschemas (not empty).
+/// - Each subschema contains an `enum` or `const`.


Possibly this can be reduced in scope if we use the existing ReplaceConstValue? EDIT: probably not; it doesn't do a lot.

Ah yeah. Should I leave as-is then?

On that note, Kubernetes might eventually support const: kubernetes/kubernetes#134684

clux · 2025-11-28T07:34:55Z

kube-core/src/schema/transform_one_of.rs

+            // 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`.


does it make sense to concat the titles?

Do you mean remove it from other when it is already in extensions?

clux · 2025-11-28T07:51:59Z

kube-core/src/schema/transform_one_of.rs

+    if types.any(|r#type| r#type != variant_type) {
+        panic!("oneOf variants must all have the same type");
+    }


probably a sane unwrap for now. might need another line about why this is true in the code.

Added extra explanation in 77af8e5

kube-core/src/schema/transform_properties.rs

clux · 2025-11-28T07:54:34Z

kube-core/src/schema/transform_properties.rs

+    if subschemas.is_empty() {
+        return;
+    }
+
+    // Ensure we aren't looking at the subschema with a null, as that is hoisted by another
+    // transformer.
+    if subschemas.len() == 2 {


these two checks could be a slice pattern match perhaps

I quickly tried both:

match subschemas.len() { ...} // and match subschemas.as_slice() { .. }

But it didn't make things any simpler.

Would you mind giving a rough example as a hint?

kube-derive/tests/crd_schema_test.rs

clux · 2025-11-28T08:03:00Z

kube-derive/Cargo.toml

 assert-json-diff.workspace = true
 runtime-macros.workspace = true
 prettyplease.workspace = true
+tokio = { workspace = true, features = ["rt", "macros"] }


should be a workspace dep

Danil-Grigorev

I guess I'd prefer not to, partly because of the possible merge conflicts, but also because I think they only solve part of the problem without seeing the broader issue.

I believe the #1821 is only mentioning the specific issue with optional enums, and we need to fix that one directly, and not "oh btw I also fixed that". Having separate issues for this PR addresses would help.

Danil-Grigorev · 2025-11-29T08:09:57Z

kube-core/src/schema/transform_any_of.rs

+    // 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");


We should not panic in this code. This produces no linter hints, and gives no trace on which field actually failed. The only correct approach with the chosen path by schemars is to split code into more domain specific parts, and log errors instead of panicking here.

Danil-Grigorev · 2025-11-29T08:14:18Z

kube-core/src/schema/transform_any_of.rs

+    use super::*;
+
+    #[test]
+    fn optional_tagged_enum_with_unit_variants() {


Is this representable with a CRD schema? While pure json seems clear here, it probably does not make sense to test internals of the generated code, as this may change over time. It may also be unrepresentable in a CRD form, in which case we shouldn't try to test such things.

With a CRD based approach, the test is focused on functionality first, so in my opinion this should follow the pattern of crd_schema_test.rs or newly added crd_complex_enum_tests.rs

Danil-Grigorev · 2025-11-29T08:22:03Z

kube-core/src/schema/transform_any_of.rs

+#[derive(Debug, Clone)]
+pub struct AnyOfSchemaRewriter;
+
+impl Transform for AnyOfSchemaRewriter {


I believe that this method has no functional differences with implementation in #1853 and therefore it will pass the tests. Yet it is split in two transforms - OptionalEnumSchemaRewriter and AnyOfSchemaRewriter instead of addressing it in one place.

Danil-Grigorev · 2025-11-29T11:25:12Z

kube-core/src/schema/transform_optional_enum_with_null.rs

+/// NOTE: The trailing null is removed because it's not needed by Kubernetes
+/// and makes the CRD more compact by removing redundant information.
+#[derive(Debug, Clone)]
+pub struct OptionalEnumSchemaRewriter;


It seems strange to have AnyOfSchemaRewriter and OptionalEnumSchemaRewriter split like this. Also OptionalEnumSchemaRewriter already overrides null containing enums with top level enum. Why is this needed? Can this be produced/tested by some CRD schema?

Danil-Grigorev · 2025-11-29T11:27:36Z

kube-core/src/schema/transform_properties.rs

+        return;
+    }
+
+    // Ensure we aren't looking at the subschema with a null, as that is hoisted by another


That should be driven by the order of transformers, not the manual if->return.

Danil-Grigorev · 2025-11-29T11:33:31Z

kube-core/src/schema/transform_any_of.rs

+        .expect("there should be exactly 2 elements. We checked earlier");
+
+    // Get the `any_of` subschema that isn't the null entry
+    let subschema_to_hoist = match entry_is_null {


Unless there is a scenario where the null enum produced first in the generated array by CRD definition, it doesn't make sense to test permutations like this.

NOTE: the kube_schema metadata does need dropping, but that happens implicitly when we replace kube_schema with the subschema

… the same

NickLarsenNZ moved this to Development: Track in Stackable Engineering Oct 29, 2025

NickLarsenNZ added this to Stackable Engineering Oct 29, 2025

sbernauer mentioned this pull request Oct 29, 2025

chore: Remove dependency on home crate, use std instead #1840

Merged

chore: Merge branch 'main' into fix/schema-hoisting

221510e

clux mentioned this pull request Nov 27, 2025

Transform optional enums to match pre kube 2.0.0 format #1853

Open

chore(schema): Split StructuralSchemaRewriter into multiple transformers

50d1b89

Signed-off-by: Nick Larsen <[email protected]>

NickLarsenNZ force-pushed the fix/schema-hoisting branch from 77eb176 to 50d1b89 Compare November 27, 2025 10:53

Merge branch 'main' into fix/schema-hoisting

03d49b7

clux reviewed Nov 28, 2025

View reviewed changes

Danil-Grigorev reviewed Nov 29, 2025

View reviewed changes

NickLarsenNZ added 4 commits December 2, 2025 14:02

chore(kube-core): use let-else

14457d1

chore(kube-core): simplify metadata handling

cd53167

NOTE: the kube_schema metadata does need dropping, but that happens implicitly when we replace kube_schema with the subschema

test(kube-core): Make variable names shorter

6eb14fc

chore(kube-core): Add further explanation about why the types must be…

77af8e5

… the same

		// TODO (@NickLarsenNZ): Return errors instead of panicking, leave panicking up to the
		// infallible schemars::Transform

		// 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`.

Uh oh!

fix: kube-core::Schema hoisting logic #1839

Are you sure you want to change the base?

fix: kube-core::Schema hoisting logic #1839

Uh oh!

Conversation

NickLarsenNZ commented Oct 29, 2025

Motivation

Solution

Uh oh!

codecov bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

NickLarsenNZ commented Oct 29, 2025

Uh oh!

sbernauer commented Oct 29, 2025

Uh oh!

Techassi commented Nov 4, 2025

Uh oh!

NickLarsenNZ commented Nov 26, 2025

Uh oh!

sbernauer commented Nov 26, 2025

Uh oh!

clux commented Nov 26, 2025

Uh oh!

NickLarsenNZ commented Nov 27, 2025

Uh oh!

NickLarsenNZ commented Nov 27, 2025

Uh oh!

clux commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clux commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clux Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 29, 2025 •

edited

Loading

clux commented Nov 27, 2025 •

edited

Loading

clux commented Nov 28, 2025 •

edited

Loading

clux Nov 28, 2025 •

edited

Loading