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.

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

apiextensionsv1.JSON does not seem to be properly processed #313

Closed
mnaser opened this issue Feb 25, 2025 · 4 comments · Fixed by #314
Closed

apiextensionsv1.JSON does not seem to be properly processed #313

mnaser opened this issue Feb 25, 2025 · 4 comments · Fixed by #314

Comments

@mnaser
Copy link
Contributor

mnaser commented Feb 25, 2025

I'm working with the following crate which relies on auto-generated objects using CRs:

https://github.com/capi-samples/cluster-api-rs

It seems like the apiextensionsv1.JSON type is being serialized in this way:

https://github.com/capi-samples/cluster-api-rs/blob/84fcaa691bbace352663a5fab60094656176283c/src/api/capi_clusterclass.rs#L731-L734

    /// enum is the list of valid values of the variable.
    /// NOTE: Can be set for all types.
    #[serde(default, skip_serializing_if = "Option::is_none", rename = "enum")]
    pub r#enum: Option<Vec<BTreeMap<String, serde_json::Value>>>,

You can find the source code here:

https://github.com/kubernetes-sigs/cluster-api/blob/15ce36f9eec61be27ec96e1044c2c642ee0828f4/api/v1beta1/clusterclass_types.go#L605-L608

I believe it should be translated into Option<Vec<serde_json.Value>.

@clux
Copy link
Member

clux commented Feb 26, 2025

Possibly another case where we need to not nest in a map like the previous bug #229

Do you have a link to the relevant part of the openapi schema?

@mnaser
Copy link
Contributor Author

mnaser commented Feb 26, 2025

@mnaser
Copy link
Contributor Author

mnaser commented Feb 26, 2025

I managed to generate a failing test here:

#314

I am still not sure confident on figuring out where the fix would live though.

@clux
Copy link
Member

clux commented Feb 27, 2025

that's great. thank you!

if you are feeling brave, you could try to copy from a similar unit test in

kopium/src/analyzer.rs

Lines 651 to 689 in 9245007

#[test]
fn empty_preserve_unknown_fields() {
init();
let schema_str = r#"
description: |-
Identifies servers in the same namespace for which this authorization applies.
required:
- selector
properties:
selector:
description: A label query over servers on which this authorization
applies.
required:
- matchLabels
properties:
matchLabels:
type: object
x-kubernetes-preserve-unknown-fields: true
type: object
type: object
"#;
let schema: JSONSchemaProps = serde_yaml::from_str(schema_str).unwrap();
//println!("schema: {}", serde_json::to_string_pretty(&schema).unwrap());
let structs = analyze(schema, "Server", Cfg::default()).unwrap().0;
//println!("{:#?}", structs);
let root = &structs[0];
assert_eq!(root.name, "Server");
assert_eq!(root.level, 0);
let root_member = &root.members[0];
assert_eq!(root_member.name, "selector");
assert_eq!(root_member.type_, "ServerSelector");
let server_selector = &structs[1];
assert_eq!(server_selector.name, "ServerSelector");
assert_eq!(server_selector.level, 1);
let match_labels = &server_selector.members[0];
assert_eq!(match_labels.name, "matchLabels");
assert_eq!(match_labels.type_, "BTreeMap<String, serde_json::Value>");
}

because it's an array, it's likely this line that needs a similar treatment as the referenced issue:

kopium/src/analyzer.rs

Lines 429 to 432 in 9245007

if s.type_.is_none() && s.x_kubernetes_preserve_unknown_fields == Some(true) {
let map_type = cfg.map.name();
return Ok((format!("Vec<{}<String, serde_json::Value>>", map_type), level));
}

EDIT: i see your pr now 🤦

mnaser added a commit to mnaser/kopium that referenced this issue Feb 27, 2025
@clux clux closed this as completed in #314 Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants